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

Fix bugs occurring in complex multistroke keybindings #14445

Closed

Conversation

haruleekim
Copy link

@haruleekim haruleekim commented Jul 14, 2024

Release Notes:

  • Fixed a bug where multistroke keybindings would sometimes trigger the wrong commands due to issues in complex key sequence processing. This update improves the handling of such keybindings, reducing the occurrence of incorrect command activation. Further enhancements are expected to fully resolve the issue.

There is a bug in gpui when handling multiple multistroke key bindings.

When strokes that could invoke different key bindings are adjacent, they do not behave as users typically expect.

For example, with Zed in vim mode and the following key bindings set through workspace::SendKeystrokes:

imap dog 🐶
imap cat 🐱

If you type dodog, instead of getting do🐶, you only get 🐶. This means the keystrokes that failed to invoke a keybinding are completely ignored.

Additionally, if you type docat in the same setup, the expected result is do🐱, but what you actually get is doca🐱.

Another example:

imap pin 📌
imap pine 🌲
imap pineapple 🍍

With Zed set up like this, if you type pin and wait for 1 second or type pineapple without pausing, you get the expected 📌 and 🍍 emojis, respectively. However, if you type pine without pausing and then wait for 1 second, you get 📌 instead of 🌲.

Although I used Vim mode and emojis to illustrate the problem, this issue is not specific to Vim mode or Unicode emojis. The problem arises because gpui fails to properly handle the interactions between multiple multistroke key bindings.

There are two issues:

  1. In certain situations, gpui ignores some keystrokes (part of the pending input) that failed to be replaced by an action. The correct behavior is to slice these keystrokes and add them to replay_pending_input.
  2. When adding bindings that the pending input could invoke, their priorities get mixed up. The correct behavior is to always sort these bindings by the hierarchy of the dispatch tree and the length of the keystrokes.

This pull request aims to fix the first issue and enforces sorting by the reverse order of keystroke lengths for the second issue.

I did not implement sorting by the hierarchy of the dispatch tree yet, as it requires further consideration.

Although this fix addresses a bug in gpui unrelated to Vim, I wasn't sure how to properly test keystrokes within gpui, so I wrote the tests in the vim crate.

The vim crate will also need these tests anyway.

Copy link

cla-bot bot commented Jul 14, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @haruleekim on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@haruleekim
Copy link
Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jul 14, 2024
Copy link

cla-bot bot commented Jul 14, 2024

The cla-bot has been summoned, and re-checked this pull request!

@zed-industries-bot
Copy link

Warnings
⚠️

This PR is missing release notes.

Please add a "Release Notes" section that describes the change:

Release Notes:

- (Added|Fixed|Improved) ... ([#<public_issue_number_if_exists>](https://github.com/zed-industries/zed/issues/<public_issue_number_if_exists>)).

If your change is not user-facing, you can use "N/A" for the entry:

Release Notes:

- N/A

Generated by 🚫 dangerJS against 16200c3

@haruleekim
Copy link
Author

@ConradIrwin Hi, this PR has passed all tests and addresses issues with complex multistroke keybindings. Could you please review it when you have a moment? If you think further improvements are needed, I am willing to dedicate additional time to ensure a complete solution.

@ConradIrwin
Copy link
Member

Hey @haruleekim! Thanks for submitting this. I think your fix is good, but I want to explore a bigger change of this system - there are a couple of other bugs and the code has become a bit messy.

if I can’t get that over the line before the next preview I will merge this

ConradIrwin added a commit that referenced this pull request Jul 22, 2024
Simplify key dispatch code.

Previously we would maintain a cache of key matchers for each context
that
would store the pending input. For the last while we've also stored the
typed prefix on the window. This is redundant, we only need one copy, so
now
it's just stored on the window, which lets us avoid the boilerplate of
keeping
all the matchers in sync.

This stops us from losing multikey bindings when the context on a node
changes
(#11009) (though we still interrupt multikey bindings if the focus
changes).

While in the code, I fixed up a few other things with multi-key bindings
that
were causing problems:

Previously we assumed that all multi-key bindings took precedence over
any
single-key binding, now this is done such that if a user binds a
single-key
binding, it will take precedence over all system-defined multi-key
bindings
(irrespective of the depth in the context tree). This was a common cause
of
confusion for new users trying to bind to `cmd-k` or `ctrl-w` in vim
mode
(#13543).

Previously after a pending multi-key keystroke failed to match, we would
drop
the prefix if it was an input event. Now we correctly replay it
(#14725).

Release Notes:

- Fixed multi-key shortcuts not working across completion menu changes
([#11009](#11009))
- Fixed multi-key shortcuts discarding earlier input
([#14445](#14445))
- vim: Fixed `jk` binding preventing you from repeating `j`
([#14725](#14725))
- vim: Fixed `escape` in normal mode to also clear the selected
register.
- Fixed key maps so user-defined mappings take precedence over builtin
multi-key mappings
([#13543](#13543))
- Fixed a bug where overridden shortcuts would still show in the Command
Palette
@ConradIrwin
Copy link
Member

I think #14942 fixed this and others.

I incorporated your tests verbatim - thank you!

@haruleekim haruleekim deleted the fix-multistroke-keybinding branch July 23, 2024 02:20
CharlesChen0823 pushed a commit to CharlesChen0823/zed that referenced this pull request Jul 29, 2024
Simplify key dispatch code.

Previously we would maintain a cache of key matchers for each context
that
would store the pending input. For the last while we've also stored the
typed prefix on the window. This is redundant, we only need one copy, so
now
it's just stored on the window, which lets us avoid the boilerplate of
keeping
all the matchers in sync.

This stops us from losing multikey bindings when the context on a node
changes
(zed-industries#11009) (though we still interrupt multikey bindings if the focus
changes).

While in the code, I fixed up a few other things with multi-key bindings
that
were causing problems:

Previously we assumed that all multi-key bindings took precedence over
any
single-key binding, now this is done such that if a user binds a
single-key
binding, it will take precedence over all system-defined multi-key
bindings
(irrespective of the depth in the context tree). This was a common cause
of
confusion for new users trying to bind to `cmd-k` or `ctrl-w` in vim
mode
(zed-industries#13543).

Previously after a pending multi-key keystroke failed to match, we would
drop
the prefix if it was an input event. Now we correctly replay it
(zed-industries#14725).

Release Notes:

- Fixed multi-key shortcuts not working across completion menu changes
([zed-industries#11009](zed-industries#11009))
- Fixed multi-key shortcuts discarding earlier input
([zed-industries#14445](zed-industries#14445))
- vim: Fixed `jk` binding preventing you from repeating `j`
([zed-industries#14725](zed-industries#14725))
- vim: Fixed `escape` in normal mode to also clear the selected
register.
- Fixed key maps so user-defined mappings take precedence over builtin
multi-key mappings
([zed-industries#13543](zed-industries#13543))
- Fixed a bug where overridden shortcuts would still show in the Command
Palette
kevmo314 pushed a commit to kevmo314/zed that referenced this pull request Jul 29, 2024
Simplify key dispatch code.

Previously we would maintain a cache of key matchers for each context
that
would store the pending input. For the last while we've also stored the
typed prefix on the window. This is redundant, we only need one copy, so
now
it's just stored on the window, which lets us avoid the boilerplate of
keeping
all the matchers in sync.

This stops us from losing multikey bindings when the context on a node
changes
(zed-industries#11009) (though we still interrupt multikey bindings if the focus
changes).

While in the code, I fixed up a few other things with multi-key bindings
that
were causing problems:

Previously we assumed that all multi-key bindings took precedence over
any
single-key binding, now this is done such that if a user binds a
single-key
binding, it will take precedence over all system-defined multi-key
bindings
(irrespective of the depth in the context tree). This was a common cause
of
confusion for new users trying to bind to `cmd-k` or `ctrl-w` in vim
mode
(zed-industries#13543).

Previously after a pending multi-key keystroke failed to match, we would
drop
the prefix if it was an input event. Now we correctly replay it
(zed-industries#14725).

Release Notes:

- Fixed multi-key shortcuts not working across completion menu changes
([zed-industries#11009](zed-industries#11009))
- Fixed multi-key shortcuts discarding earlier input
([zed-industries#14445](zed-industries#14445))
- vim: Fixed `jk` binding preventing you from repeating `j`
([zed-industries#14725](zed-industries#14725))
- vim: Fixed `escape` in normal mode to also clear the selected
register.
- Fixed key maps so user-defined mappings take precedence over builtin
multi-key mappings
([zed-industries#13543](zed-industries#13543))
- Fixed a bug where overridden shortcuts would still show in the Command
Palette
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants