Vim Mode 3 - Update hotkey infrastructure to work around various issues with react-hotkeys #2

tntmarket · 2020-06-23T08:30:34Z

This is part of a larger change to support vim style navigation (roam-unofficial#63)

This wraps react-hotkeys to make it behave in a more intuitive and correct way.

These are the main use cases that hinder vim mode, and their proposed fix:

Use Case	Status Quo	Desired Behavior
Letting go of `shift` before letting go of `V`	React hotkeys thinks the `v` key was never released, blocking all future shortcuts	Pressing `shift`-`v`-`shift up`-`v up` should trigger the `V` hotkey without blocking future hotkeys
Binding a hotkey for `J`	Only triggers when holding `shift+j`. If you bind `shift+j`, only the initial press works.	Should trigger both on the initial press of `shift+j`, and when holding `shift+j`
Binding a hotkey for `u`	Native shortcuts that use `u` such as `cmd+u` stop working	Native shortcuts should keep working
Binding a hotkey for `Esc`	If another hotkey simulates `Esc` (in order to unfocus a block for example), then it will actually trigger the `Esc` hotkey	If we are simulating a keypress trigger native actions, we probably don't want to trigger our own custom hotkeys
Binding a hotkey for a sequence like `g g`	Makes it so plain single hotkeys stop doing anything	Plain single hotkeys should keep working

I acknowledge that a longer-term solution is better, but figured that this was a good enough band-aid until we figure out what to do about roam-unofficial#68

tntmarket · 2020-06-25T04:32:07Z

src/ts/core/common/react-hotkeys-fixed/index.tsx

+    /**
+     * React Hotkeys treats j/J as different keys, which is buggy when detecting key chords.
+     * For example, if I press these keys:
+     *
+     *     [shift down, j down, shift up, j up]
+     *
+     * React Hotkeys interprets it like so:
+     *
+     *            ---------Shift+J-------
+     *            |                     |
+     *     [Shift+J down, J down, Shift+J up, j up]
+     *                       |                  |
+     *                       ----Not Released----
+     *
+     * React Hotkeys thinks that the "J" key is still down, which breaks all hotkeys until you
+     * Un-focus the browser window (resetting the key history).
+     *
+     * To workaround this issue, we normalize j/J to be the "same key" using the keycode,
+     * That way, a "J" keydown is stored as a "j" keydown in the key history, so a "j" keyup
+     * will "release" it.
+     *
+     * Related issue: https://github.com/greena13/react-hotkeys/issues/249
+     */
+    customKeyCodes: CODE_TO_KEY,


This is the issue that CODE_TO_KEY from roam-unofficial#63 is trying to solve

Stvad

the necessity of this PR makes me sad.
also would benefit from tests

Stvad · 2020-06-27T03:06:16Z

src/ts/core/common/react-hotkeys-fixed/allow-holding-shifted-keys.ts

@@ -0,0 +1,46 @@
+// A "KeySequence" is a series of key chords, such as "g g" or "control+w j"


that's a weird place to have this comment :)

Stvad · 2020-06-27T03:08:40Z

src/ts/core/common/react-hotkeys-fixed/dont-override-native-hotkeys.tsx

@@ -0,0 +1,38 @@
+import {GlobalHotKeys, KeyMap} from 'react-hotkeys'


this whole fix thing probably deserves it's own subdirectory in core vs being under common

Stvad · 2020-06-27T22:04:04Z

src/ts/core/common/react-hotkeys-fixed/allow-holding-shifted-keys.ts

+ *         The list contains multiple alternatives trigger the same handler.
+ *         See https://github.com/greena13/react-hotkeys#alternative-hotkeys
+ */
+export const allowHoldingShiftedKeys = (keySequence: KeySequence): KeyChord[] | KeySequence => {


these functions could use the Specs

Stvad · 2020-06-27T22:05:34Z

src/ts/core/common/react-hotkeys-fixed/allow-holding-shifted-keys.ts

+const convertUppercaseToLowercasePlusShift = (keyChord: KeyChord): KeyChord => {
+    const keys = keyChord.split('+')
+    const key = keys.pop()
+    const modifiers = keys.filter(key => key.toLowerCase())


filter ? this seems like it should have been a map maybe?

Stvad · 2020-06-27T22:06:52Z

src/ts/core/common/react-hotkeys-fixed/allow-holding-shifted-keys.ts

+ */
+const convertUppercaseToLowercasePlusShift = (keyChord: KeyChord): KeyChord => {
+    const keys = keyChord.split('+')
+    const key = keys.pop()


I think this should signify more explicitly what it is, maybe letter (though it probably does not have to be a letter 🤔)?

I extracted KeySequence.keySequencesForBothHeldAndInitialPress. I've been struggling to come up with good names for these hotkey wrappers :/

Stvad · 2020-06-27T22:17:03Z

src/ts/core/common/react-hotkeys-fixed/dont-trigger-hotkeys-with-simulated-presses.ts

+const keyChordMightBeSimulated = (keyChord: KeyChord) =>
+    KEYS_THAT_WE_ALSO_SIMULATE.some(key => keyChord.includes(key))
+
+let executingHandler = 0


this feels like it should be a class

Stvad · 2020-06-27T22:25:32Z

src/ts/core/common/react-hotkeys-fixed/hotkey.ts

+ */
+const clearKeyPressesAfterFinishingKeySequence = (handler: Handler): Handler => async event => {
+    await handler(event)
+    KeyEventManager.getInstance()._clearKeyHistory()


is this a global object for all instances of react-hotkeys?

Stvad · 2020-06-27T22:26:21Z

src/ts/core/common/react-hotkeys-fixed/hotkey.ts

+        return this.keySequence.includes(' ')
+    }
+
+    fixedKeySequence() {


Even though we fix them here, I don't think fixed should be part of the name. It's the internal detail of implementation

Stvad · 2020-06-27T22:44:55Z

src/ts/core/common/react-hotkeys-fixed/hotkeys.ts

+        const actionToHotkey: Dictionary<Hotkey> = {}
+        Object.keys(actionToKeySequence).forEach(action => {


nit: consider using reduce for this

Stvad · 2020-06-27T22:51:34Z

src/ts/core/common/react-hotkeys-fixed/hotkeys.ts

+    singleChordHotkeys() {
+        return new Hotkeys(
+            pickBy(this.actionToHotkey, hotkey => !hotkey.usesMultipleKeyChords())
+        )
+    }
+
+    multiChordHotkeys() {
+        return new Hotkeys(
+            pickBy(this.actionToHotkey, hotkey => hotkey.usesMultipleKeyChords())
+        )
+    }


these don't feel like they conceptually belong inside this object. maybe make them static functions?

I decided to inline these after extracting classes for KeyChord / KeySequence, since "Hotkey" as the combination of "Key+Handler" didn't seem as useful of a definition anymore.

Various utilities for browser APIs, generic data manipulation

Fix tests by updating import paths

…-hotkeys and provide a more convenient interface that does what you'd expect

tntmarket · 2020-06-28T06:27:11Z

I rebased on master, and added some tests.

I also extracted classes for KeySequence and KeyChord. I think it's more verbose, but maybe easier to understand than directly manipulating lists and strings.

Stvad · 2020-06-28T19:10:56Z

package.json

@@ -18,8 +18,10 @@
    "license": "MIT",
    "dependencies": {
        "@popperjs/core": "^2.4.0",
+        "@types/immutable": "^3.8.7",


I believe types should go to dev deps

tests/ts/common/object.test.ts

+import {zipObjects} from 'src/core/common/object'
+
+describe(zipObjects, () => {
+    it('bundles values matching the same key into a tuple', async () => {


Stvad · 2020-06-28T20:32:38Z

src/ts/core/react-hotkeys/key-handler.ts

+ */
+export type Handler = (event: KeyboardEvent) => Promise<any> | undefined
+
+let executingHandler = 0


comments didn't seem to get carried over, as file moved 🤔 anyhow - I believe this module should be a class #2 (comment)

Are you thinking something like a Command Object style class with an execute method?

class KeyHandler { private static executingHandler = 0 private handler: Handler private keySequence: KeySequence constructor(keySequence: KeySequence, handler: Handler) { this.keySequence = keySequence; this.handler = handler; } async execute(event: KeyboardEvent) { await this.executeIfTriggeredByUser(event) if (this.keySequence.usesMultipleKeyChords()) { clearKeyHistory() } } private async executeIfNothingElseIsExecuting(event: KeyboardEvent) { if (KeyHandler.executingHandler === 0) { await this.executeAndTrack(event) } } private async executeAndTrack(event: KeyboardEvent) { KeyHandler.executingHandler += 1 try { await this.handler(event) } catch (error) { console.error(error) } KeyHandler.executingHandler -= 1 } /** * If we artificially simulate a key press, that keypress should not * trigger our own hotkeys. * * For example, simulating "Esc" to unfocus a block should not trigger * our own hotkey for "Esc". * * @return a decorated version of a handler that does nothing if other * handlers are running */ private async executeIfTriggeredByUser(event: KeyboardEvent) { if (this.keySequence.mightBeSimulated()) { await this.executeIfNothingElseIsExecuting(event) } else { await this.executeAndTrack(event) } } }

Or alternatively, keeping the decorator style, but use a class:

class KeyHandler { // ... private handler: Handler constructor(handler: Handler) { this.handler = handler; } preventWhileOtherHandlersAreExecuting(): KeyHandler { return new KeyHandler( // ... decorate this.handler ) } // ... same with other methods }

I personally find decorator+class style less readable, because it adds indirection. I find command object style maybe a bit more readable, but less composable.

My first concern was to encapsulate the global state here more clearly.
I suppose it's kind of already done on a module level. So we kind of have a singleton represented by a module. So maybe my concern is coming from mostly working with languages with different module system..
But does this approach work well with multiple instances of react-hotkeys? I suppose in our case we want this to be global between 2 instances we create. Not sure it's made clear enough though that it has the global state via the current interface 🤔

I think I'd probably prefer to stay with the decorator approach as the current handler interface is what we use throughout the project and is what expected by react-hotkeys. So I think even if we use command object - we'd just use it for this specific case and would need to wrap it back to that interface to pass it on to the react-hotkeys

Also unfortunate that we can't make objects invokable.

In order to communicate to readers - "WARNING, this module uses global state, decorating one handler will affect the behavior of already decorated handlers", I'll try the following:

Include a docstring for adaptHandlerToReactHotkeys warning that it uses a shared counter, and recommend that they look at the test cases.

Inline adaptHandlerToReactHotkeys, and rename dontTriggerWhenKeyPressIsSimulated => forbidConcurrentHandling (naming it based on what it does rather what it's intended purpose is)

Stvad · 2020-06-28T20:44:17Z

src/ts/core/react-hotkeys/key-sequence.ts

+        const keyChord = this.keyChords[0]
+        if (keyChord.isCapitalLetter() && !this.usesMultipleKeyChords()) {
+            return [this, this.map(keyChord => keyChord.convertCapitalToShiftAndLowercase())]
+        }
+
+        return [this.map(keyChord => keyChord.convertCapitalToShiftAndLowercase())]


I'm confused on why we need a first special case?

Oh wow, I'm no longer able to reproduce the bug that I thought I needed the first case to solve.

I might've been using different react-hotkeys configurations (like simulateMissingKeyPressEvents and ignoreRepeatedEventsWhenKeyHeldDown) at the time I created this workaround, but I don't need it anymore so I'll just delete it.

So I guess the purpose of this is just reduced to allowing capital letters as shorthand for shift-* now.

Is it worth it just for that?

I'll cut this functionality out. It'll allow a decent amount of code to be deleted

I decided to keep KeyChord, even though it doesn't really have functionality, because I think it helps communicate the taxonomy of Key* related objects.

Also I kept KEY_TO_SHIFTED because it's still used for shift+clicking hints: #4

Also I kept KEY_TO_SHIFTED because it's still used for shift+clicking hints: #4

hmm, can't we just add shift modifier there?

Stvad · 2020-06-28T20:45:29Z

src/ts/core/react-hotkeys/key-chord.ts

+        this.modifiers = modifiers
+    }
+
+    isCapitalLetter(): string | undefined {


this name seems to imply that it'd return boolean. but it returns the unshifted version of the letter ? so maybe something like unshiftedKey property instead?

I'll inline this function, since the other usage is no longer needed due to getting rid of the special case above

tntmarket · 2020-07-03T01:35:54Z

src/ts/core/react-hotkeys/key-chord.ts

+    convertCapitalToShiftAndLowercase(): KeyChord {
+        const unshifted = this.isCapitalLetter()
+        if (unshifted) {
+            return new KeyChord(unshifted, this.modifiers.add('shift'))


Yeah, this.modifiers.add('shift') returns a copy of the set with shift added. I guess it's immutable in the sense that I'm leaving the old reference alone, but I'm still "changing" the set.

I used an immutable set, soKeyChord would behave like an immutable value object.

tntmarket · 2020-07-03T01:37:44Z

src/ts/core/react-hotkeys/key-chord.ts

+     * @return the keychord with capitals converted to shift+lowercase, or null if
+     *         the key is already lowercase
+     */
+    convertCapitalToShiftAndLowercase(): KeyChord {


Oh I think that's what I'm doing already?

oh darn, indeed 😅

Stvad · 2020-06-28T20:50:01Z

src/ts/core/react-hotkeys/key-history.ts

+ */
+export const clearKeyPressesAfterFinishingKeySequence = (handler: Handler): Handler => async event => {
+    await handler(event)
+    KeyEventManager.getInstance()._clearKeyHistory()


#2 (comment)

Upon further investigation, it does seem that KeyEventManager is global to all instances of react-hotkeys: https://sourcegraph.com/github.com/greena13/react-hotkeys/-/blob/src/lib/KeyEventManager.js#L23:3

:/

that may end up being problematic. add some logging here?

You mean like console.warn('clearing react-hotkeys history')?

something like that

Stvad · 2020-06-28T21:21:39Z

I also extracted classes for KeySequence and KeyChord

Nice, I do believe it made things clearer :)

… into both it's shifted and capital versions

Stvad · 2020-07-03T04:11:53Z

tests/ts/common/object.test.ts

+        })
+    })
+
+    it('uses undefined when there is no matching key', async () => {


hmm, this is kind of unexpected. Not an empty dict?

I'll exclude keys instead of returning tuples with undefined

…d anymore

tntmarket mentioned this pull request Jun 23, 2020

Many vim-like shortcuts for navigating and manipulating blocks roam-unofficial/roam-toolkit#63

Closed

tntmarket commented Jun 25, 2020

View reviewed changes

tntmarket force-pushed the vim_mode_2_various_utilities_to_that_are_used_by_vim_mode branch from 8ce0c2b to c695d4c Compare June 25, 2020 04:58

tntmarket force-pushed the vim_mode_3_wrap_react_hotkeys_to_make_it_work_better branch from 7a191c3 to fc942da Compare June 25, 2020 05:24

tntmarket force-pushed the vim_mode_2_various_utilities_to_that_are_used_by_vim_mode branch 2 times, most recently from 8b9e383 to 988e7df Compare June 25, 2020 05:29

tntmarket force-pushed the vim_mode_3_wrap_react_hotkeys_to_make_it_work_better branch 2 times, most recently from ff2ad2d to f8627e8 Compare June 25, 2020 05:32

tntmarket force-pushed the vim_mode_2_various_utilities_to_that_are_used_by_vim_mode branch from 988e7df to 6e43e9e Compare June 25, 2020 05:41

tntmarket force-pushed the vim_mode_3_wrap_react_hotkeys_to_make_it_work_better branch 2 times, most recently from 2c1d602 to 044eb8d Compare June 25, 2020 06:17

Stvad mentioned this pull request Jun 27, 2020

Vim Mode 2 - Various helpers I use to implement Vim Mode #1

Merged

tntmarket force-pushed the vim_mode_3_wrap_react_hotkeys_to_make_it_work_better branch from 044eb8d to 7bce72a Compare June 27, 2020 22:45

Stvad reviewed Jun 27, 2020

View reviewed changes

tntmarket force-pushed the vim_mode_2_various_utilities_to_that_are_used_by_vim_mode branch 2 times, most recently from 1afad04 to 7ca2fb6 Compare June 27, 2020 23:17

tntmarket force-pushed the vim_mode_3_wrap_react_hotkeys_to_make_it_work_better branch from 7bce72a to 6432fa9 Compare June 27, 2020 23:19

tntmarket changed the base branch from vim_mode_2_various_utilities_to_that_are_used_by_vim_mode to master June 27, 2020 23:19

Stvad and others added 7 commits June 27, 2020 16:55

Merge pull request roam-unofficial#93 from tntmarket/master

145bda3

Various utilities for browser APIs, generic data manipulation

Run tests on pull-requests

9f8308e

Merge pull request roam-unofficial#94 from tntmarket/master

96adb68

Fix tests by updating import paths

Remove unsafe-eval that I accidentally commited before

73fe2ce

Update hotkey infrastructure to work around various issues with react…

e33eb6f

…-hotkeys and provide a more convenient interface that does what you'd expect

Move react-hotkeys-fixed to core/react-hotkeys

238d8c4

Extract classes for key chords and key sequences. Add tests

c82f1d9

tntmarket force-pushed the vim_mode_3_wrap_react_hotkeys_to_make_it_work_better branch from 6432fa9 to c82f1d9 Compare June 28, 2020 06:24

tntmarket added 2 commits June 27, 2020 23:30

Use reduce instead of forEach

d18444f

Don't say Fixed in react hotkeys wrapper names

f053f85

Stvad reviewed Jun 28, 2020

View reviewed changes

tntmarket added 4 commits July 2, 2020 18:42

SQUASH hotkeys - move types to devDependencies

90af561

SQUASH hotkeys - test cases for other zipObjects cases

e41b41a

SQUASH hotkeys - test cases for other zipObjects cases

5982ff7

SQUASH hotkeys - remove unnecessary case for expanding capital letter…

040ddb5

… into both it's shifted and capital versions

Stvad reviewed Jul 3, 2020

View reviewed changes

tntmarket added 4 commits July 4, 2020 14:51

Clarify what key-handler.ts actually does, and that it uses global state

5a326a0

Delete normalization of capital letters to shift+lowercase, not neede…

011aa82

…d anymore

Log some warnings when clearing react hotkeys history

2945e4e

Exclude keys instead of returning tuples with undefined with zipObjects

943f601

Stvad approved these changes Jul 4, 2020

View reviewed changes

tntmarket merged commit 7d913d7 into master Jul 5, 2020

tntmarket mentioned this pull request Jul 5, 2020

Update hotkey infrastructure to work around various issues with react-hotkeys roam-unofficial/roam-toolkit#102

Merged

Harjot1Singh mentioned this pull request Oct 15, 2020

feat(frontend/settings): add customisable hotkeys shabados/presenter#548

Merged

10 tasks

		@@ -0,0 +1,46 @@
		// A "KeySequence" is a series of key chords, such as "g g" or "control+w j"

		@@ -0,0 +1,38 @@
		import {GlobalHotKeys, KeyMap} from 'react-hotkeys'

		const actionToHotkey: Dictionary<Hotkey> = {}
		Object.keys(actionToKeySequence).forEach(action => {

Vim Mode 3 - Update hotkey infrastructure to work around various issues with react-hotkeys #2

Vim Mode 3 - Update hotkey infrastructure to work around various issues with react-hotkeys #2

Conversation

tntmarket commented Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Stvad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntmarket commented Jun 28, 2020

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

tntmarket Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntmarket Jul 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntmarket Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntmarket Jul 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stvad commented Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tntmarket commented Jun 23, 2020 •

edited

Loading

tntmarket Jul 3, 2020 •

edited

Loading

tntmarket Jul 4, 2020 •

edited

Loading

tntmarket Jul 3, 2020 •

edited

Loading

tntmarket Jul 4, 2020 •

edited

Loading

Stvad commented Jun 28, 2020 •

edited

Loading