Skip to content

Preserve keybinding default metadata when key is not set#317032

Open
n-gist wants to merge 3 commits into
microsoft:mainfrom
n-gist:feat/keybindings-default-metadata
Open

Preserve keybinding default metadata when key is not set#317032
n-gist wants to merge 3 commits into
microsoft:mainfrom
n-gist:feat/keybindings-default-metadata

Conversation

@n-gist
Copy link
Copy Markdown
Contributor

@n-gist n-gist commented May 18, 2026

This PR enables (or fixes) the ability for extensions to define default keybinding metadata without setting an explicit key.

Currently, the metadata (most importantly the when property) is lost when defining a keybinding like this, even though the schema itself is valid (command2):

{
    "key": "shift+g",
    "command": "extension.command1",
    "when": "extensionContext1"
},
{
    "key": "",
    "command": "extension.command2",
    "when": "extensionContext2"
}

Result:

nopreserve

As a result, extensions cannot provide optional bindable commands without instructing extension users about the correct when values, which can be complex, may evolve across extension versions and generally should not be a concern for extension users when possible.

This change preserves default values, so extension users only need to set the keybinding:

preserve

Changes

What initially looked like it would require a special case turned out, after investigation, to be a straightforward fix:

const keybinding = WorkbenchKeybindingService.bindToCurrentPlatform(key, mac, linux, win);
if (!keybinding) {
return undefined;
}

This truthiness check was filtering out empty key cases. It can safely be replaced with a check for undefined only, since after that KeybindingParser.parseKeybinding(keybinding) resolves empty strings to null, which is a valid value for IExtensionKeybindingRule.

Later, another filter:

for (const rule of rules) {
if (rule.keybinding) {
result[keybindingsLen++] = {

This appears to be another historical truthiness shortcut rather than an intentional semantic filter, since:

  • Renderer paths already handle null values correctly
  • Resolver paths already handle null values correctly
  • Types already allow null

In this context, null should not mean “discard rule”, but rather “keep rule, but with no keybinding”.

Adjusting the first guard and removing the second filter prevents keybindings from being discarded, which in turn preserves the keybinding metadata.

Copilot AI review requested due to automatic review settings May 18, 2026 14:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts extension-contributed keybinding handling so entries with an empty key can preserve associated default metadata such as when clauses.

Changes:

  • Allows empty-string contributed keybindings to become null keybinding rules instead of being dropped.
  • Keeps extension keybinding registry entries even when the parsed keybinding is null.
  • Adds a resolver test for preserving an unbound keybinding override.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/vs/workbench/services/keybinding/browser/keybindingService.ts Changes the guard so only missing keybinding fields are rejected.
src/vs/platform/keybinding/common/keybindingsRegistry.ts Stores extension keybinding rules even when rule.keybinding is null.
src/vs/platform/keybinding/test/common/keybindingResolver.test.ts Adds coverage for preserving an empty keybinding override through removal handling.

Comment thread src/vs/platform/keybinding/common/keybindingsRegistry.ts
Comment thread src/vs/workbench/services/keybinding/browser/keybindingService.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants