From 41b0166c35c1e66cf23d6f6aa7df7cf97aeab138 Mon Sep 17 00:00:00 2001 From: Tilman Roeder Date: Thu, 23 Feb 2023 23:37:14 +0800 Subject: [PATCH 01/12] Add support for three or more chords in shortcuts --- .../ui/keybindingLabel/keybindingLabel.ts | 10 +-- .../common/abstractKeybindingService.ts | 55 +++++++++++++--- .../keybinding/common/keybindingResolver.ts | 64 ++++++++++++------- .../test/common/keybindingResolver.test.ts | 2 +- 4 files changed, 91 insertions(+), 40 deletions(-) diff --git a/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts b/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts index b809c1ce47852..f34a1a76732d9 100644 --- a/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts +++ b/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts @@ -93,13 +93,13 @@ export class KeybindingLabel { this.clear(); if (this.keybinding) { - const [firstChord, secondChord] = this.keybinding.getChords();// TODO@chords - if (firstChord) { - this.renderChord(this.domNode, firstChord, this.matches ? this.matches.firstPart : null); + const chords = this.keybinding.getChords();// TODO@chords done + if (chords[0]) { + this.renderChord(this.domNode, chords[0], this.matches ? this.matches.firstPart : null); } - if (secondChord) { + for (let i = 1; i < chords.length; i++) { dom.append(this.domNode, $('span.monaco-keybinding-key-chord-separator', undefined, ' ')); - this.renderChord(this.domNode, secondChord, this.matches ? this.matches.chordPart : null); + this.renderChord(this.domNode, chords[i], this.matches ? this.matches.chordPart : null); } const title = (this.options.disableTitle ?? false) ? undefined : this.keybinding.getAriaLabel() || undefined; if (title !== undefined) { diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index d12e8a3b16f7b..d6956b40e6c97 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -36,7 +36,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK return this._onDidUpdateKeybindings ? this._onDidUpdateKeybindings.event : Event.None; // Sinon stubbing walks properties on prototype } - private _currentChord: CurrentChord | null; + private _currentChord: CurrentChord[] | null; private _currentChordChecker: IntervalTimer; private _currentChordStatusMessage: IDisposable | null; private _ignoreSingleModifiers: KeybindingModifierSet; @@ -140,15 +140,15 @@ export abstract class AbstractKeybindingService extends Disposable implements IK } const contextValue = this._contextKeyService.getContext(target); - const currentChord = this._currentChord ? this._currentChord.keypress : null; + const currentChord = this._currentChord ? this._currentChord.map((({ keypress }) => keypress)) : null; return this._getResolver().resolve(contextValue, currentChord, firstChord); } private _enterMultiChordMode(firstChord: string, keypressLabel: string | null): void { - this._currentChord = { + this._currentChord = [{ keypress: firstChord, label: keypressLabel - }; + }]; this._currentChordStatusMessage = this._notificationService.status(nls.localize('first.chord', "({0}) was pressed. Waiting for second key of chord...", keypressLabel)); const chordEnterTime = Date.now(); this._currentChordChecker.cancelAndSet(() => { @@ -168,6 +168,34 @@ export abstract class AbstractKeybindingService extends Disposable implements IK IME.disable(); } + private _continueMultiChordMode(nextChord: string, keypressLabel: string | null): void { + // TODO@dyedgreen: Maybe assert this is true instead? + this._currentChord = this._currentChord ? this._currentChord : []; + this._currentChord.push({ + keypress: nextChord, + label: keypressLabel + }); + const fullKeypressLabel = this._currentChord.map(({ label }) => label).join(', '); + this._currentChordStatusMessage = this._notificationService.status(nls.localize('next.chord', "({0}) was pressed. Waiting for next key of chord...", fullKeypressLabel)); + // TODO@dyedgreen: Don't repeat this code ... + const chordEnterTime = Date.now(); + this._currentChordChecker.cancelAndSet(() => { + + if (!this._documentHasFocus()) { + // Focus has been lost => leave chord mode + this._leaveChordMode(); + return; + } + + if (Date.now() - chordEnterTime > 5000) { + // 5 seconds elapsed => leave chord mode + this._leaveChordMode(); + } + + }, 500); + IME.disable(); + } + private _leaveChordMode(): void { if (this._currentChordStatusMessage) { this._currentChordStatusMessage.dispose(); @@ -255,15 +283,16 @@ export abstract class AbstractKeybindingService extends Disposable implements IK } let firstChord: string | null = null; // the first keybinding i.e. Ctrl+K - let currentChord: string | null = null;// the "second" keybinding i.e. Ctrl+K "Ctrl+D" + let currentChord: string[] | null = null;// the "second" keybinding i.e. Ctrl+K "Ctrl+D" if (isSingleModiferChord) { const [dispatchKeyname,] = keybinding.getSingleModifierDispatchChords(); + // TODO@dyedgreen: Does this make sense? firstChord = dispatchKeyname; - currentChord = dispatchKeyname; + currentChord = null; } else { [firstChord,] = keybinding.getDispatchChords(); - currentChord = this._currentChord ? this._currentChord.keypress : null; + currentChord = this._currentChord ? this._currentChord.map(({ keypress }) => keypress) : null; } if (firstChord === null) { @@ -286,9 +315,15 @@ export abstract class AbstractKeybindingService extends Disposable implements IK } if (this._currentChord) { - if (!resolveResult || !resolveResult.commandId) { - this._log(`+ Leaving chord mode: Nothing bound to "${this._currentChord.label} ${keypressLabel}".`); - this._notificationService.status(nls.localize('missing.chord', "The key combination ({0}, {1}) is not a command.", this._currentChord.label, keypressLabel), { hideAfter: 10 * 1000 /* 10s */ }); + if (resolveResult && !resolveResult.leaveMultiChord) { + shouldPreventDefault = true; + this._continueMultiChordMode(firstChord, keypressLabel); + this._log(`+ Continuing chord mode...`); + return shouldPreventDefault; + } else if (!resolveResult || !resolveResult.commandId) { + const currentChordLabel = this._currentChord.map(({ label }) => label).join(', '); + this._log(`+ Leaving chord mode: Nothing bound to "${currentChordLabel}, ${keypressLabel}".`); + this._notificationService.status(nls.localize('missing.chord', "The key combination ({0}, {1}) is not a command.", currentChordLabel, keypressLabel), { hideAfter: 10 * 1000 /* 10s */ }); shouldPreventDefault = true; } } diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 04de1e150ae6f..4d839f91f3a34 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -61,14 +61,14 @@ export class KeybindingResolver { } } - private static _isTargetedForRemoval(defaultKb: ResolvedKeybindingItem, keypressFirstPart: string | null, keypressChordPart: string | null, when: ContextKeyExpression | undefined): boolean { - // TODO@chords - if (keypressFirstPart && defaultKb.chords[0] !== keypressFirstPart) { - return false; - } - // TODO@chords - if (keypressChordPart && defaultKb.chords[1] !== keypressChordPart) { - return false; + private static _isTargetedForRemoval(defaultKb: ResolvedKeybindingItem, keypress: string[] | null, when: ContextKeyExpression | undefined): boolean { + // TODO@chords done + if (keypress) { + for (let i = 0; i < keypress.length; i++) { + if (keypress[i] !== defaultKb.chords[i]) { + return false; + } + } } // `true` means always, as does `undefined` @@ -127,11 +127,9 @@ export class KeybindingResolver { } let isRemoved = false; for (const commandRemoval of commandRemovals) { - // TODO@chords - const keypressFirstChord = commandRemoval.chords[0]; - const keypressSecondChord = commandRemoval.chords[1]; + // TODO@chords done const when = commandRemoval.when; - if (this._isTargetedForRemoval(rule, keypressFirstChord, keypressSecondChord, when)) { + if (this._isTargetedForRemoval(rule, commandRemoval.chords, when)) { isRemoved = true; break; } @@ -162,13 +160,12 @@ export class KeybindingResolver { continue; } - const conflictHasMultipleChords = (conflict.chords.length > 1); - const itemHasMultipleChords = (item.chords.length > 1); - - // TODO@chords - if (conflictHasMultipleChords && itemHasMultipleChords && conflict.chords[1] !== item.chords[1]) { - // The conflict only shares the first chord with this command - continue; + // TODO@chords done + for (let i = 1; i < conflict.chords.length && i < item.chords.length; i++) { + if (conflict.chords[i] !== item.chords[i]) { + // The conflict diverges at the ith step + continue; + } } if (KeybindingResolver.whenIsEntirelyIncluded(conflict.when, item.when)) { @@ -272,14 +269,13 @@ export class KeybindingResolver { return items[items.length - 1]; } - public resolve(context: IContext, currentChord: string | null, keypress: string): IResolveResult | null { + public resolve(context: IContext, currentChord: string[] | null, keypress: string): IResolveResult | null { this._log(`| Resolving ${keypress}${currentChord ? ` chorded from ${currentChord}` : ``}`); let lookupMap: ResolvedKeybindingItem[] | null = null; if (currentChord !== null) { // Fetch all chord bindings for `currentChord` - - const candidates = this._map.get(currentChord); + const candidates = this._map.get(currentChord[0]); if (typeof candidates === 'undefined') { // No chords starting with `currentChord` this._log(`\\ No keybinding entries.`); @@ -289,8 +285,19 @@ export class KeybindingResolver { lookupMap = []; for (let i = 0, len = candidates.length; i < len; i++) { const candidate = candidates[i]; - // TODO@chords - if (candidate.chords[1] === keypress) { + // TODO@chords done + if (candidate.chords.length <= currentChord.length) { + continue; + } + + let prefixMatches = true; + for (let i = 1; i < currentChord.length; i++) { + if (candidate.chords[i] !== currentChord[i]) { + prefixMatches = false; + break; + } + } + if (prefixMatches && candidate.chords[currentChord.length] === keypress) { lookupMap.push(candidate); } } @@ -321,6 +328,15 @@ export class KeybindingResolver { commandArgs: null, bubble: false }; + } else if (currentChord !== null && currentChord.length + 1 < result.chords.length) { + 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 + }; } this._log(`\\ From ${lookupMap.length} keybinding entries, matched ${result.command}, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); diff --git a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts index 3c81b5076a3f3..d4879e1c6270d 100644 --- a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts +++ b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts @@ -405,7 +405,7 @@ suite('KeybindingResolver', () => { 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); + const result = resolver.resolve(ctx, previousChord ? [previousChord] : null, 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. From 2cb48a33d4c772b32b0f5d2696946b0acce10757 Mon Sep 17 00:00:00 2001 From: Tilman Roeder Date: Fri, 24 Feb 2023 00:34:45 +0800 Subject: [PATCH 02/12] Add tests and fix bug that was uncovered --- src/vs/base/common/keybindings.ts | 32 ++++++++++------ .../keybinding/common/keybindingResolver.ts | 5 +++ .../test/common/keybindingResolver.test.ts | 38 ++++++++++++++----- .../test/common/keybindingsTestUtils.ts | 2 +- 4 files changed, 54 insertions(+), 23 deletions(-) diff --git a/src/vs/base/common/keybindings.ts b/src/vs/base/common/keybindings.ts index f3523709f9fa1..375256237a69e 100644 --- a/src/vs/base/common/keybindings.ts +++ b/src/vs/base/common/keybindings.ts @@ -28,19 +28,27 @@ const enum BinaryKeybindingsMask { KeyCode = 0x000000FF } -export function decodeKeybinding(keybinding: number, OS: OperatingSystem): Keybinding | null { - if (keybinding === 0) { - return null; - } - const firstChord = (keybinding & 0x0000FFFF) >>> 0; - const secondChord = (keybinding & 0xFFFF0000) >>> 16; - if (secondChord !== 0) { - return new Keybinding([ - createSimpleKeybinding(firstChord, OS), - createSimpleKeybinding(secondChord, OS) - ]); +export function decodeKeybinding(keybinding: number | number[], OS: OperatingSystem): Keybinding | null { + if (typeof keybinding === 'number') { + if (keybinding === 0) { + return null; + } + const firstChord = (keybinding & 0x0000FFFF) >>> 0; + const secondChord = (keybinding & 0xFFFF0000) >>> 16; + if (secondChord !== 0) { + return new Keybinding([ + createSimpleKeybinding(firstChord, OS), + createSimpleKeybinding(secondChord, OS) + ]); + } + return new Keybinding([createSimpleKeybinding(firstChord, OS)]); + } else { + const chords = []; + for (let i = 0; i < keybinding.length; i++) { + chords.push(createSimpleKeybinding(keybinding[i], OS)); + } + return new Keybinding(chords); } - return new Keybinding([createSimpleKeybinding(firstChord, OS)]); } export function createSimpleKeybinding(keybinding: number, OS: OperatingSystem): KeyCodeChord { diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 4d839f91f3a34..7afe790b395e6 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -161,12 +161,17 @@ export class KeybindingResolver { } // TODO@chords done + let diverges = false; for (let i = 1; i < conflict.chords.length && i < item.chords.length; i++) { if (conflict.chords[i] !== item.chords[i]) { // The conflict diverges at the ith step + diverges = true; continue; } } + if (diverges) { + continue; + } if (KeybindingResolver.whenIsEntirelyIncluded(conflict.when, item.when)) { // `item` completely overwrites `conflict` diff --git a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts index d4879e1c6270d..53001faeb4563 100644 --- a/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts +++ b/src/vs/platform/keybinding/test/common/keybindingResolver.test.ts @@ -23,7 +23,7 @@ function createContext(ctx: any) { suite('KeybindingResolver', () => { - function kbItem(keybinding: number, command: string, commandArgs: any, when: ContextKeyExpression | undefined, isDefault: boolean): ResolvedKeybindingItem { + function kbItem(keybinding: number | number[], command: string, commandArgs: any, when: ContextKeyExpression | undefined, isDefault: boolean): ResolvedKeybindingItem { const resolvedKeybinding = createUSLayoutResolvedKeybinding(keybinding, OS); return new ResolvedKeybindingItem( resolvedKeybinding, @@ -304,7 +304,7 @@ suite('KeybindingResolver', () => { test('resolve command', () => { - function _kbItem(keybinding: number, command: string, when: ContextKeyExpression | undefined): ResolvedKeybindingItem { + function _kbItem(keybinding: number | number[], command: string, when: ContextKeyExpression | undefined): ResolvedKeybindingItem { return kbItem(keybinding, command, null, when, true); } @@ -383,12 +383,17 @@ suite('KeybindingResolver', () => { KeyMod.CtrlCmd | KeyCode.KeyG, 'eleven', undefined - ) + ), + _kbItem( + [KeyMod.CtrlCmd | KeyCode.KeyK, KeyCode.KeyA, KeyCode.KeyB], + 'long multi chord', + undefined + ), ]; const resolver = new KeybindingResolver(items, [], () => { }); - const testKey = (commandId: string, expectedKeys: number[]) => { + const testKey = (commandId: string, expectedKeys: number[] | number[][]) => { // Test lookup const lookupResult = resolver.lookupKeybindings(commandId); assert.strictEqual(lookupResult.length, expectedKeys.length, 'Length mismatch @ commandId ' + commandId); @@ -399,27 +404,37 @@ suite('KeybindingResolver', () => { } }; - const testResolve = (ctx: IContext, _expectedKey: number, commandId: string) => { + const testResolve = (ctx: IContext, _expectedKey: number | number[], commandId: string) => { const expectedKeybinding = decodeKeybinding(_expectedKey, OS)!; - let previousChord: (string | null) = null; + 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 ? [previousChord] : null, chord); + 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}`); + } 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}`); } else { - // if it's not the final chord, then we should not find a valid command, - // and there should be a chord. + // 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}`); } - previousChord = chord; + if (previousChord === null) { + previousChord = []; + } + previousChord.push(chord); } }; @@ -452,5 +467,8 @@ suite('KeybindingResolver', () => { testResolve(createContext({}), KeyMod.CtrlCmd | KeyCode.KeyG, 'eleven'); testKey('sixth', []); + + 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'); }); }); diff --git a/src/vs/platform/keybinding/test/common/keybindingsTestUtils.ts b/src/vs/platform/keybinding/test/common/keybindingsTestUtils.ts index ab839b202c953..d0b338d969153 100644 --- a/src/vs/platform/keybinding/test/common/keybindingsTestUtils.ts +++ b/src/vs/platform/keybinding/test/common/keybindingsTestUtils.ts @@ -7,7 +7,7 @@ import { decodeKeybinding, ResolvedKeybinding } from 'vs/base/common/keybindings import { OperatingSystem } from 'vs/base/common/platform'; import { USLayoutResolvedKeybinding } from 'vs/platform/keybinding/common/usLayoutResolvedKeybinding'; -export function createUSLayoutResolvedKeybinding(encodedKeybinding: number, OS: OperatingSystem): ResolvedKeybinding | undefined { +export function createUSLayoutResolvedKeybinding(encodedKeybinding: number | number[], OS: OperatingSystem): ResolvedKeybinding | undefined { if (encodedKeybinding === 0) { return undefined; } From 9086a4fa6dcecdbbe55d1a5681a30dd4338a1d67 Mon Sep 17 00:00:00 2001 From: Tilman Roeder Date: Fri, 24 Feb 2023 00:40:38 +0800 Subject: [PATCH 03/12] Clean up TODO comments --- .../ui/keybindingLabel/keybindingLabel.ts | 2 +- .../common/abstractKeybindingService.ts | 40 ++++++------------- .../keybinding/common/keybindingResolver.ts | 6 --- .../platform/quickinput/browser/quickInput.ts | 2 +- 4 files changed, 15 insertions(+), 35 deletions(-) diff --git a/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts b/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts index f34a1a76732d9..431e33048cdf4 100644 --- a/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts +++ b/src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts @@ -93,7 +93,7 @@ export class KeybindingLabel { this.clear(); if (this.keybinding) { - const chords = this.keybinding.getChords();// TODO@chords done + const chords = this.keybinding.getChords(); if (chords[0]) { this.renderChord(this.domNode, chords[0], this.matches ? this.matches.firstPart : null); } diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index d6956b40e6c97..1e82db2cdc1c3 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -144,13 +144,8 @@ export abstract class AbstractKeybindingService extends Disposable implements IK return this._getResolver().resolve(contextValue, currentChord, firstChord); } - private _enterMultiChordMode(firstChord: string, keypressLabel: string | null): void { - this._currentChord = [{ - keypress: firstChord, - label: keypressLabel - }]; - this._currentChordStatusMessage = this._notificationService.status(nls.localize('first.chord', "({0}) was pressed. Waiting for second key of chord...", keypressLabel)); - const chordEnterTime = Date.now(); + private _scheduleLeaveChordMode(): void { + const chordLastInteractedTime = Date.now(); this._currentChordChecker.cancelAndSet(() => { if (!this._documentHasFocus()) { @@ -159,17 +154,25 @@ export abstract class AbstractKeybindingService extends Disposable implements IK return; } - if (Date.now() - chordEnterTime > 5000) { + if (Date.now() - chordLastInteractedTime > 5000) { // 5 seconds elapsed => leave chord mode this._leaveChordMode(); } }, 500); + } + + private _enterMultiChordMode(firstChord: string, keypressLabel: string | null): void { + this._currentChord = [{ + 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 { - // TODO@dyedgreen: Maybe assert this is true instead? this._currentChord = this._currentChord ? this._currentChord : []; this._currentChord.push({ keypress: nextChord, @@ -177,23 +180,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK }); const fullKeypressLabel = this._currentChord.map(({ label }) => label).join(', '); this._currentChordStatusMessage = this._notificationService.status(nls.localize('next.chord', "({0}) was pressed. Waiting for next key of chord...", fullKeypressLabel)); - // TODO@dyedgreen: Don't repeat this code ... - const chordEnterTime = Date.now(); - this._currentChordChecker.cancelAndSet(() => { - - if (!this._documentHasFocus()) { - // Focus has been lost => leave chord mode - this._leaveChordMode(); - return; - } - - if (Date.now() - chordEnterTime > 5000) { - // 5 seconds elapsed => leave chord mode - this._leaveChordMode(); - } - - }, 500); - IME.disable(); + this._scheduleLeaveChordMode(); } private _leaveChordMode(): void { @@ -287,7 +274,6 @@ export abstract class AbstractKeybindingService extends Disposable implements IK if (isSingleModiferChord) { const [dispatchKeyname,] = keybinding.getSingleModifierDispatchChords(); - // TODO@dyedgreen: Does this make sense? firstChord = dispatchKeyname; currentChord = null; } else { diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 7afe790b395e6..cbeb7e1d1342c 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -56,13 +56,11 @@ export class KeybindingResolver { continue; } - // TODO@chords this._addKeyPress(k.chords[0], k); } } private static _isTargetedForRemoval(defaultKb: ResolvedKeybindingItem, keypress: string[] | null, when: ContextKeyExpression | undefined): boolean { - // TODO@chords done if (keypress) { for (let i = 0; i < keypress.length; i++) { if (keypress[i] !== defaultKb.chords[i]) { @@ -127,7 +125,6 @@ export class KeybindingResolver { } let isRemoved = false; for (const commandRemoval of commandRemovals) { - // TODO@chords done const when = commandRemoval.when; if (this._isTargetedForRemoval(rule, commandRemoval.chords, when)) { isRemoved = true; @@ -160,7 +157,6 @@ export class KeybindingResolver { continue; } - // TODO@chords done let diverges = false; for (let i = 1; i < conflict.chords.length && i < item.chords.length; i++) { if (conflict.chords[i] !== item.chords[i]) { @@ -290,7 +286,6 @@ export class KeybindingResolver { lookupMap = []; for (let i = 0, len = candidates.length; i < len; i++) { const candidate = candidates[i]; - // TODO@chords done if (candidate.chords.length <= currentChord.length) { continue; } @@ -323,7 +318,6 @@ export class KeybindingResolver { return null; } - // TODO@chords if (currentChord === null && result.chords.length > 1 && result.chords[1] !== null) { this._log(`\\ From ${lookupMap.length} keybinding entries, matched chord, when: ${printWhenExplanation(result.when)}, source: ${printSourceExplanation(result)}.`); return { diff --git a/src/vs/platform/quickinput/browser/quickInput.ts b/src/vs/platform/quickinput/browser/quickInput.ts index 8c1786f865490..c02442ca66cec 100644 --- a/src/vs/platform/quickinput/browser/quickInput.ts +++ b/src/vs/platform/quickinput/browser/quickInput.ts @@ -955,7 +955,7 @@ class QuickPick extends QuickInput implements IQuickPi // Select element when keys are pressed that signal it const quickNavKeys = this._quickNavigate.keybindings; const wasTriggerKeyPressed = quickNavKeys.some(k => { - const [firstChord, secondChord] = k.getChords();// TODO@chords + const [firstChord, secondChord] = k.getChords(); if (secondChord) { return false; } From 645569a9b6474b6a995e8c128390da0c357f08b7 Mon Sep 17 00:00:00 2001 From: Tilman Roeder Date: Fri, 24 Feb 2023 08:01:32 +0800 Subject: [PATCH 04/12] Make multi chords more explicit in quickInput.ts --- src/vs/platform/quickinput/browser/quickInput.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/vs/platform/quickinput/browser/quickInput.ts b/src/vs/platform/quickinput/browser/quickInput.ts index c02442ca66cec..f4db1e1536f89 100644 --- a/src/vs/platform/quickinput/browser/quickInput.ts +++ b/src/vs/platform/quickinput/browser/quickInput.ts @@ -955,12 +955,12 @@ class QuickPick extends QuickInput implements IQuickPi // Select element when keys are pressed that signal it const quickNavKeys = this._quickNavigate.keybindings; const wasTriggerKeyPressed = quickNavKeys.some(k => { - const [firstChord, secondChord] = k.getChords(); - if (secondChord) { + const chords = k.getChords(); + if (chords.length > 1) { return false; } - if (firstChord.shiftKey && keyCode === KeyCode.Shift) { + if (chords[0].shiftKey && keyCode === KeyCode.Shift) { if (keyboardEvent.ctrlKey || keyboardEvent.altKey || keyboardEvent.metaKey) { return false; // this is an optimistic check for the shift key being used to navigate back in quick input } @@ -968,15 +968,15 @@ class QuickPick extends QuickInput implements IQuickPi return true; } - if (firstChord.altKey && keyCode === KeyCode.Alt) { + if (chords[0].altKey && keyCode === KeyCode.Alt) { return true; } - if (firstChord.ctrlKey && keyCode === KeyCode.Ctrl) { + if (chords[0].ctrlKey && keyCode === KeyCode.Ctrl) { return true; } - if (firstChord.metaKey && keyCode === KeyCode.Meta) { + if (chords[0].metaKey && keyCode === KeyCode.Meta) { return true; } From eaeaeaf0f01883e64fb650fadb7d4031b2045e32 Mon Sep 17 00:00:00 2001 From: Tilman Roeder Date: Thu, 9 Mar 2023 20:03:08 +0800 Subject: [PATCH 05/12] Address review comments --- .../keybinding/common/keybindingResolver.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index cbeb7e1d1342c..61093e6cb6a62 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -157,15 +157,18 @@ export class KeybindingResolver { continue; } - let diverges = false; + // Test if the shorter chord is a prefix of the onger one. If the shorter chord + // is a prefix it effectively will shadow the longer chord and is considered a + // conflict. + let shorterChordIsPrefix = true; for (let i = 1; i < conflict.chords.length && i < item.chords.length; i++) { if (conflict.chords[i] !== item.chords[i]) { - // The conflict diverges at the ith step - diverges = true; - continue; + // The ith step does not conflict + shorterChordIsPrefix = false; + break; } } - if (diverges) { + if (!shorterChordIsPrefix) { continue; } From 37612b56050614f31c0dece0fe14b4f7c30ba472 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Thu, 9 Mar 2023 13:46:38 +0100 Subject: [PATCH 06/12] keybindings: resolver: align terminology with rest of code --- .../platform/keybinding/common/keybindingResolver.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/vs/platform/keybinding/common/keybindingResolver.ts b/src/vs/platform/keybinding/common/keybindingResolver.ts index 61093e6cb6a62..99b9ae115227c 100644 --- a/src/vs/platform/keybinding/common/keybindingResolver.ts +++ b/src/vs/platform/keybinding/common/keybindingResolver.ts @@ -157,18 +157,17 @@ export class KeybindingResolver { continue; } - // Test if the shorter chord is a prefix of the onger one. If the shorter chord - // is a prefix it effectively will shadow the longer chord and is considered a - // conflict. - let shorterChordIsPrefix = true; + // Test if the shorter keybinding is a prefix of the longer one. + // If the shorter keybinding is a prefix, it effectively will shadow the longer one and is considered a conflict. + let isShorterKbPrefix = true; for (let i = 1; i < conflict.chords.length && i < item.chords.length; i++) { if (conflict.chords[i] !== item.chords[i]) { // The ith step does not conflict - shorterChordIsPrefix = false; + isShorterKbPrefix = false; break; } } - if (!shorterChordIsPrefix) { + if (!isShorterKbPrefix) { continue; } From cffeedda6325f8b2341a97a921f07f924ddc104b Mon Sep 17 00:00:00 2001 From: Tilman Roeder Date: Wed, 15 Mar 2023 19:28:02 +0000 Subject: [PATCH 07/12] Allow input of >2 chords in keybinding settings --- .../preferences/browser/keybindingWidgets.ts | 77 ++++++++----------- 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts index bc81f1b650475..a3485fda069ba 100644 --- a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts +++ b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts @@ -35,14 +35,13 @@ export interface KeybindingsSearchOptions extends SearchOptions { export class KeybindingsSearchWidget extends SearchWidget { - private _firstChord: ResolvedKeybinding | null; - private _secondChord: ResolvedKeybinding | null; + private _chords: ResolvedKeybinding[] | null; private _inputValue: string; private readonly recordDisposables = this._register(new DisposableStore()); - private _onKeybinding = this._register(new Emitter<[ResolvedKeybinding | null, ResolvedKeybinding | null]>()); - readonly onKeybinding: Event<[ResolvedKeybinding | null, ResolvedKeybinding | null]> = this._onKeybinding.event; + private _onKeybinding = this._register(new Emitter()); + readonly onKeybinding: Event = this._onKeybinding.event; private _onEnter = this._register(new Emitter()); readonly onEnter: Event = this._onEnter.event; @@ -61,8 +60,7 @@ export class KeybindingsSearchWidget extends SearchWidget { ) { super(parent, options, contextViewService, instantiationService, contextKeyService, keybindingService); this._register(toDisposable(() => this.stopRecordingKeys())); - this._firstChord = null; - this._secondChord = null; + this._chords = null; this._inputValue = ''; this._reset(); @@ -93,8 +91,7 @@ export class KeybindingsSearchWidget extends SearchWidget { } private _reset() { - this._firstChord = null; - this._secondChord = null; + this._chords = null; } private _onKeyDown(keyboardEvent: IKeyboardEvent): void { @@ -117,29 +114,21 @@ export class KeybindingsSearchWidget extends SearchWidget { const info = `code: ${keyboardEvent.browserEvent.code}, keyCode: ${keyboardEvent.browserEvent.keyCode}, key: ${keyboardEvent.browserEvent.key} => UI: ${keybinding.getAriaLabel()}, user settings: ${keybinding.getUserSettingsLabel()}, dispatch: ${keybinding.getDispatchChords()[0]}`; const options = this.options as KeybindingsSearchOptions; - const hasFirstChord = (this._firstChord && this._firstChord.getDispatchChords()[0] !== null); - const hasSecondChord = (this._secondChord && this._secondChord.getDispatchChords()[0] !== null); - if (hasFirstChord && hasSecondChord) { - // Reset - this._firstChord = keybinding; - this._secondChord = null; - } else if (!hasFirstChord) { - this._firstChord = keybinding; + if (!this._chords) { + this._chords = []; + } + const lastChordIsComplete = (this._chords.length === 0 || this._chords[this._chords.length - 1].getDispatchChords()[0] !== null); + if (lastChordIsComplete) { + this._chords.push(keybinding); } else { - this._secondChord = keybinding; + this._chords[this._chords.length - 1] = keybinding; } - let value = ''; - if (this._firstChord) { - value = (this._firstChord.getUserSettingsLabel() || ''); - } - if (this._secondChord) { - value = value + ' ' + this._secondChord.getUserSettingsLabel(); - } + const value = this._chords.map((keybinding) => keybinding.getUserSettingsLabel() || '').join(' '); this.setInputValue(options.quoteRecordedKeys ? `"${value}"` : value); this.inputBox.inputElement.title = info; - this._onKeybinding.fire([this._firstChord, this._secondChord]); + this._onKeybinding.fire(this._chords); } } @@ -153,8 +142,7 @@ export class DefineKeybindingWidget extends Widget { private _outputNode: HTMLElement; private _showExistingKeybindingsNode: HTMLElement; - private _firstChord: ResolvedKeybinding | null = null; - private _secondChord: ResolvedKeybinding | null = null; + private _chords: ResolvedKeybinding[] | null = null; private _isVisible: boolean = false; private _onHide = this._register(new Emitter()); @@ -210,8 +198,7 @@ export class DefineKeybindingWidget extends Widget { this._isVisible = true; this._domNode.setDisplay('block'); - this._firstChord = null; - this._secondChord = null; + this._chords = null; this._keybindingInputWidget.setInputValue(''); dom.clearNode(this._outputNode); dom.clearNode(this._showExistingKeybindingsNode); @@ -250,20 +237,24 @@ export class DefineKeybindingWidget extends Widget { } } - private onKeybinding(keybinding: [ResolvedKeybinding | null, ResolvedKeybinding | null]): void { - const [firstChord, secondChord] = keybinding; - this._firstChord = firstChord; - this._secondChord = secondChord; + private onKeybinding(keybinding: ResolvedKeybinding[] | null): void { + this._chords = keybinding; dom.clearNode(this._outputNode); dom.clearNode(this._showExistingKeybindingsNode); + + const firstLabel = new KeybindingLabel(this._outputNode, OS, defaultKeybindingLabelStyles); - firstLabel.set(withNullAsUndefined(this._firstChord)); + firstLabel.set(withNullAsUndefined(this._chords?.[0])); + + + if (this._chords) { + for (let i = 1; i < this._chords.length; i++) { + this._outputNode.appendChild(document.createTextNode(nls.localize('defineKeybinding.chordsTo', "chord to"))); + const chordLabel = new KeybindingLabel(this._outputNode, OS, defaultKeybindingLabelStyles); + chordLabel.set(this._chords[i]); + } - if (this._secondChord) { - this._outputNode.appendChild(document.createTextNode(nls.localize('defineKeybinding.chordsTo', "chord to"))); - const chordLabel = new KeybindingLabel(this._outputNode, OS, defaultKeybindingLabelStyles); - chordLabel.set(this._secondChord); } const label = this.getUserSettingsLabel(); @@ -274,18 +265,14 @@ export class DefineKeybindingWidget extends Widget { private getUserSettingsLabel(): string | null { let label: string | null = null; - if (this._firstChord) { - label = this._firstChord.getUserSettingsLabel(); - if (this._secondChord) { - label = label + ' ' + this._secondChord.getUserSettingsLabel(); - } + if (this._chords) { + label = this._chords.map(keybinding => keybinding.getUserSettingsLabel()).join(' '); } return label; } private onCancel(): void { - this._firstChord = null; - this._secondChord = null; + this._chords = null; this.hide(); } From 55c32ae2e000df39fd631a32972f6c8a5d076103 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Thu, 16 Mar 2023 14:33:02 +0100 Subject: [PATCH 08/12] Keep the limit at 2 chords in the widget for now --- .../contrib/preferences/browser/keybindingWidgets.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts index a3485fda069ba..3fe86c30e2b20 100644 --- a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts +++ b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts @@ -117,6 +117,10 @@ export class KeybindingsSearchWidget extends SearchWidget { if (!this._chords) { this._chords = []; } + if (this._chords.length === 2) { + // TODO: keep the limit at 2 for now + this._chords = []; + } const lastChordIsComplete = (this._chords.length === 0 || this._chords[this._chords.length - 1].getDispatchChords()[0] !== null); if (lastChordIsComplete) { this._chords.push(keybinding); @@ -242,12 +246,9 @@ export class DefineKeybindingWidget extends Widget { dom.clearNode(this._outputNode); dom.clearNode(this._showExistingKeybindingsNode); - - const firstLabel = new KeybindingLabel(this._outputNode, OS, defaultKeybindingLabelStyles); firstLabel.set(withNullAsUndefined(this._chords?.[0])); - if (this._chords) { for (let i = 1; i < this._chords.length; i++) { this._outputNode.appendChild(document.createTextNode(nls.localize('defineKeybinding.chordsTo', "chord to"))); From 1344a6d35d9cfc59bf35395d38b050227482ff30 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Thu, 16 Mar 2023 15:16:04 +0100 Subject: [PATCH 09/12] Revert "Keep the limit at 2 chords in the widget for now" This reverts commit 55c32ae2e000df39fd631a32972f6c8a5d076103. --- .../contrib/preferences/browser/keybindingWidgets.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts index 3fe86c30e2b20..a3485fda069ba 100644 --- a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts +++ b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts @@ -117,10 +117,6 @@ export class KeybindingsSearchWidget extends SearchWidget { if (!this._chords) { this._chords = []; } - if (this._chords.length === 2) { - // TODO: keep the limit at 2 for now - this._chords = []; - } const lastChordIsComplete = (this._chords.length === 0 || this._chords[this._chords.length - 1].getDispatchChords()[0] !== null); if (lastChordIsComplete) { this._chords.push(keybinding); @@ -246,9 +242,12 @@ export class DefineKeybindingWidget extends Widget { dom.clearNode(this._outputNode); dom.clearNode(this._showExistingKeybindingsNode); + + const firstLabel = new KeybindingLabel(this._outputNode, OS, defaultKeybindingLabelStyles); firstLabel.set(withNullAsUndefined(this._chords?.[0])); + if (this._chords) { for (let i = 1; i < this._chords.length; i++) { this._outputNode.appendChild(document.createTextNode(nls.localize('defineKeybinding.chordsTo', "chord to"))); From c87d70bc690435c9e1240aa29462e8febd1faf48 Mon Sep 17 00:00:00 2001 From: Ulugbek Abdullaev Date: Thu, 16 Mar 2023 15:46:58 +0100 Subject: [PATCH 10/12] keybindings: UI: search widget: limit chords to 2 for now --- .../preferences/browser/keybindingWidgets.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts index a3485fda069ba..020ecc21eb7f2 100644 --- a/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts +++ b/src/vs/workbench/contrib/preferences/browser/keybindingWidgets.ts @@ -117,11 +117,16 @@ export class KeybindingsSearchWidget extends SearchWidget { if (!this._chords) { this._chords = []; } - const lastChordIsComplete = (this._chords.length === 0 || this._chords[this._chords.length - 1].getDispatchChords()[0] !== null); - if (lastChordIsComplete) { - this._chords.push(keybinding); - } else { + + // TODO: note that we allow a keybinding "shift shift", but this widget doesn't allow input "shift shift" because the first "shift" will be incomplete - this is _not_ a regression + const hasIncompleteChord = this._chords.length > 0 && this._chords[this._chords.length - 1].getDispatchChords()[0] === null; + if (hasIncompleteChord) { this._chords[this._chords.length - 1] = keybinding; + } else { + if (this._chords.length === 2) { // TODO: limit chords # to 2 for now + this._chords = []; + } + this._chords.push(keybinding); } const value = this._chords.map((keybinding) => keybinding.getUserSettingsLabel() || '').join(' '); From d7bb138a6815e687da23774d4e38d2c8b0bf380a Mon Sep 17 00:00:00 2001 From: Tilman Roeder Date: Sun, 19 Mar 2023 15:43:33 +0000 Subject: [PATCH 11/12] Fix single modifier chords --- .../keybinding/common/abstractKeybindingService.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index 1e82db2cdc1c3..89ed57fef2551 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -5,7 +5,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 { first, IntervalTimer, TimeoutTimer } from 'vs/base/common/async'; import { Emitter, Event } from 'vs/base/common/event'; import { KeyCode } from 'vs/base/common/keyCodes'; import { SingleModifierChord, ResolvedKeybinding, ResolvedChord, Keybinding } from 'vs/base/common/keybindings'; @@ -273,9 +273,12 @@ export abstract class AbstractKeybindingService extends Disposable implements IK let currentChord: string[] | null = null;// the "second" keybinding i.e. Ctrl+K "Ctrl+D" if (isSingleModiferChord) { + // The keybinding is the second keypress of a single modifier chord, e.g. "shift shift". + // A single modifier can only occur when the same modifier is pressed in short sequence, + // hence we disregard `_currentChord` and use the same modifier instead. const [dispatchKeyname,] = keybinding.getSingleModifierDispatchChords(); firstChord = dispatchKeyname; - currentChord = null; + currentChord = dispatchKeyname ? [dispatchKeyname] : []; } else { [firstChord,] = keybinding.getDispatchChords(); currentChord = this._currentChord ? this._currentChord.map(({ keypress }) => keypress) : null; From 3bf90035508f10deaf7439fdaccf0b4ede8be641 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Mon, 20 Mar 2023 08:16:10 +0100 Subject: [PATCH 12/12] Remove unused import --- src/vs/platform/keybinding/common/abstractKeybindingService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index 89ed57fef2551..edc0dabcc7dfb 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -5,7 +5,7 @@ import { WorkbenchActionExecutedClassification, WorkbenchActionExecutedEvent } from 'vs/base/common/actions'; import * as arrays from 'vs/base/common/arrays'; -import { first, IntervalTimer, TimeoutTimer } from 'vs/base/common/async'; +import { IntervalTimer, TimeoutTimer } from 'vs/base/common/async'; import { Emitter, Event } from 'vs/base/common/event'; import { KeyCode } from 'vs/base/common/keyCodes'; import { SingleModifierChord, ResolvedKeybinding, ResolvedChord, Keybinding } from 'vs/base/common/keybindings';