Fixes microsoft#141638: -command rules should only remove default rules

Author: alexdima

src/vs/platform/keybinding/common/keybindingResolver.ts

@@ -87,15 +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, { maxIndex: number; rule: ResolvedKeybindingItem; }[]>();
+		const removals = new Map<string, 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, [{ maxIndex: i, rule }]);
+					removals.set(command, [rule]);
 				} else {
-					removals.get(command)!.push({ maxIndex: i, rule });
+					removals.get(command)!.push(rule);
 				}
 			}
 		}
@@ -118,21 +118,16 @@ export class KeybindingResolver {
 				continue;
 			}
 			const commandRemovals = removals.get(rule.command);
-			if (!commandRemovals) {
+			if (!commandRemovals || !rule.isDefault) {
 				result.push(rule);
 				continue;
 			}
 			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 = removalRule.keypressParts[0];
-				const keypressChordPart = removalRule.keypressParts[1];
-				const when = removalRule.when;
+				const keypressFirstPart = commandRemoval.keypressParts[0];
+				const keypressChordPart = commandRemoval.keypressParts[1];
+				const when = commandRemoval.when;
 				if (this._isTargetedForRemoval(rule, keypressFirstPart, keypressChordPart, when)) {
 					isRemoved = true;
 					break;

src/vs/platform/keybinding/test/common/keybindingResolver.test.ts

@@ -221,6 +221,20 @@ suite('KeybindingResolver', () => {
 		]);
 	});
 
+	test('issue #141638: Keyboard Shortcuts: Change When Expression might actually remove keybinding in Insiders', () => {
+		const defaults = [
+			kbItem(KeyCode.KeyA, 'command1', null, undefined, true),
+		];
+		const overrides = [
+			kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.equals('a', '1'), false),
+			kbItem(KeyCode.KeyA, '-command1', null, undefined, false),
+		];
+		const actual = KeybindingResolver.handleRemovals([...defaults, ...overrides]);
+		assert.deepStrictEqual(actual, [
+			kbItem(KeyCode.KeyA, 'command1', null, ContextKeyExpr.equals('a', '1'), false)
+		]);
+	});
+
 	test('contextIsEntirelyIncluded', () => {
 		const toContextKeyExpression = (expr: ContextKeyExpression | string | null) => {
 			if (typeof expr === 'string' || !expr) {