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.

Sign up for GitHub

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

Jump to bottom

Display previous/next keyboard shortcuts #5759

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Display previous/next keyboard shortcuts #5759

merged 4 commits into from
Jul 16, 2024

Conversation

polyzen
Copy link
Contributor

@polyzen polyzen commented Jul 8, 2024

Changes
Document previous/next keyboard shortcuts in UI

Issues

@polyzen polyzen requested a review from a team as a code owner July 8, 2024 21:05
@thornbill thornbill added the enhancement Improve existing functionality or small fixes label Jul 11, 2024
dmitrylyzo
dmitrylyzo reviewed Jul 14, 2024
View reviewed changes
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

I'm not sure if we should add this to every button. But could you add shortcuts for the previous/next chapter buttons?

jellyfin-web/src/controllers/playback/video/index.js

Lines 1337 to 1342 in b681184

case 'PageUp':
playbackManager.nextChapter(currentPlayer);
break;
case 'PageDown':
playbackManager.previousChapter(currentPlayer);
break;

Also, I think it is fine to rebase this PR to release-10.9.z.

@polyzen polyzen changed the base branch from master to release-10.9.z July 14, 2024 21:59
@polyzen
Copy link
Contributor Author

polyzen commented Jul 14, 2024

I'm not sure if we should add this to every button. But could you add shortcuts for the previous/next chapter buttons?

jellyfin-web/src/controllers/playback/video/index.js

Lines 1337 to 1342 in b681184

case 'PageUp':
playbackManager.nextChapter(currentPlayer);
break;
case 'PageDown':
playbackManager.previousChapter(currentPlayer);
break;

Also, I think it is fine to rebase this PR to release-10.9.z.

Done

@polyzen polyzen changed the title Document previous/next keyboard shortcuts in UI Display previous/next keyboard shortcuts Jul 15, 2024
@dmitrylyzo dmitrylyzo added this to the v10.9.8 milestone Jul 15, 2024
@dmitrylyzo dmitrylyzo added the stable backport Backport into the next stable release label Jul 15, 2024
@dmitrylyzo
Copy link
Contributor

Another two:

jellyfin-web/src/controllers/playback/video/index.js

Line 499 in ed9cc5e

button.setAttribute('title', globalize.translate('ExitFullscreen') + ' (f)');

jellyfin-web/src/controllers/playback/video/index.js

Line 502 in ed9cc5e

button.setAttribute('title', globalize.translate('Fullscreen') + ' (f)');

We need to add upper case variants to:

jellyfin-web/src/controllers/playback/video/index.js

Line 1247 in ed9cc5e

case 'k':

jellyfin-web/src/controllers/playback/video/index.js

Line 1259 in ed9cc5e

case 'l':

jellyfin-web/src/controllers/playback/video/index.js

Line 1265 in ed9cc5e

case 'j':

jellyfin-web/src/controllers/playback/video/index.js

Line 1271 in ed9cc5e

case 'f':

jellyfin-web/src/controllers/playback/video/index.js

Line 1276 in ed9cc5e

case 'm':

like it is done for

jellyfin-web/src/controllers/playback/video/index.js

Lines 1279 to 1280 in ed9cc5e

case 'p':
case 'P':

Otherwise, listed shortcuts don't work with CapsLock enabled.
On YouTube, Shift and CapsLock have no effect on these actions.

@polyzen
Copy link
Contributor Author

polyzen commented Jul 16, 2024 •
edited
Loading

Another two:

jellyfin-web/src/controllers/playback/video/index.js

Line 499 in ed9cc5e

button.setAttribute('title', globalize.translate('ExitFullscreen') + ' (f)');

jellyfin-web/src/controllers/playback/video/index.js

Line 502 in ed9cc5e

button.setAttribute('title', globalize.translate('Fullscreen') + ' (f)');

We need to add upper case variants to:

jellyfin-web/src/controllers/playback/video/index.js

Line 1247 in ed9cc5e

case 'k':

https://github.com/jellyfin/jellyfin-web/b`lob/ed9cc5eb08df9f0b9bc57870589aa1c088b21a29/src/controllers/playback/video/index.js#L1259

jellyfin-web/src/controllers/playback/video/index.js

Line 1265 in ed9cc5e

case 'j':

jellyfin-web/src/controllers/playback/video/index.js

Line 1271 in ed9cc5e

case 'f':

jellyfin-web/src/controllers/playback/video/index.js

Line 1276 in ed9cc5e

case 'm':

like it is done for

jellyfin-web/src/controllers/playback/video/index.js

Lines 1279 to 1280 in ed9cc5e

case 'p':
case 'P':

Otherwise, listed shortcuts don't work with CapsLock enabled.
On YouTube, Shift and CapsLock have no effect on these actions.

I had noticed CapsLock didn't have an effect for N, which I had found odd.

Edit: ie. Shift is required, not just capital N. Though afterwards I recalled the conditionals for Shift.

@polyzen
Copy link
Contributor Author

polyzen commented Jul 16, 2024

Another two:

jellyfin-web/src/controllers/playback/video/index.js

Line 499 in ed9cc5e

button.setAttribute('title', globalize.translate('ExitFullscreen') + ' (f)');

jellyfin-web/src/controllers/playback/video/index.js

Line 502 in ed9cc5e

button.setAttribute('title', globalize.translate('Fullscreen') + ' (f)');

We need to add upper case variants to:

jellyfin-web/src/controllers/playback/video/index.js

Line 1247 in ed9cc5e

case 'k':

jellyfin-web/src/controllers/playback/video/index.js

Line 1259 in ed9cc5e

case 'l':

jellyfin-web/src/controllers/playback/video/index.js

Line 1265 in ed9cc5e

case 'j':

jellyfin-web/src/controllers/playback/video/index.js

Line 1271 in ed9cc5e

case 'f':

jellyfin-web/src/controllers/playback/video/index.js

Line 1276 in ed9cc5e

case 'm':

like it is done for

jellyfin-web/src/controllers/playback/video/index.js

Lines 1279 to 1280 in ed9cc5e

case 'p':
case 'P':

Otherwise, listed shortcuts don't work with CapsLock enabled.
On YouTube, Shift and CapsLock have no effect on these actions.

Done

@dmitrylyzo dmitrylyzo mentioned this pull request Jul 15, 2024
Merged
thornbill
thornbill reviewed Jul 16, 2024
View reviewed changes
Copy link
Member

@thornbill thornbill left a comment

Choose a reason for hiding this comment

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

We should probably add translations for special keys (Shift, PageDown, etc.) after this lands in master... We might also need to reverse the order for RTL languages if that isn't handled already somehow.

@thornbill thornbill merged commit 2d68f94 into jellyfin:release-10.9.z Jul 16, 2024
10 checks passed
@dmitrylyzo
Copy link
Contributor

We should probably add translations for special keys (Shift, PageDown, etc.) after this lands in master...

Perhaps only for some languages with special keyboards. At least in KDE they are not translated into Russian, i.e. as they are labeled on the keyboard: Alt, Ctrl, Shift, CapsLock, PgUp, PgDown, Meta. Although, ArrowLeft, ArrowRight are translated. 🤷‍♂️

@thornbill
Copy link
Member

That's good to know. I was told in chat that they are translated in Chinese but maybe we just wait to see if it's an issue for anyone or something they are accustomed to seeing. 👍

@polyzen polyzen deleted the patch-1 branch July 19, 2024 19:56
@jellyfin-bot jellyfin-bot removed the stable backport Backport into the next stable release label Jul 21, 2024
joshuaboniface pushed a commit that referenced this pull request Jul 21, 2024
@polyzen @thornbill
Backport pull request #5759 from jellyfin-web/release-10.9.z
f3d2372 
Display previous/next keyboard shortcuts

Original-merge: 2d68f94

Merged-by: thornbill <[email protected]>

Backported-by: Bill Thornton <[email protected]>
@polyzen polyzen mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thornbill thornbill thornbill left review comments

@dmitrylyzo dmitrylyzo dmitrylyzo approved these changes

Assignees
No one assigned
Labels
enhancement Improve existing functionality or small fixes
Projects
No open projects
Release 10.9.z
Status: Done
Milestone
v10.9.8
Development

Successfully merging this pull request may close these issues.
4 participants
@polyzen @dmitrylyzo @thornbill @jellyfin-bot