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

Store keybindings properly in keymaps.json and improve conflict checking #9088

Conversation

slhultgren
Copy link
Contributor

@slhultgren slhultgren commented Feb 18, 2021

Fixes issue #9087
Fixes issue #9094

What it does

Contributed by STMicroelectronics

How to test

Change some shortcuts (at least 2), observe that no "resolved" properties are stored in the keymaps.
Also, follow steps in #9094.

For example, without this patch keymaps.json might contain:

  {
    "command": "textEditor.commands.go.lastEdit",
    "keybinding": "ctrl+alt+left",
    "resolved": [
      {
        "key": {
          "code": "ArrowLeft",
          "keyCode": 37,
          "easyString": "left"
        },
        "ctrl": true,
        "shift": false,
        "alt": true,
        "meta": false
      }
    ],
    "scope": 1
  },

With this patch, keymaps.json will contain instead:

  {
    "command": "textEditor.commands.go.lastEdit",
    "keybinding": "ctrl+alt+left"
  },

Review checklist

Reminder for reviewers

Fixes issue eclipse-theia#9087

Contributed by STMicroelectronics
Signed-off-by: Samuel HULTGREN <[email protected]>
@kittaakos kittaakos self-requested a review February 18, 2021 19:23
@vince-fugnitto vince-fugnitto added the keybindings issues related to keybindings label Feb 19, 2021
@slhultgren slhultgren changed the title Serialize the proper API object for the keymaps.json file Store keybindings properly in keymaps.json and improve conflict checking Feb 20, 2021
@slhultgren slhultgren force-pushed the issue_9087_broken_shortcuts_keymaps_json branch from 63ef3c7 to 115667a Compare February 20, 2021 11:13
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have verified it; it works as expected. Thank you so much for your help, @slhultgren 👍

* Returns with the string representation of the binding.
* Any additional properties which are not described on
* the `Keybinding` API will be ignored.
* Compares two keybingings for equality.
Copy link
Contributor

Choose a reason for hiding this comment

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

keybingings -> keybindings

Make sure disablements for keymappings are only stored once in keymaps.json.
Only check conflicting keybindings against usable keybindings.

Fixes eclipse-theia#9094

Contributed by STMicroelectronics
Signed-off-by: Samuel HULTGREN <[email protected]>
@slhultgren slhultgren force-pushed the issue_9087_broken_shortcuts_keymaps_json branch from 115667a to 4119bdb Compare February 23, 2021 14:46
@slhultgren
Copy link
Contributor Author

Thanks for the review, I amended the commit and fixed the typo :)

@kittaakos
Copy link
Contributor

I consider the root INFO Navigator root INFO 1) "before each" hook for "copy file" test failure unrelated to this PR and merge the changes. Thank you for the fixes, @slhultgren 👍

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

Successfully merging this pull request may close these issues.

4 participants