-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Feature Request: Support arbitrary arguments for keybindings #1142
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.
Milestone
Comments
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
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
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
added a commit
that referenced
this issue
Jun 20, 2019
Specs #1142. Just read the spec :)
1 task
zadjii-msft
added a commit
that referenced
this issue
Aug 16, 2019
* 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
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.
This was referenced Aug 27, 2019
4 tasks
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
## 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
🎉This issue was addressed in #3391, which has now been successfully released as Handy links: |
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-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.
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.
This is a feature that needs to absolutely be spec'd before implemented.
The text was updated successfully, but these errors were encountered: