NAP 7: Key Binding Dispatch #200

kne42 · 2023-07-04T03:13:33Z

NAP for how key bindings will be handled

specific areas I would appreciate feedback on:

do you agree with the justifications of what should/should not be considered a valid key binding?
what are your thoughts on blocking all key bindings attached to the first part of a two-part key binding (e.g. if Ctrl+A S is active, Ctrl+A would not activate)
which approach do you think is better, the mapping one or the prefix tree one? is there another approach you can suggest?

…roach

jni

Wow this is very nicely written @kne42! I've left a couple of minor suggestions, but as per the NAP process, I think this can be merged very soon.

docs/naps/7-key-binding-dispatch.md

jni · 2023-07-04T06:05:30Z

docs/naps/7-key-binding-dispatch.md

+
+### Finding a match
+
+When checking if an active key binding matches the entered key sequence, the resolver will fetch the pre-sorted list of direct conflicts and check if the last entry is active using its `when` property, moving to the next entry if it is not. When it encounters a blocking rule, it will return no match, and for a negate rule, it will store the affected command in an ignore list and continue to the next entry. If no special rules are present, it will return a match if the command is not in an ignore list, otherwise continuing to the next entry, and so on, until no more entries remain.


I don't know what blocking or negate rules are... Could you add a paragraph on these?

jni · 2023-07-04T06:08:52Z

docs/naps/7-key-binding-dispatch.md

+```python
+def has_shift(key: int) -> bool:
+    return bool(key & KeyMod.Shift)
+
+def starts_with_ctrl_cmd_x(key: int) -> bool:
+    return key & 0x0000FFFF == (KeyMod.CtrlCmd | KeyCode.KeyX)
+
+def multi_part(key: int) -> bool:
+    return key > 0x0000FFFF
+
+> list(filter(has_shift, keymap))
+[<KeyCombo.CtrlCmd|Shift|KeyZ: 3115>, <KeyMod.Shift: 1024>]
+
+> list(filter(starts_with_ctrl_cmd_x, keymap))
+[
+    <KeyCombo.CtrlCmd|KeyX: 2089>,
+    KeyChord(<KeyCombo.CtrlCmd|KeyX: 2089>, <KeyCode.KeyC: 20>),
+    KeyChord(<KeyCombo.CtrlCmd|KeyX: 2089>, <KeyCode.KeyV: 39>),
+]
+
+> list(filter(multi_part, keymap))
+[
+    KeyChord(<KeyCombo.CtrlCmd|KeyX: 2089>, <KeyCode.KeyC: 20>),
+    KeyChord(<KeyCombo.CtrlCmd|KeyX: 2089>, <KeyCode.KeyV: 39>),
+]
+```


The formatting here is a bit weird... I suggest not mixing pure Python cells and cells containing prompts and outputs.

jni · 2023-07-04T06:09:26Z

docs/naps/7-key-binding-dispatch.md

+]
+```
+
+Note that because of the spec, querying for modifiers will only check the first part:


"because of the spec" — maybe be more precise about which part of the spec?

docs/naps/7-key-binding-dispatch.md

melissawm · 2023-07-04T14:48:28Z

The docs build is erroring out because MyST (and reST) tries to add a "transition" (a horizontal bar) before the footnotes, but you also have a section name for it (## References and Footnotes). To prevent this error, you can add

myst_footnote_transition = False

to conf.py. Cheers!

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

andy-sweet · 2023-07-05T16:54:38Z

docs/naps/7-key-binding-dispatch.md

+
+a **key combination** is a base key pressed with one or more modifier keys, e.g. `ctrl+c` or `ctrl+shift+z`
+
+a **key chord** consists of two parts, of which each can be either a base key or a key combination, e.g, `ctrl+x v`


What do you mean by two parts? A key combo represents simultaneously pressing several keys, but a chord represents a sequence? Must each part be pressed separately? Do they need to within a certain time gap?

yes, each part needs to be pressed separately. they do not need to be within a certain time gap, although that was a consideration for indirect conflicts (e.g. ctrl+x is both its own kb and also part of the chord ctrl+x m)

andy-sweet · 2023-07-05T17:28:13Z

docs/naps/7-key-binding-dispatch.md

+
+There are two ways indirect conflicts can exist:
+
+A. the provided key sequence is a single modifier that is a modifier in another key binding's key combination or is a modifier in the first key combination of a key binding's key chord


Maybe I don't have the right kind of brain to think about key bindings, but an example for each case would help a lot!

andy-sweet · 2023-07-05T17:45:31Z

Overall, the document is beautiful.

I haven't kept up with the key binding discussion, so some of the details are a little lost on me, but I got the gist. I had some (hopefully) simple clarifying questions. Will leave more detailed discussion and questions for those with more context and stronger opinions here.

which approach do you think is better, the mapping one or the prefix tree one? is there another approach you can suggest?

The mapping for similar reasons as given in the NAP itself - because it's simpler and the prefix tree doesn't perform much better in a fairly large/complex case.

docs/naps/7-key-binding-dispatch.md

Carreau · 2023-07-10T09:43:45Z

docs/naps/7-key-binding-dispatch.md

+- `f1-f12`, `a-z`, `0-9`
+- `` ` ``, `-`, `=`, `[`, `]`, `\`, `;`, `'`, `,`, `.`, `/`


That is a US centric view. Many of those are characters, not keys, that might be present on the same key. 6 and - are the same key on french keyboard for example.

I'm fine having a US centric implementation, but I just want to underline that the nomenclature is important at otherwise it can lead to confusion.

I would suggest pulling a-z0-9 into their own category, and saying something along "keys that support one or more of the following character are base keys".

As far as I understand for the other ones, they may be on the same physical button, (eg, pageupdown, and arrow keys), but needs the fn button button to be pressed, so at keyboard level they register as a different key.

I believe vocabulary and definitions are important otherwise we will talk pass each other, and defining say button/key/characters may help us express things we couldn't do otherwise.

In practice keys/character distinction might be important for the physical layout of where the shortcuts are. Typically games can use wasd for moving character, those are key bindings and not character binding, as on other non-english keyboard, you want the positions of the button to be the same, but the character ends up being zqsd on say french keyboard.

Oh, that's interesting. I find that explanation confusing as well, so I put down "a base key is a key that when pressed without a modifier key, produces one of the following key codes".

Do you find that satisfactory?

I think that' better, and thank for your link, it is interesting.

I think this excerpt is particularly interesting

e.g. KeyCode.KeyQ produces 'a' on a French keyboard layout which is not immediately intuitive

brisvag

Great work! Should merge asap as per NAP rules :)

psobolewskiPhD · 2023-07-13T01:24:10Z

This is really clear, but I have just one comment:
napari isn't so clear between keybindings and mouse modifiers—they're sort of handled the same and keyboard centric? (e.g. can't rebind scroll wheel).
But keyboard modifiers for mouse actions are a pretty standard UI interaction (alt click, control click, shift click/drag etc).
It's pretty intuitive to bind the left side modifier keys for different interactions, like preserve labels or fill, etc.
I think some carve-out for that sort of behavior (e.g. press & hold a modifier alone vs. press and release a key (with optional modifier)) would be worthwhile in the long run.

kne42 · 2023-07-14T19:23:39Z

It's pretty intuitive to bind the left side modifier keys for different interactions, like preserve labels or fill, etc. I think some carve-out for that sort of behavior (e.g. press & hold a modifier alone vs. press and release a key (with optional modifier)) would be worthwhile in the long run.

So have special casing for if a modifier is pressed and held vs pressed and immediately released?

I think this adds too much complexity for my liking. Advanced users could start a timer when the key is first "pressed" (this signal can be delayed due to dispatching) and then check on release if they should execute different logic based on the time. I don't really want to support/encourage the usage of modifier keys as keybindings outside of changing the GUI (e.g. press alt to change the cursor to a crosshair to make it clear that you're doing something special while adding a point)

psobolewskiPhD · 2023-07-15T01:32:13Z

Sorry if I wasn't clear, I meant a (keyboard) modifier (held) for a mouse action—exactly like your case of alt to modify a select/cursor!

kne42 · 2023-07-15T15:19:13Z

Sorry if I wasn't clear, I meant a (keyboard) modifier (held) for a mouse action—exactly like your case of alt to modify a select/cursor!

I still don't quite follow. Could you give me an example of the two different use cases someone might have and how we want to handle each?

My current understanding is that you want to have an additional registration system (outside of key bindings) for mouse bindings that similarly captures conditionals, modifiers, etc.

Some examples with most selection tools include click to change selection to an element, shift+click to add multiple to selection, and control+click to toggle selection of an element.

kne42 · 2023-07-15T17:04:05Z

The docs build is erroring out because MyST (and reST) tries to add a "transition" (a horizontal bar) before the footnotes, but you also have a section name for it (## References and Footnotes). To prevent this error, you can add
myst_footnote_transition = False
to conf.py. Cheers!

hi @melissawm im still getting this error, any ideas?

kne42 · 2023-07-15T22:04:12Z

ok, i found the issue, for some reason, it initially correctly installs markdown-it==2.2.0, but later in "Install napari dev", it overwrites it to markdown-it==3.0.0, which is incompatible with mdit-py-plugins==0.3.5

kne42 · 2023-07-20T22:22:06Z

hi @jni could you give a re-review if possible? starting a merge countdown if no one has additional feedback at this time

NAP for how key bindings will be handled --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

kne42 added 12 commits June 20, 2023 07:53

start tracking nap-7

214e5fe

flesh out data structure section

b05ac7d

build out psuedo code dispatch more

720685a

introduce alternative approach

f7739c8

add runtime complexity comparisons between different approaches

d29a461

flesh out dispatch logic

7a4d1c0

fix figure

ec1e769

flesh out uncompleted sections

af435b7

update date of creation

4c495a2

fix typo

165b95d

typos/date update

3e56040

update: map approach chosen as not significantly slower than tree app…

31efd2a

…roach

github-actions bot added the documentation Improvements or additions to documentation label Jul 4, 2023

github-actions bot assigned kne42 Jul 4, 2023

kne42 requested a review from a team July 4, 2023 03:15

kne42 mentioned this pull request Jul 4, 2023

Incorrect processing of keybindings consisting of a combination of modifiers without a key napari/napari#5747

Open

jni approved these changes Jul 4, 2023

View reviewed changes

jni added this to the 0.5.0 milestone Jul 4, 2023

typo fixes

8c07652

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

andy-sweet reviewed Jul 5, 2023

View reviewed changes

kne42 mentioned this pull request Jul 9, 2023

Keep consistent order of modifiers ? pyapp-kit/app-model#110

Closed

Carreau reviewed Jul 10, 2023

View reviewed changes

brisvag approved these changes Jul 10, 2023

View reviewed changes

Carreau mentioned this pull request Jul 10, 2023

Show partial shortcut while editing. napari/napari#6050

Merged

1 task

kne42 added 6 commits July 15, 2023 10:52

expand explanation for different types of rules

c54c18f

adjust formatting of bitwise operation example

7330774

add indirect conflict examples

d5490b7

typos

fc4d56d

change wording of base key definition

5090cfd

fix footnote errors

a13eccf

attempt to fix ci

3c0cee1

kne42 force-pushed the nap-7 branch from 977b7ae to 3c0cee1 Compare July 15, 2023 23:05

kne42 added the ready to merge label Jul 20, 2023

kne42 merged commit ff3e3b5 into napari:main Jul 24, 2023
6 checks passed

melissawm mentioned this pull request Jul 24, 2023

Plugin docs restructure of content #203

Merged

3 tasks

psobolewskiPhD removed the ready to merge label Aug 2, 2023

melissawm pushed a commit to melissawm/napari-docs that referenced this pull request Sep 6, 2023

NAP 7: Key Binding Dispatch (napari#200)

3bffbc8

NAP for how key bindings will be handled --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

Czaki modified the milestones: 0.5.0, 0.4.19 Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NAP 7: Key Binding Dispatch #200

NAP 7: Key Binding Dispatch #200

kne42 commented Jul 4, 2023

jni left a comment

jni Jul 4, 2023

jni Jul 4, 2023

jni Jul 4, 2023

melissawm commented Jul 4, 2023

andy-sweet Jul 5, 2023 •

edited

kne42 Jul 14, 2023

andy-sweet Jul 5, 2023

andy-sweet commented Jul 5, 2023 •

edited

Carreau Jul 10, 2023

kne42 Jul 15, 2023 •

edited

Carreau Jul 15, 2023

kne42 Jul 15, 2023

brisvag left a comment

psobolewskiPhD commented Jul 13, 2023

kne42 commented Jul 14, 2023

psobolewskiPhD commented Jul 15, 2023

kne42 commented Jul 15, 2023

kne42 commented Jul 15, 2023

kne42 commented Jul 15, 2023

kne42 commented Jul 20, 2023


		### Finding a match

		When checking if an active key binding matches the entered key sequence, the resolver will fetch the pre-sorted list of direct conflicts and check if the last entry is active using its `when` property, moving to the next entry if it is not. When it encounters a blocking rule, it will return no match, and for a negate rule, it will store the affected command in an ignore list and continue to the next entry. If no special rules are present, it will return a match if the command is not in an ignore list, otherwise continuing to the next entry, and so on, until no more entries remain.


		a key combination is a base key pressed with one or more modifier keys, e.g. `ctrl+c` or `ctrl+shift+z`

		a key chord consists of two parts, of which each can be either a base key or a key combination, e.g, `ctrl+x v`


		There are two ways indirect conflicts can exist:

		A. the provided key sequence is a single modifier that is a modifier in another key binding's key combination or is a modifier in the first key combination of a key binding's key chord

		- `f1-f12`, `a-z`, `0-9`
		- `` ` ``, `-`, `=`, `[`, `]`, `\`, `;`, `'`, `,`, `.`, `/`

NAP 7: Key Binding Dispatch #200

NAP 7: Key Binding Dispatch #200

Conversation

kne42 commented Jul 4, 2023

jni 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

melissawm commented Jul 4, 2023

andy-sweet Jul 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet commented Jul 5, 2023 • edited

Choose a reason for hiding this comment

kne42 Jul 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brisvag left a comment

Choose a reason for hiding this comment

psobolewskiPhD commented Jul 13, 2023

kne42 commented Jul 14, 2023

psobolewskiPhD commented Jul 15, 2023

kne42 commented Jul 15, 2023

kne42 commented Jul 15, 2023

kne42 commented Jul 15, 2023

kne42 commented Jul 20, 2023

andy-sweet Jul 5, 2023 •

edited

andy-sweet commented Jul 5, 2023 •

edited

kne42 Jul 15, 2023 •

edited