Allow 'filter' option in keybinding registry

[lucyleeow]

Would you consider having a 'filter' option in the keybinding registry to allow users to be more strict on what is allowed as a key sequence?

app-model is pretty open on what is considered a valid, but users may want to:

• not allow certain KeyCodes
• not allow certain KeyCombos - e.g., modifier only key combos, which you were also against supporting (in napari) here: https://github.com/napari/napari/issues/5747#issuecomment-1515301910,
  • also app-model allows multiple modifiers (e.g., 'alt+alt', 'alt+alt+alt'), which we may (?) want to not support. I can't quite tell if this is problematic only with napari legacy code but Kira did implement a function to de-duplicate (I do know that these two result in the same int, though alt by itself is a different int)

I'm also okay with implementing downstream if you don't think it belongs in app-model

cc @dragadoncila

[tlambert03]

where would the filter be applied? at the moment of registration? at the moment of keybinding definition?

[DragaDoncila]

at the moment of registration?

This is what I would imagine? Say, in register_keybinding_rule after validate? Or perhaps with a custom validator?

[lucyleeow]

Yes that's what I was thinking too but open to any suggestions you may have @tlambert03

[tlambert03]

it does seem reasonable to me that app-model itself would allow an app to regulate what it allows as a valid keybinding, so I'm open to that for sure. i think the biggest question in my mind is the API. We could add a line to register_keybinding_rule, but where exactly should we parametrize it? If added a parameter to register_keybinding_rule() itself how do you guys imagine providing your preferences on the napari side? There's many options, including you would instantiate your top level app with a custom KeyBindingsRegistry subclass (which, you could already do now), or it could be done at a couple other levels. Could you guys have a look at the options and propose what you think is a nice pattern that allows a user of app-model to express a constraint?

[lucyleeow]

What do you think about having a KeyBindingsRegistry 'filter' attribute (that defaults to None) that the user can set on app init? KeyBindingsRegistry.filter would then be passed on to register_keybinding_rule.

It would be similar to what is happening here with the injection_store.namespace so there is precedence.

Similar schema to what you describe here: https://github.com/pyapp-kit/app-model/issues/169#issuecomment-1860480748

[tlambert03]

sure, that works for me. so what would filter be? Callable[[KeyBindingRule], bool] ? (alternatively Callable[[KeyBindingRule], str] ... where the str is a message to include in the exception explaining why it's not an acceptable keybinding?, and an empty string implies valid)

[lucyleeow]

Certainly Callable - I am envisioning that the function will raise an error if the key sequence is not valid.

Anything else would be more complicated though possibly worthwhile, depending on use case.

We could have the user provide a list of KeyCodes, or KeyCombos to filter out (potentially via int ?). We could also allow the user to specify via bool whether modifier only key sequences are allowed. Though, in our case, I think we want to allow single modifier (e.g., ctrl as the use case is strong enough to warrant) - so this would not be useful (unless we want to allow further specification to allow single modifier but I think this is getting too complicated).

[DragaDoncila]

Certainly Callable - I am envisioning that the function will raise an error if the key sequence is not valid.

I think raising here would be strange if we name this a filter rather than a validator.

(alternatively Callable[[KeyBindingRule], str] ... where the str is a message to include in the exception explaining why it's not an acceptable keybinding?, and an empty string implies valid)

I like the idea of the message including which part of the keybinding failed and why. I imagine if we were to go with a bool, the first request we'd get is for a message

[lucyleeow]

(alternatively Callable[[KeyBindingRule], str] ... where the str is a message to include in the exception explaining why it's not an acceptable keybinding?, and an empty string implies valid)

This is great, sorry I mis-read the typing.

The only snag here is that we may want app-model to give a warning in some instances instead of error, but we can go with YAGNI for now.