Consolidate keyboard shortcuts into a single registry#7314
Consolidate keyboard shortcuts into a single registry#7314donkawechico wants to merge 3 commits intohome-assistant:devfrom
Conversation
donkawechico
force-pushed
the
keyboard_shortcut_mixin
branch
2 times, most recently
from
e0155f5 to
cff9eb6
Compare
donkawechico
force-pushed
the
keyboard_shortcut_mixin
branch
from
cff9eb6 to
433944e
Compare
donkawechico
force-pushed
the
keyboard_shortcut_mixin
branch
from
433944e to
73fdf5d
Compare
| "@typescript-eslint/no-use-before-define": 0, | ||
| "@typescript-eslint/no-non-null-assertion": 0, | ||
| "@typescript-eslint/no-explicit-any": 0, | ||
| "@typescript-eslint/no-shadow": ["error"], |
There was a problem hiding this comment.
Needed due to issue with making enum types in typescript with default eslint.
|
One question to consider is the approach for supporting keybindings. The approach shown here works, but if we get to 10-20 shortcuts, the pattern used here will begin to expand to quite a bit of code (the switch style decoding of the keypresses in particular). Might it be better to make use of a more traditional approach to avoid code bloat? Perhaps it makes sense to use a small keybinding library like the ~400B when minified tinykeys library, or borrow some code from such a library. Separately, it might be better to have shortcut actions registered without a specific keybinding specified in the registering code. Instead have the registry itself have a static mapping of actions to bindings. That approach would make it rather straightforward to eventually add customizable keybindings, if demand should arise (the existing map becomes the defaults). I'm suggesting the above only as things that might be worth thinking about. I am not objecting to the current proposed code, since it is unquestionably an improvement over the current situation. |
|
tinykeys does look nice :-) |
|
Tinykeys does look nice, but I think I'd want it to be used by But I 100% agree with what you're saying. This could get super messy if we don't keep future refactors in mind. |
donkawechico
force-pushed
the
keyboard_shortcut_mixin
branch
from
afd4574 to
50bb51c
Compare
…ntend into keyboard_shortcut_mixin
donkawechico
force-pushed
the
keyboard_shortcut_mixin
branch
from
50bb51c to
1ba7a4c
Compare
|
Yeah, I was certainly trying to suggest that if it were used, it would be used as part of the implementation of the mixin. Having every component set up shortcuts themselves with no coordination is exactly what we don't want. |
|
Still, this would not be able to tell what elements use what keyboard shortcuts, we would need a global registry for that. This mixin will only know the keys that the current element registered. |
Agreed. As I mention in the description, having the logic consolidated into one place should passively help avoid shortcut clobbering for now. But eventually I agree we need a more global dictionary. |
| private _registeredShortcuts: RegisteredShortcuts = {}; | ||
|
|
||
| public disconnectedCallback() { | ||
| this.removeEventListener("keydown", this._keydownEvent); |
There was a problem hiding this comment.
If we remove the listener on disconnected, we should add it again on connected. The firstupdated will only run once, so after the element is disconnected the listener will never be added again on connect
There was a problem hiding this comment.
Also, this will not remove it now, as the callbacks for the add and remove are different.
| callback: () => void, | ||
| element: HTMLElement | Document = this | ||
| ) { | ||
| element.addEventListener("keydown", (event) => this._keydownEvent(event)); |
There was a problem hiding this comment.
We should not do this in registerShortcut, we would get multiple listeners if you register multiple shortcuts.
|
Do we need to update this, or just close? |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
5 participants
Proposed change
Now that other folks are starting to introduce shortcuts into Home Assistant (#7207, #7230), it might be a good idea to consolidate that logic into a single place and a standardized API.
Eventually this could be used to both a) warn about, or ensure we don't overwrite existing hotkeys, and b) surface all existing application hotkeys to the user (say, in a helpful shortcut overview launched with the question mark).
This doesn't (yet) have any protections against folks overwriting shortcuts by accident, but it does make it easier to see what components are registering hotkeys, and which keys are already taken.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: