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

Allow empty bindings in override bindings #2

Closed
wants to merge 1 commit into from

Conversation

roblabla
Copy link

@roblabla roblabla commented Jul 1, 2020

This allows override bindings to not declare a bindings array. This can be useful if the bindings menu will be populated with other overrides. For instance, the sample config will now work:

{
  "whichkey.bindingOverrides": [
    {
      "keys": "g.H",
      "name": "Hunks",
      "type": "bindings"
    },
    {
      "keys": "g.H.s",
      "name": "Stage hunk",
      "type": "command",
      "command": "git.stageSelectedRanges"
    }
  ]
}

Previously, the former config would fail to populate the "Hunks" menu due to the missing bindings entry defaulting to null instead of an empty array. This would cause the overrideBindings function to behave as if the "Hunks" menu didn't exist, and fail to append the item.

This allows override bindings to not declare a bindings array. This can
be useful if the bindings menu will be populated with other overrides.
For instance, the sample config will now work:

```json
{
  "whichkey.bindingOverrides": [
    {
      "keys": "g.H",
      "name": "Hunks",
      "type": "bindings"
    },
    {
      "keys": "g.H.s",
      "name": "Stage hunk",
      "type": "command",
      "command": "git.stageSelectedRanges"
    }
  ]
}
```

Previously, the former config would fail to populate the "Hunks" menu
due to the missing bindings entry defaulting to null instead of an empty
array. This would cause the overrideBindings function to behave as if
the "Hunks" menu didn't exist, and fail to append the item.
@stevenguh
Copy link
Member

Thank you for contributing. This allows multiple ways for adding bindings which might not be ideal. The design is that when you say the type is bindings, the json should contain a bindings key, and that is consistent across the normal bindings and overrides. The follow overrides json is example of how I am imagining you would do this.

{
    "whichkey.bindingOverrides": [
        {
            "keys": "g.H",
            "name": "Hunks",
            "type": "bindings",
            "bindings": [
                {
                    "key": "s",
                    "name": "Stage hunk",
                    "type": "command",
                    "command": "git.stageSelectedRanges"
                }
            ]
        }
    ]
}

@roblabla
Copy link
Author

roblabla commented Jul 1, 2020

Well, currently the JSON I provided will cause a generic error to be shown when attempting to access the Hunks menu: "TaskQueue: Error running task. Cannot read property 'message' of undefined.". This is mostly what I set to fix with this PR.

The way I understang the code, if bindings isn't set, this.items will never be set either. Having this.items be null will cause the action to get rejected, without any information about what went wrong, which I think causes the generic error.

Furthermore, my "way" of declaring a bindings menu and subsequently overriding it still works if we just set bindings: [], like the following json. This PR isn't so much about making this "way" of creating menus from overrides work, and more about making it harder to put the extension in an invalid state through a "broken" settings file.

{
  "whichkey.bindingOverrides": [
    {
      "keys": "g.H",
      "name": "Hunks",
      "type": "bindings",
      "bindings": []
    },
    {
      "keys": "g.H.s",
      "name": "Stage hunk",
      "type": "command",
      "command": "git.stageSelectedRanges"
    }
  ]
}

@stevenguh
Copy link
Member

I understand this is making it more robust when json is "incorrect". My thinking is that if users have incorrect json, they should correct it instead of us guessing what they mean. In your example json, you can interpret it as an empty binding, and can also be interpreted as the users missed the bindings.

How about a warning like notification when the user specifies something that's not correct? Instead of assuming the intent of the missing binding key. The reason I mentioned this is that, this PR doesn't solve the issue when the user didn't specify command if the type is command or any other types.

@stevenguh
Copy link
Member

Added an error message for when executing bindings has incorrect properties in v0.6.0. Hopefully that addresses part of the issue in the PR.

@stevenguh
Copy link
Member

Closed due to stale and in favor of #7 so user will know more clearly what's wrong with their config. Thank you for contributing :)

@stevenguh stevenguh closed this Aug 22, 2020
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.

2 participants