Allow setting the action on Actions page #10220

carlos-zamora · 2021-05-26T19:28:26Z

Summary of the Pull Request

This introduces the ability to set the action for a key binding. A combo box is used to let the user select a new action.

References

#6900 - Actions page Epic
#9427 - Actions page design doc
#9949 - Actions page PR

Detailed Description of the Pull Request / Additional comments

Settings Model Changes

ActionAndArgs
- new ctor that just takes a ShortcutAction
ActionMap
- AvailableActions provides a map of all the "acceptable" actions to choose from. This is a merged list of (1) all { "command": X } style actions and (2) any actions with args that are already defined in the ActionMap (or any parents).
- RegisterKeyBinding introduces a new unnamed key binding to the action map.

Editor Changes

XAML
- Pretty straightforward, when in edit mode, we replace the text block with a combo box. This combo box just presents the actions you can choose from.
RebindKeysEventArgs --> ModifyKeyBindingEventArgs
AvailableActionAndArgs
- stores the list of actions to choose from in the combo box
- Unfortunately, KeyBindingViewModel needs this so that we can populate the combo box
- Actions stores and maintains this though. We populate this from the settings model on navigation.
ProposedAction vs CurrentAction
- similar to ProposedKeys and Keys, we need a way to distinguish the value from the settings model and the value of the control (i.e. combo box).
- CurrentAction --> settings model
- ProposedAction --> combo box selected item

Validation Steps Performed

Cancel:
- ✔️ change action --> cancel button --> begin editing action again --> original action is selected
Accept:
- ✔️ don't change anything
- ✔️ change action --> OK! --> Save!
  - NOTE: The original action is still left as a stub { "command": "closePane" }. This is intentional because we want to prevent all modifications to the command palette.
- ✔️ change action & change key chord --> OK! --> Save!
- ✔️ change action & change key chord (conflicting key chord) --> OK! --> click ok on flyout --> Save!
  - NOTE: original action is left as a stub; original key chord explicitly unbound; new command/keys combo added.

src/cascadia/TerminalSettingsEditor/Actions.cpp

leonMSFT

Other than trying out the ComboBox.Text property instead of SelectedItem,

PankajBhojwani · 2021-06-28T17:56:34Z

src/cascadia/TerminalSettingsEditor/Actions.cpp

+
+        // Check for this special case:
+        //  we're changing the key chord,
+        //  but the new key chord is already in use
        if (args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey())


Question: do we also want to display a warning for something like "this action already has a keybinding, are you sure you want to add another?"

I know having multiple keybindings for an action is totally fine and not really 'conflicting', but with the long list of actions we currently show it might be easy for a user to miss that they've already set a keybinding for an action but forgot about it

Totally okay with having this be a follow-up or a "wait until someone asks for it before we implement it" kinda thing, just wanted to know if it's been discussed already

PankajBhojwani

Exciting!

src/cascadia/TerminalSettingsEditor/Actions.cpp

lhecker · 2021-06-28T17:52:19Z

src/cascadia/TerminalSettingsEditor/Actions.h

+        // ProposedAction:   the entry selected by the combo box; may disagree with the settings model.
+        // CurrentAction:    the combo box item that maps to the settings model value.


I don't understand this description... Why can it disagree with the settings model? And why does the combo box item map to a settings model value? The way I understand the code, CurrentAction is the actual action identifier/name.

Really it's because we have the "cancel"/"ok" system (aka the "edit mode" system). CurrentAction serves as...

a record of what to set ProposedAction to on a cancellation

a form of translation between ProposedAction and the settings model

If we got rid of the "edit mode" system, we don't have to worry about (1) anymore. To get rid of (2), KeyBindingViewModel would need a reference to the ActionMap. But even then, I think it's better to keep the ActionMap reference at a higher layer so that we can identify conflicting key bindings and key chords before editing the settings model. I think that's a better separation of responsibilities imo.

lhecker · 2021-06-28T17:57:04Z

src/cascadia/TerminalSettingsEditor/Actions.cpp

+
+        // If the action was changed,
+        // update the settings model and view model appropriately
+        auto attemptSetAction = [=]() {


attemptSetAction is always called when attemptRebindKeys is.
You can just merge the two lambdas into one.

Yup! I really only separated them out so that it's easier to read. I'm down to combine them, but I'm still worried about readability. Would something like this be clear maybe?

auto applyChangesToSettingsModel = [](){ // code for 'attemptRebindKeys' // code for 'attemptSetAction' };

src/cascadia/TerminalSettingsEditor/Actions.h

src/cascadia/TerminalSettingsModel/ActionMap.cpp

src/cascadia/TerminalSettingsEditor/Actions.h

carlos-zamora · 2021-07-02T01:38:04Z

Other than trying out the ComboBox.Text property instead of SelectedItem,

Dude, I have no idea where the ComboBox.Text property goes haha. I bound to it, and the combo box just starts off empty (as in, it isn't bound to anything). There's no text around it. Nothing! Maybe it's used when you enable IsEditable?

Edit: @leonMSFT

carlos-zamora · 2021-07-02T01:40:40Z

Since we're so close to this merging, I'm gonna add "add action" in a separate PR, not this one.

I'll leave that for tomorrow though. I hope it'll only take like half a day to implement tbh.

lhecker

I think the combined applyChangesToSettingsModel would be a good idea as it prevents you from forgetting to call both at once
You wrote an excellent description for ProposedAction/CurrentAction: This should IMO be part of the comment in the code

## Summary of the Pull Request This adds the "add new" button to the actions page. It build on the work of #10220 by basically just adding a new list item to the top of the key binding list. This also makes it so that if you click the "accept changes" button when you have an invalid key chord, we don't do anything. ## References #6900 - Actions page Epic #9427 - Actions page design doc #10220 - Actions page PR - set action ## Detailed Description of the Pull Request / Additional comments - `ModifyKeyBindingEventArgs` is used to introduce new key bindings. We just ignore `OldKeys` and `OldActionName` because both didn't exist before. - `IsNewlyAdded` tracks if this is an action that was added, but has not been confirmed to add to the settings model. - `CancelChanges()` is directly bound to the cancel button. This allows us to delete the key binding when it's clicked on a "newly added" action. ## Validation Steps Performed - Cancel: - Deletes the action (because it doesn't truly exist until you confirm changes) - Accept: - Adds the new action. - If you attempt to edit it, the delete button is back. - Add Action: - Delete button should not be visible (redundant with 'Cancel') - Action should be initialized to a value - Key chord should be empty - Cannot add another action if a newly added action exists - Keyboard interaction: - escape --> cancel - enter --> accept - Accessibility: - "add new" button has a name - Interaction with other key bindings: - editing another action --> delete the "newly added" action (it hasn't been added yet) - only one action can be edited at a time

ghost · 2021-07-14T17:08:35Z

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-10-20T17:02:43Z

🎉Windows Terminal v1.11.2921.0 has been released which incorporates this pull request.:tada:

Handy links:

gpotter2 mentioned this pull request May 27, 2021

Epic: Introduce Actions page to the Settings UI #6900

Open

12 tasks

leonMSFT reviewed May 28, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Actions.cpp Outdated Show resolved Hide resolved

carlos-zamora self-assigned this Jun 7, 2021

carlos-zamora and others added 3 commits June 7, 2021 15:02

[Actions Page] Allow changes to action and args

c3568ac

remove ActionAndArgsViewModel; rely on action names instead

53ff37c

remove unnecessary ActionAndArgs::Equals changes

1453376

carlos-zamora force-pushed the dev/cazamor/actions-page/change-action branch from afcc655 to 1453376 Compare June 7, 2021 22:23

carlos-zamora added 2 commits June 7, 2021 15:36

move ActionAndArgs::Copy back to impl type

e262055

code format

652650b

carlos-zamora removed their assignment Jun 9, 2021

leonMSFT approved these changes Jun 11, 2021

View reviewed changes

PankajBhojwani reviewed Jun 28, 2021

View reviewed changes

PankajBhojwani approved these changes Jun 28, 2021

View reviewed changes

lhecker requested changes Jun 28, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 28, 2021

carlos-zamora self-assigned this Jun 28, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 28, 2021

miniksa reviewed Jun 28, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Actions.h Show resolved Hide resolved

Merge branch 'main' into dev/cazamor/actions-page/change-action

a159fb8

apply PR feedback

9b896ce

carlos-zamora removed their assignment Jul 2, 2021

carlos-zamora assigned lhecker Jul 2, 2021

lhecker self-requested a review July 2, 2021 01:47

lhecker approved these changes Jul 2, 2021

View reviewed changes

carlos-zamora assigned carlos-zamora and unassigned lhecker Jul 2, 2021

carlos-zamora mentioned this pull request Jul 2, 2021

Add the 'Add new' button to the Actions page #10550

Merged

add comment & combine lambdas

3499a72

carlos-zamora merged commit d3b9a78 into main Jul 2, 2021

carlos-zamora deleted the dev/cazamor/actions-page/change-action branch July 2, 2021 22:35

joca-bt mentioned this pull request Sep 14, 2021

New features are unavailable #11226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting the action on Actions page #10220

Allow setting the action on Actions page #10220

carlos-zamora commented May 26, 2021 •

edited

Loading

leonMSFT left a comment

PankajBhojwani Jun 28, 2021 •

edited

Loading

PankajBhojwani Jun 28, 2021

PankajBhojwani left a comment

lhecker Jun 28, 2021

carlos-zamora Jul 2, 2021

lhecker Jun 28, 2021

carlos-zamora Jul 2, 2021

carlos-zamora commented Jul 2, 2021 •

edited

Loading

carlos-zamora commented Jul 2, 2021

lhecker left a comment

ghost commented Jul 14, 2021

ghost commented Oct 20, 2021

		// ProposedAction: the entry selected by the combo box; may disagree with the settings model.
		// CurrentAction: the combo box item that maps to the settings model value.

Allow setting the action on Actions page #10220

Allow setting the action on Actions page #10220

Conversation

carlos-zamora commented May 26, 2021 • edited Loading

Summary of the Pull Request

References

Detailed Description of the Pull Request / Additional comments

Settings Model Changes

Editor Changes

Validation Steps Performed

leonMSFT left a comment

Choose a reason for hiding this comment

PankajBhojwani Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

PankajBhojwani Jun 28, 2021

Choose a reason for hiding this comment

PankajBhojwani left a comment

Choose a reason for hiding this comment

lhecker Jun 28, 2021

Choose a reason for hiding this comment

carlos-zamora Jul 2, 2021

Choose a reason for hiding this comment

lhecker Jun 28, 2021

Choose a reason for hiding this comment

carlos-zamora Jul 2, 2021

Choose a reason for hiding this comment

carlos-zamora commented Jul 2, 2021 • edited Loading

carlos-zamora commented Jul 2, 2021

lhecker left a comment

Choose a reason for hiding this comment

ghost commented Jul 14, 2021

ghost commented Oct 20, 2021

carlos-zamora commented May 26, 2021 •

edited

Loading

PankajBhojwani Jun 28, 2021 •

edited

Loading

carlos-zamora commented Jul 2, 2021 •

edited

Loading