fix: listen for modifier keys in shortcut via window#7604
fix: listen for modifier keys in shortcut via window#7604
Conversation
Summary of ChangesHello @louisch, 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 addresses a long-standing issue where interactions requiring modifier keys (e.g., 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.
Code Review
This pull request effectively addresses an issue with modifier key detection for shortcuts by introducing window-level event listeners. This ensures modifier key states are correctly captured even before the canvas gains focus. The implementation is clear: it adds new listeners specifically for modifier keys, updates the key state, and properly cleans up these listeners on destroy.
My review includes a couple of suggestions for minor improvements to enhance performance and code clarity.
Additionally, regarding your note in the description about onKeyDown: modifying it to ignore modifier keys would indeed be a good optimization to prevent redundant event processing. While I cannot comment directly on that part of the code as it's not in the diff, it's a valid point to consider for a follow-up to make the event handling more efficient.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v5 #7604 +/- ##
=======================================
Coverage 94.96% 94.96%
=======================================
Files 188 188
Lines 9982 9995 +13
Branches 2166 2119 -47
=======================================
+ Hits 9479 9492 +13
+ Misses 503 465 -38
- Partials 0 38 +38
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
As noted in a previous closed PR (#7579), there is an issue right now where when behaviors such as
brush-selectare registered with a modifier trigger key, and my suggested solution was to change the listener for shortcut keys to a window listener.The problem with that solution is that shortcut is also used for key-based shortcuts such as
Control + =for zooming.This new PR proposes adding an additional listener on window which listens to just modifier keys (
Control,Alt,Meta, andShift), and only uses these events to setthis.recordKeyinsideShortcut, which is what is used to check whether a modifier key is being held down (via thematch()function).I wasn't sure whether I should also change the existing
onKeyDownmethod to not listen to modifier keys anymore, because that will also require changing all the tests that emit a keypress to a modifier key to usewindow.dispatchEventinstead ofemitter.emit.