feat(player): Add media key control support #2001

endrl · 2023-05-04T15:29:37Z

Add support for media control keys
Add volumeUp, volumeDown to playbackManager

ferferga

I saw your comment about media keys in #1885, however I wanted to give you a response to the other stuff you brought up about TVs as well. I don't like people waiting too long for my feedback, but I'm super busy with exams so I'm addressing the smallest and oldest stuff first. But here's a sum up of our stance: We want to include everything we want, as long as there has been a reasonable demand for it, we see it can be easy to support (or we have some guarantee it will have some maintenance from the original contributor). For backwards compat stuff, if it's easy and reasonable like #1967, sure that's a good candidate too!

The question is should be "keyboard navigation" with auto focus and highlight a default feature which is always enabled? Or a build flag? Webos, tizen and any other client(*older) that might want to embed jellyfin-vue needs compatibility hacks anyway

EDIT: Wanted to give a short summary, but I think what I wrote was a good starting point too Moved this to a discussion: #2002

As for the contents of this PR itself: I don't understand why we need this? Since we already have media shortcuts and MediaSession support (I can switch tracks in all of my devices without issue. What's the purpose of those special keys then?

endrl · 2023-05-04T17:28:42Z

That's huge! But a good write up. I overlooked that implementation, but again it's chrome >=73 so may be moved as a fallback if (navigator.mediaSession) else USE_KEYS. But if i understand the idea this should be moved to a sub project (depends on how hard the line is, no one will touch this ever again. But these snippets blow up the code size).

Good luck!

Oh, can you give me hint why the navigation is not working? If i want to implement more admin stuff that would be useful

ferferga · 2023-05-04T18:01:30Z

But first of all: what are those keys for? Do you have any documentation for them (like in MDN?)

Because no harm in adding them alongside the others, they don't seem incompatible with MediaSession (?)

endrl · 2023-05-04T18:41:07Z

Multimedia Keys
I think they shouldn't be used together with media Session.

ferferga · 2023-05-04T19:49:22Z

I guess you have tested them in a desktop browser, right? If they work right, then I guess we have no issue.

Would you mind extracting them to a composable, so we can call it in both playback pages (music and video) and in the AudioControls?

We can go with the settings page later. I don't see much usefulness in an enable/disable toggle though.

endrl · 2023-06-11T23:53:37Z

Added #2039

Volume changes should be shown by an "action overlay", but that's out of scope of this PR.

ferferga

Fix SonarCloud's bugs please
Remove the changes to the type files, for some reason it looks like the linter run of those? Even though they're ignored.

ferferga · 2023-06-12T00:19:38Z

frontend/src/composables/use-playerkeys.ts

+  whenever(keys.k, playbackManager.playPause);
+  whenever(keys.m, playbackManager.toggleMute);
+
+  const callHandler = (): void => {


Why this is here? This should be in the fullscreen player page, it's not something that we need to share with the rest of the players.

I replaced it with another attempt. The blanket function is just here to prevent the undefined check. Would you prefer the latter?

Can you please clarify about what code lines we are talking? The current behaviour is that video/index || audio/index || AudioControls is mounted (but never at the same time). They all get the same key bindings and the video player has a callback fn to show the osd with a timeout when you press forward/backward.

@endrl My question was about why the OSD handler was passed as a callback here. I understood the purpose afterwards, hence why I deleted my comment, but you were fast :)

Sorry for the hassle.

ferferga · 2023-06-12T00:19:50Z

frontend/src/composables/use-playerkeys.ts

+    }
+  };
+
+  const throttledForwardFn = useThrottleFn(() => {


Why these are throttled?

onKeyStroke are continous events. I wanted to throttle them to 10 events/second.

throttle, fullscreen improvements

ferferga · 2023-06-12T17:08:01Z

@endrl Please test with my changes and let me know how it goes. I spun up a quick codespace session to test and still found no reason to have a throttle function there, it's not how Youtube or Netflix player (whose I'm using as reference) works. I also added my concerns about fully factorising the logic between the non-fullscreen and fullscreen players

ferferga · 2023-06-12T18:24:34Z

frontend/src/composables/use-playerkeys.ts

+  whenever(keys.j, backwardFn);
+  whenever(keys.l, forwardFn);
+
+  if (!isTizen() && !isWebOS() && fullscreen) {


Do we really need to skip these platforms? I'd like to have as less platform-specific stuff as possible

While thinking about it. up,down,left,right are always used for navigation except:

Use left, right to seek while player is in fullscreen

video: the osd will become visible and should focus the slider. Now you can further seek or navigate down to the control elements and use left/right without trigger seek)

audio: same as video. except that the osd is always visible (visualizer mode with (probably) hidden osd is around the corner which should behave the same)

Did if forgot something? So from code view:

Concept

Currently v-overlay unmounts the video osd so we can't do -> mount fullscreen video + osd and focus slider

Attempt: listen just for left/right when video is fullscreen and osd is unmounted (to get the osd rendered, now the osd can capture focus for slider)

or refactor v-overlay to a div with v-show to force focus and prevent the above "temporary listener hack"

Conclusion:
Depending on the code solution the guards are required as is or modified.

Out of topic: You are immediately at the key navigation . But you don't need a TV for that. Imagine you have just a keyboard with left/top/right/down. If we get that good implemented support for all kind of TV views is ready

endrl · 2023-06-13T08:45:07Z

frontend/src/composables/use-playerkeys.ts

+  whenever(keys.l, forwardFn);
+
+  if (!isTizen() && !isWebOS() && fullscreen) {
+    whenever(keys.ArrowUp, playbackManager.volumeUp);


You missed the point of onKeyStroke, you keep the btn pressed, which triggers endless events that needs to be throttled. Adds the feature "keep the button pressed to seek through audio/video or volume"

sonarcloud · 2023-06-15T10:26:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

ferferga · 2023-06-15T10:31:08Z

I see your point now. I think for WCAG compliance and (precisely) a consistent behaviour across platforms, we should focus the the relevant sliders programatically. Once those sliders are focused, we can use CSS's selector focus-within to toggle the overlay (in general I think we can refactor the overlay to be 100% CSS-based) while any of the content is focused. Does that sound good to you @endrl?

Labeling as blocked because vuetifyjs/vuetify#17602

jellyfin-bot · 2023-06-15T10:44:15Z

Cloudflare Pages deployment

Latest commit	`510a51f`
Status	✅ Deployed!
Preview URL	https://8534c923.jf-vue.pages.dev
Type	🔀 Preview

View build logs
View bot logs

endrl · 2023-06-15T11:02:26Z

Sounds interesting. So osdHandler would focus something and after timeout "unfocus".

Actually i am not sure if we should keep v-slider. From a future perspective it might be too limited when it comes to plugins and more features

mouse hover won't show the new position (you need to click on that (pressed))
- Jellyscrub to provide preview images just while you hover
The html player element provides a buffer state. We can't show it.
chapter marker?
hook other plugins to show "whatever" on the slider

This is a lot to mess around, in worst case you overwrite private api and styling which we don't want (stability)

ferferga · 2023-06-15T11:35:56Z

@endrl See this: https://github.com/jellyfin/jellyfin-vue/projects/4#card-89536907

This situation is exactly another motivation to start looking into the implementation details of Quasar as early as possible. Probably, once it gets into the client, the sliders are the first component that will get ported over (exactly for the limitation that's blocking this PR and your reasons).

See https://quasar.dev/vue-components/slider too

endrl · 2023-06-15T11:51:05Z

I know Quasar since version 0.17(!) It had already at that point an amazing documentation and lot's of component apis to work with. With that background i am sometimes annoyed of vuetify missing features and limited docs.

ferferga · 2023-06-15T12:38:51Z

@endrl You'll be very happy soon then :)

If you have prior experience with it, I want to invite you to contribute here! Though it might take some time until I can review them properly and get merged, since there are other PRs and stuff waiting first (more details in the project board I linked before).

The most self-contained and small the PRs are, the easiest that it is to review them and move along faster :)

ferferga · 2024-03-17T15:58:28Z

@endrl I added all the refactorization done in this PR to extract the common logic to a composable in #2264, as I was refactoring playerElement and I needed to apply the watchers to the page components themselves instead of the store. I added you as a co-author in the relevant commit.

Given this PR also included the extra logic for focusing the slider (and the issue it's not fixed upstream), I think this PR it's still relevant, but can you bring it up to date with current master so later on it can be picked up more easily? Do you have any interest in helping in the Vuetify removal of the client and create our new slider component?

jellyfin-bot added this to In progress in Ongoing development May 4, 2023

jellyfin-bot added the vue Pull requests that edit or add Vue files label May 4, 2023

ferferga requested changes May 4, 2023

View reviewed changes

jellyfin-bot added the merge conflict Something has merge conflicts label May 4, 2023

ferferga mentioned this pull request May 17, 2023

feat: Reworked video keyboard keys and fix osd visual #2013

Open

endrl force-pushed the mediakeys branch from f72a5da to 395c1e8 Compare May 20, 2023 22:01

jellyfin-bot removed the merge conflict Something has merge conflicts label May 20, 2023

endrl marked this pull request as ready for review May 20, 2023 22:01

endrl force-pushed the mediakeys branch from 395c1e8 to 910c258 Compare May 20, 2023 23:09

ferferga force-pushed the master branch from 2a1743e to a22eecd Compare June 1, 2023 12:29

endrl force-pushed the mediakeys branch from 910c258 to 7a7a14f Compare June 11, 2023 23:45

jellyfin-bot added the merge conflict Something has merge conflicts label Jun 11, 2023

endrl force-pushed the mediakeys branch from 7a7a14f to 49128ee Compare June 12, 2023 00:10

jellyfin-bot removed the merge conflict Something has merge conflicts label Jun 12, 2023

ferferga requested changes Jun 12, 2023

View reviewed changes

endrl force-pushed the mediakeys branch from 49128ee to 2339ff6 Compare June 12, 2023 13:03

endrl and others added 2 commits June 12, 2023 16:54

feat(player): implement keyboard keys

d57e40d

refactor: replace with noop function

a5ddde8

ferferga force-pushed the mediakeys branch from 7003649 to 868eb1c Compare June 12, 2023 16:54

ferferga added 2 commits June 12, 2023 17:05

refactor: useMagicKeys only, remove unnecessary

e540643

throttle, fullscreen improvements

refactor: remove unnecessary checks since covered already by the setters

f4d07b9

ferferga force-pushed the mediakeys branch from 868eb1c to f4d07b9 Compare June 12, 2023 17:06

ferferga reviewed Jun 12, 2023

View reviewed changes

endrl commented Jun 13, 2023

View reviewed changes

ferferga added 3 commits June 13, 2023 21:19

refactor: check playerElement instead

78668b6

refactor: focus sliders programatically

8128fdc

feat: reorder logic

c8eccfe

ferferga force-pushed the mediakeys branch from d7cd817 to c8eccfe Compare June 15, 2023 10:25

ferferga added the blocked Something depends on another issue or Pull Request label Jun 15, 2023

jellyfin-bot added the merge conflict Something has merge conflicts label Jun 19, 2023

ferferga force-pushed the master branch 2 times, most recently from bc856ac to 7336bfd Compare August 16, 2023 19:17

ferferga force-pushed the master branch from 75811cc to ad19e26 Compare December 16, 2023 15:48

ferferga force-pushed the master branch 2 times, most recently from f598745 to 00c2c75 Compare December 31, 2023 18:39

ferferga force-pushed the master branch from d4dbd9c to ff9f91e Compare January 10, 2024 11:27

ferferga mentioned this pull request Mar 17, 2024

refactor&fix: store syncing, connection and fixes for playback and stuck loading in login pages #2264

Merged

ferferga force-pushed the master branch 8 times, most recently from 937e458 to 6c210e6 Compare April 16, 2024 08:57

ferferga force-pushed the master branch from e907a77 to 3264021 Compare May 8, 2024 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(player): Add media key control support #2001

feat(player): Add media key control support #2001

endrl commented May 4, 2023 •

edited

ferferga left a comment •

edited

endrl commented May 4, 2023 •

edited

ferferga commented May 4, 2023

endrl commented May 4, 2023

ferferga commented May 4, 2023

endrl commented Jun 11, 2023

ferferga left a comment

ferferga Jun 12, 2023

endrl Jun 12, 2023

endrl Jun 12, 2023

ferferga Jun 12, 2023

ferferga Jun 12, 2023

endrl Jun 12, 2023

ferferga commented Jun 12, 2023

ferferga Jun 12, 2023

endrl Jun 13, 2023 •

edited

endrl Jun 13, 2023 •

edited

sonarcloud bot commented Jun 15, 2023

ferferga commented Jun 15, 2023

jellyfin-bot commented Jun 15, 2023

endrl commented Jun 15, 2023

ferferga commented Jun 15, 2023

endrl commented Jun 15, 2023

ferferga commented Jun 15, 2023

ferferga commented Mar 17, 2024

feat(player): Add media key control support #2001

Are you sure you want to change the base?

feat(player): Add media key control support #2001

Conversation

endrl commented May 4, 2023 • edited

ferferga left a comment • edited

Choose a reason for hiding this comment

endrl commented May 4, 2023 • edited

ferferga commented May 4, 2023

endrl commented May 4, 2023

ferferga commented May 4, 2023

endrl commented Jun 11, 2023

ferferga left a comment

Choose a reason for hiding this comment

ferferga Jun 12, 2023

Choose a reason for hiding this comment

endrl Jun 12, 2023

Choose a reason for hiding this comment

endrl Jun 12, 2023

Choose a reason for hiding this comment

ferferga Jun 12, 2023

Choose a reason for hiding this comment

ferferga Jun 12, 2023

Choose a reason for hiding this comment

endrl Jun 12, 2023

Choose a reason for hiding this comment

ferferga commented Jun 12, 2023

ferferga Jun 12, 2023

Choose a reason for hiding this comment

endrl Jun 13, 2023 • edited

Choose a reason for hiding this comment

endrl Jun 13, 2023 • edited

Choose a reason for hiding this comment

sonarcloud bot commented Jun 15, 2023

ferferga commented Jun 15, 2023

jellyfin-bot commented Jun 15, 2023

Cloudflare Pages deployment

endrl commented Jun 15, 2023

ferferga commented Jun 15, 2023

endrl commented Jun 15, 2023

ferferga commented Jun 15, 2023

ferferga commented Mar 17, 2024

endrl commented May 4, 2023 •

edited

ferferga left a comment •

edited

endrl commented May 4, 2023 •

edited

endrl Jun 13, 2023 •

edited

endrl Jun 13, 2023 •

edited