Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for three or more chord keyboard shortcuts #175253

Merged
merged 24 commits into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
41b0166
Add support for three or more chords in shortcuts
dyedgreen Feb 23, 2023
2cb48a3
Add tests and fix bug that was uncovered
dyedgreen Feb 23, 2023
9086a4f
Clean up TODO comments
dyedgreen Feb 23, 2023
c705bac
Merge branch 'main' into support-three-or-more-chords
dyedgreen Feb 23, 2023
645569a
Make multi chords more explicit in quickInput.ts
dyedgreen Feb 24, 2023
12b086c
Merge branch 'main' into support-three-or-more-chords
dyedgreen Feb 24, 2023
f4e6a83
Merge branch 'main' into support-three-or-more-chords
dyedgreen Feb 25, 2023
11b3cfd
Merge branch 'main' into support-three-or-more-chords
dyedgreen Feb 26, 2023
3c05b51
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 1, 2023
0dfd715
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 2, 2023
1df309c
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 4, 2023
5605923
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 4, 2023
8bd1a68
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 7, 2023
decf8c1
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 8, 2023
eaeaeaf
Address review comments
dyedgreen Mar 9, 2023
a0d1293
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 9, 2023
37612b5
keybindings: resolver: align terminology with rest of code
ulugbekna Mar 9, 2023
cffeedd
Allow input of >2 chords in keybinding settings
dyedgreen Mar 15, 2023
5303e9d
Merge branch 'main' into support-three-or-more-chords
dyedgreen Mar 16, 2023
55c32ae
Keep the limit at 2 chords in the widget for now
alexdima Mar 16, 2023
1344a6d
Revert "Keep the limit at 2 chords in the widget for now"
ulugbekna Mar 16, 2023
c87d70b
keybindings: UI: search widget: limit chords to 2 for now
ulugbekna Mar 16, 2023
d7bb138
Fix single modifier chords
dyedgreen Mar 19, 2023
3bf9003
Remove unused import
alexdima Mar 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/vs/base/browser/ui/keybindingLabel/keybindingLabel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
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) {
Expand Down
32 changes: 20 additions & 12 deletions src/vs/base/common/keybindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
53 changes: 37 additions & 16 deletions src/vs/platform/keybinding/common/abstractKeybindingService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -140,17 +140,12 @@ 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 = {
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()) {
Expand All @@ -159,15 +154,35 @@ 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 {
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));
this._scheduleLeaveChordMode();
}

private _leaveChordMode(): void {
if (this._currentChordStatusMessage) {
this._currentChordStatusMessage.dispose();
Expand Down Expand Up @@ -255,15 +270,15 @@ 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();
firstChord = dispatchKeyname;
currentChord = dispatchKeyname;
currentChord = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a keybinding like shift shift still work after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might not. I’ll take a look at how we can make this work again 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed some changes to restore the previous behavior. The way the single modifier chords are set-up, they can't be chorded to arbitrary length, but I feel like that's okay? (It fully restores the old behavior, while allowing arbitrary length chords for regular keys.)

} else {
[firstChord,] = keybinding.getDispatchChords();
currentChord = this._currentChord ? this._currentChord.keypress : null;
currentChord = this._currentChord ? this._currentChord.map(({ keypress }) => keypress) : null;
}

if (firstChord === null) {
Expand All @@ -286,9 +301,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;
}
}
Expand Down
67 changes: 42 additions & 25 deletions src/vs/platform/keybinding/common/keybindingResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,19 +58,17 @@ export class KeybindingResolver {
continue;
}

// TODO@chords
this._addKeyPress(k.chords[0], k);
}
}

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 {
if (keypress) {
for (let i = 0; i < keypress.length; i++) {
if (keypress[i] !== defaultKb.chords[i]) {
return false;
}
}
}

// `true` means always, as does `undefined`
Expand Down Expand Up @@ -129,11 +127,8 @@ export class KeybindingResolver {
}
let isRemoved = false;
for (const commandRemoval of commandRemovals) {
// TODO@chords
const keypressFirstChord = commandRemoval.chords[0];
const keypressSecondChord = commandRemoval.chords[1];
const when = commandRemoval.when;
if (this._isTargetedForRemoval(rule, keypressFirstChord, keypressSecondChord, when)) {
if (this._isTargetedForRemoval(rule, commandRemoval.chords, when)) {
isRemoved = true;
break;
}
Expand Down Expand Up @@ -164,12 +159,17 @@ 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
// 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++) {
ulugbekna marked this conversation as resolved.
Show resolved Hide resolved
if (conflict.chords[i] !== item.chords[i]) {
// The ith step does not conflict
isShorterKbPrefix = false;
break;
}
}
if (!isShorterKbPrefix) {
continue;
}

Expand Down Expand Up @@ -274,14 +274,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.`);
Expand All @@ -291,8 +290,18 @@ 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) {
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);
}
}
Expand All @@ -313,7 +322,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 {
Expand All @@ -323,6 +331,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)}.`);
Expand Down
36 changes: 27 additions & 9 deletions src/vs/platform/keybinding/test/common/keybindingResolver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -399,10 +404,10 @@ 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(<KeyCodeChord>expectedKeybinding.chords[i]);
const result = resolver.resolve(ctx, previousChord, chord);
Expand All @@ -412,14 +417,24 @@ suite('KeybindingResolver', () => {
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);
}
};

Expand Down Expand Up @@ -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');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
ulugbekna marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading