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

Make Cmd left/right go back/forward #6466

Merged
merged 5 commits into from
Jan 19, 2025
Merged

Make Cmd left/right go back/forward #6466

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aeffacb
* Make Cmd left/right go back/forward
PikachuEXE Dec 24, 2024
a32e332
! Fix wrong new shortcut
PikachuEXE Dec 29, 2024
98d8e39
* Allow shortcut label separator to be translated
PikachuEXE Dec 29, 2024
aa69384
* Show one action per shortcut action with N shortcuts
PikachuEXE Jan 2, 2025
7a81b13
* Remove duplicate entry, use array not map, remove unused locale ent…
PikachuEXE Jan 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ const KeyboardShortcuts = {
SHOW_SHORTCUTS: 'shift+?',
HISTORY_BACKWARD: 'alt+arrowleft',
HISTORY_FORWARD: 'alt+arrowright',
HISTORY_BACKWARD_ALT_MAC: 'cmd+arrowleft',
HISTORY_FORWARD_ALT_MAC: 'cmd+arrowright',
FULLSCREEN: 'f11',
NAVIGATE_TO_SETTINGS: 'ctrl+,',
NAVIGATE_TO_HISTORY: 'ctrl+H',
Expand Down
28 changes: 28 additions & 0 deletions src/main/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1702,6 +1702,20 @@ function runApp() {
},
type: 'normal',
},
...(process.platform === 'darwin'
? [
{
label: 'Back',
accelerator: 'Cmd+Left',
click: (_menuItem, browserWindow, _event) => {
if (browserWindow == null) { return }

browserWindow.webContents.navigationHistory.goBack()
},
visible: false,
},
]
: []),
{
label: 'Forward',
accelerator: 'Alt+Right',
Expand All @@ -1712,6 +1726,20 @@ function runApp() {
},
type: 'normal',
},
...(process.platform === 'darwin'
? [
{
label: 'Forward',
accelerator: 'Cmd+Right',
click: (_menuItem, browserWindow, _event) => {
if (browserWindow == null) { return }

browserWindow.webContents.navigationHistory.goForward()
},
visible: false,
},
]
: []),
]
},
{
Expand Down
8 changes: 8 additions & 0 deletions src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ const localizedShortcutNameDictionary = computed(() => {
['SHOW_SHORTCUTS', t('KeyboardShortcutPrompt.Show Keyboard Shortcuts')],
['HISTORY_BACKWARD', t('KeyboardShortcutPrompt.History Backward')],
['HISTORY_FORWARD', t('KeyboardShortcutPrompt.History Forward')],
...(
isMac
? [
['HISTORY_BACKWARD_ALT_MAC', t('KeyboardShortcutPrompt.History Backward (Mac only)')],
['HISTORY_FORWARD_ALT_MAC', t('KeyboardShortcutPrompt.History Forward (Mac only)')],
]
: []
),
Copy link
Collaborator

@kommunarr kommunarr Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@PikachuEXE PikachuEXE Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated
image

Also in PR description

['FULLSCREEN', t('KeyboardShortcutPrompt.Fullscreen')],
['NAVIGATE_TO_SETTINGS', t('KeyboardShortcutPrompt.Navigate to Settings')],
(
Expand Down
14 changes: 12 additions & 2 deletions src/renderer/components/top-nav/top-nav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,26 @@ export default defineComponent({
},

forwardText: function () {
const shortcuts = [KeyboardShortcuts.APP.GENERAL.HISTORY_FORWARD]
if (process.platform === 'darwin') {
shortcuts.push(KeyboardShortcuts.APP.GENERAL.HISTORY_FORWARD_ALT_MAC)
}

return localizeAndAddKeyboardShortcutToActionTitle(
this.$t('Forward'),
KeyboardShortcuts.APP.GENERAL.HISTORY_FORWARD
shortcuts,
) + this.navigationHistoryAddendum
},

backwardText: function () {
const shortcuts = [KeyboardShortcuts.APP.GENERAL.HISTORY_BACKWARD]
if (process.platform === 'darwin') {
shortcuts.push(KeyboardShortcuts.APP.GENERAL.HISTORY_BACKWARD_ALT_MAC)
}

return localizeAndAddKeyboardShortcutToActionTitle(
this.$t('Back'),
KeyboardShortcuts.APP.GENERAL.HISTORY_BACKWARD
shortcuts,
) + this.navigationHistoryAddendum
},

Expand Down
12 changes: 8 additions & 4 deletions src/renderer/helpers/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,10 +1021,14 @@ export function addKeyboardShortcutToActionTitle(actionTitle, shortcut) {

/**
* @param {string} localizedActionTitle
* @param {string} unlocalizedShortcut
* @param {string|string[]} sometimesManyUnlocalizedShortcuts
* @returns {string} the localized action title with keyboard shortcut
*/
export function localizeAndAddKeyboardShortcutToActionTitle(localizedActionTitle, unlocalizedShortcut) {
const localizedShortcut = getLocalizedShortcut(unlocalizedShortcut)
return addKeyboardShortcutToActionTitle(localizedActionTitle, localizedShortcut)
export function localizeAndAddKeyboardShortcutToActionTitle(localizedActionTitle, sometimesManyUnlocalizedShortcuts) {
let unlocalizedShortcuts = sometimesManyUnlocalizedShortcuts
if (!Array.isArray(sometimesManyUnlocalizedShortcuts)) {
unlocalizedShortcuts = [unlocalizedShortcuts]
}
const localizedShortcuts = unlocalizedShortcuts.map((s) => getLocalizedShortcut(s))
return addKeyboardShortcutToActionTitle(localizedActionTitle, localizedShortcuts.join('/'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

}
2 changes: 2 additions & 0 deletions static/locales/en-US.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,6 +1146,8 @@ KeyboardShortcutPrompt:
Show Keyboard Shortcuts: Show keyboard shortcuts
History Backward: Go back one page
History Forward: Go forward one page
History Backward (Mac only): Go back one page (Mac only)
History Forward (Mac only): Go forward one page (Mac only)
Copy link
Member

Choose a reason for hiding this comment

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

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

New Window: Create a new window
Navigate to Settings: Navigate to the Settings page
Navigate to History: Navigate to the History page
Expand Down
Loading