feat(playback): add functions to handle mediasession inputs #595

camc314 · 2021-01-18T10:16:14Z

Before

After

Unfortunately chrome desktop cannot seek through media tracks however it should not be too difficult to add:

navigator.mediaSession.setActionHandler('seekto', (time: number) => { 
  this.changeCurrentTime(time)
});

ferferga

Can you also implement seekforward, seekbackward and seekto, so this is finally 100% complete?

Imo we should at this point also create the store that handles playbacksettings (even if not configurable yet), just so we store the seekforward and backward values there, alongside for the ones for the video player, instead of having numbers scattered all around.

components/Players/PlayerManager.vue

camc314 · 2021-01-18T10:30:29Z

Updated.

codecov-io · 2021-01-18T10:49:11Z

Codecov Report

Merging #595 (b02f93d) into master (bbb46e9) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
- Coverage   10.28%   10.15%   -0.13%     
==========================================
  Files         105      105              
  Lines        2908     2945      +37     
  Branches      444      449       +5     
==========================================
  Hits          299      299              
- Misses       2593     2630      +37     
  Partials       16       16

Impacted Files	Coverage Δ
components/Players/PlayerManager.vue	`0.00% <0.00%> (ø)`
pages/playback/index.vue	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbb46e9...b02f93d. Read the comment docs.

ferferga · 2021-01-18T10:50:28Z

@camc314 I might be able to test them on Edge Android. Hopefully I can do it tomorrow, if nobody else comes first.

I'll report you back with my results 👍🏻.

camc314 · 2021-01-18T10:51:20Z

I got out an old android phone, should all work properly now.

ferferga · 2021-01-18T11:29:02Z

Receiving this

camc314 · 2021-01-18T11:44:32Z

Seems like this is an issue since MediaSession is only a working drive w3c/mediasession#228 . We don't check if it is supported in web. I guess we could wrap it in a try catch block?

ferferga · 2021-01-18T11:57:54Z

@camc314 I would say we should wrap each action handler in its own try/catch block, so handlers that are down in the chain but still compatible can still be used.

heyhippari · 2021-01-18T12:38:10Z

Saw something like this used, which should reduce the number of lines nicely:

const actionHandlers = [
  ['play',          () => { /* ... */ }],
  ['pause',         () => { /* ... */ }],
  ['previoustrack', () => { /* ... */ }],
  ['nexttrack',     () => { /* ... */ }],
  ['stop',          () => { /* ... */ }],
  ['seekbackward',  (details) => { /* ... */ }],
  ['seekforward',   (details) => { /* ... */ }],
  ['seekto',        (details) => { /* ... */ }],
];

for (const [action, handler] of actionHandlers) {
  try {
    navigator.mediaSession.setActionHandler(action, handler);
  } catch (error) {
    console.log(`The media session action "${action}" is not supported yet.`);
  }
}

heyhippari · 2021-01-18T12:39:18Z

Also note we should unset them when playback ends, with this:

navigator.mediaSession.setActionHandler("nexttrack", null);

heyhippari · 2021-01-18T12:41:13Z

components/Players/PlayerManager.vue

@@ -388,6 +391,33 @@ export default Vue.extend({
        }
      }
    },
+    addMediaHandlers(): void {


It would be nice to also set play and pause here, just to ensure everything is handled the same across browser, and making sure to set navigator.mediaSession.playbackState.

ferferga · 2021-01-18T13:56:05Z

components/Players/PlayerManager.vue

+            'play',
+            async (): Promise<void> => {
+              await this.play();
+              if (navigator.mediaSession) {
+                navigator.mediaSession.playbackState = 'playing';
+              }
+            }
+          ],


Suggested change

'play',

async (): Promise<void> => {

await this.play();

if (navigator.mediaSession) {

navigator.mediaSession.playbackState = 'playing';

}

}

],

'play',

(): void => {

this.unpause();

if (navigator.mediaSession) {

navigator.mediaSession.playbackState = 'playing';

}

}

],

I think this is what you want instead

Ah yeah I get confused with this.play vs this.unpause

ferferga · 2021-01-18T13:56:30Z

components/Players/PlayerManager.vue

@@ -341,8 +343,11 @@ export default Vue.extend({
      'setPreviousTrack',
      'setLastItemIndex',
      'playPause',
+      'pause',
+      'play',


Suggested change

'play',

'unpause',

See my other comment

ferferga · 2021-01-18T13:57:24Z

components/Players/PlayerManager.vue

+            navigator.mediaSession.setActionHandler(action, handler);
+          } catch (error) {
+            // eslint-disable-next-line no-console
+            console.log(`Error removing "${action}".`);


Suggested change

console.log(`Error removing "${action}".`);

console.log(`Error removing mediaSession action: "${action}".`);

ferferga

Two things:

Not possible to go back. At first press it restarts the song as usual, at the second it goes back to the previous song. However, the track that it's reported in MediaSession it's still the same
We should disable previous/next buttons to avoid console errors, undefined items and also better experience (right now you press on it and it does nothing, and that's not good UX imo). Set a watcher for getPreviousItem and getNextItem and nullify the next/previous handlers there.

camc314 · 2021-01-21T10:05:36Z

Not possible to go back. At first press it restarts the song as usual, at the second it goes back to the previous song. However, the track that it's reported in MediaSession it's still the same

This is fixed.

We should disable previous/next buttons to avoid console errors, undefined items and also better experience (right now you press on it and it does nothing, and that's not good UX imo). Set a watcher for getPreviousItem and getNextItem and nullify the next/previous handlers there.

I couldn't find a way to disable the buttons short of setting them to null - which hides them, which is even worse if we have got buttons disappearing and reappearing. So now, when pressing prev, it will go to the beginning of the item, and when pressed a second time to the previous item (if it exists). Pressing next will go to the next song, or end playback if there are no more items in the queue.

ferferga · 2021-01-21T10:39:57Z

@camc314 Imo it's better to have them disappear than do anything. Basically because all the desktop media controls I've used (Windows, Mac, elementaryOS), if I recall correctly, disables the buttons. Only mobile devices tend to make them disappear. Although we should keep in mind those devices, Android for instance is going native route and I think the majority of the users that use web will always be in desktop, so it's a matter of priorities imo. Buttons appearing/disappearing in something that's hidden in a notification panel it's not, in my opinion, such a big deal.

Here's a Windows screenshot for reference:

With this said, back to testing this

Edit: Just checked and Spotify changed its behaviour (single item in queue):

ferferga

Now going back is fixed, but the old track is still being displayed in the controls:

camc314 · 2021-02-04T10:11:48Z

Now going back is fixed, but the old track is still being displayed in the controls:

Can you check again? This is working for me (chrome)

ferferga · 2021-02-04T11:12:55Z

I rebased to see if CI passes (as your branch is still without the SSR bits). Back to testing

ferferga · 2021-02-04T11:26:33Z

components/Players/PlayerManager.vue

+
+            this.removeMediaHandlers();


Okay, it works now, but this makes the controls blink:

2021-02-04.12-20-03.mp4

I would create a updateMetadata method that updates the metadata instead of plain removing the handlers and clearing the metadata. I don't see why we need a resetMetadata one, that updateMetadata method should clear the metadata that's missing of the item, but if we want to clear up everything, it's because the playing session ended, so we're better calling removeMediaHandlers and that's it.

That updateMetadata method can also take handle of removing the actions of the previous/next track if there's no next/previous track in the queue.

We don't remove or read the media handlers when we navigate between tracks, only when playback is stopped, not we navigate between tracks

I can't seem to reproduce this. I have tested in both Chrome and Brave.

Does this still still occur in other browsers?

@camc314 True, GH code preview confusing me a bit with the collapses. See how Spotify app works:

spoty.mp4

I added console.log calls to removeMediaHandler and updateMetadata and I can confirm that updateMetadata is called only once per track change and removeMediaHandler is also called only when stopping.

Tested Chrome 88 in browserstacks and I can't reproduce the issue, so this does seem indeed and Edge Chromium bug.

Filed a bug report for this.

sonarqubecloud · 2021-02-04T11:32:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

camc314 requested a review from ferferga January 18, 2021 10:19

ferferga reviewed Jan 18, 2021

View reviewed changes

components/Players/PlayerManager.vue Outdated Show resolved Hide resolved

components/Players/PlayerManager.vue Outdated Show resolved Hide resolved

camc314 force-pushed the mediasession branch from 63c953f to e60de8c Compare January 18, 2021 10:46

heyhippari reviewed Jan 18, 2021

View reviewed changes

camc314 force-pushed the mediasession branch 2 times, most recently from bd9a97e to 6477b1f Compare January 18, 2021 13:51

ferferga requested changes Jan 18, 2021

View reviewed changes

camc314 force-pushed the mediasession branch from 0de9c80 to b02f93d Compare January 18, 2021 20:41

ferferga requested changes Jan 18, 2021

View reviewed changes

ferferga requested changes Jan 21, 2021

View reviewed changes

camc314 force-pushed the mediasession branch from 02e2d45 to 2a980fc Compare February 4, 2021 10:07

ferferga force-pushed the mediasession branch from 2a980fc to bb67dd0 Compare February 4, 2021 11:12

ferferga requested changes Feb 4, 2021

View reviewed changes

camc314 added 3 commits February 4, 2021 11:30

feat(playback): add functions to handle mediasession inputs

1568fc7

feat(player manager): remove MediaHandlers on playback stop

b99dc4d

fix(mediasession): refresh metadata when going to the previous track

fbcebf6

camc314 force-pushed the mediasession branch from bb67dd0 to fbcebf6 Compare February 4, 2021 11:30

ferferga approved these changes Feb 4, 2021

View reviewed changes

ferferga merged commit d9e630e into master Feb 4, 2021

ferferga deleted the mediasession branch February 4, 2021 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(playback): add functions to handle mediasession inputs #595

feat(playback): add functions to handle mediasession inputs #595

camc314 commented Jan 18, 2021 •

edited

Loading

ferferga left a comment

camc314 commented Jan 18, 2021 •

edited

Loading

codecov-io commented Jan 18, 2021 •

edited

Loading

ferferga commented Jan 18, 2021

camc314 commented Jan 18, 2021

ferferga commented Jan 18, 2021

camc314 commented Jan 18, 2021

ferferga commented Jan 18, 2021

heyhippari commented Jan 18, 2021

heyhippari commented Jan 18, 2021

heyhippari Jan 18, 2021

ferferga Jan 18, 2021

camc314 Jan 18, 2021

ferferga Jan 18, 2021

ferferga Jan 18, 2021

ferferga left a comment

camc314 commented Jan 21, 2021

ferferga commented Jan 21, 2021 •

edited

Loading

ferferga left a comment

camc314 commented Feb 4, 2021 •

edited

Loading

ferferga commented Feb 4, 2021 •

edited

Loading

ferferga Feb 4, 2021

camc314 Feb 4, 2021 •

edited

Loading

ferferga Feb 4, 2021 •

edited

Loading

sonarqubecloud bot commented Feb 4, 2021

	console.log(`Error removing "${action}".`);
	console.log(`Error removing mediaSession action: "${action}".`);

feat(playback): add functions to handle mediasession inputs #595

feat(playback): add functions to handle mediasession inputs #595

Conversation

camc314 commented Jan 18, 2021 • edited Loading

ferferga left a comment

Choose a reason for hiding this comment

camc314 commented Jan 18, 2021 • edited Loading

codecov-io commented Jan 18, 2021 • edited Loading

Codecov Report

ferferga commented Jan 18, 2021

camc314 commented Jan 18, 2021

ferferga commented Jan 18, 2021

camc314 commented Jan 18, 2021

ferferga commented Jan 18, 2021

heyhippari commented Jan 18, 2021

heyhippari commented Jan 18, 2021

heyhippari Jan 18, 2021

Choose a reason for hiding this comment

ferferga Jan 18, 2021

Choose a reason for hiding this comment

camc314 Jan 18, 2021

Choose a reason for hiding this comment

ferferga Jan 18, 2021

Choose a reason for hiding this comment

ferferga Jan 18, 2021

Choose a reason for hiding this comment

ferferga left a comment

Choose a reason for hiding this comment

camc314 commented Jan 21, 2021

ferferga commented Jan 21, 2021 • edited Loading

Edit: Just checked and Spotify changed its behaviour (single item in queue):

ferferga left a comment

Choose a reason for hiding this comment

camc314 commented Feb 4, 2021 • edited Loading

ferferga commented Feb 4, 2021 • edited Loading

ferferga Feb 4, 2021

Choose a reason for hiding this comment

camc314 Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

ferferga Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 4, 2021

camc314 commented Jan 18, 2021 •

edited

Loading

camc314 commented Jan 18, 2021 •

edited

Loading

codecov-io commented Jan 18, 2021 •

edited

Loading

ferferga commented Jan 21, 2021 •

edited

Loading

camc314 commented Feb 4, 2021 •

edited

Loading

ferferga commented Feb 4, 2021 •

edited

Loading

camc314 Feb 4, 2021 •

edited

Loading

ferferga Feb 4, 2021 •

edited

Loading