Add a spec draft for Keybindings Arguments#1349
Merged
zadjii-msft merged 6 commits intomasterfrom Aug 16, 2019
Merged
Conversation
Specs #1142. Just read the spec :)
zadjii-msft
added
Issue-Docs
miniksa
reviewed
Jun 24, 2019
miniksa
reviewed
Jun 24, 2019
miniksa
reviewed
Jun 24, 2019
miniksa
reviewed
Jun 24, 2019
| binding automatically recieves the number pressed as an arg? I couldn't find | ||
| any prior art of this, so it doesn't seem worth it to try and invent | ||
| currently. This might be something that we want to loop back on, but for the | ||
| time being, it remains out of scope of this PR. |
Member
There was a problem hiding this comment.
Link future idea to open issue?
miniksa
reviewed
Jun 24, 2019
Member
miniksa
left a comment
miniksa
left a comment
There was a problem hiding this comment.
Good spec draft. This sets a great example. I didn't have any major issues.
Co-Authored-By: Carlos Zamora <[email protected]>
5 tasks
3 tasks
Member
|
Might not need to be on the spec but don't forget about Dustin's Design Notes here regarding |
DHowett-MSFT
suggested changes
Aug 9, 2019
| ``` | ||
|
|
||
| Note that instead of having 9 different `newTabProfile<N>` actions, we have a | ||
| singular `newTabProfile` action, and that action requires a `profileIndex` in |
Contributor
There was a problem hiding this comment.
is this informative or normative? That is: does the spec specify that newTabProfile MUST have an index or that it MAY have an index? What about a GUID?
Member
Author
There was a problem hiding this comment.
Huh. It makes sense that it could alternatively have a GUID. But then how would we handle if both existed in the args? I guess we can't really have an optional member, now can we?
ghost
added
Needs-Author-Feedback
* Split up ActionArgs and ActionEventArgs * Clarify _not_ handling an action * Add some notes on parsing args * Add some future considerations on extensions
DHowett-MSFT
suggested changes
Aug 13, 2019
ghost
added
Needs-Author-Feedback
…ntArgs` implementations, as they're redundant.
DHowett-MSFT
approved these changes
Aug 14, 2019
3 tasks
carlos-zamora
approved these changes
Aug 16, 2019
miniksa
approved these changes
Aug 16, 2019
DHowett-MSFT
pushed a commit
that referenced
this pull request
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 26, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
The goal of this change is to both simplify the keybindings, and also enable far
more flexibility when editing a user's keybindings.
Currently, we have many actions that are very similar in implementation - for
example,
newTabProfile0,newTabProfile1,newTabProfile2, etc. All theseactions are fundamentally the same function. However, we've needed to define 9
different actions to enable the user to provide different values to the
newTabfunction.
With this change, we'll be able to remove these essentially duplicated events,
and allow the user to specify arbitrary arguments to these functions.
References
Might play into #968, since there's some discussion of to munge or not to munge with the default keybinding for copy. This would make it easy to add both.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Just read the spec :)
Validation Steps Performed
It's a spec.