Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: how to specify KeyMod of Control for all platforms? #229

Open
psobolewskiPhD opened this issue Dec 14, 2024 · 27 comments · May be fixed by #230
Open

Question: how to specify KeyMod of Control for all platforms? #229

psobolewskiPhD opened this issue Dec 14, 2024 · 27 comments · May be fixed by #230

Comments

@psobolewskiPhD
Copy link
Contributor

KeyMod.CtrlCmd results in Ctrl on windows (and linux?) but Cmd on macOS
KeyMod.WinCtrl says it does Meta on Windows and Ctrl on macOS

Is there a way to specify a Control-Foo binding that uses Control on all platforms?
KeyMod.Ctrl does't exist, but there is KeyCode.Ctrl

Based on

KEYMOD_FROM_QT = {
Qt.KeyboardModifier.NoModifier: KeyMod.NONE,
QALT: KeyMod.Alt,
QCTRL: KeyMod.CtrlCmd,
QSHIFT: KeyMod.Shift,
QMETA: KeyMod.WinCtrl,
}
MAC_KEYMOD_FROM_QT = {**KEYMOD_FROM_QT, QCTRL: KeyMod.WinCtrl, QMETA: KeyMod.CtrlCmd}
KEYMOD_TO_QT = {
KeyMod.NONE: Qt.KeyboardModifier.NoModifier,
KeyMod.CtrlCmd: QCTRL,
KeyMod.Alt: QALT,
KeyMod.Shift: QSHIFT,
KeyMod.WinCtrl: QMETA,
}
MAC_KEYMOD_TO_QT = {**KEYMOD_TO_QT, KeyMod.WinCtrl: QCTRL, KeyMod.CtrlCmd: QMETA}

it looks like it wouldn't be easy to have two different KeyMod map to the same QCTRL...

@tlambert03
Copy link
Member

Is there a way to specify

can you clarify where exactly you're specifying these? You might be looking a bit low in the codebase...

My expectation was that, most of the time, keybindings would be declared as keybinding rules (either during the declaration of a plugin or in an Action for an application). is that what you're trying to do here? or are you doing something special at a lower level

@tlambert03
Copy link
Member

a general answer to your question is that KeybindingRules allow for platform specific logic. So, if just saying KeyBindingRule(primary='ctrl+X') doesn't do what you need, you can do KeyBindingRule(primary='ctrl+X', mac='ctrl+X', linux='ctrl+X', win='ctrl+X').

since this all follows the vscode specification for declaring keybindings, you might find it useful to see how a vscode app/extensions declares stuff. Here's an example with a ton of keybindings declared:

https://github.com/microsoft/vscode-sublime-keybindings/blob/5d7eac6603cf57b8a8f1b5cff6f63c1f59529111/package.json#L262

I can also write up a short example of how you could test stuff out locally a little more directly if that would be helpful

@psobolewskiPhD
Copy link
Contributor Author

psobolewskiPhD commented Dec 14, 2024

Well, specifically here:
https://github.com/napari/napari/blob/b2edccd6e40e04467ccfeec0257c2160783f7187/napari/utils/shortcuts.py#L12

    'napari:delete_selected_layers': [KeyMod.CtrlCmd | KeyCode.Delete],

That resolves to Control-Del everywhere, but Command-Del on macOS which is a system keybind and as a result don't work. The idea was to make it Control-Del everywhere, so I naively tried KeyMod.Ctrl, but that doesn't exist.
I tried KeyCode.Ctrl but that made the binding resolve to PageDown on macOS for some reason!

@tlambert03
Copy link
Member

I see, that list has lost the ability to declare platform specificity. Well, how about you try WinCtrl (regardless of what my comment said), and if it doesn't do what you need, you might need to extend the concept of your default key bindings to allow for platform specificity

@psobolewskiPhD
Copy link
Contributor Author

I think it's best to make that a 2nd key binding. Thanks for the insight -- I will close as an explicit KeyMod.Ctrl isn't going to be a thing.

@tlambert03
Copy link
Member

tlambert03 commented Dec 15, 2024

an explicit KeyMod.Ctrl isn't going to be a thing.

I'm not 100% sure about this yet :) reopening. been looking some more, I haven't found a good explanation yet of why not to have KeyMod.Ctrl (i find it surprising that vscode would have never needed to express that concept yet... so I want to look into why they don't have it, but then might add it)

@psobolewskiPhD
Copy link
Contributor Author

psobolewskiPhD commented Dec 15, 2024

Eeep! Sorry!
❤️

@tlambert03
Copy link
Member

one additional thing I noted: if you use the string form: ctrl+X ... then you will get ctrl on both platforms (I think ... please double check). It's just the integer form using a KeyMod enum that doesn't have an "always ctrl" variant

@psobolewskiPhD
Copy link
Contributor Author

psobolewskiPhD commented Dec 15, 2024

Maybe i'm dense, but:
'napari:delete_selected_layers': [ctrl+Delete],
doesn't work:
NameError: name 'ctrl' is not defined

using " " instead of [ ] gives
TypeError: unsupported operand type(s) for &: 'str' and 'int'
so I think a secondary keybinding is the way to go for now

Full traceback for the record

Traceback (most recent call last):
  File "/Users/piotrsobolewski/Dev/miniforge3/envs/napari-dev/bin/napari", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/piotrsobolewski/Dev/napari/napari/__main__.py", line 574, in main
    _maybe_rerun_with_macos_fixes()
  File "/Users/piotrsobolewski/Dev/napari/napari/__main__.py", line 452, in _maybe_rerun_with_macos_fixes
    from napari._qt import API_NAME
  File "/Users/piotrsobolewski/Dev/napari/napari/_qt/__init__.py", line 90, in <module>
    from napari._qt.qt_event_loop import get_app, get_qapp, gui_qt, quit_app, run
  File "/Users/piotrsobolewski/Dev/napari/napari/_qt/qt_event_loop.py", line 14, in <module>
    from napari import Viewer, __version__
  File "<frozen importlib._bootstrap>", line 1412, in _handle_fromlist
  File "/Users/piotrsobolewski/Dev/miniforge3/envs/napari-dev/lib/python3.12/site-packages/lazy_loader/__init__.py", line 82, in __getattr__
    submod = importlib.import_module(submod_path)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/piotrsobolewski/Dev/miniforge3/envs/napari-dev/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/piotrsobolewski/Dev/napari/napari/viewer.py", line 8, in <module>
    from napari.components.viewer_model import ViewerModel
  File "/Users/piotrsobolewski/Dev/napari/napari/components/__init__.py", line 19, in <module>
    from napari.components.layerlist import LayerList
  File "/Users/piotrsobolewski/Dev/napari/napari/components/layerlist.py", line 13, in <module>
    from napari.layers import Layer
  File "/Users/piotrsobolewski/Dev/napari/napari/layers/__init__.py", line 10, in <module>
    from napari.layers.base import Layer
  File "/Users/piotrsobolewski/Dev/napari/napari/layers/base/__init__.py", line 2, in <module>
    from napari.layers.base.base import Layer, no_op
  File "/Users/piotrsobolewski/Dev/napari/napari/layers/base/base.py", line 51, in <module>
    from napari.settings import get_settings
  File "/Users/piotrsobolewski/Dev/napari/napari/settings/__init__.py", line 5, in <module>
    from napari.settings._napari_settings import (
  File "/Users/piotrsobolewski/Dev/napari/napari/settings/_napari_settings.py", line 16, in <module>
    from napari.settings._shortcuts import ShortcutsSettings
  File "/Users/piotrsobolewski/Dev/napari/napari/settings/_shortcuts.py", line 6, in <module>
    from napari.utils.shortcuts import default_shortcuts
  File "/Users/piotrsobolewski/Dev/napari/napari/utils/shortcuts.py", line 106, in <module>
    name: [KeyBinding.from_int(kb) for kb in value]
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/piotrsobolewski/Dev/miniforge3/envs/napari-dev/lib/python3.12/site-packages/app_model/types/_keys/_keybindings.py", line 231, in from_int
    first_part = key_int & 0x0000FFFF
                 ~~~~~~~~^~~~~~~~~~~~
TypeError: unsupported operand type(s) for &: 'str' and 'int'

@tlambert03
Copy link
Member

indeed. it would need to be a string :) 'ctrl+Delete'

@tlambert03
Copy link
Member

let's back up a bit... keys can be encoded as strings 'ctrl+alt+delete or as integers using KeyMod and KeyCode: KeyMod.CtrlCmd | KeyMod.Alt | KeyCode.Delete ...

you definitely can't combine the two, so you can't do 'ctrl' + Key.Delete and you can't do KeyMod.CtrlCmd + 'delete', or you'll get the operand error you showed above.

when it comes time to actually convert those to an OS-bound keybinding, it will pass through Keybinding.validate:

def validate(cls, v: Any) -> "KeyBinding":
"""Validate a SimpleKeyBinding."""
if isinstance(v, KeyBinding):
return v
if isinstance(v, SimpleKeyBinding):
return cls(parts=[v])
if isinstance(v, int):
return cls.from_int(v)
if isinstance(v, str):
return cls.from_str(v)
raise TypeError("invalid keybinding") # pragma: no cover

and there, it will receive slightly different behavior depending on if it's in a string or integer form:

@classmethod
def from_str(cls, key_str: str) -> "SimpleKeyBinding":
"""Parse a string into a SimpleKeyBinding."""
mods, remainder = _parse_modifiers(key_str.strip())
key = KeyCode.from_string(remainder)
return cls(**mods, key=key)
@classmethod
def from_int(
cls, key_int: int, os: Optional[OperatingSystem] = None
) -> "SimpleKeyBinding":
"""Create a SimpleKeyBinding from an integer."""
ctrl_cmd = bool(key_int & KeyMod.CtrlCmd)
win_ctrl = bool(key_int & KeyMod.WinCtrl)
shift = bool(key_int & KeyMod.Shift)
alt = bool(key_int & KeyMod.Alt)
os = OperatingSystem.current() if os is None else os
ctrl = win_ctrl if os.is_mac else ctrl_cmd
meta = ctrl_cmd if os.is_mac else win_ctrl
key = key_int & 0x000000FF # keycode mask
return cls(ctrl=ctrl, shift=shift, alt=alt, meta=meta, key=key)

note that the CtrlCmd logic is only differentiated in the int form (since it's an int)... but if you want always ctrl, i think you can just use 'ctrl'

so, without looking deeper at how napari actually processes those lists you have there, I would expect you should be able to use:

'napari:delete_selected_layers': ['ctrl+delete'],

@tlambert03
Copy link
Member

but Command-Del on macOS which is a system keybind and as a result don't work. The idea was to make it Control-Del everywhere,

I would actually dig into this a bit more... i would 100% expect to be able to delete a layer with cmd+del on macos... and ctrl+del feels like an odd workaround. why can't you use cmd+del? what error exactly do you hit?

@psobolewskiPhD
Copy link
Contributor Author

There is no error, I assume it's processed by the OS.

@tlambert03
Copy link
Member

i guess I mean, what do you mean when you say "as a result don't work" ... what is the behavior? does nothing at all?

@psobolewskiPhD
Copy link
Contributor Author

Yeah, nothing. If I bind Shift-Del or whatever it works. But Command-Del doesn't -- I assume because it's a system binding.

@tlambert03
Copy link
Member

tlambert03 commented Dec 15, 2024

yeah: what I'm saying is that ... if it's just an assumption, before you commit to an unusual non-standard keybinding... seems like it would be worth answering that question convincingly. it would actually surprise me if, in Qt, you can't bind cmd-del to anything you want. It may be that you need to override some default Qt behavior, but i would be surprised if it's actually a lower level OS limitation (plenty of applications use cmd-del to delete stuff)

@psobolewskiPhD
Copy link
Contributor Author

I almost wonder if it's related to backspace vs delete?
macOS keyboard key is labeled delete but behaves like backspace....

@tlambert03
Copy link
Member

it would be super helpful to reduce the problem to a mwe... rather than going through the entire napari app. if you can do that and demonstrate no Qt response, i'd be happy to look into it

@psobolewskiPhD
Copy link
Contributor Author

psobolewskiPhD commented Dec 15, 2024

Boom. If i change it to :
'napari:delete_selected_layers': [KeyMod.CtrlCmd | KeyCode.Backspace],
it works.

@tlambert03
Copy link
Member

Delete = auto() # ⌦. The forward delete key. NOT the Delete key on a mac

Backspace = auto() # Backspace or ⌫. Labelled Delete on Apple keyboards.

:)

@psobolewskiPhD
Copy link
Contributor Author

Yup! if I go back to the original and use Fn-Command-delete-key it works.
So everything is working correctly in principle.
I do think it would be useful to be able to explicitly bind Control-foo on macOS. macOS does have Control-bar bindings -- mostly for text handling that I know, but still.

@tlambert03
Copy link
Member

tlambert03 commented Dec 15, 2024

I do think it would be useful to be able to explicitly bind Control-foo on macOS.

you can: with strings... 'ctrl+backspace' <-- this is explicitly, always control-foo on both mac and windows

as I look into this, one reason I think they probably avoided having both the more useful KeyMod.CtrlCmd (which does the think you usually want it to) and KeyMod.Ctrl (which also seems rather straightforward, but would require lots of conditionals elsewhere if it were the only one available) is that if both are present, it gets a little less unambiguous when these are used as bit flags in a chain: KeyMod.CtrlCmd | KeyMod.Ctrl | KeyMod.X ... it introduces overlapping masks

@psobolewskiPhD
Copy link
Contributor Author

Makes sense.
In my defense -- 🤣 -- part of my confusion comes from:
'napari:delete_selected_points': [ KeyCode.Digit1, KeyCode.Delete, KeyCode.Backspace, ],
So here the button I push on my keyboard (labeled delete, but working like backspace) works, but in the GUI we show just the first 2, so I didn't realize why it worked.

@tlambert03
Copy link
Member

I'm not quite sure exactly what napari does with that list: [ KeyCode.Digit1, KeyCode.Delete, KeyCode.Backspace, ] ... is that a sequence of key presses? alternate key presses?

@psobolewskiPhD
Copy link
Contributor Author

psobolewskiPhD commented Dec 15, 2024

It's 3 bindings--any of those will trigger the action. But the gui editor only shows/edits the first two:
image
And because attention to detail is not my thing, I didn't notice the icon is actually forward delete...
(Also, the editor will flag a conflict between backspace and that binding, which is true, but isn't obvious because the backspace isn't shown. Now it all makes sense though, so huge thanks! ❤️ )

@psobolewskiPhD
Copy link
Contributor Author

psobolewskiPhD commented Dec 15, 2024

it gets a little less unambiguous when these are used as bit flags in a chain: KeyMod.CtrlCmd | KeyMod.Ctrl | KeyMod.X ... it introduces overlapping masks

So how would you encode that without KeyMod.Ctrl?
On macOS Command-Control-Foo is a legit combo for bindings.
I guess this would be KeyMod.CtrlCmd | KeyMod.WinCtrl | Foo?

@tlambert03
Copy link
Member

I guess this would be KeyMod.CtrlCmd | KeyMod.WinCtrl | Foo?

yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants