Use app-model keybindings internally

[kne42]

Description

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

see #4860

[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:

napari/napari/layers/labels/_labels_key_bindings.py

Lines 47 to 49 in 31ee236

	@register_label_mode_action(trans._('Pan/zoom mode'))
	def activate_label_pan_zoom_mode(layer: Labels):
	layer.mode = Mode.PAN_ZOOM

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

[liu-ziyang]

can you add a few test cases to show this?

[kne42]

this is deleted code that was re-added. i dont know a good way to test this and i dont think its worth my time since the code for handling this will change in the next PR i work on related to handling keypress events

[github-actions]

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

[github-actions]

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

[codecov]

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%)	⬆️
napari/utils/key_bindings.py	95.89% <98.24%> (+2.64%)	⬆️
napari/_qt/dialogs/preferences_dialog.py	89.81% <100.00%> (+0.19%)	⬆️
napari/layers/image/_image_key_bindings.py	69.23% <100.00%> (+0.60%)	⬆️
napari/layers/labels/_labels_key_bindings.py	91.80% <100.00%> (+0.13%)	⬆️
napari/layers/points/_points_key_bindings.py	100.00% <100.00%> (ø)
napari/layers/shapes/_shapes_key_bindings.py	97.89% <100.00%> (+0.02%)	⬆️
napari/settings/_shortcuts.py	93.75% <100.00%> (+0.41%)	⬆️
napari/settings/_yaml.py	91.66% <100.00%> (+0.75%)	⬆️
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]

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 pydantic/pydantic#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 pyapp-kit/app-model#65

[kne42]

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

[Czaki]

Few comments from fast review