Fix keymap async command sequence execution order #4244
Closed
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
kl
force-pushed
the
keymap_commands_execution_order
branch
from
8e1758a
to
34132c4
Compare
kirawi
added
C-bug
helix-term/src/ui/editor.rs
Outdated
Comment on lines
1401
to
1402
| if !self.pending_commands.commands.is_empty() | ||
| && command_status == commands::CommandStatus::Finished |
There was a problem hiding this comment.
I think that returning CommandStatus is not necessary: we can detect if there is another on-next-key callback by checking if cx.on_next_key.is_some()
There was a problem hiding this comment.
Yes, you're right. 👍 I removed that part of the code and rebased on latest master.
kl
force-pushed
the
keymap_commands_execution_order
branch
from
34132c4
to
4bd2f38
Compare
If a keymap triggered a command sequence and the sequence contained a command that needs user input, the following commands were executed before the command that was waiting for input, which resulted in commands being executed in the wrong order.
kl
force-pushed
the
keymap_commands_execution_order
branch
from
4bd2f38
to
2b36d9d
Compare
|
Clsoing this PR as stale. This also doesn't cover all the cases and is ultimaetly blocked by some larger architectureal changes (see ##4709). Thank you for contributing! |
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.
If a keymap triggered a command sequence and the sequence contained a command that needs user input, the following commands were executed before the command that was waiting for input, which resulted in commands being executed in the wrong order.
This commit fixes that by checking if a command is waiting for input and if so storing the remaining commands in the editor and executing them after the
on_next_keycallback has been called.One slightly tricky part is that I also opted to store some context state along with the pending commands (that is the values of
registerandcountas they were when the command sequence was first executed). These values are then restored on the context object when the pending commands are resumed, so that they get the original context state. If more state is added to the context in the future this state also needs to be added and restored when executing pending commands (which would be easy to forget). A better option would be to store the entire context object instead of justregisterandcountbut that is not possible becauseContextis not 'static butEditorViewis. Any thoughts on this? Feedback is appreciated!fixes #4013