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

Keybind C to auto select subtitle track #6121

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

Conversation

BakerAO
Copy link

@BakerAO BakerAO commented Sep 26, 2024

Keybind C to auto select subtitle track

Changes
Add c case to onKeyDown. The function selects the first available subtitle track, or if subtitles are already selected, disables them.

Issues
Feature Request: https://features.jellyfin.org/posts/1245/enable-disable-subtitles-using-a-keyboard-shortcut

@BakerAO BakerAO requested a review from a team as a code owner September 26, 2024 19:29
@viown
Copy link
Member

viown commented Sep 27, 2024

I'm not sure if the user always expects the first subtitle track to be selected as they may have different subtitle preferences.

Maybe you can use currentPlayer.streamInfo.mediaSource.DefaultSubtitleStreamIndex and fallback to streams[0].Index if it's -1. Or, alternatively, something like DefaultSubtitleStreamIndex -> Subtitle Track with 'default' flag on -> streams[0].Index.

You could also take advantage of the user's subtitle language preference, but that may be overcomplicating things as DefaultSubtitleStreamIndex should already point to the 'best' track unless it was manually changed or turned off.

You can also avoid guessing altogether if this feature instead allowed you to cycle through the subtitle tracks by repeatedly pressing C, eventually cycling back to off. Though you'd need a visual message showing which track it's currently on. This is, IMO, most preferable.

@BakerAO
Copy link
Author

BakerAO commented Sep 27, 2024

Thank you for the feedback, that makes sense. I like the idea of using DefaultSubtitleStreamIndex. I imagine most users don't want to cycle through the options (like English to French to Italian, etc), typically they just want them on or off in their language, but that would work as well.

@Shadowghost
Copy link
Contributor

allowed you to cycle through the subtitle tracks by repeatedly pressing C

This might be a very bad user experience in case we are transcoding and burning in subs beause it will always restart the transcode

@viown
Copy link
Member

viown commented Sep 28, 2024

Good point, I didn't consider that.

@BakerAO
Copy link
Author

BakerAO commented Sep 30, 2024

@viown and @Shadowghost, updated to use the default index if its set

Co-authored-by: viown <48097677+viown@users.noreply.github.com>
@BakerAO
Copy link
Author

BakerAO commented Sep 30, 2024

Commited your change @viown, that made sense

@BakerAO BakerAO requested a review from viown September 30, 2024 18:46
Co-authored-by: viown <48097677+viown@users.noreply.github.com>
@BakerAO BakerAO requested a review from viown September 30, 2024 19:49
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Oct 2, 2024

Cloudflare Pages deployment

Latest commit 122c395
Status ✅ Deployed!
Preview URL https://2541d8ba.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link

sonarcloud bot commented Nov 12, 2024

@BakerAO
Copy link
Author

BakerAO commented Nov 12, 2024

@viown does this PR still look good to you? let me know if you want any changes

@viown
Copy link
Member

viown commented Nov 12, 2024

Code-wise, it looks fine to me and it works as expected in my testing. But it would have to be approved by thornbill or another core team member in order to decide if it's a desired feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants