Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store 'source' info in KeyBindingRegistry #207

Closed
lucyleeow opened this issue Jul 6, 2024 · 9 comments · Fixed by #226
Closed

Store 'source' info in KeyBindingRegistry #207

lucyleeow opened this issue Jul 6, 2024 · 9 comments · Fixed by #226

Comments

@lucyleeow
Copy link
Collaborator

Enable KeyBindingRegistry to store information on the 'source' of the keybinding e.g., whether user, extension or core set the keybinding. Sources will have priority order e.g., user > ext > core

I am not sure whether the source options are immutable, set to be user > ext > core , or if we allow the user to specify 'source' and their priority order. In the latter case we could default to user > ext > core. WDYT @tlambert03 @DragaDoncila ?

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

@tlambert03 is this different from KeyBindingRules when (which currently is not implemented)?

Migrated from discussion in #177

@tlambert03
Copy link
Member

source should be immutable. we definitely want to know who contributed a keybinding at registration time and I don't see any need for that to be mutable. i can see a world in which priority is mutable... but that seems like a separate item, and can come later.

@tlambert03 is this different from KeyBindingRules when (which currently is not implemented)?

no, not different. that is the when clause. That is already available on keybindings. what's not implemented is run-time keybinding lookup based on a context. (keybindings should not be returned from get_keybinding if they have a when clause that does not evaluate to True)

@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Jul 6, 2024

Sorry when I say 'source' can be set by the user, I mean the potential 'source' names and their priorities. i.e., maybe instead of the classic user, ext, core, they may want to add a 4th one named 'something else'. Or they want something completely different? I don't know enough about use cases to judge this.

i can see a world in which priority is mutable... but that seems like a separate item, and can come later.

I had not thought about this at all but agree, I'd go with YAGNI for now too.

what's not implemented is run-time keybinding lookup based on a context. (keybindings should not be returned from get_keybinding if they have a when clause that does not evaluate to True)

Thanks! It's on my to do list to make another issue on implementing when, source priority and 'weight' (though I am not sure how useful weight is in the context of 'source', in what case would we be interested in registering several commands to the same key sequence from the same source, e.g., user?). I still need to have some more discussion and thoughts before that bit of work is fleshed out. I think that can be separate to this for now.

@lucyleeow
Copy link
Collaborator Author

Or I wonder if we don't even bother with 'source' names and just have e.g., 3 sources 'a' > 'b' > 'c' ...?

@tlambert03
Copy link
Member

in what case would we be interested in registering several commands to the same key sequence from the same source, e.g., user?

what if multiple plugins register the same keybinding? how do you plan on picking?

Or I wonder if we don't even bother with 'source' names and just have e.g., 3 sources 'a' > 'b' > 'c' ...?

I do think that system, user, third_party (i.e. plugin) are pretty general concepts. Either the application, the end-user, or something else are generally going to be registering keybindings, and that seems worth tracking

@lucyleeow
Copy link
Collaborator Author

system, user, third_party (i.e. plugin) are pretty general concepts.

Sounds good. And would you want to let the user amend these source names?

what if multiple plugins register the same keybinding?

Yup thats a good question. Kira sorted keybindings in the registry by weight, then their registration order. This was meant to make get_keybinding easier at runtime.

I wasn't thinking of exposing weight to plugins so I guess we can use weight in lieu of registration order (if we decided not to 'pre' sort the keybindings in the registry).

though I am not sure how useful weight is in the context of 'source', in what case would we be interested in registering several commands to the same key sequence from the same source, e.g., user?

I guess in the context of when, you may be interested in having the same key sequence do different things depending on what is 'active' currently, though this does not require the weight field.

@tlambert03
Copy link
Member

Sounds good. And would you want to let the user amend these source names?

lets start with no, add later if needed

Kira sorted keybindings in the registry by weight, then their registration order. This was meant to make get_keybinding easier at runtime.

i haven't been following what happened in the napari dispatcher. my original intention for keybinding lookup was this:

  1. narrow by context (where keybindings without a when clause are always active)
  2. then prioritize by source (user > plugin > system)
  3. then order by weight

@lucyleeow
Copy link
Collaborator Author

Kira added a keymap to her keybinding registry, of format: {key sequence: [key binding entries]} and sorted on weight (meat of it is here in case you are interested)

I like the key map idea at least as it means you don't need to loop through all the keybindings to find the entries with a specific key sequence. WDYT?

@tlambert03
Copy link
Member

tlambert03 commented Jul 6, 2024

yep, a reverse index is definitely going to be useful here.

Actually, I'm remembering now that get_keybinding is not intended for runtime lookup upon keypress. it's just a way to get a keybinding from a command id. So yes, we need a new method with an inverse map of {key sequence: [key binding entries]}, where the entries are filtered and prioritized as mentioned above

@lucyleeow
Copy link
Collaborator Author

Will open a new issue with a proposal for this soon(ish)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants