feat: extend shortcuts to support generic keys #4685
Conversation
Some shortcuts are "generic" like Cmd + N for new chat, Cmd + F for search. This PR tries to keep that behaviour in DC Desktop by reacting to both, the "key" and the code (in case the key is not supported like in non latin layouts)
There was a problem hiding this comment.
This approach feels wrong. In practice this sometimes adds two shortcuts to execute one action, depending on keyboard layout.
I still think we need to look up a library or something to solve the problem.
But I guess as long as this MR only affects shortcuts that involve the Ctrl key, it's not likely to break things badly. Especially that we already used to rely on event.key in the past, prior to #4140. So this MR is probably gonna be an improvement overall.
Please see if you'd like to address my code comments. Otherwise approved.
| // this would be the approach to show users how to access | ||
| // certain keyboard codes according to their layout |
There was a problem hiding this comment.
"Would be", but why it's not the case already? Hard to understand without context,
There was a problem hiding this comment.
This was just a comment related to the discussion we had before (in chat). I will remove it before merging
| @@ -94,15 +94,15 @@ export function keyDownEvent2Action( | |||
| // } else if (ev.altKey && ev.code === 'ArrowLeft') { | |||
| // disabled until we find a better keycombination (see https://github.com/deltachat/deltachat-desktop/issues/1796) | |||
| // return KeybindAction.ChatList_ScrollToSelectedChat | |||
| } else if (ev.ctrlKey && ev.code === 'KeyF') { | |||
| } else if (ev.ctrlKey && (ev.key === 'f' || ev.code === 'KeyF')) { | |||
There was a problem hiding this comment.
Would be nice to add comments. To an untrained eye this can look super weird.
|
Also, why #skip-changelog? It's not an insignificant change. But I won't insist. |
There was a problem hiding this comment.
Makes sense. And as @nicodh said, we can switch to something more elaborate later if needed. This makes things better already right?
| } else if (ev.ctrlKey && (ev.key === 'f' || ev.code === 'KeyF')) { | ||
| // https://github.com/deltachat/deltachat-desktop/issues/4579 | ||
| if (ev.shiftKey) { | ||
| return KeybindAction.ChatList_SearchInChat | ||
| } | ||
| return KeybindAction.ChatList_FocusSearchInput | ||
| } else if (ev.ctrlKey && ev.code === 'KeyN') { | ||
| } else if (ev.ctrlKey && (ev.key === 'n' || ev.code === 'KeyN')) { | ||
| return KeybindAction.NewChat_Open | ||
| } else if (ev.ctrlKey && ev.code === 'KeyM') { | ||
| } else if (ev.ctrlKey && (ev.key === 'm' || ev.code === 'KeyM')) { |
There was a problem hiding this comment.
Note that with this PR, if there was a layout where 'n' and 'f' have places switched, then pressing either key executes the Search action, and there is no way to execute the Composer_Focus action.
As I could find out, there is no keyboard where just the three keybindings we have so far make problems, but if you add more keybindings later then at some point there will be problems.
I think it would be better to do:
if key == 'f' {
Search
} else if key == 'n' {
NewChat
} else if key == 'm' {
Composer_Focus
} else if code == 'KeyF' {
Search
} else if code == 'KeyN' {
NewChat
} else if code == 'KeyM' {
Composer_Focus
}
Only for people that have a "weird" layout, where the key on the hardware is not the key on the keywdown event. |
I'm afraid not. On the Russian layout there are now two shortcuts for each "Open Settings" and "Open Shortcuts dialog". |
So what's the problem? Does it collide with another shortcut? |
|
It doesn't, in the case of the Russian layout. Just saying that there is probably a more correct way to do this. |
Some shortcuts are "generic" like Cmd + N for new chat, Cmd + F for search. This PR tries to keep that behaviour in DC Desktop by reacting to both, the "key" and the code (in case the key is not supported like in non latin layouts)
If this is not enough we may hav to implement a more allaborated approach like mentioned here:
https://www.kravchyk.com/keyboard-shortcuts-on-non-latin-alphabet-layouts/
using
https://github.com/mckravchyk/shocut
#skip-changelog