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.

Sign up for GitHub

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

Jump to bottom

refactor keybinding resolver #178699

Merged
merged 10 commits into from Apr 4, 2023
Merged

refactor keybinding resolver #178699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3dd6c77
abstractKeybindingService: refactor: don't use `null` when an empty a…
ulugbekna Mar 29, 2023
65faeae
keybindingResolver: refactor: easier case first
ulugbekna Mar 29, 2023
a3016be
keybindingResolver: refactor: use a discriminated union to
ulugbekna Mar 29, 2023
a6a783a
keybindingResolver: refactor: `resolve` doesn't take a null
ulugbekna Mar 29, 2023
38a0fbe
keybindingResolver: refactor: move handle-removal tests into a sub-suite
ulugbekna Mar 30, 2023
5d73bb4
keybindingResolver: tests: refactor: make tests more extensible to wr…
ulugbekna Mar 30, 2023
2981d63
keybindingResolver: tests: refactor: split into separate tests
ulugbekna Mar 30, 2023
38a5871
keybindingResolver: more cleanup
ulugbekna Mar 30, 2023
c85f7be
keybindingResolver: simplify and comment `resolve`
ulugbekna Apr 1, 2023
dcd0d14
keybindingResolver: simplify algo but with some additional allocation
ulugbekna Apr 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/vs/editor/contrib/hover/browser/hover.ts
View file
Open in desktop
Expand Up @@ -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 {

Expand Down Expand Up @@ -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) {
Expand Down
146 changes: 89 additions & 57 deletions src/vs/platform/keybinding/common/abstractKeybindingService.ts
View file
Open in desktop
Expand Up @@ -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';
Expand All @@ -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';
Expand All @@ -30,6 +31,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<void> = this._register(new Emitter<void>());
Expand All @@ -44,7 +46,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;
Expand All @@ -55,7 +57,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(
Expand All @@ -67,7 +69,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;
Expand Down Expand Up @@ -134,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()) {
Expand All @@ -149,7 +153,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.map((({ keypress }) => keypress));
return this._getResolver().resolve(contextValue, currentChords, firstChord);
}

Expand All @@ -171,25 +175,28 @@ export abstract class AbstractKeybindingService extends Disposable implements IK
}, 500);
}

private _enterMultiChordMode(firstChord: string, keypressLabel: string | null): void {
this._currentChords = [{
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 = this._currentChords ? this._currentChords : [];
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 {
Expand All @@ -198,7 +205,7 @@ export abstract class AbstractKeybindingService extends Disposable implements IK
this._currentChordStatusMessage = null;
}
this._currentChordChecker.cancel();
this._currentChords = null;
this._currentChords = [];
IME.enable();
}

Expand Down Expand Up @@ -287,10 +294,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.map(({ keypress }) => keypress);
}

if (userPressedChord === null) {
Expand All @@ -304,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) {
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 */ });
shouldPreventDefault = true;
}
}

this._leaveChordMode();
case ResultKind.MoreChordsNeeded: {

this._logService.trace('KeybindingService#dispatch', keypressLabel, `[ Several keybindings match - more chords needed ]`);

if (resolveResult && resolveResult.commandId) {
if (!resolveResult.bubble) {
shouldPreventDefault = true;
this._expectAnotherChord(userPressedChord, keypressLabel);
this._log(this._currentChords.length === 1 ? `+ Entering multi-chord mode...` : `+ Continuing multi-chord mode...`);
return shouldPreventDefault;
}
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<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { id: resolveResult.commandId, from: 'keybinding', detail: userKeypress.getUserSettingsLabel() ?? undefined });

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<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { id: resolveResult.commandId, from: 'keybinding', detail: userKeypress.getUserSettingsLabel() ?? undefined });
}
}

return shouldPreventDefault;
}
}

return shouldPreventDefault;
}

mightProducePrintableCharacter(event: IKeyboardEvent): boolean {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/platform/keybinding/common/keybinding.ts
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;

Expand Down