Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

endrl
Copy link
Contributor

@endrl endrl commented May 4, 2023

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

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

@ferferga ferferga left a 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?

@jellyfin-bot jellyfin-bot added the merge conflict Something has merge conflicts label May 4, 2023
@endrl
Copy link
Contributor Author

endrl commented May 4, 2023

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
Copy link
Member

ferferga commented May 4, 2023

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
Copy link
Contributor Author

endrl commented May 4, 2023

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

@ferferga
Copy link
Member

ferferga commented May 4, 2023

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
Copy link
Contributor Author

endrl commented Jun 11, 2023

Added #2039

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

@jellyfin-bot jellyfin-bot removed the merge conflict Something has merge conflicts label Jun 12, 2023
Copy link
Member

@ferferga ferferga left a 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 => {
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Member

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(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why these are throttled?

Copy link
Contributor Author

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.

@ferferga
Copy link
Member

@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) {
Copy link
Member

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

Copy link
Contributor Author

@endrl endrl Jun 13, 2023

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 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

whenever(keys.l, forwardFn);

if (!isTizen() && !isWebOS() && fullscreen) {
whenever(keys.ArrowUp, playbackManager.volumeUp);
Copy link
Contributor Author

@endrl endrl Jun 13, 2023

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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something depends on another issue or Pull Request merge conflict Something has merge conflicts vue Pull requests that edit or add Vue files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants