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

Add schema for key binding file #10613

Merged
merged 3 commits into from
Jan 14, 2022
Merged

Conversation

colin-grant-work
Copy link
Contributor

What it does

Fixes #5290 by adding a schema for keybindings that is updated whenever commands are registered or unregistered. It also adds an annotation to our various existing schemas (workspace files, tasks, debug, preferences) indicating that those files should be allowed to have trailing commas and comments, since they are parsed using the JSONC parser rather than standard JSON.

How to test

  1. Create and open a keymaps.json file either yourself or from the Keybindings Widget.
  2. Try to create a new keybinding by creating an array at the top level (if it hasn't already been created) and then creating an object in the array.
  3. Trigger auto-complete suggestions (ctrl + .).
  4. Observe that four fields are suggested (command, key, when, args).
  5. Select command
  6. Trigger autocompletion again.
  7. You should see a list of all the commands available to be bound.
  8. If you write something other than one of the available commands, you shouldn't get an error.
  9. If you otherwise deviate from the schema (top-level object, non-string value for any of the fields except args), you should get a warning of some kind.

  1. Adding trailing commas or comments to a preference file should no longer trigger a warning in the editor.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added the keybindings issues related to keybindings label Jan 10, 2022
@colin-grant-work colin-grant-work force-pushed the feature/schema-for-keybindings branch from 532d3a3 to 68c7261 Compare January 10, 2022 22:43
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@colin-grant-work I'm getting some odd behaviors, perhaps you can confirm:

  1. I sometimes get preferences when triggering (this happened for me initially, and later on as well when a proper schema was used):
keybindings-prefs.mp4
  1. The keybindings-widget does not seem to populate key which seems to be required by the schema (is it something we should update as part of the pr?):

kb-key

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Jan 11, 2022

@vince-fugnitto

  1. I sometimes get preferences when triggering (this happened for me initially, and later on as well when a proper schema was used):

This is not great, but not the fault of this code. Those suggestions are coming from the 'any string' suggestion machinery, and that appears to be very difficult to deactivate (happy for any suggestions!). @kenneth-marut-work has had the same difficulty in the breakpoint widget, and we haven't yet found a way to stop those string completions from coming through.

  1. The keybindings-widget does not seem to populate key which seems to be required by the schema (is it something we should update as part of the pr?):

There seem to be a number of differences between our keybinding machinery and VSCode's. Their filename is keybindings.json ours in keymaps.json, and apparently we use keybinding in place of VSCode's key. I will update the schema to accept our keys with a deprecation warning, and update the machinery to handle VSCode's, so that the key binding file can function in the same way as the other preference files: we can accept either VSCode's or ours, although in this case, it's only ever stored in user storage. Still better to let people C&P their VSCode config in, rather than making them reformat.

@vince-fugnitto
Copy link
Member

Those suggestions are coming from the 'any string' suggestion machinery, and that appears to be very difficult to deactivate (happy for any suggestions!)

I'm fine with the behavior we have for the moment, just wanted to point it out in case it was actually a bug with the pr, thank you for clarifying!

  1. The keybindings-widget does not seem to populate key which seems to be required by the schema (is it something we should update as part of the pr?):

There seem to be a number of differences between our keybinding machinery and VSCode's. Their filename is keybindings.json ours in keymaps.json, and apparently we use keybinding in place of VSCode's key.

The filename difference is likely not so critical, the file is located in the user's home and we would not read .vscode/keybindings.json (or wherever it is located) in any case.

I will update the schema to accept our keys with a deprecation warning, and update the machinery to handle VSCode's, so that the key binding file can function in the same way as the other preference files

Sounds good to me!

@colin-grant-work colin-grant-work force-pushed the feature/schema-for-keybindings branch from 68c7261 to fd40d94 Compare January 12, 2022 00:11
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I can confirm that the recent changes work well 👍

I have a question, when we edit the keybinding for a command that has a context (deprecated in favor of when), we set context correctly in the keymaps.json. The issue is that hovering over context does not yield any information:

For example:

[
    {
        "command": "workbench.action.debug.pause",
        "keybinding": "ctrl+e",
        "context": "inDebugMode"
    },
    {
        "command": "-workbench.action.debug.pause",
        "keybinding": "f6",
        "context": "inDebugMode",
    },
]
context.mp4

@colin-grant-work colin-grant-work force-pushed the feature/schema-for-keybindings branch from fd40d94 to d9922a2 Compare January 13, 2022 19:53
@colin-grant-work
Copy link
Contributor Author

@vince-fugnitto, I've added the context field to the schema with a deprecation warning. That means that it is omitted from auto-complete suggestions, but if it's present, it will be validated, a description will be available, and a deprecation warning will be shown.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well for me 👍

I confirmed that:

  • the keymaps.json now has a schema
  • comments and trailing commas are permitted, and do not produce markers
  • the schema has auto-completion, and intelligence (hovering) correctly
  • the deprecated context is supported, and does not show up in the auto-completion

@colin-grant-work colin-grant-work merged commit fc97caa into master Jan 14, 2022
@colin-grant-work colin-grant-work deleted the feature/schema-for-keybindings branch January 14, 2022 15:46
@github-actions github-actions bot added this to the 1.22.0 milestone Jan 14, 2022
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.

Keymaps.json should have JSON schema attached
2 participants