FreeTube icon indicating copy to clipboard operation
FreeTube copied to clipboard

Add keyboard shortcuts to titles

Open kommunarr opened this issue 1 year ago • 6 comments

Add keyboard shortcuts to titles

Pull Request Type

  • [ ] Bugfix
  • [X] Feature Implementation
  • [ ] Documentation
  • [ ] Other

Related issue

closes #1587

Description

  • Adds keyboard shortcut information to their button labels, e.g., Mute (m)
    • This includes the video player buttons, the feed refresh buttons, the side nav History/Settings titles, and the top nav HistoryForward/HistoryBackward/OpenNewWindow buttons, but NOT Electron context menu labels
  • Implementation note: To do this properly for the built-in Shaka controls that we don't modify in our code, this PR updates the base Shaka label localization values to include the shortcut whenever the Shaka locale is set.
  • Implementation note: Constants were added for the aforementioned keyboard shortcuts.

Screenshots

Screenshot_20241013_181816

Testing

  • Test that buttons with corresponding keyboard shortcuts have the shortcut referenced in the label, be they in "on" or "off" mode (e.g., Full screen (f) / Exit full screen (f)). See the official list of shortcuts for reference.

Desktop

  • OS: OpenSUSE
  • OS Version: TW

Additional context

The addition of the new constants should help make enhancements like #251 easier in the future.

kommunarr avatar Oct 13 '24 23:10 kommunarr

Label for captions image image

PikachuEXE avatar Oct 14 '24 05:10 PikachuEXE

Another good catch; I wasn't testing with a video that had a captions option. Just fixed the issue and added logic preventing label modification if the Shaka localization key does not exist. Also note that this specific Captions label will be the only one with another parenthetical present. This is fine IMO.

Screenshot_20241014_080659

kommunarr avatar Oct 14 '24 13:10 kommunarr

Added localization of shortcuts & multi-key shortcuts and caught a dangling instance of the wrong constant name for CAPTIONS

kommunarr avatar Oct 14 '24 18:10 kommunarr

Question: Navigating to the History tab and Settings tab also got shortcuts. Is there a reason those weren't added?

@efb4f5ff-1298-471a-8973-3d47447115dc I specifically added the shortcuts with corresponding button labels. The closest equivalent to those would be the corresponding side nav options, which I suppose would work. I can add those as well.

kommunarr avatar Oct 14 '24 19:10 kommunarr

Still on the fence about whether we should use the Mac key symbols for darwin (⌘/⇧/⌥/etc.)

Edit: did it. I used the version of the arrow icons that are used in Mac context menus, but it is worth noting that FF for Mac uses the more standard-looking ←→↑↓. I'm also open to using the arrow icons for non-Mac systems; I just went with the text version of those from looking at FF for Linux.

kommunarr avatar Oct 20 '24 20:10 kommunarr

Still on the fence about whether we should use the Mac key symbols for darwin (⌘/⇧/⌥/etc.)

Edit: did it. I used the version of the arrow icons that are used in Mac context menus, but it is worth noting that FF for Mac uses the more standard-looking ←→↑↓. I'm also open to using the arrow icons for non-Mac systems; I just went with the text version of those from looking at FF for Linux.

@PikachuEXE what do you think of the change in the latest commit

I think there is no need to use short form on labels The places I see short forms are app menu bar & keymap menus with limited spaces Even short forms should be used the arrows chars should be ←→↑↓ at least I haven't checked how this looks on Windows yet

Update 1: Checked windows version

  • MacOS only symbol: prefer text, probably fine with symbol too if someone else want it
  • Arrow: Just use text

Zen Browser image RubyMine image Vivaldi image

PikachuEXE avatar Oct 23 '24 00:10 PikachuEXE

I can change the arrow symbols. I'm not sure if we have other active contributors who are Mac users to provide additional opinions, but maybe if any users have any thoughts

kommunarr avatar Oct 23 '24 01:10 kommunarr

Updated the arrow icons for Mac:

Screenshot 2024-10-23 at 8 24 31 AM

kommunarr avatar Oct 23 '24 13:10 kommunarr

I preferred the stats button before this pull request a lot more as it matched the appearance of the other controls, now it looks out of place in the player.

Before:

before

After: after

Other controls: other-controls

absidue avatar Oct 28 '24 13:10 absidue

Ah, for the stats button, you're referring to the text content? I noticed that the labels we were setting for it weren't being used, so I thought that was an error. I can change that back if desired.

I can also look into removing the shortcuts from the overflow menu if that is desired as well. My only concern with that is that I'm not sure if we will be adding more shortcuts or moving more controls to a default overflow menu in the future.

kommunarr avatar Oct 28 '24 14:10 kommunarr

Sorry, I'm still a bit confused on what is being asked to be changed. Just fixing full window to have the shortcut show in the overflow menu? @absidue @efb4f5ff-1298-471a-8973-3d47447115dc

kommunarr avatar Oct 31 '24 16:10 kommunarr

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Nov 02 '24 10:11 github-actions[bot]

Just fixing full window to have the shortcut show in the overflow menu?

correct

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Nov 23 '24 14:11 github-actions[bot]