-
Notifications
You must be signed in to change notification settings - Fork 37.6k
Hover on keyboard modifier should trigger instantly #276582
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
Changes from 2 commits
e9bfc8b
a99f4cb
d0a9afc
000d4d5
5c609a4
6e5a676
6eb20fd
bb6706e
5488e63
74f50df
9deb5f5
f022558
d0edd8c
2b4c1e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,7 +17,7 @@ import { IKeybindingService } from '../../../../platform/keybinding/common/keybi | |||||||||||
| import { ResultKind } from '../../../../platform/keybinding/common/keybindingResolver.js'; | ||||||||||||
| import { HoverVerbosityAction } from '../../../common/languages.js'; | ||||||||||||
| import { RunOnceScheduler } from '../../../../base/common/async.js'; | ||||||||||||
| import { isMousePositionWithinElement, shouldShowHover } from './hoverUtils.js'; | ||||||||||||
| import { isMousePositionWithinElement, shouldShowHover, isTriggerModifierPressed } from './hoverUtils.js'; | ||||||||||||
| import { ContentHoverWidgetWrapper } from './contentHoverWidgetWrapper.js'; | ||||||||||||
| import './hover.css'; | ||||||||||||
| import { Emitter } from '../../../../base/common/event.js'; | ||||||||||||
|
|
@@ -269,6 +269,29 @@ export class ContentHoverController extends Disposable implements IEditorContrib | |||||||||||
| if (this._ignoreMouseEvents) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
| // New behavior: if hover is configured for keyboard modifier and the user presses the triggering modifier | ||||||||||||
| // we should show the hover immediately for the current (last known) mouse location even without moving the mouse again. | ||||||||||||
| if (this._hoverSettings.enabled === 'onKeyboardModifier' && this._mouseMoveEvent) { | ||||||||||||
| const multiCursorModifier = this._editor.getOption(EditorOption.multiCursorModifier); // 'altKey' | 'ctrlKey' | 'metaKey' | ||||||||||||
| const triggerPressed = isTriggerModifierPressed(multiCursorModifier, e); | ||||||||||||
| if (triggerPressed) { | ||||||||||||
|
||||||||||||
| 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)) { |
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.
Fixed.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,20 @@ export function isMousePositionWithinElement(element: HTMLElement, posx: number, | |
| * @param mouseEvent - The current mouse event containing modifier key states | ||
| * @returns true if hover should be shown, false otherwise | ||
| */ | ||
| /** | ||
| * Returns true if the trigger modifier (inverse of multi-cursor modifier) is pressed. | ||
| * This works with both mouse and keyboard events by relying only on the modifier flags. | ||
| */ | ||
| export function isTriggerModifierPressed( | ||
benvillalobos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| multiCursorModifier: 'altKey' | 'ctrlKey' | 'metaKey', | ||
| event: { ctrlKey: boolean; metaKey: boolean; altKey: boolean } | ||
| ): boolean { | ||
| if (multiCursorModifier === 'altKey') { | ||
| return event.ctrlKey || event.metaKey; | ||
| } | ||
| return event.altKey; // multiCursorModifier is ctrlKey or metaKey | ||
| } | ||
|
|
||
| export function shouldShowHover( | ||
| hoverEnabled: 'on' | 'off' | 'onKeyboardModifier', | ||
| multiCursorModifier: 'altKey' | 'ctrlKey' | 'metaKey', | ||
|
|
@@ -37,9 +51,6 @@ export function shouldShowHover( | |
| if (hoverEnabled === 'off') { | ||
| return false; | ||
| } | ||
| if (multiCursorModifier === 'altKey') { | ||
| return mouseEvent.event.ctrlKey || mouseEvent.event.metaKey; | ||
| } else { | ||
| return mouseEvent.event.altKey; | ||
| } | ||
| // onKeyboardModifier | ||
|
||
| return isTriggerModifierPressed(multiCursorModifier, mouseEvent.event); | ||
| } | ||
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.
We can remove this comment. The code explains itself here.
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.
Removed.