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

Display a warning when a user tries to bind an action to multiple keys in a single chord #4239

Closed
angelog0 opened this issue Jan 15, 2020 · 2 comments · Fixed by #4746
Closed
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@angelog0
Copy link

After upgrading to WT 0.8, I added this to my profile.json:

"keybindings":
    [
        // "index": 1 ==> "Ubuntu"
        { "keys": ["ctrl+2+plus"],
          "command": {"action": "splitPane", "split": "vertical", "index": 1}
        },
        { "keys": ["ctrl+2+-"],
          "command": {"action": "splitPane", "split": "horizontal", "index": 1}
        }//,
        // "index": 2 ==> "CMD"
        // { "keys": ["ctrl+3+plus"],
        //   "command": {"action": "splitPane", "split": "vertical", "index": 2}
        // },
        // { "keys": ["ctrl+3+-"],
        //   "command": {"action": "splitPane", "split": "horizontal", "index": 2}
        // }
    ]

and it works as expected: profile index 0 is MSYS2 and index 1 is Ubuntu, so ctrl+2+... can split current profile calling Ubuntu.

If I uncomment the other lines (index 2 for CMD), both ctrl+2+... and ctrl+3+... open CMD profile. In short adding similar lines for other profile, always open the last profile and not what they should open...

I wonder what is wrong with those lines.. maybe i have misunderstood? Thanks...

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 15, 2020
@zadjii-msft
Copy link
Member

Oh we should probably display some sort of warning in this case. You can't really add keybindings with multiple keys like that. You can have any combination of Ctrl, Alt and Shift, but only one actual key like 3 or +. I bet as we're deserializing this, we're finding the 3, setting the Vkey to 3, then finding the + and resetting the vkey to +, effectively overriding the other binding like it.

We should be able to catch this at parse time and display a warning to the user.

@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jan 15, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 15, 2020
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Jan 15, 2020
@zadjii-msft zadjii-msft changed the title Splitting panes for a profile Display a warning when a user tries to bind an action to multiple keys in a single chord Jan 15, 2020
@DHowett-MSFT
Copy link
Contributor

We should definitely warn about this, but the feature you'll eventually want is #1334. That one's not slated for 1.0.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 16, 2020
@cinnamon-msft cinnamon-msft added Priority-3 A description (P3) v1-Scrubbed and removed Priority-2 A description (P2) labels Jan 23, 2020
zadjii-msft added a commit that referenced this issue Feb 28, 2020
  * When the keybindings are missing required args (#3522)
  * When more than one "keys" aws specified (#4239)
@ghost ghost added the In-PR This issue has a related PR label Feb 28, 2020
@ghost ghost closed this as completed in #4746 Mar 5, 2020
@ghost ghost removed the In-PR This issue has a related PR label Mar 5, 2020
ghost pushed a commit that referenced this issue Mar 5, 2020
## Summary of the Pull Request

Adds warning messages for a pair of keybindings-related scenarios. This covers the following two bugs:
* #4239 - If the user has supplied more than one key chord in their `"keys"` array.
* #3522 - If a keybinding has a _required_ argument, then we'll display a message to the user
  - currently, the only required parameter is the `direction` parameter for both `resizePane` and `moveFocus`

## References

When we get to #1334, we'll want to remove the `TooManyKeysForChord` warning.

## PR Checklist
* [x] Closes #4239
* [x] Closes #3522
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

![image](https://user-images.githubusercontent.com/18356694/75593132-f18ec700-5a49-11ea-9d26-6acd0d28b0b7.png)


## Validation Steps Performed
Tested manually, added tests.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Mar 5, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants