From 616040415b1811781606aea039a90ff4926732c1 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Tue, 9 May 2023 15:04:02 +0200 Subject: [PATCH 1/3] fix `softDispatch` returning `NoMatchingKb` vs null See https://github.com/microsoft/vscode/issues/179528#issuecomment-1535454131 --- src/vs/workbench/services/hover/browser/hoverService.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/services/hover/browser/hoverService.ts b/src/vs/workbench/services/hover/browser/hoverService.ts index 47127a6254954..31059480f5c04 100644 --- a/src/vs/workbench/services/hover/browser/hoverService.ts +++ b/src/vs/workbench/services/hover/browser/hoverService.ts @@ -16,6 +16,7 @@ import { DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifec import { addDisposableListener, EventType } from 'vs/base/browser/dom'; import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; import { StandardKeyboardEvent } from 'vs/base/browser/keyboardEvent'; +import { ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; export class HoverService implements IHoverService { declare readonly _serviceBrand: undefined; @@ -114,7 +115,7 @@ export class HoverService implements IHoverService { } const event = new StandardKeyboardEvent(e); const keybinding = this._keybindingService.resolveKeyboardEvent(event); - if (keybinding.getSingleModifierDispatchChords().some(value => !!value) || this._keybindingService.softDispatch(event, event.target)) { + if (keybinding.getSingleModifierDispatchChords().some(value => !!value) || this._keybindingService.softDispatch(event, event.target)?.kind !== ResultKind.NoMatchingKb) { return; } if (hideOnKeyDown && (!this._currentHoverOptions?.trapFocus || e.key !== 'Tab')) { From a4a3e2cc29f00a2ccd47205408fca1f5d5ae2ea7 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Tue, 9 May 2023 15:26:53 +0200 Subject: [PATCH 2/3] refactor: `softDispatch` doesn't return null --- src/vs/editor/contrib/hover/browser/hover.ts | 2 +- .../keybinding/common/abstractKeybindingService.ts | 10 +++++----- src/vs/platform/keybinding/common/keybinding.ts | 2 +- .../platform/keybinding/common/keybindingResolver.ts | 2 +- .../keybinding/test/common/mockKeybindingService.ts | 6 +++--- src/vs/platform/list/browser/listService.ts | 2 +- src/vs/workbench/browser/actions/developerActions.ts | 6 +++--- .../workbench/browser/parts/dialogs/dialogHandler.ts | 2 +- .../contrib/terminal/browser/terminalInstance.ts | 4 ++-- .../workbench/services/hover/browser/hoverService.ts | 2 +- .../services/progress/browser/progressService.ts | 2 +- 11 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/vs/editor/contrib/hover/browser/hover.ts b/src/vs/editor/contrib/hover/browser/hover.ts index 6d831dcec3421..eb134a8ca6f56 100644 --- a/src/vs/editor/contrib/hover/browser/hover.ts +++ b/src/vs/editor/contrib/hover/browser/hover.ts @@ -206,7 +206,7 @@ export class ModesHoverController implements IEditorContribution { const resolvedKeyboardEvent = this._keybindingService.softDispatch(e, this._editor.getDomNode()); // If the beginning of a multi-chord keybinding is pressed, or the command aims to focus the hover, set the variable to true, otherwise false - const mightTriggerFocus = (resolvedKeyboardEvent?.kind === ResultKind.MoreChordsNeeded || (resolvedKeyboardEvent && resolvedKeyboardEvent.kind === ResultKind.KbFound && resolvedKeyboardEvent.commandId === 'editor.action.showHover' && this._contentWidget?.isVisible())); + const mightTriggerFocus = (resolvedKeyboardEvent?.kind === ResultKind.MoreChordsNeeded || (resolvedKeyboardEvent.kind === ResultKind.KbFound && resolvedKeyboardEvent.commandId === 'editor.action.showHover' && this._contentWidget?.isVisible())); if (e.keyCode !== KeyCode.Ctrl && e.keyCode !== KeyCode.Alt && e.keyCode !== KeyCode.Meta && e.keyCode !== KeyCode.Shift && !mightTriggerFocus) { diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index 8b0c59b20187c..bcfdf21f7ff54 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -17,7 +17,7 @@ import * as nls from 'vs/nls'; import { ICommandService } from 'vs/platform/commands/common/commands'; import { IContextKeyService, IContextKeyServiceTarget } from 'vs/platform/contextkey/common/contextkey'; import { IKeybindingService, IKeyboardEvent, KeybindingsSchemaContribution } from 'vs/platform/keybinding/common/keybinding'; -import { ResolutionResult, KeybindingResolver, ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; +import { ResolutionResult, KeybindingResolver, ResultKind, NoMatchingKb } from 'vs/platform/keybinding/common/keybindingResolver'; import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem'; import { ILogService } from 'vs/platform/log/common/log'; import { INotificationService } from 'vs/platform/notification/common/notification'; @@ -138,18 +138,18 @@ export abstract class AbstractKeybindingService extends Disposable implements IK // TODO@ulugbekna: update namings to align with `_doDispatch` // TODO@ulugbekna: this fn doesn't seem to take into account single-modifier keybindings, eg `shift shift` - public softDispatch(e: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null { + public softDispatch(e: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult { this._log(`/ Soft dispatching keyboard event`); const keybinding = this.resolveKeyboardEvent(e); if (keybinding.hasMultipleChords()) { - console.warn('Unexpected keyboard event mapped to multiple chords'); - return null; + console.warn('keyboard event should not be mapped to multiple chords'); + return NoMatchingKb; } const [firstChord,] = keybinding.getDispatchChords(); if (firstChord === null) { // cannot be dispatched, probably only modifier keys this._log(`\\ Keyboard event cannot be dispatched`); - return null; + return NoMatchingKb; } const contextValue = this._contextKeyService.getContext(target); diff --git a/src/vs/platform/keybinding/common/keybinding.ts b/src/vs/platform/keybinding/common/keybinding.ts index ba156046afa4f..9a1e812a22e89 100644 --- a/src/vs/platform/keybinding/common/keybinding.ts +++ b/src/vs/platform/keybinding/common/keybinding.ts @@ -63,7 +63,7 @@ export interface IKeybindingService { /** * Resolve and dispatch `keyboardEvent`, but do not invoke the command or change inner state. */ - softDispatch(keyboardEvent: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null; + softDispatch(keyboardEvent: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult; dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void; diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index d69e0136eb819..f337eea01573c 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -27,7 +27,7 @@ export type ResolutionResult = // util definitions to make working with the above types easier within this module: -const NoMatchingKb: ResolutionResult = { kind: ResultKind.NoMatchingKb }; +export const NoMatchingKb: ResolutionResult = { kind: ResultKind.NoMatchingKb }; const MoreChordsNeeded: ResolutionResult = { kind: ResultKind.MoreChordsNeeded }; function KbFound(commandId: string | null, commandArgs: any, isBubble: boolean): ResolutionResult { return { kind: ResultKind.KbFound, commandId, commandArgs, isBubble }; diff --git a/src/vs/platform/keybinding/test/common/mockKeybindingService.ts b/src/vs/platform/keybinding/test/common/mockKeybindingService.ts index d700bf491df4f..9e47434c12543 100644 --- a/src/vs/platform/keybinding/test/common/mockKeybindingService.ts +++ b/src/vs/platform/keybinding/test/common/mockKeybindingService.ts @@ -8,7 +8,7 @@ import { ResolvedKeybinding, KeyCodeChord, Keybinding } from 'vs/base/common/key import { OS } from 'vs/base/common/platform'; import { ContextKeyExpression, ContextKeyValue, IContextKey, IContextKeyChangeEvent, IContextKeyService, IContextKeyServiceTarget, IScopedContextKeyService } from 'vs/platform/contextkey/common/contextkey'; import { IKeybindingService, IKeyboardEvent } from 'vs/platform/keybinding/common/keybinding'; -import { ResolutionResult } from 'vs/platform/keybinding/common/keybindingResolver'; +import { NoMatchingKb, ResolutionResult } from 'vs/platform/keybinding/common/keybindingResolver'; import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem'; import { USLayoutResolvedKeybinding } from 'vs/platform/keybinding/common/usLayoutResolvedKeybinding'; @@ -135,8 +135,8 @@ export class MockKeybindingService implements IKeybindingService { return 0; } - public softDispatch(keybinding: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null { - return null; + public softDispatch(keybinding: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult { + return NoMatchingKb; } public dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void { diff --git a/src/vs/platform/list/browser/listService.ts b/src/vs/platform/list/browser/listService.ts index b6591233db09e..3a44a11558766 100644 --- a/src/vs/platform/list/browser/listService.ts +++ b/src/vs/platform/list/browser/listService.ts @@ -810,7 +810,7 @@ function createKeyboardNavigationEventFilter(keybindingService: IKeybindingServi const result = keybindingService.softDispatch(event, event.target); - if (result?.kind === ResultKind.MoreChordsNeeded) { + if (result.kind === ResultKind.MoreChordsNeeded) { inMultiChord = true; return false; } diff --git a/src/vs/workbench/browser/actions/developerActions.ts b/src/vs/workbench/browser/actions/developerActions.ts index cb7929d181d37..0a1e007292b55 100644 --- a/src/vs/workbench/browser/actions/developerActions.ts +++ b/src/vs/workbench/browser/actions/developerActions.ts @@ -260,7 +260,7 @@ class ToggleScreencastModeAction extends Action2 { const shortcut = keybindingService.softDispatch(event, event.target); // Hide the single arrow key pressed - if (shortcut && shortcut.kind === ResultKind.KbFound && shortcut.commandId && configurationService.getValue('screencastMode.hideSingleEditorCursorMoves') && ( + if (shortcut.kind === ResultKind.KbFound && shortcut.commandId && configurationService.getValue('screencastMode.hideSingleEditorCursorMoves') && ( ['cursorLeft', 'cursorRight', 'cursorUp', 'cursorDown'].includes(shortcut.commandId)) ) { return; @@ -324,8 +324,8 @@ class ToggleScreencastModeAction extends Action2 { ToggleScreencastModeAction.disposable = disposables; } - private _isKbFound(resolutionResult: ResolutionResult | null): resolutionResult is { kind: ResultKind.KbFound; commandId: string | null; commandArgs: any; isBubble: boolean } { - return resolutionResult !== null && resolutionResult.kind === ResultKind.KbFound; + private _isKbFound(resolutionResult: ResolutionResult): resolutionResult is { kind: ResultKind.KbFound; commandId: string | null; commandArgs: any; isBubble: boolean } { + return resolutionResult.kind === ResultKind.KbFound; } } diff --git a/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts b/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts index 1dbc24af2da69..34f825431065e 100644 --- a/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts +++ b/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts @@ -128,7 +128,7 @@ export class BrowserDialogHandler extends AbstractDialogHandler { type: this.getDialogType(type), keyEventProcessor: (event: StandardKeyboardEvent) => { const resolved = this.keybindingService.softDispatch(event, this.layoutService.container); - if (resolved && resolved.kind === ResultKind.KbFound && resolved.commandId) { + if (resolved.kind === ResultKind.KbFound && resolved.commandId) { if (BrowserDialogHandler.ALLOWABLE_COMMANDS.indexOf(resolved.commandId) === -1) { EventHelper.stop(event, true); } diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 290782665d46c..1c8c090251f6e 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -953,7 +953,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { // Respect chords if the allowChords setting is set and it's not Escape. Escape is // handled specially for Zen Mode's Escape, Escape chord, plus it's important in // terminals generally - const isValidChord = resolveResult?.kind === ResultKind.MoreChordsNeeded && this._configHelper.config.allowChords && event.key !== 'Escape'; + const isValidChord = resolveResult.kind === ResultKind.MoreChordsNeeded && this._configHelper.config.allowChords && event.key !== 'Escape'; if (this._keybindingService.inChordMode || isValidChord) { event.preventDefault(); return false; @@ -973,7 +973,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { // for keyboard events that resolve to commands described // within commandsToSkipShell, either alert or skip processing by xterm.js - if (resolveResult && resolveResult.kind === ResultKind.KbFound && resolveResult.commandId && this._skipTerminalCommands.some(k => k === resolveResult.commandId) && !this._configHelper.config.sendKeybindingsToShell) { + if (resolveResult.kind === ResultKind.KbFound && resolveResult.commandId && this._skipTerminalCommands.some(k => k === resolveResult.commandId) && !this._configHelper.config.sendKeybindingsToShell) { // don't alert when terminal is opened or closed if (this._storageService.getBoolean(SHOW_TERMINAL_CONFIG_PROMPT_KEY, StorageScope.APPLICATION, true) && this._hasHadInput && diff --git a/src/vs/workbench/services/hover/browser/hoverService.ts b/src/vs/workbench/services/hover/browser/hoverService.ts index 31059480f5c04..faefae51c91e2 100644 --- a/src/vs/workbench/services/hover/browser/hoverService.ts +++ b/src/vs/workbench/services/hover/browser/hoverService.ts @@ -115,7 +115,7 @@ export class HoverService implements IHoverService { } const event = new StandardKeyboardEvent(e); const keybinding = this._keybindingService.resolveKeyboardEvent(event); - if (keybinding.getSingleModifierDispatchChords().some(value => !!value) || this._keybindingService.softDispatch(event, event.target)?.kind !== ResultKind.NoMatchingKb) { + if (keybinding.getSingleModifierDispatchChords().some(value => !!value) || this._keybindingService.softDispatch(event, event.target).kind !== ResultKind.NoMatchingKb) { return; } if (hideOnKeyDown && (!this._currentHoverOptions?.trapFocus || e.key !== 'Tab')) { diff --git a/src/vs/workbench/services/progress/browser/progressService.ts b/src/vs/workbench/services/progress/browser/progressService.ts index def35449ea6b3..b9d54ac98cbeb 100644 --- a/src/vs/workbench/services/progress/browser/progressService.ts +++ b/src/vs/workbench/services/progress/browser/progressService.ts @@ -557,7 +557,7 @@ export class ProgressService extends Disposable implements IProgressService { disableDefaultAction: options.sticky, keyEventProcessor: (event: StandardKeyboardEvent) => { const resolved = this.keybindingService.softDispatch(event, this.layoutService.container); - if (resolved && resolved.kind === ResultKind.KbFound && resolved.commandId) { + if (resolved.kind === ResultKind.KbFound && resolved.commandId) { if (!allowableCommands.includes(resolved.commandId)) { EventHelper.stop(event, true); } From c9fa4633ab8a362cbea7e8cb7e46ca540a4b5c0b Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 12 May 2023 10:40:09 +0200 Subject: [PATCH 3/3] Adopt softDispatch changes --- src/vs/editor/contrib/hover/browser/hover.ts | 2 +- src/vs/platform/list/browser/listService.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vs/editor/contrib/hover/browser/hover.ts b/src/vs/editor/contrib/hover/browser/hover.ts index eb134a8ca6f56..47d43fb1c4720 100644 --- a/src/vs/editor/contrib/hover/browser/hover.ts +++ b/src/vs/editor/contrib/hover/browser/hover.ts @@ -206,7 +206,7 @@ export class ModesHoverController implements IEditorContribution { const resolvedKeyboardEvent = this._keybindingService.softDispatch(e, this._editor.getDomNode()); // If the beginning of a multi-chord keybinding is pressed, or the command aims to focus the hover, set the variable to true, otherwise false - const mightTriggerFocus = (resolvedKeyboardEvent?.kind === ResultKind.MoreChordsNeeded || (resolvedKeyboardEvent.kind === ResultKind.KbFound && resolvedKeyboardEvent.commandId === 'editor.action.showHover' && this._contentWidget?.isVisible())); + const mightTriggerFocus = (resolvedKeyboardEvent.kind === ResultKind.MoreChordsNeeded || (resolvedKeyboardEvent.kind === ResultKind.KbFound && resolvedKeyboardEvent.commandId === 'editor.action.showHover' && this._contentWidget?.isVisible())); if (e.keyCode !== KeyCode.Ctrl && e.keyCode !== KeyCode.Alt && e.keyCode !== KeyCode.Meta && e.keyCode !== KeyCode.Shift && !mightTriggerFocus) { diff --git a/src/vs/platform/list/browser/listService.ts b/src/vs/platform/list/browser/listService.ts index 3a44a11558766..69f70223ba94a 100644 --- a/src/vs/platform/list/browser/listService.ts +++ b/src/vs/platform/list/browser/listService.ts @@ -816,7 +816,7 @@ function createKeyboardNavigationEventFilter(keybindingService: IKeybindingServi } inMultiChord = false; - return result === null || result.kind === ResultKind.NoMatchingKb; + return result.kind === ResultKind.NoMatchingKb; }; }