pyapp-kit / app-model

Generic application schema implemented in python.
https://app-model.rtfd.io
BSD 3-Clause "New" or "Revised" License
18 stars 12 forks source link

Store 'source' info in `KeyBindingRegistry` #207

Open lucyleeow opened 1 month ago

lucyleeow commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

tlambert03 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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