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.

Sign up for GitHub

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

Jump to bottom

Feature Request: Support arbitrary arguments for keybindings #1142

Closed
zadjii-msft opened this issue Jun 4, 2019 · 1 comment · Fixed by #3391
Closed

Feature Request: Support arbitrary arguments for keybindings #1142

zadjii-msft opened this issue Jun 4, 2019 · 1 comment · Fixed by #3391
Assignees
@zadjii-msft
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
Specification Tracker
Milestone
Terminal-1911

Comments

@zadjii-msft
Copy link
Member

From discussion with @carlos-zamora and @DHowett-MSFT, and possibly in #968

Keybindings need to support arbitrary args, so that the handlers can parse their own arg values.

{
    keys: ["ctrl+shift+c"],
    action: copy,
    args:{
        type: "mungeData"
    }
},
{
    keys: ["ctrl+shift+pgup"],
    action: scroll,
    args:{
        amount: -1
    }
}

This is a feature that needs to absolutely be spec'd before implemented.
@zadjii-msft zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Jun 4, 2019
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Jun 4, 2019
@zadjii-msft zadjii-msft added this to Spec Needed ❓ in Specification Tracker via automation Jun 4, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 4, 2019
@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 Jun 10, 2019
@zadjii-msft zadjii-msft mentioned this issue Jun 19, 2019
Closed
@zadjii-msft zadjii-msft moved this from Spec Needed ❓ to Spec In Progress ✏ in Specification Tracker Jun 19, 2019
zadjii-msft added a commit that referenced this issue Jun 20, 2019
@zadjii-msft
Add a spec draft for Keybindings Arguments.
784528f 
  Specs #1142.

  Just read the spec :)
@zadjii-msft zadjii-msft mentioned this issue Jun 20, 2019
Merged
1 task
@zadjii-msft zadjii-msft moved this from Spec In Progress ✏ to Spec In Review ⏰ in Specification Tracker Jun 20, 2019
@zadjii-msft zadjii-msft mentioned this issue Jul 2, 2019
Closed
@zadjii-msft zadjii-msft mentioned this issue Jul 11, 2019
Closed
@zadjii-msft zadjii-msft mentioned this issue Jul 23, 2019
Closed
zadjii-msft added a commit that referenced this issue Aug 16, 2019
@zadjii-msft
Add a spec draft for Keybindings Arguments (#1349)
c70fb49 
* Add a spec draft for Keybindings Arguments.

  Specs #1142.

  Just read the spec :)

* Apply suggestions from code review

Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>

* Include notes on reliability, security, and `Handle`ing Keybinding Args

* Add some extra details from review

  * Split up ActionArgs and ActionEventArgs
  * Clarify _not_ handling an action
  * Add some notes on parsing args
  * Add some future considerations on extensions

* Updating spec to remove the bulk of the `IActionArgs` and `IActionEventArgs` implementations, as they're redundant.
DHowett-MSFT pushed a commit that referenced this issue Aug 16, 2019
@zadjii-msft @DHowett
Don't copy text if there's no selection (#2446)
734fc1d 
This commit also transitions our keybinding events and event handlers to a
TypedEventHandler model with an "event args" class, as specified in the
keybinding arguments specification (#1349). In short, every event can be marked
Handled independently, and a Handled event will stop bubbling out to the
terminal. An unhandled event will be passed off to the terminal as a standard
keypress.

This unifies our keybinding event model and provides a convenient place for
binding arguments to live.

Fixes #2285.
Related to #1349, #1142.
@zadjii-msft zadjii-msft moved this from Spec In Review ⏰ to Spec Accepted! 🎉 in Specification Tracker Aug 20, 2019
@zadjii-msft zadjii-msft self-assigned this Aug 22, 2019
@zadjii-msft zadjii-msft mentioned this issue Aug 26, 2019
Open
This was referenced Aug 27, 2019
Closed
Closed
Closed
Open
@carlos-zamora carlos-zamora mentioned this issue Sep 21, 2019
Merged
@zadjii-msft zadjii-msft mentioned this issue Oct 4, 2019
Open
59 tasks
@zadjii-msft zadjii-msft mentioned this issue Oct 31, 2019
Merged
4 tasks
@ghost ghost added the In-PR This issue has a related PR label Oct 31, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Nov 14, 2019
zadjii-msft added a commit that referenced this issue Nov 14, 2019
@zadjii-msft
Add support for arbitrary args in keybindings (#3391)
6a4c737 
## Summary of the Pull Request

Enables the user to provide arbitrary argument values to shortcut actions through a new `args` member of keybindings. For some keybindings, like `NewTabWithProfile<N>`, we previously needed 9 different `ShortcutAction`s, one for each value of `Index`. If a user wanted to have a `NewTabWithProfile11` keybinding, that was simply impossible. Now that the args are in their own separate json object, each binding can accept any number of arbitrary argument values.

So instead of:
```json
        { "command": "newTab", "keys": ["ctrl+shift+t"] },
        { "command": "newTabProfile0", "keys": ["ctrl+shift+1"] },
        { "command": "newTabProfile1", "keys": ["ctrl+shift+2"] },
        { "command": "newTabProfile2", "keys": ["ctrl+shift+3"] },
        { "command": "newTabProfile3", "keys": ["ctrl+shift+4"] },
```

We can now use:

```json
        { "command": "newTab", "keys": ["ctrl+shift+t"] },
        { "command": { "action": "newTab", "index": 0 }, "keys": ["ctrl+shift+1"] },
        { "command": { "action": "newTab", "index": 1 }, "keys": ["ctrl+shift+2"] },
        { "command": { "action": "newTab", "index": 2 }, "keys": ["ctrl+shift+3"] },
```

Initially, this does seem more verbose. However, for cases where there are multiple args, or there's a large range of values for the args, this will quickly become a more powerful system of expressing keybindings.

The "legacy" keybindings are _left in_ in this PR. They have helper methods to generate appropriate `IActionArgs` values. Prior to releasing 1.0, I think we should remove them, if only to remove some code bloat.

## References

See [the spec](https://github.com/microsoft/terminal/blob/master/doc/specs/%231142%20-%20Keybinding%20Arguments.md) for more details.

This is part two of the implementation, part one was #2446

## PR Checklist
* [x] Closes #1142
* [x] I work here
* [x] Tests added/passed
* [x] Schema updated

## Validation Steps Performed

* Ran Tests
* Removed the legacy keybindings from the `defaults.json`, everything still works
* Tried leaving the legacy keybingings in my `profiles.json`, everything still works.

-------------------------------------------------
* this is a start, but there's a weird linker bug if I take the SetKeybinding(ShortcutAction, KeyChord) implementation out, which I don't totally understand

* a good old-fashioned clean will fix that right up

* all these things work

* hey this actually _functionally_ works

* Mostly cleanup and completion of implementation

* Hey I bet we could just make NewTab the handler for NewTabWithProfile

* Start writing tests for Keybinding args

* Add tests

* Revert a bad sln change, and clean out dead code

* Change to include "command" as a single object

  This is a change to make @DHowett-MSFT happy. Changes the args to be a part
  of the "command" object, as opposed to an object on their own.

  EX:

  ```jsonc

    // Old style
    { "command": "switchToTab0", "keys": ["ctrl+1"] },
    { "command": { "action": "switchToTab", "index": 0 }, "keys": ["ctrl+alt+1"] },

    // new style
    { "command": "switchToTab0", "keys": ["ctrl+1"] },
    { "command": "switchToTab", "args": { "index": 0 } "keys": ["ctrl+alt+1"] },

  ```

* schemas are hard yo

* Fix the build?

* wonder why my -Wall settings are different than CI...

* this makes me hate things

* Comments from PR

  * Add a `Direction::None`
  * LOAD BEARING
  * add some GH ids to TODOs

* add a comment

* PR nits from carlos
@ghost
Copy link

ghost commented Nov 26, 2019

🎉This issue was addressed in #3391, which has now been successfully released as Windows Terminal Preview v0.7.3291.0.:tada:

Handy links:

@zadjii-msft zadjii-msft mentioned this issue Apr 9, 2020
Closed
@github-actions github-actions bot mentioned this issue Mar 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zadjii-msft zadjii-msft

Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
Specification Tracker
  
Spec Accepted! 🎉
Milestone
Terminal-1911
Development

Successfully merging a pull request may close this issue.

Add support for arbitrary args in keybindings microsoft/terminal
2 participants
@DHowett-MSFT @zadjii-msft