app-model icon indicating copy to clipboard operation
app-model copied to clipboard

Use OS-specific modifier names when writing out SimpleKeyBindings

Open jni opened this issue 1 year ago • 1 comments

  • app-model version: 0.2.7
  • Python version: 3.11
  • Operating System: macOS Sonoma 14.5

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.

jni avatar Jul 08 '24 04:07 jni

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

tlambert03 avatar Jul 11 '24 15:07 tlambert03