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

Keep consistent order of modifiers ? #110

Closed Carreau closed 1 year ago

Carreau commented 1 year ago

Description

Order of modifiers can be inconsistant, I think it would be good to always have shortcuts in an order that is consistant, an likely the one the current platform uses.

>>> str(KeyBinding.from_str('Meta+Alt'))
'Meta+Alt'

>>> str(KeyBinding.from_str('Meta+Alt+Shift'))
'Alt+Meta+Shift'

I'm workin on napari UI shortcut editor, and while editing a shortcut if I have Alt-Meta pressed, and pressed shift, it is weird to see Alt and Meta swap places.

In general there seem to be some reordering:

In [2]: str(KeyBinding.from_str('Meta+Alt'))
Out[2]: 'Meta+Alt'  # order unchanged from input

In [3]: str(KeyBinding.from_str('Alt+Meta'))
Out[3]: 'Alt+Meta'  # order unchanged from input

In [4]: str(KeyBinding.from_str('Alt+Meta+Shift'))
Out[4]: 'Alt+Meta+Shift' # order unchanged from input

In [5]: str(KeyBinding.from_str('Meta+Alt+Shift'))
Out[5]: 'Alt+Meta+Shift' # order changed ! from input

So another possibility is to keep the order that was passed in, but I think that would be more complicated.

It's unclear to me why this is happening as the __str__s are pretty straitforward and should keep the order:

    def __str__(self) -> str:
        return " ".join(str(part) for part in self.parts)

    def __str__(self) -> str:
        out = ""
        if self.ctrl:
            out += "Ctrl+"
        if self.shift:
            out += "Shift+"
        if self.alt:
            out += "Alt+"
        if self.meta:
            out += "Meta+"
        if self.key:
            out += str(self.key)
        return out

So Im pretty sure there is a right in my face that I don't see, and it's likely related to having modifiers only shortcuts while starting to type a chord that I'm not seeing.

Carreau commented 1 year ago

Which I know realize is because the last component is considered the Key and not a modifier:

In [22]: KeyBinding.from_str('Meta+Alt+Shift').parts
Out[22]: [SimpleKeyBinding(ctrl=False, shift=False, alt=True, meta=True, key=<KeyCode.Shift: 56>)]
                                           ^ set to false

In [23]: KeyBinding.from_str('Meta+Shift+Alt').parts
Out[23]: [SimpleKeyBinding(ctrl=False, shift=True, alt=False, meta=True, key=<KeyCode.Alt: 49>)]
                                                     ^ set to false

Though those are events you can get via QT and both modifier and keys are set.

So this may be a question on how to handle shortcuts with only modifiers.

tlambert03 commented 1 year ago

yep, that's an open question. I hadn't considered modifier only keybindings, so, as you noticed, that last item is always the "key" being pressed. So, I would probably re-title this issue to "how to handle modifier-only keybindings". Do you need them? In general, I'm not a fan of supporting them, but of course, if you have a compelling use case let me know. (edit: I can imagine a single modifier, but do you need combinations of modifiers without target keys?)

See also: https://github.com/napari/napari/issues/5747

tlambert03 commented 1 year ago

also pinging @kne42 cause I think she had good opinions on this issue

kne42 commented 1 year ago

see also NAP 7

tlambert03 commented 1 year ago

ok, so, quoting from @kne42's NAP:

The proposed restrictions on what key sequences can be used in a key binding aim to allow for the simplest user need while cutting down on any unnecessary complexities:

  • key combinations containing any modifier keys as a base key are invalid
  • key chords cannot contain a base key that is a modifier key (aka a single modifier)

this suggests that napari won't deal with modifier only key combos... which suggests we can close this. (I think that, as far as app-model is concerned, the model itself is internally consistent and is allowing for napari to do whatever it needs/wants. It allows for the concept of a modifier being the last key, and whether users of app-model decide to reject key combos where they .key attribute is a modifier is totally fine.

tlambert03 commented 1 year ago

closing, but feel free to reopen if you disagree

Carreau commented 1 year ago

I understand not dealing with modifier only shortcut, the problem is while you are binding in the napari shortcut editor I want the current key pressed to be shown and this is currently affected.

I basically have to bypass the full app-model key handling just to show those keys, so I believe it would be good to actually support this logic in app-model.

feel free to reopen if you disagree

Up to you, but as a note, if a maintainer closes an issue, it can't be reopened by the author. It can only be reopened if the author closed himself.

tlambert03 commented 1 year ago

By "reopen" I just meant, tell me if you want it to be open.

If you can give me an example in the context of the gui you're trying to create, that would helpful.

Do you want the concept of an "incomplete" keybinding?

tlambert03 commented 1 year ago

basically have to bypass the full app-model key handling just to show those keys, so I believe it would be good to actually support this logic in app-model.

Or couldn’t you just use a subclass or something that modifies string parsing to ignore modifiers as the last key?

Feel free to propose anything that would be convenient for you that doesn’t impose any napari-specific constraints or concerns

tlambert03 commented 1 year ago

hey @Carreau ... do let me know if you have anything actionable for us to list here. To me, it kinda feels like the "while you are binding" problem is a gui issue that would need some special handling; that is, if the napari NAP says that you don't intend to handle combos of modifiers, etc... then you're going to need some concept of an "in-process" keybinding... (similar to a QLabel with a validator that isn't quite finished). That's not a concept that app-model yet has, and unless we have a keybinding editor inside of app-model, it does seem to me like handling that logic makes sense in the gui you're building.

anyway, i'll give this a week or so to wait to hear from you, but lacking a clear path forward here i'll close it otherwise

kne42 commented 1 year ago

i agree, performing the keybinding validation is logic that should be in napari

On Wed, Jul 12, 2023 at 8:30 PM Talley Lambert @.***> wrote:

hey @Carreau https://github.com/Carreau ... do let me know if you have anything actionable for us to list here. To me, it kinda feels like the "while you are binding" problem is a gui issue that would need some special handling; that is, if the napari NAP says that you don't intend to handle combos of modifiers, etc... then you're going to need some concept of an "in-process" keybinding... (similar to a QLabel with a validator that isn't quite finished). That's not a concept that app-model yet has, and unless we have a keybinding editor inside of app-model, it does seem to me like handling that logic makes sense in the gui you're building.

anyway, i'll give this a week or so to wait to hear from you, but lacking a clear path forward here i'll close it otherwise

— Reply to this email directly, view it on GitHub https://github.com/pyapp-kit/app-model/issues/110#issuecomment-1633405551, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG6QLUYHUVVMSMGXV732ZJTXP5FSDANCNFSM6AAAAAA2DNEZ4M . You are receiving this because you were mentioned.Message ID: @.***>