From 3104a4e68ceaabe480893d1864d4af92e9010619 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 4 Apr 2023 13:02:01 -0700 Subject: [PATCH 01/17] Remove uses of `X-UA-Compatible` meta tag (#177739) * Remove uses of `X-UA-Compatible` meta tag We no longer support IE Mainly removing this from the webview html but figured we should remove the other references to it as well * Update tests * Fix test --- build/builtin/index.html | 3 +-- src/vs/base/test/node/pfs/fixtures/index.html | 1 - .../files/test/node/fixtures/resolver/index.html | 1 - .../files/test/node/fixtures/service/index.html | 1 - src/vs/workbench/contrib/webview/browser/pre/fake.html | 1 - .../contrib/webview/browser/pre/index-no-csp.html | 2 -- .../workbench/contrib/webview/browser/pre/index.html | 2 -- .../services/search/test/node/fixtures/index.html | 1 - .../search/test/node/textSearch.integrationTest.ts | 10 +++++----- 9 files changed, 6 insertions(+), 16 deletions(-) diff --git a/build/builtin/index.html b/build/builtin/index.html index 13c84e0375c..dc2a7ca6d0d 100644 --- a/build/builtin/index.html +++ b/build/builtin/index.html @@ -5,7 +5,6 @@ - Manage Built-in Extensions @@ -43,4 +42,4 @@
- \ No newline at end of file + diff --git a/src/vs/base/test/node/pfs/fixtures/index.html b/src/vs/base/test/node/pfs/fixtures/index.html index bccd24d9272..165a4ecbccd 100644 --- a/src/vs/base/test/node/pfs/fixtures/index.html +++ b/src/vs/base/test/node/pfs/fixtures/index.html @@ -1,7 +1,6 @@ - Strada diff --git a/src/vs/platform/files/test/node/fixtures/resolver/index.html b/src/vs/platform/files/test/node/fixtures/resolver/index.html index bccd24d9272..165a4ecbccd 100644 --- a/src/vs/platform/files/test/node/fixtures/resolver/index.html +++ b/src/vs/platform/files/test/node/fixtures/resolver/index.html @@ -1,7 +1,6 @@ - Strada diff --git a/src/vs/platform/files/test/node/fixtures/service/index.html b/src/vs/platform/files/test/node/fixtures/service/index.html index bccd24d9272..165a4ecbccd 100644 --- a/src/vs/platform/files/test/node/fixtures/service/index.html +++ b/src/vs/platform/files/test/node/fixtures/service/index.html @@ -1,7 +1,6 @@ - Strada diff --git a/src/vs/workbench/contrib/webview/browser/pre/fake.html b/src/vs/workbench/contrib/webview/browser/pre/fake.html index 726a69397f8..960d1186be5 100644 --- a/src/vs/workbench/contrib/webview/browser/pre/fake.html +++ b/src/vs/workbench/contrib/webview/browser/pre/fake.html @@ -3,7 +3,6 @@ - Fake diff --git a/src/vs/workbench/contrib/webview/browser/pre/index-no-csp.html b/src/vs/workbench/contrib/webview/browser/pre/index-no-csp.html index 471bebaa5fe..6e8462b4c2c 100644 --- a/src/vs/workbench/contrib/webview/browser/pre/index-no-csp.html +++ b/src/vs/workbench/contrib/webview/browser/pre/index-no-csp.html @@ -7,8 +7,6 @@ - - diff --git a/src/vs/workbench/contrib/webview/browser/pre/index.html b/src/vs/workbench/contrib/webview/browser/pre/index.html index 514dfccb746..251b047c7c2 100644 --- a/src/vs/workbench/contrib/webview/browser/pre/index.html +++ b/src/vs/workbench/contrib/webview/browser/pre/index.html @@ -10,8 +10,6 @@ - - diff --git a/src/vs/workbench/services/search/test/node/fixtures/index.html b/src/vs/workbench/services/search/test/node/fixtures/index.html index bccd24d9272..165a4ecbccd 100644 --- a/src/vs/workbench/services/search/test/node/fixtures/index.html +++ b/src/vs/workbench/services/search/test/node/fixtures/index.html @@ -1,7 +1,6 @@ - Strada diff --git a/src/vs/workbench/services/search/test/node/textSearch.integrationTest.ts b/src/vs/workbench/services/search/test/node/textSearch.integrationTest.ts index 50fd21a0883..c8095086203 100644 --- a/src/vs/workbench/services/search/test/node/textSearch.integrationTest.ts +++ b/src/vs/workbench/services/search/test/node/textSearch.integrationTest.ts @@ -157,7 +157,7 @@ flakySuite('TextSearch-integration', function () { contentPattern: { pattern: 'e' } }; - return doSearchTest(config, 788); + return doSearchTest(config, 781); }); test('Text: e (with excludes)', () => { @@ -167,7 +167,7 @@ flakySuite('TextSearch-integration', function () { excludePattern: { '**/examples': true } }; - return doSearchTest(config, 394); + return doSearchTest(config, 387); }); test('Text: e (with includes)', () => { @@ -344,12 +344,12 @@ flakySuite('TextSearch-integration', function () { return doSearchTest(config, 4).then(results => { assert.strictEqual(results.length, 4); - assert.strictEqual((results[0].results![0]).lineNumber, 25); + assert.strictEqual((results[0].results![0]).lineNumber, 24); assert.strictEqual((results[0].results![0]).text, ' compiler.addUnit(prog,"input.ts");'); // assert.strictEqual((results[1].results[0]).preview.text, ' compiler.typeCheck();\n'); // See https://github.com/BurntSushi/ripgrep/issues/1095 - assert.strictEqual((results[2].results![0]).lineNumber, 27); + assert.strictEqual((results[2].results![0]).lineNumber, 26); assert.strictEqual((results[2].results![0]).text, ' compiler.emit();'); - assert.strictEqual((results[3].results![0]).lineNumber, 28); + assert.strictEqual((results[3].results![0]).lineNumber, 27); assert.strictEqual((results[3].results![0]).text, ''); }); }); From 73fde83632acf0eacdd12cb6ee6b61d3f8100260 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Wed, 29 Mar 2023 11:28:51 +0200 Subject: [PATCH 02/17] abstractKeybindingService: refactor: don't use `null` when an empty array is enough --- .../common/abstractKeybindingService.ts | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index 22fc1ada9c1..aece3b261d6 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -30,6 +30,7 @@ interface CurrentChord { const HIGH_FREQ_COMMANDS = /^(cursor|delete|undo|redo|tab|editor\.action\.clipboard)/; export abstract class AbstractKeybindingService extends Disposable implements IKeybindingService { + public _serviceBrand: undefined; protected readonly _onDidUpdateKeybindings: Emitter = this._register(new Emitter()); @@ -44,7 +45,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK * "cmd+k" would be stored in this array, when on pressing "cmd+i", the service * would invoke the command bound by the keybinding */ - private _currentChords: CurrentChord[] | null; + private _currentChords: CurrentChord[]; private _currentChordChecker: IntervalTimer; private _currentChordStatusMessage: IDisposable | null; @@ -55,7 +56,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK protected _logging: boolean; public get inChordMode(): boolean { - return !!this._currentChords; + return this._currentChords.length > 0; } constructor( @@ -67,7 +68,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK ) { super(); - this._currentChords = null; + this._currentChords = []; this._currentChordChecker = new IntervalTimer(); this._currentChordStatusMessage = null; this._ignoreSingleModifiers = KeybindingModifierSet.EMPTY; @@ -149,7 +150,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK } const contextValue = this._contextKeyService.getContext(target); - const currentChords = this._currentChords ? this._currentChords.map((({ keypress }) => keypress)) : null; + const currentChords = this._currentChords.length > 0 ? this._currentChords.map((({ keypress }) => keypress)) : null; // TODO@ulugbekna: adapt `resolve` to accept [] return this._getResolver().resolve(contextValue, currentChords, firstChord); } @@ -172,21 +173,14 @@ export abstract class AbstractKeybindingService extends Disposable implements IK } private _enterMultiChordMode(firstChord: string, keypressLabel: string | null): void { - this._currentChords = [{ - keypress: firstChord, - label: keypressLabel - }]; + this._currentChords.push({ keypress: firstChord, label: keypressLabel }); this._currentChordStatusMessage = this._notificationService.status(nls.localize('first.chord', "({0}) was pressed. Waiting for second key of chord...", keypressLabel)); this._scheduleLeaveChordMode(); IME.disable(); } private _continueMultiChordMode(nextChord: string, keypressLabel: string | null): void { - this._currentChords = this._currentChords ? this._currentChords : []; - this._currentChords.push({ - keypress: nextChord, - label: keypressLabel - }); + this._currentChords.push({ keypress: nextChord, label: keypressLabel }); const fullKeypressLabel = this._currentChords.map(({ label }) => label).join(', '); this._currentChordStatusMessage = this._notificationService.status(nls.localize('next.chord', "({0}) was pressed. Waiting for next key of chord...", fullKeypressLabel)); this._scheduleLeaveChordMode(); @@ -198,7 +192,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK this._currentChordStatusMessage = null; } this._currentChordChecker.cancel(); - this._currentChords = null; + this._currentChords = []; IME.enable(); } @@ -287,10 +281,10 @@ export abstract class AbstractKeybindingService extends Disposable implements IK // hence we disregard `_currentChord` and use the same modifier instead. const [dispatchKeyname,] = userKeypress.getSingleModifierDispatchChords(); userPressedChord = dispatchKeyname; - currentChords = dispatchKeyname ? [dispatchKeyname] : []; + currentChords = dispatchKeyname ? [dispatchKeyname] : []; // TODO@ulugbekna: in the `else` case we assign an empty array - make sure `resolve` can handle an empty array well } else { [userPressedChord,] = userKeypress.getDispatchChords(); - currentChords = this._currentChords ? this._currentChords.map(({ keypress }) => keypress) : null; + currentChords = this._currentChords.length > 0 ? this._currentChords.map(({ keypress }) => keypress) : null; } if (userPressedChord === null) { @@ -313,7 +307,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK return shouldPreventDefault; } - if (this._currentChords) { + if (this._currentChords.length > 0) { if (resolveResult && !resolveResult.leaveMultiChord) { shouldPreventDefault = true; this._continueMultiChordMode(userPressedChord, keypressLabel); From 9a90445e3c7334a4c247a6de0508ccbbd898f212 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Wed, 29 Mar 2023 11:38:53 +0200 Subject: [PATCH 03/17] keybindingResolver: refactor: easier case first --- .../keybinding/common/keybindingResolver.ts | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 938a78d85a8..3f14d059010 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -283,12 +283,22 @@ export class KeybindingResolver { public resolve(context: IContext, currentChords: string[] | null, keypress: string): IResolveResult | null { this._log(`| Resolving ${keypress}${currentChords ? ` chorded from ${currentChords}` : ``}`); + let lookupMap: ResolvedKeybindingItem[] | null = null; - if (currentChords !== null) { + if (currentChords === null) { + const candidates = this._map.get(keypress); + if (candidates === undefined) { + // No bindings with `keypress` + this._log(`\\ No keybinding entries.`); + return null; + } + + lookupMap = candidates; + } else { // Fetch all chord bindings for `currentChords` const candidates = this._map.get(currentChords[0]); - if (typeof candidates === 'undefined') { + if (candidates === undefined) { // No chords starting with `currentChords` this._log(`\\ No keybinding entries.`); return null; @@ -312,15 +322,6 @@ export class KeybindingResolver { lookupMap.push(candidate); } } - } else { - const candidates = this._map.get(keypress); - if (typeof candidates === 'undefined') { - // No bindings with `keypress` - this._log(`\\ No keybinding entries.`); - return null; - } - - lookupMap = candidates; } const result = this._findCommand(context, lookupMap); From 02f8853ec7984ef1dd6c9ff2ee8ade5b2564d69c Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Wed, 29 Mar 2023 16:54:18 +0200 Subject: [PATCH 04/17] keybindingResolver: refactor: use a discriminated union to make it easier to reason about keybinding resolution result --- src/vs/editor/contrib/hover/browser/hover.ts | 3 +- .../common/abstractKeybindingService.ts | 132 +++++++++++------- .../platform/keybinding/common/keybinding.ts | 4 +- .../keybinding/common/keybindingResolver.ts | 71 +++++----- .../test/common/keybindingResolver.test.ts | 34 +++-- .../test/common/mockKeybindingService.ts | 4 +- src/vs/platform/list/browser/listService.ts | 3 +- .../browser/actions/developerActions.ts | 13 +- .../browser/parts/dialogs/dialogHandler.ts | 3 +- .../terminal/browser/terminalInstance.ts | 5 +- .../progress/browser/progressService.ts | 3 +- 11 files changed, 165 insertions(+), 110 deletions(-) diff --git a/src/vs/editor/contrib/hover/browser/hover.ts b/src/vs/editor/contrib/hover/browser/hover.ts index 10df5c93513..3e7b591bc73 100644 --- a/src/vs/editor/contrib/hover/browser/hover.ts +++ b/src/vs/editor/contrib/hover/browser/hover.ts @@ -30,6 +30,7 @@ import { MarkerHoverParticipant } from 'vs/editor/contrib/hover/browser/markerHo import 'vs/css!./hover'; import { InlineSuggestionHintsContentWidget } from 'vs/editor/contrib/inlineCompletions/browser/inlineSuggestionHintsWidget'; import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; +import { ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; export class ModesHoverController implements IEditorContribution { @@ -205,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?.enterMultiChord || (resolvedKeyboardEvent?.commandId === 'editor.action.showHover' && this._contentWidget?.isVisible())); + const mightTriggerFocus = (resolvedKeyboardEvent?.kind === ResultKind.MoreChordsNeeded || (resolvedKeyboardEvent && 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 aece3b261d6..f8aa3fd5708 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -6,6 +6,7 @@ import { WorkbenchActionExecutedClassification, WorkbenchActionExecutedEvent } from 'vs/base/common/actions'; import * as arrays from 'vs/base/common/arrays'; import { IntervalTimer, TimeoutTimer } from 'vs/base/common/async'; +import { illegalState } from 'vs/base/common/errors'; import { Emitter, Event } from 'vs/base/common/event'; import { IME } from 'vs/base/common/ime'; import { KeyCode } from 'vs/base/common/keyCodes'; @@ -16,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 { IResolveResult, KeybindingResolver } from 'vs/platform/keybinding/common/keybindingResolver'; +import { ResolutionResult, KeybindingResolver, ResultKind } 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'; @@ -135,7 +136,9 @@ export abstract class AbstractKeybindingService extends Disposable implements IK return this._dispatch(e, target); } - public softDispatch(e: IKeyboardEvent, target: IContextKeyServiceTarget): IResolveResult | null { + // 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 { this._log(`/ Soft dispatching keyboard event`); const keybinding = this.resolveKeyboardEvent(e); if (keybinding.hasMultipleChords()) { @@ -172,18 +175,28 @@ export abstract class AbstractKeybindingService extends Disposable implements IK }, 500); } - private _enterMultiChordMode(firstChord: string, keypressLabel: string | null): void { - this._currentChords.push({ keypress: firstChord, label: keypressLabel }); - this._currentChordStatusMessage = this._notificationService.status(nls.localize('first.chord', "({0}) was pressed. Waiting for second key of chord...", keypressLabel)); - this._scheduleLeaveChordMode(); - IME.disable(); - } + private _expectAnotherChord(firstChord: string, keypressLabel: string | null): void { + + this._currentChords.push({ keypress: firstChord, label: keypressLabel }); + + switch (this._currentChords.length) { + case 0: + throw illegalState('impossible'); + case 1: + // TODO@ulugbekna: revise this message and the one below (at least, fix terminology) + this._currentChordStatusMessage = this._notificationService.status(nls.localize('first.chord', "({0}) was pressed. Waiting for second key of chord...", keypressLabel)); + break; + default: { + const fullKeypressLabel = this._currentChords.map(({ label }) => label).join(', '); + this._currentChordStatusMessage = this._notificationService.status(nls.localize('next.chord', "({0}) was pressed. Waiting for next key of chord...", fullKeypressLabel)); + } + } - private _continueMultiChordMode(nextChord: string, keypressLabel: string | null): void { - this._currentChords.push({ keypress: nextChord, label: keypressLabel }); - const fullKeypressLabel = this._currentChords.map(({ label }) => label).join(', '); - this._currentChordStatusMessage = this._notificationService.status(nls.localize('next.chord', "({0}) was pressed. Waiting for next key of chord...", fullKeypressLabel)); this._scheduleLeaveChordMode(); + + if (IME.enabled) { + IME.disable(); + } } private _leaveChordMode(): void { @@ -298,47 +311,72 @@ export abstract class AbstractKeybindingService extends Disposable implements IK const resolveResult = this._getResolver().resolve(contextValue, currentChords, userPressedChord); - this._logService.trace('KeybindingService#dispatch', keypressLabel, resolveResult?.commandId); + switch (resolveResult.kind) { - if (resolveResult && resolveResult.enterMultiChord) { - shouldPreventDefault = true; - this._enterMultiChordMode(userPressedChord, keypressLabel); - this._log(`+ Entering chord mode...`); - return shouldPreventDefault; - } + case ResultKind.NoMatchingKb: { - if (this._currentChords.length > 0) { - if (resolveResult && !resolveResult.leaveMultiChord) { - shouldPreventDefault = true; - this._continueMultiChordMode(userPressedChord, keypressLabel); - this._log(`+ Continuing chord mode...`); + this._logService.trace('KeybindingService#dispatch', keypressLabel, `[ No matching keybinding ]`); + + if (this.inChordMode) { + const currentChordsLabel = this._currentChords.map(({ label }) => label).join(', '); + this._log(`+ Leaving multi-chord mode: Nothing bound to "${currentChordsLabel}, ${keypressLabel}".`); + this._notificationService.status(nls.localize('missing.chord', "The key combination ({0}, {1}) is not a command.", currentChordsLabel, keypressLabel), { hideAfter: 10 * 1000 /* 10s */ }); + this._leaveChordMode(); + + shouldPreventDefault = true; + } return shouldPreventDefault; - } else if (!resolveResult || !resolveResult.commandId) { - const currentChordsLabel = this._currentChords.map(({ label }) => label).join(', '); - this._log(`+ Leaving chord mode: Nothing bound to "${currentChordsLabel}, ${keypressLabel}".`); - this._notificationService.status(nls.localize('missing.chord', "The key combination ({0}, {1}) is not a command.", currentChordsLabel, keypressLabel), { hideAfter: 10 * 1000 /* 10s */ }); + } + + case ResultKind.MoreChordsNeeded: { + + this._logService.trace('KeybindingService#dispatch', keypressLabel, `[ Several keybindings match - more chords needed ]`); + shouldPreventDefault = true; + this._expectAnotherChord(userPressedChord, keypressLabel); + this._log(this._currentChords.length === 1 ? `+ Entering multi-chord mode...` : `+ Continuing multi-chord mode...`); + return shouldPreventDefault; + } + + case ResultKind.KbFound: { + + this._logService.trace('KeybindingService#dispatch', keypressLabel, `[ Will dispatch command ${resolveResult.commandId} ]`); + + if (resolveResult.commandId === null) { + + if (this.inChordMode) { + const currentChordsLabel = this._currentChords.map(({ label }) => label).join(', '); + this._log(`+ Leaving chord mode: Nothing bound to "${currentChordsLabel}, ${keypressLabel}".`); + this._notificationService.status(nls.localize('missing.chord', "The key combination ({0}, {1}) is not a command.", currentChordsLabel, keypressLabel), { hideAfter: 10 * 1000 /* 10s */ }); + this._leaveChordMode(); + } + + shouldPreventDefault = true; + + } else { + if (this.inChordMode) { + this._leaveChordMode(); + } + + if (!resolveResult.isBubble) { + shouldPreventDefault = true; + } + + this._log(`+ Invoking command ${resolveResult.commandId}.`); + if (typeof resolveResult.commandArgs === 'undefined') { + this._commandService.executeCommand(resolveResult.commandId).then(undefined, err => this._notificationService.warn(err)); + } else { + this._commandService.executeCommand(resolveResult.commandId, resolveResult.commandArgs).then(undefined, err => this._notificationService.warn(err)); + } + + if (!HIGH_FREQ_COMMANDS.test(resolveResult.commandId)) { + this._telemetryService.publicLog2('workbenchActionExecuted', { id: resolveResult.commandId, from: 'keybinding', detail: userKeypress.getUserSettingsLabel() ?? undefined }); + } + } + + return shouldPreventDefault; } } - - this._leaveChordMode(); - - if (resolveResult && resolveResult.commandId) { - if (!resolveResult.bubble) { - shouldPreventDefault = true; - } - this._log(`+ Invoking command ${resolveResult.commandId}.`); - if (typeof resolveResult.commandArgs === 'undefined') { - this._commandService.executeCommand(resolveResult.commandId).then(undefined, err => this._notificationService.warn(err)); - } else { - this._commandService.executeCommand(resolveResult.commandId, resolveResult.commandArgs).then(undefined, err => this._notificationService.warn(err)); - } - if (!HIGH_FREQ_COMMANDS.test(resolveResult.commandId)) { - this._telemetryService.publicLog2('workbenchActionExecuted', { id: resolveResult.commandId, from: 'keybinding', detail: userKeypress.getUserSettingsLabel() ?? undefined }); - } - } - - return shouldPreventDefault; } mightProducePrintableCharacter(event: IKeyboardEvent): boolean { diff --git a/src/vs/platform/keybinding/common/keybinding.ts b/src/vs/platform/keybinding/common/keybinding.ts index 3d4851e8cde..ba156046afa 100644 --- a/src/vs/platform/keybinding/common/keybinding.ts +++ b/src/vs/platform/keybinding/common/keybinding.ts @@ -9,7 +9,7 @@ import { KeyCode } from 'vs/base/common/keyCodes'; import { ResolvedKeybinding, Keybinding } from 'vs/base/common/keybindings'; import { IContextKeyService, IContextKeyServiceTarget } from 'vs/platform/contextkey/common/contextkey'; import { createDecorator } from 'vs/platform/instantiation/common/instantiation'; -import { IResolveResult } from 'vs/platform/keybinding/common/keybindingResolver'; +import { ResolutionResult } from 'vs/platform/keybinding/common/keybindingResolver'; import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem'; export interface IUserFriendlyKeybinding { @@ -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): IResolveResult | null; + softDispatch(keyboardEvent: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null; 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 3f14d059010..b9b0d18a688 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -6,16 +6,35 @@ import { implies, ContextKeyExpression, ContextKeyExprType, IContext, IContextKeyService, expressionsAreEqualWithConstantSubstitution } from 'vs/platform/contextkey/common/contextkey'; import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem'; -export interface IResolveResult { - /** Whether the resolved keybinding is entering a multi chord */ - enterMultiChord: boolean; - /** Whether the resolved keybinding is leaving (and executing) a multi chord keybinding */ - leaveMultiChord: boolean; - commandId: string | null; - commandArgs: any; - bubble: boolean; +//#region resolution-result + +export const enum ResultKind { + /** No keybinding found this sequence of chords */ + NoMatchingKb, + + /** There're several keybindings that have the given sequence of chords as a prefix */ + MoreChordsNeeded, + + /** A single keybinding found to be dispatched/invoked */ + KbFound } +export type ResolutionResult = + | { kind: ResultKind.NoMatchingKb } + | { kind: ResultKind.MoreChordsNeeded } + | { kind: ResultKind.KbFound; commandId: string | null; commandArgs: any; isBubble: boolean }; + + +// util definitions to make working with the above types easier within this module: + +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 }; +} + +//#endregion + /** * Stores mappings from keybindings to commands and from commands to keybindings. * Given a sequence of chords, `resolve`s which keybinding it matches @@ -281,7 +300,7 @@ export class KeybindingResolver { return items[items.length - 1]; } - public resolve(context: IContext, currentChords: string[] | null, keypress: string): IResolveResult | null { + public resolve(context: IContext, currentChords: string[] | null, keypress: string): ResolutionResult { this._log(`| Resolving ${keypress}${currentChords ? ` chorded from ${currentChords}` : ``}`); let lookupMap: ResolvedKeybindingItem[] | null = null; @@ -291,7 +310,7 @@ export class KeybindingResolver { if (candidates === undefined) { // No bindings with `keypress` this._log(`\\ No keybinding entries.`); - return null; + return NoMatchingKb; } lookupMap = candidates; @@ -301,7 +320,7 @@ export class KeybindingResolver { if (candidates === undefined) { // No chords starting with `currentChords` this._log(`\\ No keybinding entries.`); - return null; + return NoMatchingKb; } lookupMap = []; @@ -327,37 +346,19 @@ export class KeybindingResolver { const result = this._findCommand(context, lookupMap); if (!result) { this._log(`\\ From ${lookupMap.length} keybinding entries, no when clauses matched the context.`); - return null; + return NoMatchingKb; } - if (currentChords === null && result.chords.length > 1 && result.chords[1] !== null) { + if (currentChords === null && result.chords.length > 1 && result.chords[1] !== null) { // first chord of a multi-chord KB matched this._log(`\\ From ${lookupMap.length} keybinding entries, matched chord, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); - return { - enterMultiChord: true, - leaveMultiChord: false, - commandId: null, - commandArgs: null, - bubble: false - }; - } else if (currentChords !== null && currentChords.length + 1 < result.chords.length) { + return MoreChordsNeeded; + } else if (currentChords !== null && currentChords.length + 1 < result.chords.length) { // prefix (ie, a sequence of chords) of a multi-chord KB matched, eg 2 out of 3 matched - still need one more to dispatch the KB this._log(`\\ From ${lookupMap.length} keybinding entries, continued chord, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); - return { - enterMultiChord: false, - leaveMultiChord: false, - commandId: null, - commandArgs: null, - bubble: false - }; + return MoreChordsNeeded; } this._log(`\\ From ${lookupMap.length} keybinding entries, matched ${result.command}, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); - return { - enterMultiChord: false, - leaveMultiChord: result.chords.length > 1, - commandId: result.command, - commandArgs: result.commandArgs, - bubble: result.bubble - }; + return KbFound(result.command, result.commandArgs, result.bubble); } private _findCommand(context: IContext, matches: ResolvedKeybindingItem[]): ResolvedKeybindingItem | null { diff --git a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts index 53001faeb45..e3397378558 100644 --- a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts +++ b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts @@ -8,7 +8,7 @@ import { decodeKeybinding, createSimpleKeybinding, KeyCodeChord } from 'vs/base/ import { KeyChord, KeyCode, KeyMod } from 'vs/base/common/keyCodes'; import { OS } from 'vs/base/common/platform'; import { ContextKeyExpr, ContextKeyExpression, IContext } from 'vs/platform/contextkey/common/contextkey'; -import { KeybindingResolver } from 'vs/platform/keybinding/common/keybindingResolver'; +import { KeybindingResolver, ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem'; import { USLayoutResolvedKeybinding } from 'vs/platform/keybinding/common/usLayoutResolvedKeybinding'; import { createUSLayoutResolvedKeybinding } from 'vs/platform/keybinding/test/common/keybindingsTestUtils'; @@ -50,8 +50,13 @@ suite('KeybindingResolver', () => { assert.strictEqual(contextRules.evaluate(createContext({ bar: 'bz' })), false); const resolver = new KeybindingResolver([keybindingItem], [], () => { }); - assert.strictEqual(resolver.resolve(createContext({ bar: 'baz' }), null, getDispatchStr(runtimeKeybinding))!.commandId, 'yes'); - assert.strictEqual(resolver.resolve(createContext({ bar: 'bz' }), null, getDispatchStr(runtimeKeybinding)), null); + + const r1 = resolver.resolve(createContext({ bar: 'baz' }), null, getDispatchStr(runtimeKeybinding)); + assert.ok(r1.kind === ResultKind.KbFound); + assert.strictEqual(r1.commandId, 'yes'); + + const r2 = resolver.resolve(createContext({ bar: 'bz' }), null, getDispatchStr(runtimeKeybinding)); + assert.strictEqual(r2.kind, ResultKind.NoMatchingKb); }); test('resolve key with arguments', () => { @@ -62,7 +67,10 @@ suite('KeybindingResolver', () => { const keybindingItem = kbItem(keybinding, 'yes', commandArgs, contextRules, true); const resolver = new KeybindingResolver([keybindingItem], [], () => { }); - assert.strictEqual(resolver.resolve(createContext({ bar: 'baz' }), null, getDispatchStr(runtimeKeybinding))!.commandArgs, commandArgs); + + const r = resolver.resolve(createContext({ bar: 'baz' }), null, getDispatchStr(runtimeKeybinding)); + assert.ok(r.kind === ResultKind.KbFound); + assert.strictEqual(r.commandArgs, commandArgs); }); test('KeybindingResolver.handleRemovals simple 1', () => { @@ -408,28 +416,26 @@ suite('KeybindingResolver', () => { const expectedKeybinding = decodeKeybinding(_expectedKey, OS)!; let previousChord: string[] | null = null; + for (let i = 0, len = expectedKeybinding.chords.length; i < len; i++) { + const chord = getDispatchStr(expectedKeybinding.chords[i]); + const result = resolver.resolve(ctx, previousChord, chord); + if (i === len - 1) { // if it's the final chord, then we should find a valid command, // and there should not be a chord. - assert.ok(result !== null, `Enters multi chord for ${commandId} at chord ${i}`); - assert.strictEqual(result!.commandId, commandId, `Enters multi chord for ${commandId} at chord ${i}`); - assert.strictEqual(result!.enterMultiChord, false, `Enters multi chord for ${commandId} at chord ${i}`); + assert.ok(result.kind === ResultKind.KbFound, `Enters multi chord for ${commandId} at chord ${i}`); + assert.strictEqual(result.commandId, commandId, `Enters multi chord for ${commandId} at chord ${i}`); } else if (i > 0) { // if this is an intermediate chord, we should not find a valid command, // and there should be an open chord we continue. - assert.ok(result !== null, `Continues multi chord for ${commandId} at chord ${i}`); - assert.strictEqual(result!.commandId, null, `Continues multi chord for ${commandId} at chord ${i}`); - assert.strictEqual(result!.enterMultiChord, false, `Is already in multi chord for ${commandId} at chord ${i}`); - assert.strictEqual(result!.leaveMultiChord, false, `Does not leave multi chord for ${commandId} at chord ${i}`); + assert.ok(result.kind === ResultKind.MoreChordsNeeded, `Continues multi chord for ${commandId} at chord ${i}`); } else { // if it's not the final chord and not an intermediate, then we should not // find a valid command, and we should enter a chord. - assert.ok(result !== null, `Enters multi chord for ${commandId} at chord ${i}`); - assert.strictEqual(result!.commandId, null, `Enters multi chord for ${commandId} at chord ${i}`); - assert.strictEqual(result!.enterMultiChord, true, `Enters multi chord for ${commandId} at chord ${i}`); + assert.ok(result.kind === ResultKind.MoreChordsNeeded, `Enters multi chord for ${commandId} at chord ${i}`); } if (previousChord === null) { previousChord = []; diff --git a/src/vs/platform/keybinding/test/common/mockKeybindingService.ts b/src/vs/platform/keybinding/test/common/mockKeybindingService.ts index dcb6155d25f..d700bf491df 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 { IResolveResult } from 'vs/platform/keybinding/common/keybindingResolver'; +import { ResolutionResult } from 'vs/platform/keybinding/common/keybindingResolver'; import { ResolvedKeybindingItem } from 'vs/platform/keybinding/common/resolvedKeybindingItem'; import { USLayoutResolvedKeybinding } from 'vs/platform/keybinding/common/usLayoutResolvedKeybinding'; @@ -135,7 +135,7 @@ export class MockKeybindingService implements IKeybindingService { return 0; } - public softDispatch(keybinding: IKeyboardEvent, target: IContextKeyServiceTarget): IResolveResult | null { + public softDispatch(keybinding: IKeyboardEvent, target: IContextKeyServiceTarget): ResolutionResult | null { return null; } diff --git a/src/vs/platform/list/browser/listService.ts b/src/vs/platform/list/browser/listService.ts index 62e4fdf6b7e..992f2f2f09e 100644 --- a/src/vs/platform/list/browser/listService.ts +++ b/src/vs/platform/list/browser/listService.ts @@ -26,6 +26,7 @@ import { IContextViewService } from 'vs/platform/contextview/browser/contextView import { IEditorOptions } from 'vs/platform/editor/common/editor'; import { createDecorator, IInstantiationService, ServicesAccessor } from 'vs/platform/instantiation/common/instantiation'; import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding'; +import { ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; import { Registry } from 'vs/platform/registry/common/platform'; import { IStyleOverride, defaultFindWidgetStyles, defaultListStyles, getListStyles } from 'vs/platform/theme/browser/defaultStyles'; @@ -809,7 +810,7 @@ function createKeyboardNavigationEventFilter(keybindingService: IKeybindingServi const result = keybindingService.softDispatch(event, event.target); - if (result?.enterMultiChord) { + 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 269b9af1bd9..cb7929d181d 100644 --- a/src/vs/workbench/browser/actions/developerActions.ts +++ b/src/vs/workbench/browser/actions/developerActions.ts @@ -29,6 +29,7 @@ import { IWorkingCopyService } from 'vs/workbench/services/workingCopy/common/wo import { ServicesAccessor } from 'vs/platform/instantiation/common/instantiation'; import { Categories } from 'vs/platform/action/common/actionCommonCategories'; import { IWorkingCopyBackupService } from 'vs/workbench/services/workingCopy/common/workingCopyBackup'; +import { ResolutionResult, ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; class InspectContextKeysAction extends Action2 { @@ -259,7 +260,7 @@ class ToggleScreencastModeAction extends Action2 { const shortcut = keybindingService.softDispatch(event, event.target); // Hide the single arrow key pressed - if (shortcut?.commandId && configurationService.getValue('screencastMode.hideSingleEditorCursorMoves') && ( + if (shortcut && shortcut.kind === ResultKind.KbFound && shortcut.commandId && configurationService.getValue('screencastMode.hideSingleEditorCursorMoves') && ( ['cursorLeft', 'cursorRight', 'cursorUp', 'cursorDown'].includes(shortcut.commandId)) ) { return; @@ -278,7 +279,7 @@ class ToggleScreencastModeAction extends Action2 { const format = configurationService.getValue<'keys' | 'command' | 'commandWithGroup' | 'commandAndKeys' | 'commandWithGroupAndKeys'>('screencastMode.keyboardShortcutsFormat'); const keybinding = keybindingService.resolveKeyboardEvent(event); - const command = shortcut?.commandId ? MenuRegistry.getCommand(shortcut.commandId) : null; + const command = (this._isKbFound(shortcut) && shortcut.commandId) ? MenuRegistry.getCommand(shortcut.commandId) : null; let titleLabel = ''; let keyLabel: string | undefined | null = keybinding.getLabel(); @@ -290,7 +291,7 @@ class ToggleScreencastModeAction extends Action2 { titleLabel = `${typeof command.category === 'string' ? command.category : command.category.value}: ${titleLabel} `; } - if (shortcut?.commandId) { + if (this._isKbFound(shortcut) && shortcut.commandId) { const keybindings = keybindingService.lookupKeybindings(shortcut.commandId) .filter(k => k.getLabel()?.endsWith(keyLabel ?? '')); @@ -306,7 +307,7 @@ class ToggleScreencastModeAction extends Action2 { append(keyboardMarker, $('span.title', {}, `${titleLabel} `)); } - if (onlyKeyboardShortcuts || !titleLabel || shortcut?.commandId && (format === 'keys' || format === 'commandAndKeys' || format === 'commandWithGroupAndKeys')) { + if (onlyKeyboardShortcuts || !titleLabel || (this._isKbFound(shortcut) && shortcut.commandId) && (format === 'keys' || format === 'commandAndKeys' || format === 'commandWithGroupAndKeys')) { // Fix label for arrow keys keyLabel = keyLabel?.replace('UpArrow', '↑') ?.replace('DownArrow', '↓') @@ -322,6 +323,10 @@ 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; + } } class LogStorageAction extends Action2 { diff --git a/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts b/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts index 64dc789c045..1dbc24af2da 100644 --- a/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts +++ b/src/vs/workbench/browser/parts/dialogs/dialogHandler.ts @@ -19,6 +19,7 @@ import { fromNow } from 'vs/base/common/date'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { MarkdownRenderer } from 'vs/editor/contrib/markdownRenderer/browser/markdownRenderer'; import { defaultButtonStyles, defaultCheckboxStyles, defaultDialogStyles, defaultInputBoxStyles } from 'vs/platform/theme/browser/defaultStyles'; +import { ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; export class BrowserDialogHandler extends AbstractDialogHandler { @@ -127,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?.commandId) { + if (resolved && 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 85c116e3115..70d75777da4 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -88,6 +88,7 @@ import { editorBackground } from 'vs/platform/theme/common/colorRegistry'; import { PANEL_BACKGROUND, SIDE_BAR_BACKGROUND } from 'vs/workbench/common/theme'; import { TerminalExtensionsRegistry } from 'vs/workbench/contrib/terminal/browser/terminalExtensions'; import { ResolvedKeybinding } from 'vs/base/common/keybindings'; +import { ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; const enum Constants { /** @@ -941,7 +942,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?.enterMultiChord && 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; @@ -961,7 +962,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.commandId && this._skipTerminalCommands.some(k => k === resolveResult.commandId) && !this._configHelper.config.sendKeybindingsToShell) { + if (resolveResult && 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/progress/browser/progressService.ts b/src/vs/workbench/services/progress/browser/progressService.ts index 56cbf5259aa..def35449ea6 100644 --- a/src/vs/workbench/services/progress/browser/progressService.ts +++ b/src/vs/workbench/services/progress/browser/progressService.ts @@ -25,6 +25,7 @@ import { IViewsService, IViewDescriptorService, ViewContainerLocation } from 'vs import { IPaneCompositePartService } from 'vs/workbench/services/panecomposite/browser/panecomposite'; import { stripIcons } from 'vs/base/common/iconLabels'; import { defaultButtonStyles, defaultCheckboxStyles, defaultDialogStyles, defaultInputBoxStyles } from 'vs/platform/theme/browser/defaultStyles'; +import { ResultKind } from 'vs/platform/keybinding/common/keybindingResolver'; export class ProgressService extends Disposable implements IProgressService { @@ -556,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?.commandId) { + if (resolved && resolved.kind === ResultKind.KbFound && resolved.commandId) { if (!allowableCommands.includes(resolved.commandId)) { EventHelper.stop(event, true); } From a5a66eb80841784fafe11dd6258c83c09ee1852a Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Wed, 29 Mar 2023 17:39:10 +0200 Subject: [PATCH 05/17] keybindingResolver: refactor: `resolve` doesn't take a null --- .../keybinding/common/abstractKeybindingService.ts | 4 ++-- .../platform/keybinding/common/keybindingResolver.ts | 9 +++++---- .../keybinding/test/common/keybindingResolver.test.ts | 11 ++++------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index f8aa3fd5708..8b0c59b2018 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -153,7 +153,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK } const contextValue = this._contextKeyService.getContext(target); - const currentChords = this._currentChords.length > 0 ? this._currentChords.map((({ keypress }) => keypress)) : null; // TODO@ulugbekna: adapt `resolve` to accept [] + const currentChords = this._currentChords.map((({ keypress }) => keypress)); return this._getResolver().resolve(contextValue, currentChords, firstChord); } @@ -297,7 +297,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK currentChords = dispatchKeyname ? [dispatchKeyname] : []; // TODO@ulugbekna: in the `else` case we assign an empty array - make sure `resolve` can handle an empty array well } else { [userPressedChord,] = userKeypress.getDispatchChords(); - currentChords = this._currentChords.length > 0 ? this._currentChords.map(({ keypress }) => keypress) : null; + currentChords = this._currentChords.map(({ keypress }) => keypress); } if (userPressedChord === null) { diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index b9b0d18a688..37e0cf8d510 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -300,12 +300,13 @@ export class KeybindingResolver { return items[items.length - 1]; } - public resolve(context: IContext, currentChords: string[] | null, keypress: string): ResolutionResult { + public resolve(context: IContext, currentChords: string[], keypress: string): ResolutionResult { + this._log(`| Resolving ${keypress}${currentChords ? ` chorded from ${currentChords}` : ``}`); let lookupMap: ResolvedKeybindingItem[] | null = null; - if (currentChords === null) { + if (currentChords.length === 0) { const candidates = this._map.get(keypress); if (candidates === undefined) { // No bindings with `keypress` @@ -349,10 +350,10 @@ export class KeybindingResolver { return NoMatchingKb; } - if (currentChords === null && result.chords.length > 1 && result.chords[1] !== null) { // first chord of a multi-chord KB matched + if (currentChords.length === 0 && result.chords.length > 1 && result.chords[1] !== null) { // first chord of a multi-chord KB matched this._log(`\\ From ${lookupMap.length} keybinding entries, matched chord, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); return MoreChordsNeeded; - } else if (currentChords !== null && currentChords.length + 1 < result.chords.length) { // prefix (ie, a sequence of chords) of a multi-chord KB matched, eg 2 out of 3 matched - still need one more to dispatch the KB + } else if (currentChords.length !== 0 && currentChords.length + 1 < result.chords.length) { // prefix (ie, a sequence of chords) of a multi-chord KB matched, eg 2 out of 3 matched - still need one more to dispatch the KB this._log(`\\ From ${lookupMap.length} keybinding entries, continued chord, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); return MoreChordsNeeded; } diff --git a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts index e3397378558..220dd4598c2 100644 --- a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts +++ b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts @@ -51,11 +51,11 @@ suite('KeybindingResolver', () => { const resolver = new KeybindingResolver([keybindingItem], [], () => { }); - const r1 = resolver.resolve(createContext({ bar: 'baz' }), null, getDispatchStr(runtimeKeybinding)); + const r1 = resolver.resolve(createContext({ bar: 'baz' }), [], getDispatchStr(runtimeKeybinding)); assert.ok(r1.kind === ResultKind.KbFound); assert.strictEqual(r1.commandId, 'yes'); - const r2 = resolver.resolve(createContext({ bar: 'bz' }), null, getDispatchStr(runtimeKeybinding)); + const r2 = resolver.resolve(createContext({ bar: 'bz' }), [], getDispatchStr(runtimeKeybinding)); assert.strictEqual(r2.kind, ResultKind.NoMatchingKb); }); @@ -68,7 +68,7 @@ suite('KeybindingResolver', () => { const resolver = new KeybindingResolver([keybindingItem], [], () => { }); - const r = resolver.resolve(createContext({ bar: 'baz' }), null, getDispatchStr(runtimeKeybinding)); + const r = resolver.resolve(createContext({ bar: 'baz' }), [], getDispatchStr(runtimeKeybinding)); assert.ok(r.kind === ResultKind.KbFound); assert.strictEqual(r.commandArgs, commandArgs); }); @@ -415,7 +415,7 @@ suite('KeybindingResolver', () => { const testResolve = (ctx: IContext, _expectedKey: number | number[], commandId: string) => { const expectedKeybinding = decodeKeybinding(_expectedKey, OS)!; - let previousChord: string[] | null = null; + const previousChord: string[] = []; for (let i = 0, len = expectedKeybinding.chords.length; i < len; i++) { @@ -437,9 +437,6 @@ suite('KeybindingResolver', () => { // find a valid command, and we should enter a chord. assert.ok(result.kind === ResultKind.MoreChordsNeeded, `Enters multi chord for ${commandId} at chord ${i}`); } - if (previousChord === null) { - previousChord = []; - } previousChord.push(chord); } }; From 9448be4bfab6cc8f4e26fe3e3ef1c8ba8aa5123a Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Thu, 30 Mar 2023 12:31:04 +0200 Subject: [PATCH 06/17] keybindingResolver: refactor: move handle-removal tests into a sub-suite --- .../test/common/keybindingResolver.test.ts | 441 +++++++++--------- 1 file changed, 222 insertions(+), 219 deletions(-) diff --git a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts index 220dd4598c2..9e6afa815a5 100644 --- a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts +++ b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts @@ -73,241 +73,244 @@ suite('KeybindingResolver', () => { assert.strictEqual(r.commandArgs, commandArgs); }); - test('KeybindingResolver.handleRemovals simple 1', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true) - ]; - const overrides = [ - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), false), - ]); - }); + suite('handle keybinding removals', () => { - test('KeybindingResolver.handleRemovals simple 2', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(KeyCode.KeyC, 'yes3', null, ContextKeyExpr.equals('3', 'c'), false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true), - kbItem(KeyCode.KeyC, 'yes3', null, ContextKeyExpr.equals('3', 'c'), false), - ]); - }); + test('simple 1', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true) + ]; + const overrides = [ + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), false), + ]); + }); - test('KeybindingResolver.handleRemovals removal with not matching when', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(KeyCode.KeyA, '-yes1', null, ContextKeyExpr.equals('1', 'b'), false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]); - }); + test('simple 2', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(KeyCode.KeyC, 'yes3', null, ContextKeyExpr.equals('3', 'c'), false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true), + kbItem(KeyCode.KeyC, 'yes3', null, ContextKeyExpr.equals('3', 'c'), false), + ]); + }); - test('KeybindingResolver.handleRemovals removal with not matching keybinding', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(KeyCode.KeyB, '-yes1', null, ContextKeyExpr.equals('1', 'a'), false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]); - }); + test('removal with not matching when', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(KeyCode.KeyA, '-yes1', null, ContextKeyExpr.equals('1', 'b'), false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]); + }); - test('KeybindingResolver.handleRemovals removal with matching keybinding and when', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(KeyCode.KeyA, '-yes1', null, ContextKeyExpr.equals('1', 'a'), false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]); - }); + test('removal with not matching keybinding', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(KeyCode.KeyB, '-yes1', null, ContextKeyExpr.equals('1', 'a'), false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]); + }); - test('KeybindingResolver.handleRemovals removal with unspecified keybinding', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(0, '-yes1', null, ContextKeyExpr.equals('1', 'a'), false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]); - }); + test('removal with matching keybinding and when', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(KeyCode.KeyA, '-yes1', null, ContextKeyExpr.equals('1', 'a'), false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]); + }); - test('KeybindingResolver.handleRemovals removal with unspecified when', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(KeyCode.KeyA, '-yes1', null, undefined, false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]); - }); + test('removal with unspecified keybinding', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(0, '-yes1', null, ContextKeyExpr.equals('1', 'a'), false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]); + }); - test('KeybindingResolver.handleRemovals removal with unspecified when and unspecified keybinding', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(0, '-yes1', null, undefined, false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]); - }); + test('removal with unspecified when', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(KeyCode.KeyA, '-yes1', null, undefined, false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]); + }); - test('issue #138997 KeybindingResolver.handleRemovals removal in default list', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'yes1', null, undefined, true), - kbItem(KeyCode.KeyB, 'yes2', null, undefined, true), - kbItem(0, '-yes1', null, undefined, false) - ]; - const overrides: ResolvedKeybindingItem[] = []; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyB, 'yes2', null, undefined, true) - ]); - }); + test('removal with unspecified when and unspecified keybinding', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(0, '-yes1', null, undefined, false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]); + }); - test('issue #612#issuecomment-222109084 cannot remove keybindings for commands with ^', () => { - const defaults = [ - kbItem(KeyCode.KeyA, '^yes1', null, ContextKeyExpr.equals('1', 'a'), true), - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]; - const overrides = [ - kbItem(KeyCode.KeyA, '-yes1', null, undefined, false) - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) - ]); - }); + test('issue #138997 - removal in default list', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'yes1', null, undefined, true), + kbItem(KeyCode.KeyB, 'yes2', null, undefined, true), + kbItem(0, '-yes1', null, undefined, false) + ]; + const overrides: ResolvedKeybindingItem[] = []; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyB, 'yes2', null, undefined, true) + ]); + }); - test('issue #140884 Unable to reassign F1 as keybinding for Show All Commands', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'command1', null, undefined, true), - ]; - const overrides = [ - kbItem(KeyCode.KeyA, '-command1', null, undefined, false), - kbItem(KeyCode.KeyA, 'command1', null, undefined, false), - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyA, 'command1', null, undefined, false) - ]); - }); + test('issue #612#issuecomment-222109084 cannot remove keybindings for commands with ^', () => { + const defaults = [ + kbItem(KeyCode.KeyA, '^yes1', null, ContextKeyExpr.equals('1', 'a'), true), + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]; + const overrides = [ + kbItem(KeyCode.KeyA, '-yes1', null, undefined, false) + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyB, 'yes2', null, ContextKeyExpr.equals('2', 'b'), true) + ]); + }); - test('issue #141638: Keyboard Shortcuts: Change When Expression might actually remove keybinding in Insiders', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'command1', null, undefined, true), - ]; - const overrides = [ - kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.equals('a', '1'), false), - kbItem(KeyCode.KeyA, '-command1', null, undefined, false), - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, [ - kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.equals('a', '1'), false) - ]); - }); + test('issue #140884 Unable to reassign F1 as keybinding for Show All Commands', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'command1', null, undefined, true), + ]; + const overrides = [ + kbItem(KeyCode.KeyA, '-command1', null, undefined, false), + kbItem(KeyCode.KeyA, 'command1', null, undefined, false), + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyA, 'command1', null, undefined, false) + ]); + }); - test('issue #157751: Auto-quoting of context keys prevents removal of keybindings via UI', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.deserialize(`editorTextFocus && activeEditor != workbench.editor.notebook && editorLangId in julia.supportedLanguageIds`), true), - ]; - const overrides = [ - kbItem(KeyCode.KeyA, '-command1', null, ContextKeyExpr.deserialize(`editorTextFocus && activeEditor != 'workbench.editor.notebook' && editorLangId in 'julia.supportedLanguageIds'`), false), - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, []); - }); + test('issue #141638: Keyboard Shortcuts: Change When Expression might actually remove keybinding in Insiders', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'command1', null, undefined, true), + ]; + const overrides = [ + kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.equals('a', '1'), false), + kbItem(KeyCode.KeyA, '-command1', null, undefined, false), + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, [ + kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.equals('a', '1'), false) + ]); + }); - test('issue #160604: Remove keybindings with when clause does not work', () => { - const defaults = [ - kbItem(KeyCode.KeyA, 'command1', null, undefined, true), - ]; - const overrides = [ - kbItem(KeyCode.KeyA, '-command1', null, ContextKeyExpr.true(), false), - ]; - const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); - assert.deepStrictEqual(actual, []); - }); + test('issue #157751: Auto-quoting of context keys prevents removal of keybindings via UI', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.deserialize(`editorTextFocus && activeEditor != workbench.editor.notebook && editorLangId in julia.supportedLanguageIds`), true), + ]; + const overrides = [ + kbItem(KeyCode.KeyA, '-command1', null, ContextKeyExpr.deserialize(`editorTextFocus && activeEditor != 'workbench.editor.notebook' && editorLangId in 'julia.supportedLanguageIds'`), false), + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, []); + }); - test('contextIsEntirelyIncluded', () => { - const toContextKeyExpression = (expr: ContextKeyExpression | string | null) => { - if (typeof expr === 'string' || !expr) { - return ContextKeyExpr.deserialize(expr); - } - return expr; - }; - const assertIsIncluded = (a: ContextKeyExpression | string | null, b: ContextKeyExpression | string | null) => { - assert.strictEqual(KeybindingResolver.whenIsEntirelyIncluded(toContextKeyExpression(a), toContextKeyExpression(b)), true); - }; - const assertIsNotIncluded = (a: ContextKeyExpression | string | null, b: ContextKeyExpression | string | null) => { - assert.strictEqual(KeybindingResolver.whenIsEntirelyIncluded(toContextKeyExpression(a), toContextKeyExpression(b)), false); - }; + test('issue #160604: Remove keybindings with when clause does not work', () => { + const defaults = [ + kbItem(KeyCode.KeyA, 'command1', null, undefined, true), + ]; + const overrides = [ + kbItem(KeyCode.KeyA, '-command1', null, ContextKeyExpr.true(), false), + ]; + const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]); + assert.deepStrictEqual(actual, []); + }); - assertIsIncluded(null, null); - assertIsIncluded(null, ContextKeyExpr.true()); - assertIsIncluded(ContextKeyExpr.true(), null); - assertIsIncluded(ContextKeyExpr.true(), ContextKeyExpr.true()); - assertIsIncluded('key1', null); - assertIsIncluded('key1', ''); - assertIsIncluded('key1', 'key1'); - assertIsIncluded('key1', ContextKeyExpr.true()); - assertIsIncluded('!key1', ''); - assertIsIncluded('!key1', '!key1'); - assertIsIncluded('key2', ''); - assertIsIncluded('key2', 'key2'); - assertIsIncluded('key1 && key1 && key2 && key2', 'key2'); - assertIsIncluded('key1 && key2', 'key2'); - assertIsIncluded('key1 && key2', 'key1'); - assertIsIncluded('key1 && key2', ''); - assertIsIncluded('key1', 'key1 || key2'); - assertIsIncluded('key1 || !key1', 'key2 || !key2'); - assertIsIncluded('key1', 'key1 || key2 && key3'); + test('contextIsEntirelyIncluded', () => { + const toContextKeyExpression = (expr: ContextKeyExpression | string | null) => { + if (typeof expr === 'string' || !expr) { + return ContextKeyExpr.deserialize(expr); + } + return expr; + }; + const assertIsIncluded = (a: ContextKeyExpression | string | null, b: ContextKeyExpression | string | null) => { + assert.strictEqual(KeybindingResolver.whenIsEntirelyIncluded(toContextKeyExpression(a), toContextKeyExpression(b)), true); + }; + const assertIsNotIncluded = (a: ContextKeyExpression | string | null, b: ContextKeyExpression | string | null) => { + assert.strictEqual(KeybindingResolver.whenIsEntirelyIncluded(toContextKeyExpression(a), toContextKeyExpression(b)), false); + }; - assertIsNotIncluded('key1', '!key1'); - assertIsNotIncluded('!key1', 'key1'); - assertIsNotIncluded('key1 && key2', 'key3'); - assertIsNotIncluded('key1 && key2', 'key4'); - assertIsNotIncluded('key1', 'key2'); - assertIsNotIncluded('key1 || key2', 'key2'); - assertIsNotIncluded('', 'key2'); - assertIsNotIncluded(null, 'key2'); + assertIsIncluded(null, null); + assertIsIncluded(null, ContextKeyExpr.true()); + assertIsIncluded(ContextKeyExpr.true(), null); + assertIsIncluded(ContextKeyExpr.true(), ContextKeyExpr.true()); + assertIsIncluded('key1', null); + assertIsIncluded('key1', ''); + assertIsIncluded('key1', 'key1'); + assertIsIncluded('key1', ContextKeyExpr.true()); + assertIsIncluded('!key1', ''); + assertIsIncluded('!key1', '!key1'); + assertIsIncluded('key2', ''); + assertIsIncluded('key2', 'key2'); + assertIsIncluded('key1 && key1 && key2 && key2', 'key2'); + assertIsIncluded('key1 && key2', 'key2'); + assertIsIncluded('key1 && key2', 'key1'); + assertIsIncluded('key1 && key2', ''); + assertIsIncluded('key1', 'key1 || key2'); + assertIsIncluded('key1 || !key1', 'key2 || !key2'); + assertIsIncluded('key1', 'key1 || key2 && key3'); + + assertIsNotIncluded('key1', '!key1'); + assertIsNotIncluded('!key1', 'key1'); + assertIsNotIncluded('key1 && key2', 'key3'); + assertIsNotIncluded('key1 && key2', 'key4'); + assertIsNotIncluded('key1', 'key2'); + assertIsNotIncluded('key1 || key2', 'key2'); + assertIsNotIncluded('', 'key2'); + assertIsNotIncluded(null, 'key2'); + }); }); test('resolve command', () => { From ce36e4014fef7715afea1c19854aa279a75b369d Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Thu, 30 Mar 2023 12:40:52 +0200 Subject: [PATCH 07/17] keybindingResolver: tests: refactor: make tests more extensible to write more specification-like tests --- .../test/common/keybindingResolver.test.ts | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts index 9e6afa815a5..2344e5fd058 100644 --- a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts +++ b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts @@ -313,7 +313,7 @@ suite('KeybindingResolver', () => { }); }); - test('resolve command', () => { + suite('resolve command', () => { function _kbItem(keybinding: number | number[], command: string, when: ContextKeyExpression | undefined): ResolvedKeybindingItem { return kbItem(keybinding, command, null, when, true); @@ -404,7 +404,7 @@ suite('KeybindingResolver', () => { const resolver = new KeybindingResolver(items, [], () => { }); - const testKey = (commandId: string, expectedKeys: number[] | number[][]) => { + const testKbLookupByCommand = (commandId: string, expectedKeys: number[] | number[][]) => { // Test lookup const lookupResult = resolver.lookupKeybindings(commandId); assert.strictEqual(lookupResult.length, expectedKeys.length, 'Length mismatch @ commandId ' + commandId); @@ -444,37 +444,40 @@ suite('KeybindingResolver', () => { } }; - testKey('first', []); + test('resolve command - several', () => { - testKey('second', [KeyCode.KeyZ, KeyCode.KeyX]); - testResolve(createContext({ key2: true }), KeyCode.KeyX, 'second'); - testResolve(createContext({}), KeyCode.KeyZ, 'second'); + testKbLookupByCommand('first', []); - testKey('third', [KeyCode.KeyX]); - testResolve(createContext({ key3: true }), KeyCode.KeyX, 'third'); + testKbLookupByCommand('second', [KeyCode.KeyZ, KeyCode.KeyX]); + testResolve(createContext({ key2: true }), KeyCode.KeyX, 'second'); + testResolve(createContext({}), KeyCode.KeyZ, 'second'); - testKey('fourth', []); + testKbLookupByCommand('third', [KeyCode.KeyX]); + testResolve(createContext({ key3: true }), KeyCode.KeyX, 'third'); - testKey('fifth', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyY, KeyCode.KeyZ)]); - testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyY, KeyCode.KeyZ), 'fifth'); + testKbLookupByCommand('fourth', []); - testKey('seventh', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyK)]); - testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyK), 'seventh'); + testKbLookupByCommand('fifth', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyY, KeyCode.KeyZ)]); + testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyY, KeyCode.KeyZ), 'fifth'); - testKey('uncomment lines', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyU)]); - testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyU), 'uncomment lines'); + testKbLookupByCommand('seventh', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyK)]); + testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyK), 'seventh'); - testKey('comment lines', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC)]); - testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC), 'comment lines'); + testKbLookupByCommand('uncomment lines', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyU)]); + testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyU), 'uncomment lines'); - testKey('unreachablechord', []); + testKbLookupByCommand('comment lines', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC)]); + testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC), 'comment lines'); - testKey('eleven', [KeyMod.CtrlCmd | KeyCode.KeyG]); - testResolve(createContext({}), KeyMod.CtrlCmd | KeyCode.KeyG, 'eleven'); + testKbLookupByCommand('unreachablechord', []); - testKey('sixth', []); + testKbLookupByCommand('eleven', [KeyMod.CtrlCmd | KeyCode.KeyG]); + testResolve(createContext({}), KeyMod.CtrlCmd | KeyCode.KeyG, 'eleven'); - testKey('long multi chord', [[KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB]]); - testResolve(createContext({}), [KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB], 'long multi chord'); + testKbLookupByCommand('sixth', []); + + testKbLookupByCommand('long multi chord', [[KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB]]); + testResolve(createContext({}), [KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB], 'long multi chord'); + }); }); }); From 7655273888f4bddcd6be67870d3b595233ebead3 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Thu, 30 Mar 2023 14:51:49 +0200 Subject: [PATCH 08/17] keybindingResolver: tests: refactor: split into separate tests --- .../test/common/keybindingResolver.test.ts | 49 ++++++++++++++++--- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts index 2344e5fd058..72e8026e5ca 100644 --- a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts +++ b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts @@ -381,25 +381,35 @@ suite('KeybindingResolver', () => { undefined ), _kbItem( - KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC), + KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC), // cmd+k cmd+c 'comment lines', undefined ), _kbItem( - KeyChord(KeyMod.CtrlCmd | KeyCode.KeyG, KeyMod.CtrlCmd | KeyCode.KeyC), + KeyChord(KeyMod.CtrlCmd | KeyCode.KeyG, KeyMod.CtrlCmd | KeyCode.KeyC), // cmd+g cmd+c 'unreachablechord', undefined ), _kbItem( - KeyMod.CtrlCmd | KeyCode.KeyG, + KeyMod.CtrlCmd | KeyCode.KeyG, // cmd+g 'eleven', undefined ), _kbItem( - [KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB], + [KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB], // cmd+k a b 'long multi chord', undefined ), + _kbItem( + [KeyMod.CtrlCmd | KeyCode.KeyB, KeyMod.CtrlCmd | KeyCode.KeyC], // cmd+b cmd+c + 'shadowed by long-multi-chord-2', + undefined + ), + _kbItem( + [KeyMod.CtrlCmd | KeyCode.KeyB, KeyMod.CtrlCmd | KeyCode.KeyC, KeyCode.KeyI], // cmd+b cmd+c i + 'long-multi-chord-2', + undefined + ) ]; const resolver = new KeybindingResolver(items, [], () => { }); @@ -444,40 +454,67 @@ suite('KeybindingResolver', () => { } }; - test('resolve command - several', () => { - + test('resolve command - 1', () => { testKbLookupByCommand('first', []); + }); + test('resolve command - 2', () => { testKbLookupByCommand('second', [KeyCode.KeyZ, KeyCode.KeyX]); testResolve(createContext({ key2: true }), KeyCode.KeyX, 'second'); testResolve(createContext({}), KeyCode.KeyZ, 'second'); + }); + test('resolve command - 3', () => { testKbLookupByCommand('third', [KeyCode.KeyX]); testResolve(createContext({ key3: true }), KeyCode.KeyX, 'third'); + }); + test('resolve command - 4', () => { testKbLookupByCommand('fourth', []); + }); + test('resolve command - 5', () => { testKbLookupByCommand('fifth', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyY, KeyCode.KeyZ)]); testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyY, KeyCode.KeyZ), 'fifth'); + }); + test('resolve command - 6', () => { testKbLookupByCommand('seventh', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyK)]); testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyK), 'seventh'); + }); + test('resolve command - 7', () => { testKbLookupByCommand('uncomment lines', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyU)]); testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyU), 'uncomment lines'); + }); + test('resolve command - 8', () => { testKbLookupByCommand('comment lines', [KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC)]); testResolve(createContext({}), KeyChord(KeyMod.CtrlCmd | KeyCode.KeyK, KeyMod.CtrlCmd | KeyCode.KeyC), 'comment lines'); + }); + test('resolve command - 9', () => { testKbLookupByCommand('unreachablechord', []); + }); + test('resolve command - 10', () => { testKbLookupByCommand('eleven', [KeyMod.CtrlCmd | KeyCode.KeyG]); testResolve(createContext({}), KeyMod.CtrlCmd | KeyCode.KeyG, 'eleven'); + }); + test('resolve command - 11', () => { testKbLookupByCommand('sixth', []); + }); + test('resolve command - 12', () => { testKbLookupByCommand('long multi chord', [[KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB]]); testResolve(createContext({}), [KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB], 'long multi chord'); }); + + const emptyContext = createContext({}); + + test('KBs having common prefix - the one defined later is returned', () => { + testResolve(emptyContext, [KeyMod.CtrlCmd | KeyCode.KeyB, KeyMod.CtrlCmd | KeyCode.KeyC, KeyCode.KeyI], 'long-multi-chord-2'); + }); }); }); From 0366fbc8129ffd6a94fed008b6f1c18ac3370aa3 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Thu, 30 Mar 2023 18:03:49 +0200 Subject: [PATCH 09/17] keybindingResolver: more cleanup --- src/vs/platform/keybinding/common/keybindingResolver.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 37e0cf8d510..005322cd5a2 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -350,15 +350,14 @@ export class KeybindingResolver { return NoMatchingKb; } - if (currentChords.length === 0 && result.chords.length > 1 && result.chords[1] !== null) { // first chord of a multi-chord KB matched - this._log(`\\ From ${lookupMap.length} keybinding entries, matched chord, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); - return MoreChordsNeeded; - } else if (currentChords.length !== 0 && currentChords.length + 1 < result.chords.length) { // prefix (ie, a sequence of chords) of a multi-chord KB matched, eg 2 out of 3 matched - still need one more to dispatch the KB - this._log(`\\ From ${lookupMap.length} keybinding entries, continued chord, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); + if (currentChords.length + 1 /* keypress */ < result.chords.length) { + // The chord sequence is not complete + this._log(`\\ From ${lookupMap.length} keybinding entries, awaiting ${result.chords.length - currentChords.length - 1} more chord(s), when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); return MoreChordsNeeded; } this._log(`\\ From ${lookupMap.length} keybinding entries, matched ${result.command}, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); + return KbFound(result.command, result.commandArgs, result.bubble); } From b0188312859fbbf97c39483548a265a1c4901398 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Sat, 1 Apr 2023 15:36:42 +0200 Subject: [PATCH 10/17] keybindingResolver: simplify and comment `resolve` --- .../keybinding/common/keybindingResolver.ts | 41 +++++++++++-------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 005322cd5a2..481ede6b312 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -300,34 +300,37 @@ export class KeybindingResolver { return items[items.length - 1]; } + /** + * Looks up a keybinding trigged as a result of pressing a sequence of chords - `[...currentChords, keypress]` + * + * Example: resolving 3 chords pressed sequentially - `cmd+k cmd+p cmd+i`: + * `currentChords = [ 'cmd+k' , 'cmd+p' ]` and `keypress = `cmd+i` - last pressed chord + */ public resolve(context: IContext, currentChords: string[], keypress: string): ResolutionResult { this._log(`| Resolving ${keypress}${currentChords ? ` chorded from ${currentChords}` : ``}`); + const firstChord = currentChords.length === 0 ? keypress : currentChords[0]; + + const kbCandidates = this._map.get(firstChord); + if (kbCandidates === undefined) { + // No bindings with `keypress` + this._log(`\\ No keybinding entries.`); + return NoMatchingKb; + } + let lookupMap: ResolvedKeybindingItem[] | null = null; if (currentChords.length === 0) { - const candidates = this._map.get(keypress); - if (candidates === undefined) { - // No bindings with `keypress` - this._log(`\\ No keybinding entries.`); - return NoMatchingKb; - } - - lookupMap = candidates; + lookupMap = kbCandidates; } else { // Fetch all chord bindings for `currentChords` - const candidates = this._map.get(currentChords[0]); - if (candidates === undefined) { - // No chords starting with `currentChords` - this._log(`\\ No keybinding entries.`); - return NoMatchingKb; - } - lookupMap = []; - for (let i = 0, len = candidates.length; i < len; i++) { - const candidate = candidates[i]; - if (candidate.chords.length <= currentChords.length) { + for (let i = 0, len = kbCandidates.length; i < len; i++) { + + const candidate = kbCandidates[i]; + + if (currentChords.length >= candidate.chords.length) { // # of pressed chords can't be less than # of chords in a keybinding to invoke continue; } @@ -344,12 +347,14 @@ export class KeybindingResolver { } } + // check there's a keybinding with a matching when clause const result = this._findCommand(context, lookupMap); if (!result) { this._log(`\\ From ${lookupMap.length} keybinding entries, no when clauses matched the context.`); return NoMatchingKb; } + // check we got all chords necessary to be sure a particular keybinding needs to be invoked if (currentChords.length + 1 /* keypress */ < result.chords.length) { // The chord sequence is not complete this._log(`\\ From ${lookupMap.length} keybinding entries, awaiting ${result.chords.length - currentChords.length - 1} more chord(s), when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); From cde5bfd7a1efff188fdcdc46c9cc9963e16f5921 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Sat, 1 Apr 2023 15:40:59 +0200 Subject: [PATCH 11/17] keybindingResolver: simplify algo but with some additional allocation --- .../keybinding/common/keybindingResolver.ts | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 481ede6b312..d69e0136eb8 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -308,20 +308,20 @@ export class KeybindingResolver { */ public resolve(context: IContext, currentChords: string[], keypress: string): ResolutionResult { - this._log(`| Resolving ${keypress}${currentChords ? ` chorded from ${currentChords}` : ``}`); + const pressedChords = [...currentChords, keypress]; - const firstChord = currentChords.length === 0 ? keypress : currentChords[0]; + this._log(`| Resolving ${pressedChords}`); - const kbCandidates = this._map.get(firstChord); + const kbCandidates = this._map.get(pressedChords[0]); if (kbCandidates === undefined) { - // No bindings with `keypress` + // No bindings with such 0-th chord this._log(`\\ No keybinding entries.`); return NoMatchingKb; } let lookupMap: ResolvedKeybindingItem[] | null = null; - if (currentChords.length === 0) { + if (pressedChords.length < 2) { lookupMap = kbCandidates; } else { // Fetch all chord bindings for `currentChords` @@ -330,18 +330,18 @@ export class KeybindingResolver { const candidate = kbCandidates[i]; - if (currentChords.length >= candidate.chords.length) { // # of pressed chords can't be less than # of chords in a keybinding to invoke + if (pressedChords.length > candidate.chords.length) { // # of pressed chords can't be less than # of chords in a keybinding to invoke continue; } let prefixMatches = true; - for (let i = 1; i < currentChords.length; i++) { - if (candidate.chords[i] !== currentChords[i]) { + for (let i = 1; i < pressedChords.length; i++) { + if (candidate.chords[i] !== pressedChords[i]) { prefixMatches = false; break; } } - if (prefixMatches && candidate.chords[currentChords.length] === keypress) { + if (prefixMatches) { lookupMap.push(candidate); } } @@ -355,9 +355,9 @@ export class KeybindingResolver { } // check we got all chords necessary to be sure a particular keybinding needs to be invoked - if (currentChords.length + 1 /* keypress */ < result.chords.length) { + if (pressedChords.length < result.chords.length) { // The chord sequence is not complete - this._log(`\\ From ${lookupMap.length} keybinding entries, awaiting ${result.chords.length - currentChords.length - 1} more chord(s), when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); + this._log(`\\ From ${lookupMap.length} keybinding entries, awaiting ${result.chords.length - pressedChords.length} more chord(s), when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); return MoreChordsNeeded; } From 20aee57124038aa00babe5c0c595ca3055427f39 Mon Sep 17 00:00:00 2001 From: Ladislau Szomoru <3372902+lszomoru@users.noreply.github.com> Date: Tue, 4 Apr 2023 23:10:17 +0200 Subject: [PATCH 12/17] Git - better validation for the branchProtection setting (#179183) --- extensions/git/src/repository.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/extensions/git/src/repository.ts b/extensions/git/src/repository.ts index 6c59db8c15e..a783a0758b1 100644 --- a/extensions/git/src/repository.ts +++ b/extensions/git/src/repository.ts @@ -2360,7 +2360,12 @@ export class Repository implements Disposable { private updateBranchProtectionMatcher(): void { const scopedConfig = workspace.getConfiguration('git', Uri.file(this.repository.root)); - const branchProtectionGlobs = scopedConfig.get('branchProtection')!.map(bp => bp.trim()).filter(bp => bp !== ''); + const branchProtectionConfig = scopedConfig.get('branchProtection') ?? []; + const branchProtectionValues = Array.isArray(branchProtectionConfig) ? branchProtectionConfig : [branchProtectionConfig]; + + const branchProtectionGlobs = branchProtectionValues + .map(bp => typeof bp === 'string' ? bp.trim() : '') + .filter(bp => bp !== ''); if (branchProtectionGlobs.length === 0) { this.isBranchProtectedMatcher = undefined; From 602c01b26890b7569675174e9225f8ee49b169e2 Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Tue, 4 Apr 2023 14:27:40 -0700 Subject: [PATCH 13/17] Encode filepaths with whitespace in links (#179167) --- extensions/github/src/commands.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/github/src/commands.ts b/extensions/github/src/commands.ts index 8a653e523c4..b43f7b1cf11 100644 --- a/extensions/github/src/commands.ts +++ b/extensions/github/src/commands.ts @@ -17,7 +17,7 @@ async function copyVscodeDevLink(gitAPI: GitAPI, useSelection: boolean, context: try { const permalink = getLink(gitAPI, useSelection, getVscodeDevHost(), 'headlink', context, includeRange); if (permalink) { - return vscode.env.clipboard.writeText(permalink); + return vscode.env.clipboard.writeText(encodeURI(permalink)); } } catch (err) { vscode.window.showErrorMessage(err.message); From 1db062d1887bbec9de63e77328ed4c7c1d303608 Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Tue, 4 Apr 2023 14:27:51 -0700 Subject: [PATCH 14/17] Finalize `editor/lineNumber/context` menu (#179168) --- extensions/github/package.json | 1 - .../services/actions/common/menusExtensionPoint.ts | 3 +-- .../services/extensions/common/extensionsApiProposals.ts | 1 - .../vscode.proposed.contribEditorLineNumberMenu.d.ts | 8 -------- 4 files changed, 1 insertion(+), 12 deletions(-) delete mode 100644 src/vscode-dts/vscode.proposed.contribEditorLineNumberMenu.d.ts diff --git a/extensions/github/package.json b/extensions/github/package.json index 68cbd72cbfa..f9e5cf40ef4 100644 --- a/extensions/github/package.json +++ b/extensions/github/package.json @@ -27,7 +27,6 @@ }, "enabledApiProposals": [ "contribShareMenu", - "contribEditorLineNumberMenu", "contribEditSessions" ], "contributes": { diff --git a/src/vs/workbench/services/actions/common/menusExtensionPoint.ts b/src/vs/workbench/services/actions/common/menusExtensionPoint.ts index f19d36af357..78815028817 100644 --- a/src/vs/workbench/services/actions/common/menusExtensionPoint.ts +++ b/src/vs/workbench/services/actions/common/menusExtensionPoint.ts @@ -311,8 +311,7 @@ const apiMenus: IAPIMenu[] = [ { key: 'editor/lineNumber/context', id: MenuId.EditorLineNumberContext, - description: localize('editorLineNumberContext', "The contributed editor line number context menu"), - proposed: 'contribEditorLineNumberMenu' + description: localize('editorLineNumberContext', "The contributed editor line number context menu") }, { key: 'mergeEditor/result/title', diff --git a/src/vs/workbench/services/extensions/common/extensionsApiProposals.ts b/src/vs/workbench/services/extensions/common/extensionsApiProposals.ts index 0b80f5c768c..6d3ac59883e 100644 --- a/src/vs/workbench/services/extensions/common/extensionsApiProposals.ts +++ b/src/vs/workbench/services/extensions/common/extensionsApiProposals.ts @@ -14,7 +14,6 @@ export const allApiProposals = Object.freeze({ contribCommentThreadAdditionalMenu: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.contribCommentThreadAdditionalMenu.d.ts', contribEditSessions: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.contribEditSessions.d.ts', contribEditorContentMenu: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.contribEditorContentMenu.d.ts', - contribEditorLineNumberMenu: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.contribEditorLineNumberMenu.d.ts', contribLabelFormatterWorkspaceTooltip: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.contribLabelFormatterWorkspaceTooltip.d.ts', contribMenuBarHome: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.contribMenuBarHome.d.ts', contribMergeEditorMenus: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.contribMergeEditorMenus.d.ts', diff --git a/src/vscode-dts/vscode.proposed.contribEditorLineNumberMenu.d.ts b/src/vscode-dts/vscode.proposed.contribEditorLineNumberMenu.d.ts deleted file mode 100644 index 7ee400c56b3..00000000000 --- a/src/vscode-dts/vscode.proposed.contribEditorLineNumberMenu.d.ts +++ /dev/null @@ -1,8 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -// empty placeholder declaration for the `editor/lineNumber/context` menu contribution point - -// https://github.com/microsoft/vscode/issues/175945 @joyceerhl From 9d486269f1f4dc67398d2aeef315f777693868e4 Mon Sep 17 00:00:00 2001 From: Ladislau Szomoru <3372902+lszomoru@users.noreply.github.com> Date: Tue, 4 Apr 2023 23:32:22 +0200 Subject: [PATCH 15/17] Engineering - update agent pool (#179190) --- build/azure-pipelines/product-build-pr.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/build/azure-pipelines/product-build-pr.yml b/build/azure-pipelines/product-build-pr.yml index 1d7b379aa03..789996060ec 100644 --- a/build/azure-pipelines/product-build-pr.yml +++ b/build/azure-pipelines/product-build-pr.yml @@ -96,7 +96,7 @@ jobs: - job: Windowsx64UnitTests displayName: Windows (Unit Tests) - pool: 1es-oss-windows-2022-x64 + pool: 1es-oss-windows-2019-x64 timeoutInMinutes: 30 variables: VSCODE_ARCH: x64 @@ -112,7 +112,7 @@ jobs: - job: Windowsx64IntegrationTests displayName: Windows (Integration Tests) - pool: 1es-oss-windows-2022-x64 + pool: 1es-oss-windows-2019-x64 timeoutInMinutes: 30 variables: VSCODE_ARCH: x64 @@ -128,7 +128,7 @@ jobs: # - job: Windowsx64SmokeTests # displayName: Windows (Smoke Tests) - # pool: 1es-oss-windows-2022-x64 + # pool: 1es-oss-windows-2019-x64 # timeoutInMinutes: 30 # variables: # VSCODE_ARCH: x64 @@ -153,7 +153,7 @@ jobs: - job: Windowsx64MaintainNodeModulesCache displayName: Windows (Maintain node_modules cache) - pool: 1es-oss-windows-2022-x64 + pool: 1es-oss-windows-2019-x64 timeoutInMinutes: 30 variables: VSCODE_ARCH: x64 From a1694a86281905920cdb28b51077496d8d538b3a Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Tue, 4 Apr 2023 14:36:05 -0700 Subject: [PATCH 16/17] Bump windows-mutex (#179196) --- build/.moduleignore | 10 ++++----- package.json | 3 +-- src/typings/windows-mutex.d.ts | 14 +++++++++++++ src/vs/code/electron-main/app.ts | 2 +- .../node/nativeModules.integrationTest.ts | 8 +++---- .../electron-main/updateService.win32.ts | 2 +- yarn.lock | 21 +++++++------------ 7 files changed, 34 insertions(+), 26 deletions(-) create mode 100644 src/typings/windows-mutex.d.ts diff --git a/build/.moduleignore b/build/.moduleignore index 04ba4fd8f50..2aa99ddc295 100644 --- a/build/.moduleignore +++ b/build/.moduleignore @@ -21,16 +21,16 @@ fsevents/test/** @vscode/sqlite3/src/** !@vscode/sqlite3/build/Release/*.node +@vscode/windows-mutex/binding.gyp +@vscode/windows-mutex/build/** +@vscode/windows-mutex/src/** +!@vscode/windows-mutex/**/*.node + @vscode/windows-registry/binding.gyp @vscode/windows-registry/src/** @vscode/windows-registry/build/** !@vscode/windows-registry/build/Release/*.node -windows-mutex/binding.gyp -windows-mutex/build/** -windows-mutex/src/** -!windows-mutex/**/*.node - native-keymap/binding.gyp native-keymap/build/** native-keymap/src/** diff --git a/package.json b/package.json index b51f8352437..196a85cd516 100644 --- a/package.json +++ b/package.json @@ -120,7 +120,6 @@ "@types/webpack": "^5.28.1", "@types/wicg-file-system-access": "^2020.9.5", "@types/windows-foreground-love": "^0.3.0", - "@types/windows-mutex": "^0.4.0", "@types/windows-process-tree": "^0.2.0", "@types/winreg": "^1.2.30", "@types/yauzl": "^2.9.1", @@ -230,9 +229,9 @@ "url": "https://github.com/microsoft/vscode/issues" }, "optionalDependencies": { + "@vscode/windows-mutex": "0.4.2", "@vscode/windows-registry": "1.0.6", "windows-foreground-love": "0.5.0", - "windows-mutex": "0.4.1", "windows-process-tree": "0.4.0" }, "resolutions": { diff --git a/src/typings/windows-mutex.d.ts b/src/typings/windows-mutex.d.ts new file mode 100644 index 00000000000..d2cb1f8560e --- /dev/null +++ b/src/typings/windows-mutex.d.ts @@ -0,0 +1,14 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +declare module '@vscode/windows-mutex' { + export class Mutex { + constructor(name: string); + isActive(): boolean; + release(): void; + } + + export function isActive(name: string): boolean; +} diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts index 12213667782..da5f77daa4c 100644 --- a/src/vs/code/electron-main/app.ts +++ b/src/vs/code/electron-main/app.ts @@ -1221,7 +1221,7 @@ export class CodeApplication extends Disposable { const win32MutexName = this.productService.win32MutexName; if (isWindows && win32MutexName) { try { - const WindowsMutex = await import('windows-mutex'); + const WindowsMutex = await import('@vscode/windows-mutex'); const mutex = new WindowsMutex.Mutex(win32MutexName); once(this.lifecycleMainService.onWillShutdown)(() => mutex.release()); } catch (error) { diff --git a/src/vs/platform/environment/test/node/nativeModules.integrationTest.ts b/src/vs/platform/environment/test/node/nativeModules.integrationTest.ts index 345ca8d44a3..03572dd0c35 100644 --- a/src/vs/platform/environment/test/node/nativeModules.integrationTest.ts +++ b/src/vs/platform/environment/test/node/nativeModules.integrationTest.ts @@ -109,10 +109,10 @@ flakySuite('Native Modules (all platforms)', () => { (!isWindows ? suite.skip : suite)('Native Modules (Windows)', () => { - (process.type === 'renderer' ? test.skip /* TODO@electron module is not context aware yet and thus cannot load in Electron renderer used by tests */ : test)('windows-mutex', async () => { - const mutex = await import('windows-mutex'); - assert.ok(mutex && typeof mutex.isActive === 'function', testErrorMessage('windows-mutex')); - assert.ok(typeof mutex.isActive === 'function', testErrorMessage('windows-mutex')); + (process.type === 'renderer' ? test.skip /* TODO@electron module is not context aware yet and thus cannot load in Electron renderer used by tests */ : test)('@vscode/windows-mutex', async () => { + const mutex = await import('@vscode/windows-mutex'); + assert.ok(mutex && typeof mutex.isActive === 'function', testErrorMessage('@vscode/windows-mutex')); + assert.ok(typeof mutex.isActive === 'function', testErrorMessage('@vscode/windows-mutex')); }); test('windows-foreground-love', async () => { diff --git a/src/vs/platform/update/electron-main/updateService.win32.ts b/src/vs/platform/update/electron-main/updateService.win32.ts index cadd4a0dd7b..5dd48d144af 100644 --- a/src/vs/platform/update/electron-main/updateService.win32.ts +++ b/src/vs/platform/update/electron-main/updateService.win32.ts @@ -224,7 +224,7 @@ export class Win32UpdateService extends AbstractUpdateService { }); const readyMutexName = `${this.productService.win32MutexName}-ready`; - const mutex = await import('windows-mutex'); + const mutex = await import('@vscode/windows-mutex'); // poll for mutex-ready pollUntil(() => mutex.isActive(readyMutexName)) diff --git a/yarn.lock b/yarn.lock index 7589e4449b9..2eff8542500 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1153,11 +1153,6 @@ resolved "https://registry.yarnpkg.com/@types/windows-foreground-love/-/windows-foreground-love-0.3.0.tgz#26bc230b2568aa7ab7c56d35bb5653c0a6965a42" integrity sha512-tFUVA/fiofNqOh6lZlymvQiQYPY+cZXZPR9mn9wN6/KS8uwx0zgH4Ij/jmFyRYr+x+DGZWEIeknS2BMi7FZJAQ== -"@types/windows-mutex@^0.4.0": - version "0.4.0" - resolved "https://registry.yarnpkg.com/@types/windows-mutex/-/windows-mutex-0.4.0.tgz#d27070418aa26047c6c860c704952ff26aeb961b" - integrity sha512-zUMH4nutIURLARU6f10Ls6XcZWhUkwmzVEALs26dt1ZbaLv/qxsGdYiePUbwhQmrQUp3EPZ1HxopWpzzC8ot5A== - "@types/windows-process-tree@^0.2.0": version "0.2.0" resolved "https://registry.yarnpkg.com/@types/windows-process-tree/-/windows-process-tree-0.2.0.tgz#2fa205c838a8ef0a07697cd747c954653978d22c" @@ -1391,6 +1386,14 @@ dependencies: node-addon-api "^3.0.2" +"@vscode/windows-mutex@0.4.2": + version "0.4.2" + resolved "https://registry.yarnpkg.com/@vscode/windows-mutex/-/windows-mutex-0.4.2.tgz#3f796896c3bc2cf32bfe7148e608edb0cb7247db" + integrity sha512-7MSBH22cI7OwxXImjJ3VJ7QeHrEbumtyi/xqTgv6xqZCIZfUk/eu5MMBD2Vkehut8dGZU4xIjqt5AGdm0uPixQ== + dependencies: + bindings "^1.2.1" + nan "^2.14.0" + "@vscode/windows-registry@1.0.6": version "1.0.6" resolved "https://registry.yarnpkg.com/@vscode/windows-registry/-/windows-registry-1.0.6.tgz#8b9fb9a55bf5a0be4ea11849c45ae94c6910e3e4" @@ -10842,14 +10845,6 @@ windows-foreground-love@0.5.0: resolved "https://registry.yarnpkg.com/windows-foreground-love/-/windows-foreground-love-0.5.0.tgz#7672b04eb05f934a6543cacdc3cd16ff34e3cb10" integrity sha512-yjBwmKEmQBDk3Z7yg/U9hizGWat8C6Pe4MQWl5bN6mvPU81Bt6HV2k/6mGlK3ETJLW1hCLhYx2wcGh+ykUUCyA== -windows-mutex@0.4.1: - version "0.4.1" - resolved "https://registry.yarnpkg.com/windows-mutex/-/windows-mutex-0.4.1.tgz#2eccdfc312e15e7f212fb16280f060fc6b5f00cd" - integrity sha512-RW1xN6yzw8hAcfy1BKVClhJZg/PzuNz4Qz+CNhYmmdzJiwKSU9CqvVcmAvNrtdinNkXfSqTZVBVh5kUATg6xOA== - dependencies: - bindings "^1.2.1" - nan "^2.14.0" - windows-process-tree@0.4.0: version "0.4.0" resolved "https://registry.yarnpkg.com/windows-process-tree/-/windows-process-tree-0.4.0.tgz#31ac49c5da557e628ce7e37a5800972173d3349a" From f53ed0bcabeaa5aba3bf717da5fee623e762ca0b Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Tue, 4 Apr 2023 14:54:44 -0700 Subject: [PATCH 17/17] Fix spelling in `LogOutputChannel` jsdoc (#179197) --- src/vscode-dts/vscode.d.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/vscode-dts/vscode.d.ts b/src/vscode-dts/vscode.d.ts index 863434bb693..115daed94aa 100644 --- a/src/vscode-dts/vscode.d.ts +++ b/src/vscode-dts/vscode.d.ts @@ -6499,7 +6499,7 @@ declare module 'vscode' { /** * Outputs the given trace message to the channel. Use this method to log verbose information. * - * The message is only loggeed if the channel is configured to display {@link LogLevel.Trace trace} log level. + * The message is only logged if the channel is configured to display {@link LogLevel.Trace trace} log level. * * @param message trace message to log */ @@ -6508,7 +6508,7 @@ declare module 'vscode' { /** * Outputs the given debug message to the channel. * - * The message is only loggeed if the channel is configured to display {@link LogLevel.Debug debug} log level or lower. + * The message is only logged if the channel is configured to display {@link LogLevel.Debug debug} log level or lower. * * @param message debug message to log */ @@ -6517,7 +6517,7 @@ declare module 'vscode' { /** * Outputs the given information message to the channel. * - * The message is only loggeed if the channel is configured to display {@link LogLevel.Info info} log level or lower. + * The message is only logged if the channel is configured to display {@link LogLevel.Info info} log level or lower. * * @param message info message to log */ @@ -6526,7 +6526,7 @@ declare module 'vscode' { /** * Outputs the given warning message to the channel. * - * The message is only loggeed if the channel is configured to display {@link LogLevel.Warning warning} log level or lower. + * The message is only logged if the channel is configured to display {@link LogLevel.Warning warning} log level or lower. * * @param message warning message to log */ @@ -6535,7 +6535,7 @@ declare module 'vscode' { /** * Outputs the given error or error message to the channel. * - * The message is only loggeed if the channel is configured to display {@link LogLevel.Error error} log level or lower. + * The message is only logged if the channel is configured to display {@link LogLevel.Error error} log level or lower. * * @param error Error or error message to log */