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.

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

Fix override of default key bindings #14668

Merged
merged 1 commit into from
Dec 24, 2024
Merged

Fix override of default key bindings #14668

merged 1 commit into from
Dec 24, 2024

Conversation

JonasHelming
Copy link
Contributor

fixed #14667

What it does

fixes #14667
If a key binding is overridden by the user, an entry like this is added to the keymaps.json to indicate that the original keybinding is not valid anymore (with a '-' in the beginning of the command id:

    {
        "command": "-editor.action.inlineSuggest.trigger",
        "keybinding": "Shift+Space",
        "when": "!editorReadonly && editorTextFocus"
    }

The code in 'matchKeybinding' (see only change in this PR) records this and makes the key binding invalid also in upper contexts. However, it first checks "isEnabled" which in 'isEnabledInScope' checks 'this.commandRegistry.isEnabled(binding.command, binding.args)'
This is always false for invalid commands and the '-' makes commands invalid. As a consequence, 'isUsable' is not checked anymore, and the isUsable list is not correctly filled.
Therefore, turning the checks around, i.e. first checking isUsable fixes the issues, as overriden commands are now correctly remembers.

How to test

see #14667

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Review checklist

Reminder for reviewers

@tsmaeder
Copy link
Contributor

I'm not sure I understand the bug here: when I go to the keyboard shortcuts editor and I press "edit" on the "Copy" binding with no "when" clause, I can easily rebind that to "ctrl-i". What's different in your example?

@JonasHelming
Copy link
Contributor Author

After rebinding, the original default binding will still be active. Press Shift Space and it will still trigger the inline suggestion (in addition to Ctrl+I which will also work)

@JonasHelming JonasHelming merged commit e4ca643 into master Dec 24, 2024
11 checks passed
@github-actions github-actions bot added this to the 1.58.0 milestone Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Cannot override default key bindings
2 participants