Split `keybindings` on `keybindings` and `default_keybindings`

[Czaki]

In napari we would like to make keybindings configurable via settings.
However, the application has only one register of shortcuts.

Also the QModelMenu logic depends on self._app.keybindings (in QCommandAction) to build menu.

My proposition is to add a Application.default_keybindings property to get public access to the default registered keybindings. Then allow the user in the subclass to overwrite the keybindings property with a custom provider.

Then update QMenuItemAction.create to trigger refreshment of keybinding on rebuild of the menus or other action refreshment. (also add proper method to QCommandAction to be called).

What did you think about that?
I'm open to implementing this, but want to get your opinion before starting to write code.

[tlambert03]

Can you help me understand how this relates (or does not relate) to #177

My current picture out how settings-configurable key bindings would work is that you would have three scopes: system, plugin, user.

System is default, plugin is next, and finally user is the user configurable one. So, rather than having multiple registries, we had discussed using a single registry with scopes to solve this

Can you help me understand how scopes don't solve this and now we need multiple registries?

Cc @lucyleeow @DragaDoncila

[Czaki]

Ok. I forgot about source.
Using source still requires update actions to create and refresh logic, to prevent creating QActions with conflicted shortcuts
(For example, the app.keybindings.get_keybinding should not return a system keybinding for action if another action have this keybind defined on user-level).

And all other points with refreshment on keybind edit is still valid.

So my proposition is to:

1. Improve KeyBindingsRegistry documentation (as I hit the same confussion a second time).
2. Add a method to get all keybindings associated with a given action (we allow 2 user defined keybindingigs for napari)
3. Allow removing keybinding using KeyBindingRule (to not need to store disposal in a separate register)
4. Update KeyBindingsRegistry.get_keybinding to not allow return same keybinding for different actions
5. Add refreshment of Action keybindings on create call.

Is this look correct for you?

[tlambert03]

sure, I would very much welcome and improvements to the Documentation!

Add a method to get all keybindings associated with a given action

maybe just improve this method?

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

Lines 194 to 200 in 25b4799

	def get_keybinding(self, command_id: 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 == command_id),
	None,
	)

Allow removing keybinding using KeyBindingRule (to not need to store disposal in a separate register)

I'm open to that ... but what do you mean by "to not need to store disposal in a separate register"?

If you do it, make sure that someone calling the original DisposeCallable is still safe.

Update KeyBindingsRegistry.get_keybinding to not allow return same keybinding for different actions

I think this might be a little too simplistic. I do agree that the keybinding/action retrieval methods definitely need expansion here... but I don't think we can assert global uniqueness between keybinding->action, since it forgets the fact that these can be context sensitive too. for example:

Ctrl+S → "file.save" when editor_focused == true
Ctrl+S → "project.save" when project_view_focused == true

So this proposal would limit the power of the current design. I think the uniqueness logic should be on the resolution side, not on the registration side ideally. We could potentially warn somewhere along the way, but that warning would need to take into account not only the keybinding->command.id mapping, but also the source and when expression.

However, let me know what your ultimate need is here and we can discuss where best to handle it (whether at the level of the registry, or a higher level thing that takes the info in the registry and does something with it)

Add refreshment of Action keybindings on create call.

sure sounds good. definitely possible that some updates are missing there

[Czaki]

I'm open to that ... but what do you mean by "to not need to store disposal in a separate register"?

For example, we have shortcuts in settings. Then we set it in the keybind register with level user. With the current implementation, we need to keep DisposeCallable to allow a user to create new shortcut with reoving old one.

I want to be able to overwrite user -evel keybindings, knowing only the action ID.

I think this might be a little too simplistic. I do agree that the keybinding/action retrieval methods definitely need expansion here... but I don't think we can assert global uniqueness between keybinding->action, since it forgets the fact that these can be context sensitive too. for example:

We need such a level of simplicity when creating QActions (for example, menu items with assigned shortcuts). For more complex cases (for example, layer actions or your example), you are totally right.

maybe just improve this method?

improve, meaning return a list of all keybindings? It changes the semantics of public API.

[tlambert03]

We need such a level of simplicity when creating QActions

one of the goals of #40 was to allow an app like napari to experiment a bit with their own registry designs. So, one possibility here, if you want to enforce a stricter contract, would be for you to subclass the registry and make modifications in your app:

from app_model.registries import KeyBindingsRegistry

class MyReg(KeyBindingsRegistry): ...

class NapariApplication(Application):
    def __init__(self) -> None:
        super().__init__(
            'napari',
            raise_synchronous_exceptions=True,
            keybindings_reg_class=MyReg,
        )

of course... it would be a shame if you used that pattern to diverge dramatically such that you were effectively forking the patterns here. But, I wanted to point that out in case you ever want to experiment with a pattern for a while, and then upstream it once you think it's well designed and generally useful?

for example, menu items with assigned shortcuts

that said... I'm still not sure why menu items would need to be any different than layer actions. They also can have contexts, and enablement clauses, and could conceptually be change by context (as it is in, say, vscode). So I'm not sure it's the right mental model to say that menu items always have a unique global (context free) mapping.

But again, that just constricts what the model should be. It doesn't constrict the general possibility that, at any given time, for a given context, any given keybinding resolves to one and only one action. It just means that you can't enforce a simple Keybinding -> Action map globally in the full registry. and it also means that we can't update KeyBindingsRegistry.get_keybinding to not allow return same keybinding for different actions ... (unless you, for example, also add a scope and whenclause to that method... which would be a reasonable thing to do!)

improve, meaning return a list of all keybindings? It changes the semantics of public API.

yes, naturally you wouldn't just do it without some consideration. I just wanted to point out that there's a big "# TODO: improve me. " note on that method. So it's clearly been marked for improvement :) We could add a new boolean flag that defaults to return_first: bool = True ... or make a new method, i'm open to options.

[kne42]

Hi @tlambert03! We made our own custom registry, validation/conflict detection, and dispatching in napari/napari#6204.

However, it seems some core devs did not want this to live in napari itself. Which implemented parts would you be happy to accept as core app-model functionality?

The plan for weights was as you discussed: system, plugin, and user-level keybindings. Our registry allows for the jettisoning of all bindings of a specific weight, so when e.g. the user file or proxy for bindings is updated, it deletes all user-level bindings and then reloads them from the updated source.

You can read more about keybinding and dispatch rules in NAP 7.

[tlambert03]

Hi @kne42, I'm very sorry, but I think i will need this to be presented a bit more organized. That's an unmerged 5000 line PR, so it would definitely take me a long time to dig through it, figure out which parts are different from app-model, think about the merits of why it needs to be different, etc... It would be really helpful if you (or any core devs) could present this a bit more as a feature proposal to app-model, explaining your motivations, and perhaps why napari devs think it doesn't live in napari. I do think it's reasonable/expected that downstream users would have at least some customization of app-model behavior, and as mentioned that was indeed part of why I did #40... but naturally, if you have extended the functionality of these registries in a way that you think would benefit all users of app-model, and not add too much burden to the mental model, then I would definitely welcome those improvements! Can you maybe break this up into a couple more conventional feature issues? (i.e. small "app model doesn't" problem statement along with how your code fixes that)

[kne42]

Ah sorry, I included the full PR just as a reference. The key files were linked earlier in the sentence and their functionality is explained in the NAP. I can pick out the parts that I think would fit best in app-model, though it really depends on what level of logic belongs in app-model, as it’s a complete implementation of the VSCode keybinding system. Frankly, I have never been a part of the camp that wants this to live in app-model, so I cannot explain the exact motivations to you.

[tlambert03]

Frankly, I have never been a part of the camp that wants this to live in app-model, so I cannot explain the exact motivations to you.

😂 Well, I'm sorry you're in such a position then. (and I guess I'm in it with you 😄)

ok, well thanks for the background, i'll try to dig through it soon. A heads up, I'm just about to move, so I will be out of commission for a while, and this won't happen soon.

[kne42]

Cool, I’ll make an issue presenting the part I think could belong best in app-model, which would be the custom registry + util functions since I think they’re close enough to core VSCode functionality (that I think app-model wants anyways) without being too napari-specific.

[kne42]

Oh and good luck on your move!