feat(frontend/settings): add customisable hotkeys

[Harjot1Singh]

Subject

Adds customisable hotkeys

Changes

I found the thread below hard to work against, so I've consolidated it here:

• Prevent unusable hotkeys from being assigned
• Reserve "highly safe" hotkeys
• Inform user of invalid recorded hotkey
• Investigate hotkeys when info modal is open
• Fix being able to assign reserved hotkeys
• Disable shift-f or even any shift-solo keys
• Zoom in and Zoom out
• Fullscreen controller toggle does not check whether toggle controller is toggled
• Prevent restricted keystrokes ofsubsequences anywhere
• Prevent reuse of subsequences from beginning

Linked issues

Closes #176, Closes #426

Before

N/A

After

Tests

• Shift + any key as a modifier => deliberately ignores
• Modifiers only => deliberately ignores
• Maps to cmd on Mac
• Required hotkeys are not removable or corrupted

TTR

At least 30 hours? Most time spent was not due to complexity of feature, but resolving 3rd party lib bugs.

Reviewers

@bhajneet

[bhajneet]

I cannot use the following hotkey combos:

• ctrl w (closes window)
• ctrl r (refreshes window)
• ctrl m (minimizes window)

The following can be used but are already hotkeys:

• ctrl a (selects all text)
• ctrl - (minus text size)
• ctrl shift = (plus text size)
• ctrl 0 (resets zoom)

The following are reserved/taken, but don't work as intended perhaps:

• ctrl q (does not quit app in settings window, does work in other windows -- btw this is a bit too easy to hit, no? perhaps it should be ctrl shift q)
• ctrl g (goes to search when there is no history/navigator available, instead it should do nothing, reproduce by going to bookmarks and typing ctrl g on fresh open of shabad os)
• ctrl , and ctrl p open new settings windows. I'm not sure there is a use case for multiple settings windows as there is for multiple controllers

The following are hotkeys I don't think need to be set:

• ctrl shift x (duplicate of ctrl x)
• ctrl shift h (duplicate of ctrl h)

I'm guessing some of these are because of dev. In the off chance they are not: Reserve all the hotkeys in first two lists and don't allow deletion of them or for them to be assigned elsewhere.

I cannot test mac, but I'm curious how some of these work with their hotkeys. I think cmd h is used to hide windows, what do we do to hide the controller vs hide the app on macOS?

EDIT:

Also note I can set some of these hotkeys as parts of sequence:

(This did not toggle fullscreen)

[bhajneet]

Hotkey recording broken after shift+f (no actual hotkey after that is recorded). Better would be to tell the user yes, you typed shift+f but we don't allow that. ("Taken by reserved hotkeys list" or something).

[bhajneet]

Lol:

This sequence actually worked, however it also triggered all the "jump to" hotkeys in the navigator for longer shabads...

Perhaps all sequences should require safe modifiers? Ctrl+C Ctrl+D Ctrl+K etc. Note this sequence also takes me to history and copies it from there:

[bhajneet]

Reserve highly considered "safe" hotkeys like up and down arrow keys, space for autoselect, enter / return to select line. These are very critical to reserve. I would argue reserving b, page up, page down, esc, ctrl+shift+b are also key for using presenter/projector remotes.

Though I'm impressed this didn't break search much:

[bhajneet]

Ctrl+X, Ctrl+H shows blank screen even if line is activated. Clicking on blank area will advance line. I think there might be an out-of-scope issue open for this. Need to check before creating an issue out of this comment.

[bhajneet]

Side note, I cannot copy with the pop over, but the hotkey works for all lines haha:

[bhajneet]

The second one will not work after the first one:

Ctrl Shift A is broken. Until I unfocus and refocus entire electron window. Happens with other Ctrl Shift hotkeys as well.

This doesn't happen with ctrl+shift+b for esc (reserved)

[bhajneet]

I shouldn't be allowed to re-use a reserved hotkey... Wonder what will happen!

(It just escapes the screen, it doesn't quit the app haha)

[saihaj]

Side note, I cannot copy with the pop over, but the hotkey works for all lines haha:

That is because it is a Bani and #500 will make it happen

[AkalUstat]

btw @Harjot1Singh react-hotkeys has problems with hotkeys that begin with alt/option

source: my work on sttm-web shortcuts

[Harjot1Singh]

I cannot use the following hotkey combos:

• ctrl w (closes window)
• ctrl r (refreshes window)
• ctrl m (minimizes window)

The following can be used but are already hotkeys:

• ctrl a (selects all text)
• ctrl - (minus text size)
• ctrl shift = (plus text size)
• ctrl 0 (resets zoom)

These all are dev-only, except ctrl-a. Would you like to disable these anyway, since they apply in the browser?

ctrl shift h (duplicate of ctrl h)

This exists purely because in-browser, ctrl-h => history.

[Harjot1Singh]

Ctrl+X, Ctrl+H shows blank screen even if line is activated. Clicking on blank area will advance line. I think there might be an out-of-scope issue open for this. Need to check before creating an issue out of this comment.

Not sure I can see what's going on here, appears to work for me.

[bhajneet]

...
These all are dev-only, except ctrl-a. Would you like to disable these anyway, since they apply in the browser?

I think it would be nice to use zoom in and out in prod as well, have had a couple people ask about changing font sizes for navigator

ctrl shift h (duplicate of ctrl h)

This exists purely because in-browser, ctrl-h => history.

Okay, no worries on this

[bhajneet]

Ctrl+X, Ctrl+H shows blank screen even if line is activated. Clicking on blank area will advance line. I think there might be an out-of-scope issue open for this. Need to check before creating an issue out of this comment.

Not sure I can see what's going on here, appears to work for me.

See here for example (notice the background)

[Harjot1Singh]

Side note, I cannot copy with the pop over, but the hotkey works for all lines haha:

Seems to work for me btw, unless I misunderstood how to reproduce.

[saihaj]

Seems to work for me btw, unless I misunderstood how to reproduce.

I think he meant that you can copy with hotkeys but you cannot press the copy button in popover. Don’t worry about this (we have separate issue in tracker)

[lgtm-com]

This pull request introduces 2 alerts when merging 3dca681 into 792fc77 - view on LGTM.com

new alerts:

• 1 for Duplicate property
• 1 for Unused variable, import, function or class

[bhajneet]

Prevent unusable hotkeys from being assigned

This is still incorrect. Go to the "open a new controller" hotkey, and assign a new one as Ctrl+F and it will say this is already assigned. However, because you can combo hotkeys, I can then do Ctrl+C (total hotkey chord becomes Ctrl+F, Ctrl+C) and this is allowed. Now in usage, if I do Ctrl+F it will fullscreen the window and then if I do Ctrl+C afterwards it opens a new controller.

This also causes lots of issues:

If a hotkey is being used as part of a chord, it must be used only as part of a chord. If a hotkey is used at the end of a chord or as a singular hotkey command, it cannot be used as part of a chord

Put another way:

• Chord hotkeys cannot be hotkeys in and of themselves, they can only be used as part of a trigger / sequence to an end hotkey combo
• The last key combo in a sequence or a single combo used as a hotkey cannot be used anywhere else, and definitely not as part of a sequence/chord.

Reserve "highly safe" hotkeys

Enter and Return are highly safe hotkeys, lol

Disable shift-f or even any shift-solo keys

What about this? I removed the hotkey and now I cannot re-assign it.

[bhajneet]

Also being able to assign these hotkeys on their own will mess with navigator (I assume)

[Harjot1Singh]

Sequenced hotkeys appear to muck up shorter sequences: greena13/react-hotkeys#219 and greena13/react-hotkeys#255.

Resolving.

[Harjot1Singh]

tntmarket/roam-toolkit#2 is a great reference to resolve many of the issues we are also having

[Harjot1Singh]

1 semi-serious limitation/bug remaining - if you carry on holding a modifier key after a hotkey has completed, it will not work again unless you focus away and back. Will try to resolve.

[bhajneet]

For notification purposes, I've removed myself from review until you're finished with the remaining fixes

[Harjot1Singh]

@bhajneet

Prevent unusable hotkeys from being assigned

This is still incorrect. Go to the "open a new controller" hotkey, and assign a new one as Ctrl+F and it will say this is already assigned. However, because you can combo hotkeys, I can then do Ctrl+C (total hotkey chord becomes Ctrl+F, Ctrl+C) and this is allowed. Now in usage, if I do Ctrl+F it will fullscreen the window and then if I do Ctrl+C afterwards it opens a new controller.

This also causes lots of issues:

If a hotkey is being used as part of a chord, it must be used only as part of a chord. If a hotkey is used at the end of a chord or as a singular hotkey command, it cannot be used as part of a chord

Put another way:

• Chord hotkeys cannot be hotkeys in and of themselves, they can only be used as part of a trigger / sequence to an end hotkey combo
• The last key combo in a sequence or a single combo used as a hotkey cannot be used anywhere else, and definitely not as part of a sequence/chord.

Why not just prevent starting sequences being reused?

Case 1:

Action 1: ctrl+c g
Adding Action 2: ctrl+c <= Unassignable, since ctrl+c matches action 1 already

Case 2:

Action 1: ctrl+c g
Adding Action 2: g <= Ok, since does not match any sequence from the beginning. Keys also will not conflict, since if ctrl+c has been pressed, will wait for g or any other key and not trigger action 2.

Case 3:

Action 1: ctrl+a ctrl+c
Adding Action 2: ctrl+a ctrl+c d e f g <= fails, since we have a matching starting sequeunce
Adding Action 3: ctrl+c ctrl+a ctrl+c <= ok, since action 3 is not a subsequence from the beginning of any previous hotkeys
Adding Action 4: ctrl+a <= fails, since a subsequence of action 1

This way, no conflicts could ever happen, and it's logical/intuitive. What do you think?

Disable shift-f or even any shift-solo keys

What about this? I removed the hotkey and now I cannot re-assign it.

Did you want to reassign these mappings?

[bhajneet]

Why not just prevent starting sequences being reused?

The logic is sound, go for it

Did you want to reassign these mappings?

I want them to be locked/permanent (non-removable), so then a user will not be in a situation where they are unable to "fix" them since shift keys are not allowed (afaicr)

[bhajneet]

Congratulations ㊗️

Please file an issue to fix ctrlDown, hotkey, hotkey, ctrlUp only triggering once

Otherwise LGTM

🚀 Can squash and merge!