Skip to content

Commit

Permalink
Fixes #140884: removal rules only target rules that appear before
Browse files Browse the repository at this point in the history
  • Loading branch information
alexdima committed Jan 25, 2022
1 parent bb774f6 commit c563008
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 8 deletions.
24 changes: 16 additions & 8 deletions src/vs/platform/keybinding/common/keybindingResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,15 @@ export class KeybindingResolver {
*/
public static handleRemovals(rules: ResolvedKeybindingItem[]): ResolvedKeybindingItem[] {
// Do a first pass and construct a hash-map for removals
const removals = new Map<string, ResolvedKeybindingItem[]>();
for (const rule of rules) {
const removals = new Map<string, { maxIndex: number; rule: ResolvedKeybindingItem; }[]>();
for (let i = 0, len = rules.length; i < len; i++) {
const rule = rules[i];
if (rule.command && rule.command.charAt(0) === '-') {
const command = rule.command.substring(1);
if (!removals.has(command)) {
removals.set(command, [rule]);
removals.set(command, [{ maxIndex: i, rule }]);
} else {
removals.get(command)!.push(rule);
removals.get(command)!.push({ maxIndex: i, rule });
}
}
}
Expand All @@ -106,7 +107,9 @@ export class KeybindingResolver {

// Do a second pass and keep only non-removed keybindings
const result: ResolvedKeybindingItem[] = [];
for (const rule of rules) {
for (let i = 0, len = rules.length; i < len; i++) {
const rule = rules[i];

if (!rule.command || rule.command.length === 0) {
result.push(rule);
continue;
Expand All @@ -121,10 +124,15 @@ export class KeybindingResolver {
}
let isRemoved = false;
for (const commandRemoval of commandRemovals) {
if (i > commandRemoval.maxIndex) {
// this command removal is above this rule, so it cannot influence it
continue;
}
const removalRule = commandRemoval.rule;
// TODO@chords
const keypressFirstPart = commandRemoval.keypressParts[0];
const keypressChordPart = commandRemoval.keypressParts[1];
const when = commandRemoval.when;
const keypressFirstPart = removalRule.keypressParts[0];
const keypressChordPart = removalRule.keypressParts[1];
const when = removalRule.when;
if (this._isTargetedForRemoval(rule, keypressFirstPart, keypressChordPart, when)) {
isRemoved = true;
break;
Expand Down
14 changes: 14 additions & 0 deletions src/vs/platform/keybinding/test/common/keybindingResolver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,20 @@ suite('KeybindingResolver', () => {
]);
});

test('issue #140884 Unable to reassign F1 as keybinding for Show All Commands', () => {
const defaults = [
kbItem(KeyCode.KeyA, 'command1', null, undefined, true),
];
const overrides = [
kbItem(KeyCode.KeyA, '-command1', null, undefined, false),
kbItem(KeyCode.KeyA, 'command1', null, undefined, false),
];
const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]);
assert.deepStrictEqual(actual, [
kbItem(KeyCode.KeyA, 'command1', null, undefined, false)
]);
});

test('contextIsEntirelyIncluded', () => {
const toContextKeyExpression = (expr: ContextKeyExpression | string | null) => {
if (typeof expr === 'string' || !expr) {
Expand Down

0 comments on commit c563008

Please sign in to comment.