pyapp-kit
Editable keybinding
Currently, Action is frozen and does not allow editing keybinding.
So either, next to default keybinding there should be an option to provide overwrite of keybinding, or builders (like QModelMenu) should accept dict with overwrite of keybindings (action id to keybind like in napari settings).
The third option is to store key bind overwrite in Application instance, as it is already passed and used.
this is something I've discussed with @DragaDoncila a few times. can't remember where those conversations went.
I'm not sure I would agree that these should be editable. Instead, I was imagining essentially three levels of keybinding maps (similar to how vscode does it). There is a System keybinding (i.e. in the case of napari, your builtin keybindings), a plugin -level keybinding, and a user level keybinding (overrides by the user). The action object itself wouldn't be the place to mutate all that stuff, rather, you would have something like a chain map that first looks in the user map, then the plugin map, then the system map to resolve a key to an action.
So, what's "immutable" (and what I would argue should stay immutable) is the fact that napari, or some plugin, or some user, says "I associate this keybinding with this action". What can be mutable however is the user map at runtime, which is not something that app-model has begun to implement
just for clarity... what I'm referring to is the "source" column when you go to edit your keybindings in vscode:
this is something I've discussed with @DragaDoncila a few times. can't remember where those conversations went.
most of it is summarized in this comment from @lucyleeow, but yes I'm in full agreement with all of this, from the immutable "default" keybinding to the different sources/priorities and potentially the editable user keybinding (although I think it should actually always be immutable and it's not an edit action but a pop and replace action but that's semantic).
actually... following that issue, I can see that @lucyleeow already responded to your concert about this @Czaki in https://github.com/napari/napari/issues/6516#issuecomment-1886127528 ... but you didn't clarify your concern there
No. She described some possible organization of storage, but it still needs to be implemented in app-model, as app-model QModelMenu is used to build menu.
Current implementation resolves shotcuts in register order https://github.com/pyapp-kit/app-model/blob/7f853e606daee0356cc6751e6a6e882c7179724e/src/app_model/backends/qt/_qaction.py#L45-L45
https://github.com/pyapp-kit/app-model/blob/7f853e606daee0356cc6751e6a6e882c7179724e/src/app_model/registries/_keybindings_reg.py#L77-L82
Uses private class for store information, so there is no promise of a stable API to retrieve information: https://github.com/pyapp-kit/app-model/blob/7f853e606daee0356cc6751e6a6e882c7179724e/src/app_model/registries/_keybindings_reg.py#L19
Of course, napari could go back to its own implementation of shortcuts dispatch, but as I understand the idea of app-model is to support such us case.
Or I miss what you point as my concerns.
Current implementation resolves shotcuts in register order
yes, because, as mentioned above, app-model only has a single keybinding registry.
And also yes, as described by @lucyleeow and @DragaDoncila in those comments in the napari repo, it's definitely been a long term goal of all of ours to have a publicly condoned way to have either multiple registries (with cascading lookup), or add a source type field to keybindings, and some way to resolve priority of the lookup. Definitely welcome suggestions on how to implement those things, but Action itself should remain immutable i think
With regard to shortcuts dispatch, I see it was implemented in https://github.com/napari/napari/pull/6204 . I think there was talk about the dispatch being handled in app-model and moving the dispatch implementation in that PR to app-model (I think that PR is going to be split up anyway) ?
I think there was talk about the dispatch being handled in app-model and moving the dispatch implementation in that PR to app-model (I think that PR is going to be split up anyway) ?
@lucyleeow that's right - we'd rather not merge that PR as is, because we'd have to refactor the dispatcher out to app-model anyway eventually. But I'm not sure yet what order things are happening in. I would say we want some consensus here before we implement the dispatcher.
I think we're all on the same page with what needs to be done...
If we want to split things up and have Talley take ownership over the dispatcher being implemented into app-model I'm all for it.
I'm glad we're having the conversation, sorry for being kind of MIA, I feel like I'm drowning in everything rn...
I'm also happy to fix the merge conflicts and see if we can get it into main since all the work is already done (minus UI updates)
Talley take ownership over the dispatcher being implemented into app-model
Ah I'm not sure that Talley has capacity for that, but he's indicated before he'd be happy to review.
I'm also happy to fix the merge conflicts and see if we can get it into main since all the work is already done (minus UI updates)
Yeah we chatted about that at PR party but there were concerns about the size of the PR, the need to resolve some other stuff before we migrate more actions, and the desire to split out the dispatching to app-model. We can take this discussion back to zulip or napari/#6204 though as it's not directly relevant here.
@tlambert03 could I commandeer this issue to be about storing 'source' information in keybinding registry? AFAICT the keybinding registry is editable at runtime, and the history in this issue is relevant.
it's definitely been a long term goal of all of ours to have a publicly condoned way to have either multiple registries (with cascading lookup), or add a source type field to keybindings, and some way to resolve priority of the lookup.
Do you have a preference between the methods:
- multiple registries, one for each source, with cascading lookup (not sure I understand this part, but I assume it is to deal with source priority?)
- one registry that stores 'source' information
- in this case should the possible sources and their priority be immutable (e.g., have core > extension > user) or should it be configurable when you e.g., init the
app?
- in this case should the possible sources and their priority be immutable (e.g., have core > extension > user) or should it be configurable when you e.g., init the
cc @DragaDoncila
FWIW I think my preference would be one registry with configurable sources & priority list
i agree. one reg, with added source.
another thing that i always intended is for get_keybinding to take in a context. For example, using the image from above again:
the action associated with a keybinding also should depend on the context. So get_keybinding should be a function that takes a pressed key, (potentially) narrows keybindings based on context, and then picks a command based on the prioritization of the source
since none of this falls under the original proposal of editable keybinding objects... can we start a new issue for this?
Thanks, I've created #207.
I'm going to close this as AFAICT the keybinding registry is editable at run time. Please feel free to re-open.