Make Cmd left/right go back/forward by PikachuEXE · Pull Request #6466 · FreeTubeApp/FreeTube

PikachuEXE · 2024-12-28T02:00:52Z

Pull Request Type

Bugfix
Feature Implementation
Documentation
Other

Related issue

Closes #5966

Description

For MacOS only, make Cmd left/right as an additional keyboard shortcut to go back/forward in history

Screenshots

Old, outdated

Testing

Ensure existing back/forward shortcuts working (All OS) (Alt Left/Right)
Ensure new back/forward shortcuts working (macOS only) (Cmd Left/Right)
Ensure tooltip on back/forward buttons unchanged (non macOS only)
Ensure tooltip on back/forward buttons updated (macOS only)
Open keyboard modal, ensure shortcuts unchanged (non macOS only)
Open keyboard modal, ensure shortcut list updated (MacOS only)

Desktop

OS: macOS
OS Version:
FreeTube version:

Additional context

kommunarr · 2024-12-28T15:57:33Z

src/renderer/helpers/utils.js

+    unlocalizedShortcuts = [unlocalizedShortcuts]
+  }
+  const localizedShortcuts = unlocalizedShortcuts.map((s) => getLocalizedShortcut(s))
+  return addKeyboardShortcutToActionTitle(localizedActionTitle, localizedShortcuts.join('/'))


suggestion (blocking): I think we should localize this like we do here for the plus sign. This is the best quick source I could find, but I think it's apparent enough that some languages do this differently:

American English frequently uses the slash symbol to represent the idea of “or” or “and.” To minimize ambiguity in your source English text for translation, it is recommendable to actually use the words “or” or “and.”

const shortcutJoinOperator = i18n.t('shortcutJoinOperator') return localizedShortcuts.join(shortcutJoinOperator)

kommunarr · 2024-12-28T16:08:33Z

src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue

+    ...(
+      isMac
+        ? [
+            ['HISTORY_BACKWARD_ALT_MAC', t('KeyboardShortcutPrompt.History Backward (Mac only)')],
+            ['HISTORY_FORWARD_ALT_MAC', t('KeyboardShortcutPrompt.History Forward (Mac only)')],
+          ]
+        : []
+    ),


suggestion (non-blocking): I'm curious if showing a non-monospace slash character joining the different alts would be more elegant than separate entries for alts. I was thinking about storing alts by storing the shortcut constant as A|B, which is then parsed into A<span>/</span>B, where we can then apply a separate styling rule (no monospace) to the nested spans. But that would need some special logic in the caller as well for Mac-only additional shortcut cases like this one, or a separate character for representing non-Mac and Mac-exclusive shortcut alts (e.g., ┛A|┗B|C|D for Windows/Mac-specific alt A, Mac-specific alt B, and universal alts C and D).

Way too complicated to understand
Left for another PR at least -_-

Sounds good - this will currently be the only action with two displayed shortcuts we have in the modal (I excluded the dupe alt shortcuts in the original PR), so I suggest we move to a more elegant solution when we want to add more of them

As this extra shortcut is only displayed to MacOS users, do we really need to specify in the text that it is macOS only? It also uses the command key which doesn't exist on Windows or Linux, so it seems unlikely that someone would try to use it on Windows or Linux anyway if they happen to use macOS + Windows or Linux.

The best way is to have 1 action to N shortcut(s)
Not sure how to do that yet...
Let me take a look ~_~

Can't you just have two mappings that use the same title, the Map uses the shortcuts as the key, so it should work? That way it'll still be displayed as two separate lines/rows but with different shortcuts.

Updated

Also in PR description

absidue · 2024-12-28T17:05:33Z

This pull request doesn't actually close the linked issue, this adds Command+Arrow Left and Command+Arrow Right but the feature request is asking for Cmd+[ and Cmd+].

Would you prefer to unlink the issue so that it stays open for someone else to implement in the future or change the code to use the keyboard shortcuts that that person asked for?

Edit: According to the Apple Support link that was provided in the issue:

Command–Left Bracket ([): Go to the previous folder.
Command–Right Bracket (]): Go to the next folder.

PikachuEXE · 2024-12-29T00:54:19Z

@absidue Yup my Vivaldi uses Cmd+arrow key so I got it wrong. Fixed now

absidue

Would hardcoding a wider separator be an option, just the slash without any whitespace makes it difficult to easily tell that they are two separate shortcuts.

absidue · 2025-01-03T17:18:00Z

static/locales/en-US.yaml

+  History Backward (Mac only): Go back one page (Mac only)
+  History Forward (Mac only): Go forward one page (Mac only)


As these new strings are no longer used please remove them.

absidue

As we now just use it as a list and don't actually use it for the look ups, we can just return an array of arrays.

absidue · 2025-01-03T17:27:25Z

src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue


-const localizedShortcutNameDictionary = computed(() => {
+const localizedShortcutNameToShortcutsMap = computed(() => {
  return new Map([


Suggested change

return new Map([

return [

absidue · 2025-01-03T17:27:34Z

src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue

+    [t('KeyboardShortcutPrompt.Next Chapter'), ['NEXT_CHAPTER']],
+    [t('KeyboardShortcutPrompt.Last Frame'), ['LAST_FRAME']],
+    [t('KeyboardShortcutPrompt.Next Frame'), ['NEXT_FRAME']],
  ])


Suggested change

])

]

absidue · 2025-01-03T17:27:47Z

src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue

+  const shortcutNameToShortcutsMap = localizedShortcutNameToShortcutsMap.value
+  const shortcutLabelSeparator = t('shortcutLabelSeparator')
+
+  return shortcutNameToShortcutsMap.entries()


Suggested change

return shortcutNameToShortcutsMap.entries()

return shortcutNameToShortcutsMap

PikachuEXE · 2025-01-04T02:18:04Z

If using /

If using ｜ (https://www.amp-what.com/unicode/search/vertical%20line

Going for later assuming that's good enough

…ries, update shortcut separator

absidue

I don't have a mac so I wasn't able to test this properly but the code looks like it should work and the keyboard shortcuts prompt seems to work the same as it did before on Windows.

* * Make Cmd left/right go back/forward * ! Fix wrong new shortcut * * Allow shortcut label separator to be translated * * Show one action per shortcut action with N shortcuts * * Remove duplicate entry, use array not map, remove unused locale entries, update shortcut separator

* Make Cmd left/right go back/forward

aeffacb

PikachuEXE requested review from ChunkyProgrammer, absidue, efb4f5ff-1298-471a-8973-3d47447115dc and kommunarr December 28, 2024 02:00

FreeTubeBot enabled auto-merge (squash) December 28, 2024 02:01

github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 28, 2024

kommunarr reviewed Dec 28, 2024

View reviewed changes

efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 28, 2024

PikachuEXE added 2 commits December 29, 2024 08:51

! Fix wrong new shortcut

a32e332

* Allow shortcut label separator to be translated

98d8e39

kommunarr previously approved these changes Dec 29, 2024

View reviewed changes

PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jan 2, 2025

* Show one action per shortcut action with N shortcuts

aa69384

PikachuEXE dismissed kommunarr’s stale review via aa69384 January 2, 2025 22:47

absidue reviewed Jan 3, 2025

View reviewed changes

* Remove duplicate entry, use array not map, remove unused locale ent…

7a81b13

…ries, update shortcut separator

absidue approved these changes Jan 14, 2025

View reviewed changes

PikachuEXE requested a review from kommunarr January 15, 2025 00:11

kommunarr approved these changes Jan 15, 2025

View reviewed changes

efb4f5ff-1298-471a-8973-3d47447115dc approved these changes Jan 19, 2025

View reviewed changes

FreeTubeBot merged commit e8d2624 into FreeTubeApp:development Jan 19, 2025

github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 19, 2025

		History Backward (Mac only): Go back one page (Mac only)
		History Forward (Mac only): Go forward one page (Mac only)

	return shortcutNameToShortcutsMap.entries()
	return shortcutNameToShortcutsMap

Conversation

PikachuEXE commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Type

Related issue

Description

Screenshots

Testing

Desktop

Additional context

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kommunarr Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PikachuEXE Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

absidue commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PikachuEXE commented Dec 29, 2024

Uh oh!

absidue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

absidue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PikachuEXE commented Jan 4, 2025

Uh oh!

absidue left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PikachuEXE commented Dec 28, 2024 •

edited

Loading

kommunarr Dec 28, 2024 •

edited

Loading

PikachuEXE Jan 2, 2025 •

edited

Loading

absidue commented Dec 28, 2024 •

edited

Loading