-
-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(player): Add media key control support #2001
base: master
Are you sure you want to change the base?
Conversation
endrl
commented
May 4, 2023
•
edited
Loading
edited
- Add support for media control keys
- Add volumeUp, volumeDown to playbackManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 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 |
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 (?) |
Multimedia Keys |
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. |
Added #2039 Volume changes should be shown by an "action overlay", but that's out of scope of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
whenever(keys.k, playbackManager.playPause); | ||
whenever(keys.m, playbackManager.toggleMute); | ||
|
||
const callHandler = (): void => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced it with another attempt. The blanket function is just here to prevent the undefined
check. Would you prefer the latter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
} | ||
}; | ||
|
||
const throttledForwardFn = useThrottleFn(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these are throttled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onKeyStroke are continous events. I wanted to throttle them to 10 events/second.
throttle, fullscreen improvements
@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 |
whenever(keys.j, backwardFn); | ||
whenever(keys.l, forwardFn); | ||
|
||
if (!isTizen() && !isWebOS() && fullscreen) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to skip these platforms? I'd like to have as less platform-specific stuff as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withv-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
whenever(keys.l, forwardFn); | ||
|
||
if (!isTizen() && !isWebOS() && fullscreen) { | ||
whenever(keys.ArrowUp, playbackManager.volumeUp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
acb6ecf
to
5d6a7c8
Compare
c360429
to
f3260d9
Compare
fe041a3
to
3fbf550
Compare