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

Aligned keybindings with VSCode #5621

Closed
wants to merge 1 commit into from
Closed

Aligned keybindings with VSCode #5621

wants to merge 1 commit into from

Conversation

akosyakov
Copy link
Member

fixed #5303

It's rebase of #5326, since the original PR is opened from a fork without right to commit for maintainers.

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
@akosyakov
Copy link
Member Author

@vince-fugnitto Could you test whether the keybinding widget still shows context and source properly? There was a merge conflict with it. If it is fine please approve and we can merge it.

@vince-fugnitto
Copy link
Member

@vince-fugnitto Could you test whether the keybinding widget still shows context and source properly? There was a merge conflict with it. If it is fine please approve and we can merge it.

It seems to work correctly, but the reset code in the keybindings-widget needs to be adjusted.
It looks like when the keybinding resets (goes back to its default value), the removal rules are not removed from the keymaps.json.

Example:

  1. update a command's keybinding.
[
    {
        "command": "core.close.tab",
        "keybinding": "alt+a",
        "context": ""
    },
    {
        "command": "-core.close.tab",
        "keybinding": "alt+w",
        "context": ""
    }
]
  1. Perform a reset (the following remains in the keymaps.json).
[
    {
        "command": "-core.close.tab",
        "keybinding": "alt+w",
        "context": ""
    }
]

Is it outside the scope of the PR?

@akosyakov
Copy link
Member Author

@vince-fugnitto I've seen it, but it does not seem to break keybindings handling? Does it?

@vince-fugnitto
Copy link
Member

@vince-fugnitto I've seen it, but it does not seem to break keybindings handling? Does it?

I'm not sure, but I do know that if I have a custom keybinding that I set, then reset it, I lose the original keybinding due to the removal rule.

For example:

  1. I update the keybinding for Close Tab from alt+w to alt+a
  2. the keybinding alt+w no longer works while alt+a now does
  3. I reset the Close Tab keybinding
  4. The removal rule for alt+w still exists in the keymaps.json meaning I now lost the keybinding

@akosyakov
Copy link
Member Author

akosyakov commented Jul 2, 2019

The removal rule for alt+w still exists in the keymaps.json meaning I now lost the keybinding

@vince-fugnitto I thought - means that a keybinding should not be considered. It should not mean that it should be removed. Checking again...

@vince-fugnitto
Copy link
Member

The removal rule for alt+w still exists in the keymaps.json meaning I now lost the keybinding

@vince-fugnitto I thought - means that a keybinding should not be considered. It should not mean that it should be removed. Checking again...

Sorry if I'm being confusing, when I mean removed I mean that I can no longer use that keybinding as a user, the command becomes useless. I attempt to close tabs using the default keybinding and it no longer works (since it exists in the keymaps as a removal rule).

@akosyakov
Copy link
Member Author

@vince-fugnitto tried and was able to reproduce your issue, could you please comment it on the original PR #5326, it should be taken care of before merging. I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[keybindings] Allow removal rules in keybindings
3 participants