zed icon indicating copy to clipboard operation
zed copied to clipboard

Fix bugs occurring in complex multistroke keybindings

Open haruleekim opened this issue 1 year ago β€’ 3 comments

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.

haruleekim avatar Jul 14 '24 13:07 haruleekim

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[bot] avatar Jul 14 '24 13:07 cla-bot[bot]

@cla-bot check

haruleekim avatar Jul 14 '24 13:07 haruleekim

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

cla-bot[bot] avatar Jul 14 '24 13:07 cla-bot[bot]

Warnings
:warning:

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 :no_entry_sign: dangerJS against 16200c330f255a69d1588c40b6a0613e4be88d4f

zed-industries-bot avatar Jul 18 '24 07:07 zed-industries-bot

@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.

haruleekim avatar Jul 19 '24 22:07 haruleekim

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 avatar Jul 20 '24 01:07 ConradIrwin

I think #14942 fixed this and others.

I incorporated your tests verbatim - thank you!

ConradIrwin avatar Jul 22 '24 18:07 ConradIrwin