Use app-model keybindings internally

[kne42]

trafficstars

Description

Converts the internal representation of keybindings from strings to app-model's KeyBinding.

see #4860

TODO

add hash function to app-model directly and remove it from this code

[Czaki]

If we touch this, maybe remove the strict key bind for actions and allow configuring it through preferences?

[kne42]

If we touch this, maybe remove the strict key bind for actions and allow configuring it through preferences?

what are you referring to exactly? i may address this in a future PR

[Czaki]

For most actions we define shortcuts here: https://github.com/napari/napari/blob/31ee23683e5b38b589cce9633e8aca00200e0905/napari/utils/shortcuts.py

and in proper files we define only Actions: https://github.com/napari/napari/blob/31ee23683e5b38b589cce9633e8aca00200e0905/napari/layers/labels/_labels_key_bindings.py#L47-L49

And this allows to set actions shortcuts to personal preferences:

So as we touch this, we may want to stop using hardcoded shortcuts.

[kne42]

ah, thank you for pointing this out! ill replace those with app-model keybindings in this PR too then

[github-actions[bot]]

Click here to download the docs artifacts docs (zip file)

[github-actions[bot]]

Click here to download the docs artifacts docs (zip file)

[codecov[bot]]

Codecov Report

Merging #5103 (8811f6a) into main (d5ffbac) will increase coverage by 0.01%. The diff coverage is 96.00%.

@@            Coverage Diff             @@
##             main    #5103      +/-   ##
==========================================
+ Coverage   88.76%   88.77%   +0.01%     
==========================================
  Files         579      579              
  Lines       49093    49109      +16     
==========================================
+ Hits        43576    43597      +21     
+ Misses       5517     5512       -5     

Impacted Files	Coverage Δ
napari/utils/interactions.py	74.52% <86.20%> (+2.43%)	:arrow_up:
napari/utils/key_bindings.py	95.89% <98.24%> (+2.64%)	:arrow_up:
napari/_qt/dialogs/preferences_dialog.py	89.81% <100.00%> (+0.19%)	:arrow_up:
napari/layers/image/_image_key_bindings.py	69.23% <100.00%> (+0.60%)	:arrow_up:
napari/layers/labels/_labels_key_bindings.py	91.80% <100.00%> (+0.13%)	:arrow_up:
napari/layers/points/_points_key_bindings.py	100.00% <100.00%> (ø)
napari/layers/shapes/_shapes_key_bindings.py	97.89% <100.00%> (+0.02%)	:arrow_up:
napari/settings/_shortcuts.py	93.75% <100.00%> (+0.41%)	:arrow_up:
napari/settings/_yaml.py	91.66% <100.00%> (+0.75%)	:arrow_up:
napari/utils/_tests/test_interactions.py	100.00% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[github-actions[bot]]

Click here to download the docs artifacts docs (zip file)

[liu-ziyang]

@kne42 can you take a look at the failed tests? thanks!

[kne42]

There's an issue serializing KeyBinding to json, since to properly use the json_encoders config settings on other models, you must pass models_as_dict=False (see https://github.com/pydantic/pydantic/pull/3542). However, this aspect doesn't seem to be passed between json calls, so neither overriding the method to change the default on ShortcutSettings nor calling get_settings().json(models_as_dict=False) fixes the problem.

As a workaround, it looks like we'll need to implement KeyBinding with a custom root type. If we want to address this later, I can revert the shortcuts changes. I may also want to fix this upstream in pydantic at some point as well.

[nclack]

I can't say I completely understand the issue but I do see this:

In [4]: KeyBinding.from_str('ctrl-c').json()
Out[4]: '{"parts": [{"ctrl": true, "shift": false, "alt": false, "meta": false, "key": 20}]}'

In [5]: KeyBinding.from_str('ctrl-c').dict()
Out[5]: 
{'parts': [{'ctrl': True,
   'shift': False,
   'alt': False,
   'meta': False,
   'key': <KeyCode.KeyC: 20>}]}

I suspect we need to add use_enum_values to the SimpleKeyBinding Config (StackOverflow).

[kne42]

tests work on my machine with the version of app-model in https://github.com/napari/app-model/pull/65

[kne42]

ppl were not in favour of the custom root type solution i proposed so we put a hack in here, enjoy

[nclack]

I am seeing this when I start w napari -vvv:

/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: LeftArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])
/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: RightArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])
/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: Alt+UpArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])
/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: Alt+DownArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])

[kne42]

I am seeing this when I start w napari -vvv:

/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: LeftArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])
/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: RightArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])
/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: Alt+UpArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])
/Users/nclack/src/CZI/napari/napari/utils/action_manager.py:320: UserWarning: Alt+DownArrow does not seem to be a valid shortcut Key.
  shorts = jstr.join(f"{Shortcut(s)}" for s in self._shortcuts[name])

That’s most likely because you need to update to app-model 0.1.0 but I will verify on my machine first when I get back to the office and let you know.

[nclack]

That’s most likely because you need to update to app-model 0.1.0

Thanks! That was it. Works beautifully.