Closed Harjot1Singh closed 3 years ago
I cannot use the following hotkey combos:
The following can be used but are already hotkeys:
The following are reserved/taken, but don't work as intended perhaps:
The following are hotkeys I don't think need to be set:
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)
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).
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:
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:
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.
Side note, I cannot copy with the pop over, but the hotkey works for all lines haha:
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
This doesn't happen with ctrl+shift+b for esc (reserved)
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)
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
btw @Harjot1Singh react-hotkeys has problems with hotkeys that begin with alt/option
source: my work on sttm-web shortcuts
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.
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.
... 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
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)
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.
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)
This pull request introduces 2 alerts when merging 3dca681fe4cb3ab6d1ff600955cededc58601596 into 792fc77870ded61333e2cb0d2aa35438e9d4da8f - view on LGTM.com
new alerts:
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:
Enter and Return are highly safe hotkeys, lol
What about this? I removed the hotkey and now I cannot re-assign it.
Also being able to assign these hotkeys on their own will mess with navigator (I assume)
Sequenced hotkeys appear to muck up shorter sequences: https://github.com/greena13/react-hotkeys/issues/219 and https://github.com/greena13/react-hotkeys/issues/255.
Resolving.
https://github.com/tntmarket/roam-toolkit/pull/2 is a great reference to resolve many of the issues we are also having
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.
For notification purposes, I've removed myself from review until you're finished with the remaining fixes
@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?
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
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)
So we are still having shift+
hotkeys, but users cannot assign any. FYI, the shift+
hotkeys do not work when in search, which I believe is why we disabled them in the first instance.
Please test @bhajneet @saihaj
Subject
Adds customisable hotkeys
Changes
I found the thread below hard to work against, so I've consolidated it here:
Linked issues
Closes #176, Closes #426
Before
N/A
After
Tests
TTR
At least 30 hours? Most time spent was not due to complexity of feature, but resolving 3rd party lib bugs.
Reviewers
@bhajneet