Skip to content

Commit

Permalink
fix #7836: respect keybinding scope and registration order during eva…
Browse files Browse the repository at this point in the history 
…luation

Also match only first keybinding on keydown, don't traverse over all keybindings.

Signed-off-by: Anton Kosyakov <[email protected]>
  • Loading branch information
@akosyakov
akosyakov committed May 19, 2020
1 parent bb43e9e commit 9afdbd3
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 82 deletions.
41 changes: 20 additions & 21 deletions packages/core/src/browser/keybinding.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,15 @@ describe('keybindings', () => {

keybindingRegistry.setKeymap(KeybindingScope.WORKSPACE, keybindingsSpecific);

let bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.CtrlCmd] })]).full;
expect(bindings).to.have.lengthOf(1);
let match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.CtrlCmd] })]);
expect(match && match.kind).to.be.equal('full');

bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_B, modifiers: [KeyModifier.CtrlCmd] })]).full;
expect(bindings).to.have.lengthOf(1);
match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_B, modifiers: [KeyModifier.CtrlCmd] })]);
expect(match && match.kind).to.be.equal('full');

bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] })]).full;
const keyCode = KeyCode.parse(bindings[0].keybinding);
expect(keyCode.key).to.be.equal(validKeyCode.key);
match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] })]);
const keyCode = match && KeyCode.parse(match.binding.keybinding);
expect(keyCode?.key).to.be.equal(validKeyCode.key);
});

it('should return partial keybinding matches', () => {
Expand All @@ -249,8 +249,8 @@ describe('keybindings', () => {
validKeyCodes.push(KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] }));
validKeyCodes.push(KeyCode.createKeyCode({ first: Key.KEY_T }));

const bindings = keybindingRegistry.getKeybindingsForKeySequence(KeySequence.parse('ctrlcmd+x'));
expect(bindings.partial.length > 0);
const match = keybindingRegistry.matchKeybiding(KeySequence.parse('ctrlcmd+x'));
expect(match && match.kind).to.be.equal('partial');
});

it('should not register a shadowing keybinding', () => {
Expand All @@ -276,7 +276,6 @@ describe('keybindings', () => {

it('shadowed bindings should be returned last', () => {
const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] });
let bindings: Keybinding[];

const ignoredDefaultBinding: Keybinding = {
keybinding: keyCode.toString(),
Expand All @@ -303,29 +302,29 @@ describe('keybindings', () => {
keybindingRegistry.setKeymap(KeybindingScope.WORKSPACE, [workspaceBinding]);
// now WORKSPACE bindings are overriding the other scopes

bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
expect(bindings).to.have.lengthOf(3);
expect(bindings[0].command).to.be.equal(workspaceBinding.command);
let match = keybindingRegistry.matchKeybiding([keyCode]);
expect(match?.kind).to.be.equal('full');
expect(match?.binding?.command).to.be.equal(workspaceBinding.command);

keybindingRegistry.resetKeybindingsForScope(KeybindingScope.WORKSPACE);
// now it should find USER bindings

bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
expect(bindings).to.have.lengthOf(2);
expect(bindings[0].command).to.be.equal(userBinding.command);
match = keybindingRegistry.matchKeybiding([keyCode]);
expect(match?.kind).to.be.equal('full');
expect(match?.binding?.command).to.be.equal(userBinding.command);

keybindingRegistry.resetKeybindingsForScope(KeybindingScope.USER);
// and finally it should fallback to DEFAULT bindings.

bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
expect(bindings).to.have.lengthOf(1);
expect(bindings[0].command).to.be.equal(defaultBinding.command);
match = keybindingRegistry.matchKeybiding([keyCode]);
expect(match?.kind).to.be.equal('full');
expect(match?.binding?.command).to.be.equal(defaultBinding.command);

keybindingRegistry.resetKeybindingsForScope(KeybindingScope.DEFAULT);
// now the registry should be empty

bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
expect(bindings).to.be.empty;
match = keybindingRegistry.matchKeybiding([keyCode]);
expect(match).to.be.undefined;

});
});
Expand Down
105 changes: 49 additions & 56 deletions packages/core/src/browser/keybinding.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import * as common from '../common/keybinding';

export enum KeybindingScope {
DEFAULT,
DEFAULT_OVERRIDING,
USER,
WORKSPACE,
END
Expand All @@ -56,7 +55,7 @@ export interface ResolvedKeybinding extends Keybinding {

export interface ScopedKeybinding extends Keybinding {
/** Current keybinding scope */
scope?: KeybindingScope;
scope: KeybindingScope;
}

export const KeybindingContribution = Symbol('KeybindingContribution');
Expand Down Expand Up @@ -100,7 +99,7 @@ export class KeybindingRegistry {
protected keySequence: KeySequence = [];

protected readonly contexts: { [id: string]: KeybindingContext } = {};
protected readonly keymaps: Keybinding[][] = [...Array(KeybindingScope.length)].map(() => []);
protected readonly keymaps: ScopedKeybinding[][] = [...Array(KeybindingScope.length)].map(() => []);

@inject(KeyboardLayoutService)
protected readonly keyboardLayoutService: KeyboardLayoutService;
Expand Down Expand Up @@ -167,11 +166,12 @@ export class KeybindingRegistry {
/**
* Register a default keybinding to the registry.
*
* Keybindings registered later have higher priority during evaluation.
*
* @param binding
* @param override Override existed keybinding
*/
registerKeybinding(binding: Keybinding, override?: boolean): Disposable {
return this.doRegisterKeybinding(binding, !!override ? KeybindingScope.DEFAULT_OVERRIDING : undefined);
registerKeybinding(binding: Keybinding): Disposable {
return this.doRegisterKeybinding(binding);
}

/**
Expand Down Expand Up @@ -219,12 +219,14 @@ export class KeybindingRegistry {
protected doRegisterKeybinding(binding: Keybinding, scope: KeybindingScope = KeybindingScope.DEFAULT): Disposable {
try {
this.resolveKeybinding(binding);
if (this.containsKeybinding(this.keymaps[scope], binding) && scope !== KeybindingScope.DEFAULT_OVERRIDING) {
// TODO: get rid of this check as soon as `context` property is removed
if (this.containsKeybinding(this.keymaps[scope], binding)) {
throw new Error(`"${binding.keybinding}" is in collision with something else [scope:${scope}]`);
}
this.keymaps[scope].push(binding);
const scoped = Object.assign(binding, { scope });
this.keymaps[scope].unshift(scoped);
return Disposable.create(() => {
const index = this.keymaps[scope].indexOf(binding);
const index = this.keymaps[scope].indexOf(scoped);
if (index !== -1) {
this.keymaps[scope].splice(index, 1);
}
Expand Down Expand Up @@ -266,7 +268,7 @@ export class KeybindingRegistry {
* @param bindings the keybinding reference list
* @param binding the keybinding to test collisions for
*/
containsKeybinding(bindings: Keybinding[], binding: Keybinding): boolean {
containsKeybinding(bindings: ScopedKeybinding[], binding: Keybinding): boolean {
const bindingKeySequence = this.resolveKeybinding(binding);
const collisions = this.getKeySequenceCollisions(bindings, bindingKeySequence)
.filter(b => b.context === binding.context && !b.when && !binding.when);
Expand Down Expand Up @@ -363,30 +365,13 @@ export class KeybindingRegistry {
}
}

/**
* Finds collisions for a binding inside a list of bindings (error-free)
*
* @param bindings the reference bindings
* @param binding the binding to match
*/
protected getKeybindingCollisions(bindings: Keybinding[], binding: Keybinding): KeybindingRegistry.KeybindingsResult {
const result = new KeybindingRegistry.KeybindingsResult();
try {
const bindingKeySequence = this.resolveKeybinding(binding);
result.merge(this.getKeySequenceCollisions(bindings, bindingKeySequence));
} catch (error) {
this.logger.warn(error);
}
return result;
}

/**
* Finds collisions for a key sequence inside a list of bindings (error-free)
*
* @param bindings the reference bindings
* @param candidate the sequence to match
*/
protected getKeySequenceCollisions(bindings: Keybinding[], candidate: KeySequence): KeybindingRegistry.KeybindingsResult {
protected getKeySequenceCollisions(bindings: ScopedKeybinding[], candidate: KeySequence): KeybindingRegistry.KeybindingsResult {
const result = new KeybindingRegistry.KeybindingsResult();
for (const binding of bindings) {
try {
Expand All @@ -413,28 +398,6 @@ export class KeybindingRegistry {
return result;
}

/**
* Get the lists of keybindings matching fully or partially matching a KeySequence.
*
* @param keySequence The key sequence for which we are looking for keybindings.
*/
getKeybindingsForKeySequence(keySequence: KeySequence): KeybindingRegistry.KeybindingsResult {
const result = new KeybindingRegistry.KeybindingsResult();

for (let scope = KeybindingScope.END; --scope >= KeybindingScope.DEFAULT;) {
const matches = this.getKeySequenceCollisions(this.keymaps[scope], keySequence);

if (scope === KeybindingScope.DEFAULT_OVERRIDING) {
matches.full.reverse();
matches.partial.reverse();
}
matches.full.forEach(binding => binding.scope = scope);
matches.partial.forEach(binding => binding.scope = scope);
result.merge(matches);
}
return result;
}

/**
* Get the keybindings associated to commandId.
*
Expand Down Expand Up @@ -578,10 +541,9 @@ export class KeybindingRegistry {

this.keyboardLayoutService.validateKeyCode(keyCode);
this.keySequence.push(keyCode);
const bindings = this.getKeybindingsForKeySequence(this.keySequence);
const full = bindings.full.find(binding => this.isEnabled(binding, event));
const partial = bindings.partial.find(binding => (!full || binding.scope! > full.scope!) && this.isEnabled(binding, event));
if (partial) {
const match = this.matchKeybiding(this.keySequence, event);

if (match && match.kind === 'partial') {
/* Accumulate the keysequence */
event.preventDefault();
event.stopPropagation();
Expand All @@ -592,14 +554,41 @@ export class KeybindingRegistry {
priority: 2
});
} else {
if (full) {
this.executeKeyBinding(full, event);
if (match && match.kind === 'full') {
this.executeKeyBinding(match.binding, event);
}
this.keySequence = [];
this.statusBar.removeElement('keybinding-status');
}
}

/**
* Match first full binding or partial if it belongs to the higher scope in the current context.
*
* Keybinidngs priority:
* - keybining scope
* - registraion order within scope
*/
matchKeybiding(keySequence: KeySequence, event?: KeyboardEvent): KeybindingRegistry.Match {
let partial: ScopedKeybinding | undefined;
for (let scope = KeybindingScope.END; --scope >= KeybindingScope.DEFAULT;) {
if (partial) {
return { kind: 'partial', binding: partial };
}
for (const binding of this.keymaps[scope]) {
const resolved = this.resolveKeybinding(binding);
const compareResult = KeySequence.compare(keySequence, resolved);
if (compareResult === KeySequence.CompareResult.FULL && (!event || this.isEnabled(binding, event))) {
return { kind: 'full', binding };
}
if (!partial && compareResult === KeySequence.CompareResult.PARTIAL && (!event || this.isEnabled(binding, event))) {
partial = binding;
}
}
}
return undefined;
}

/**
* Return true of string a pseudo-command id, in other words a command id
* that has a special meaning and that we won't find in the command
Expand Down Expand Up @@ -641,6 +630,10 @@ export class KeybindingRegistry {
}

export namespace KeybindingRegistry {
export type Match = {
kind: 'full' | 'partial'
binding: ScopedKeybinding
} | undefined;
export class KeybindingsResult {
full: ScopedKeybinding[] = [];
partial: ScopedKeybinding[] = [];
Expand Down
5 changes: 1 addition & 4 deletions packages/monaco/src/browser/monaco-keybinding.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ export class MonacoKeybindingContribution implements KeybindingContribution {

registerKeybindings(registry: KeybindingRegistry): void {
const defaultKeybindings = monaco.keybindings.KeybindingsRegistry.getDefaultKeybindings();
// register in reverse order to align with Monaco dispatch logic:
// https://github.com/TypeFox/vscode/blob/70b8db24a37fafc77247de7f7cb5bb0195120ed0/src/vs/platform/keybinding/common/keybindingResolver.ts#L302
for (let i = defaultKeybindings.length - 1; i >= 0; i--) {
const item = defaultKeybindings[i];
for (const item of defaultKeybindings) {
const command = this.commands.validate(item.command);
if (command) {
const when = item.when && item.when.serialize();
Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class KeybindingsContributionPointHandler {
for (const raw of contributions.keybindings) {
const keybinding = this.toKeybinding(raw);
if (keybinding) {
toDispose.push(this.keybindingRegistry.registerKeybinding(keybinding, true));
toDispose.push(this.keybindingRegistry.registerKeybinding(keybinding));
}
}
return toDispose;
Expand Down

0 comments on commit 9afdbd3

Please sign in to comment.