-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Support command/ctrl/alt backspace correctly #17175
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
Conversation
Summary of ChangesHello @scidomino, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the accuracy and consistency of keyboard shortcut handling by introducing a clear distinction between the 'Alt' and 'Meta' modifier keys. This refactoring involves updating the underlying key event parsing, modifying the keybinding configuration structure, and adjusting all relevant default shortcuts and documentation. The change ensures that the application responds precisely to 'Alt' and 'Meta' key presses, which is particularly important for users on macOS where the 'Option' key (Alt) and 'Command' key (Meta) serve different functions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the keyboard shortcut handling to properly distinguish between Alt and Meta (Command) keys, which is a significant improvement for cross-platform consistency and accuracy. The changes involve updating the KeyBinding interface, modifying the key parsing logic in KeypressContext, and adjusting key matching in keyMatchers. Corresponding documentation and test files have also been updated to reflect these distinctions. Overall, the changes are well-implemented and address a crucial aspect of keyboard input handling.
|
Size Change: -12 B (0%) Total Size: 23.2 MB ℹ️ View Unchanged
|
30ebb2d to
4276f88
Compare
68821db to
035ba3e
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great refactoring to properly distinguish between alt and cmd modifier keys, which were previously conflated. The changes are extensive and touch upon keybinding definitions, parsing logic, tests, and documentation. The code is now much clearer. I found one minor issue where the alt key was not being checked in the vim input handler, which could lead to incorrect behavior for Alt+Enter. Other than that, the changes look solid.
035ba3e to
eda5e16
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly refactors the key handling logic to distinguish between alt and cmd modifiers, which were previously conflated. This is a significant improvement for correctness and clarity. The changes are consistently applied across the key binding definitions, the key press event parsing logic, and all related tests and components. The code is now more explicit and easier to understand. I've reviewed the changes and found no issues. Great work!
eda5e16 to
2688e8a
Compare
| name: 'a', | ||
| sequence: 'a', | ||
| ctrl: false, | ||
| shift: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: all these calls were verbose and they are even more verbose now. should we modify the core helper so all the modifiers are optional rather than required parameters defaulting to false to make these tests more terse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is we have no core helper. Will do that in a follow up.
d13a7da to
6243a0d
Compare
jacob314
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Summary
Support command/ctrl/alt backspace correctly
Details
The bulk of this PR is devoted to distinguishing between
altandcmdwhich we previously did not. This also reorgs code to always sortshift,alt,ctrl,cmdin that order (matching the kitty code order)Related Issues
Fixes #16791
How to Validate
Pre-Merge Checklist