Editable keybinding

[Czaki]

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.

[tlambert03]

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.

[tlambert03]

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

[tlambert03]

just for clarity... what I'm referring to is the "source" column when you go to edit your keybindings in vscode:

[Czaki]

All my proposition allows for this distinguish.

[DragaDoncila]

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).

[tlambert03]

thanks for the link @DragaDoncila,

[tlambert03]

actually... following that issue, I can see that @lucyleeow already responded to your concert about this @Czaki in napari/napari#6516 (comment) ... but you didn't clarify your concern there

[Czaki]

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

app-model/src/app_model/backends/qt/_qaction.py

Line 45 in 7f853e6

if kb := self._app.keybindings.get_keybinding(command_id):

app-model/src/app_model/registries/_keybindings_reg.py

Lines 77 to 82 in 7f853e6

	def get_keybinding(self, key: str) -> _RegisteredKeyBinding | None:
	"""Return the first keybinding that matches the given command ID."""
	# TODO: improve me.
	return next(
	(entry for entry in self._keybindings if entry.command_id == key), None
	)

Uses private class for store information, so there is no promise of a stable API to retrieve information:

app-model/src/app_model/registries/_keybindings_reg.py

Line 19 in 7f853e6

class _RegisteredKeyBinding(NamedTuple):

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.

[tlambert03]

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

[lucyleeow]

With regard to shortcuts dispatch, I see it was implemented in napari/napari#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) ?

[DragaDoncila]

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...

[kne42]

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...

[kne42]

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)

[DragaDoncila]

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.

[lucyleeow]

@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 ?

cc @DragaDoncila