pyapp-kit / app-model

Generic application schema implemented in python.
https://app-model.rtfd.io
BSD 3-Clause "New" or "Revised" License
18 stars 12 forks source link

Use OS-specific modifier names when writing out SimpleKeyBindings #208

Open jni opened 2 months ago

jni commented 2 months ago

Description

I'm working on upgrading napari settings from 0.4.19 to 0.5.0 in napari/napari#5316. This is my first encounter with the app-model keybindings so please bear with me. 😅 🙏

In our old settings (0.4.19), the settings were written out in an "OS-agnostic" way, e.g. "Control-Y" was the saved value for toggle_ndisplay, regardless of the operating system, and this was interpreted as Control-Y on Windows and Linux, and as Command-Y on macOS.

Since napari/napari#5103, which switched our keybindings settings to app-model, the keybinding is written out as Ctrl+Y on Windows/Linux and Meta+Y on macOS.

Arguably, Meta+Y is an awkward hybrid of OS-independent and OS-dependent output, since "Meta" is not really a term used in macOS.

What I Did

In [13]: from app_model.types import KeyBinding

In [14]: KeyBinding.from_str('Cmd-Y')
Out[14]: <KeyBinding at 0x31079d8d0: Meta+Y>

In [15]: str(KeyBinding.from_str('Cmd-Y'))
Out[15]: 'Meta+Y'

I think that str should by default use the representation that makes sense for the current OS.

This is closely related to #89 but is more about the default representation of str() than about a special method.

Happy to submit a PR if there's agreement on this.

tlambert03 commented 1 month ago

i definitely agree that when you write out your preferences to disk in napari, that you should be writing them in os-specific ways: { "key": "shift+cmd+z", "command": "redo" } on a mac and { "key": "ctrl+shift+z", "command": "redo" } on windows. (and... you should also think about how someone would be able to use the same settings file going between the two, either with when clauses, or by taking advantage of the win, mac and linux fields in keybinding rules)

and I think it could be ok to make the value returned by str() be os-specific, but i'm much more confident about the former than the latter. Need to think about the consequences