Add support for long keybindings #705
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Keybindings of more than 2 chars were not possible, as the `TempKeyStorage` was cleaned on every access to `get_keys`. `get_keys` is invoked by every call of `_process_event`. Removing the call to `clean` in `get_keys` leaves the code correct, as the keystore is actively cleaned when no binding matches or on a timeout.
The keyhint always shows the last pressed key, instead of all previously pressed keys. The reason was that for signal `partial_matches` only the last pressed key was passed, instead of the full sequence.
jcjgraf
force-pushed
the
fix/multikeybinding
branch
from
2babf1d to
d8cab67
Compare
|
Seems reasonable to me, I will take some more time thinking why it cleared previously, especially as there was a dedicated test case, but I think everything is good as-is. Just need a bit more time to review and think 😊 |
|
It's been a while, but I finally did some more testing and playing around, and to me this seems superior to the current behaviour in every way. So if you give me a quick thumbs up, happy to merge 😊 |
|
I am glad that you like it. It still looks good to me 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for keybindings of arbitrary-length key sequences. Longer key sequences were actually almost supported, but due two bugs (probably you just never really thought about them) prevented their (proper) working.
How to test
Adds support for bindings like
:bind abc log info success. Without this PR, such a binding does not workChanges
Two things prevented the working of such a binding and required adoption:
Eventhandleronly considered current and previous keystroke but forgot about older onesProblem
gui.eventhandler.EventHandlerMixin._process_event()is called for each keystroke. That function callsself.partial_handler.keys.get_keys(), which in turn callsself._keys.clear(). On a partial match,self.partial_handler.keys.add_keys(*sequence)is run.This means that for each new keypress the keys from the keystore are retrieved and the store is wiped. On a partial match,
sequence, which is only the last pressed key, is added to the store again. This means, that only one key is stored in the keystore at the time.Fix
I fixed that issue by removing
self._keys.clear()inTempKeyStorage.keys.get_keys(). I really do not see why this call should be required. If however it is, the issue could also be fixed by adding not onlysequence, but actually the full sequence (all previously pressed keys) to the keystore.Keyhint shows wrong "already typed keys" (aka. prefix)
The keyhint always displays the most recently pressed key as the prefix and not the whole sequence of pressed keys.
Problem
In
gui.eventhandler.EventHandlerMixin._process_event()on a partial match, the signalpartial_matchesis emitted. As argumentnameis passed. However,nameis based solely onsequence, which is the currently pressed key` and not the whole sequence of pressed keys.Solution
Pass all keys retrieved from the keystore as the first argument to the signal.