zed
zed copied to clipboard
Fix bugs occurring in complex multistroke keybindings
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:
- In certain situations,
gpuiignores 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 toreplay_pending_input. - 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.
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'.
@cla-bot check
The cla-bot has been summoned, and re-checked this pull request!
| Warnings | |
|---|---|
| :warning: |
This PR is missing release notes. Please add a "Release Notes" section that describes the change:
If your change is not user-facing, you can use "N/A" for the entry:
|
Generated by :no_entry_sign: dangerJS against 16200c330f255a69d1588c40b6a0613e4be88d4f
@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.
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
I think #14942 fixed this and others.
I incorporated your tests verbatim - thank you!