Hover on keyboard modifier should trigger instantly #276582
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR makes hover trigger instantly when a keyboard modifier is pressed, eliminating the need to move the mouse again. When hover is configured for "onKeyboardModifier" mode, pressing the triggering modifier key (opposite of the multi-cursor modifier) now immediately shows the hover at the last known mouse position.
Key changes:
- Added tracking of hover delay setting in IHoverSettings interface
- Implemented instant hover triggering on keyboard modifier press in _onKeyDown method
- Used last known mouse position from _mouseMoveEvent to display hover without requiring mouse movement
|
Thanks for the contribution @ChaseKnowlden ! Please take a look at Copilot's feedback, I'll take a look once the comments are resolved. |
|
Ready to check. |
There was a problem hiding this comment.
Looking good! I noticed a bug where the hover wouldn't go away when moving the mouse to an area that wasn't the squiggly or the hover itself.
When the mouse moves away from the hover and the squiggly, the hover should go away. It should essentially mimic the behavior of how hover works in the "On" state.
20251119-1914-00.5824948.mp4
| if (this._ignoreMouseEvents) { | ||
| return; | ||
| } | ||
| // New behavior: if hover is configured for keyboard modifier and the user presses the triggering modifier |
There was a problem hiding this comment.
We can remove this comment. The code explains itself here.
| const multiCursorModifier = this._editor.getOption(EditorOption.multiCursorModifier); // 'altKey' | 'ctrlKey' | 'metaKey' | ||
| const triggerPressed = isTriggerModifierPressed(multiCursorModifier, e); | ||
| if (triggerPressed) { |
There was a problem hiding this comment.
Nit, this is simpler:
| const multiCursorModifier = this._editor.getOption(EditorOption.multiCursorModifier); // 'altKey' | 'ctrlKey' | 'metaKey' | |
| const triggerPressed = isTriggerModifierPressed(multiCursorModifier, e); | |
| if (triggerPressed) { | |
| const multiCursorModifier = this._editor.getOption(EditorOption.multiCursorModifier); | |
| if (isTriggerModifierPressed(multiCursorModifier, e)) { |
| } else { | ||
| return mouseEvent.event.altKey; | ||
| } | ||
| // onKeyboardModifier |
There was a problem hiding this comment.
nit: This comment is unnecessary
ChaseKnowlden
force-pushed
the
ChaseKnowlden/issue276558
branch
from
9b83ac2 to
d0a9afc
Compare
|
Thanks for the updates @ChaseKnowlden! Before we merge, we should fix the issue mentioned here: #276582 (review) Any idea what might be causing it? Happy to take a look as well if you'd like. |
Fixed. |
|
Hey @ChaseKnowlden, thanks again for the work here. Also letting you know that I'm on vacation and plan to get this PR through when I get back early next year. Happy holidays! |
Fixes issue where hovering a glyph also hovers the content
|
Made a few changes:
Looking to get this merged later this week, it will have ~2 weeks to breathe before the next release kicks in. Feel free to test ahead of time! |
2 participants
Fixes #276558