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

Improve key binding rendering #10801

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Feb 24, 2022

What it does

Fixes #10789 and fixes #5377 by using utilities from the keybinding registry to determine the shape of the rendered item. It also refactors the code to frontload the handling of fuzzy matching and not rebuild the whole list of keybinding candidates on every search. This entailed modifying the interfaces in the file so that the data maintained on the widget was a better input into the render cycle.

MacOS:

image

Ubuntu:

image

How to test

  1. Open the keybinding widget.
  2. Observe that the keys are rendered nicely (symbols on MacOS, capitalized keys without any ctrlcmd elsewhere).
  3. Search for keybindings.
  4. Observe that the results are as you expect, given our fuzzy implementation. In particular:
    • Observe that keys are only matched on a perfect matched
    • Observe that both + and whitespace can be used as term separators for keys

Reminder for reviewers

@colin-grant-work colin-grant-work added the keybindings issues related to keybindings label Feb 24, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that the feature works for the most part, especially on macos 👍

  • confirmed that on macos the key symbols are displayed but not their string values
  • confirmed that updating keybinding will properly render afterwards
  • confirmed that fuzzy matching still works

I did notice an issue on Linux when using electron (the browser seems fine), the Escape keybinding rendering is broken:

keybinding-error

@colin-grant-work
Copy link
Contributor Author

@vince-fugnitto, good catch re: special characters in Electron. Esc, Delete, and Backspace appear to have special unicode characters that come from nativekeymap. I've added special handling in the method that generates the text representations for those values.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me 👍
I confirmed that my previous feedback has been resolved.

@colin-grant-work colin-grant-work merged commit 6b45b27 into master Feb 25, 2022
@colin-grant-work colin-grant-work deleted the bugfix/ctrlcmd-in-keybinding-widget branch February 25, 2022 20:25
@github-actions github-actions bot added this to the 1.24.0 milestone Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings
Projects
None yet
2 participants