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

## 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

src/cascadia/TerminalSettingsEditor/Actions.cpp

@@ -21,6 +21,9 @@ using namespace winrt::Microsoft::Terminal::Settings::Model;
 
 namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 {
+    KeyBindingViewModel::KeyBindingViewModel(const Windows::Foundation::Collections::IObservableVector<hstring>& availableActions) :
+        KeyBindingViewModel(nullptr, availableActions.First().Current(), availableActions) {}
+
     KeyBindingViewModel::KeyBindingViewModel(const Control::KeyChord& keys, const hstring& actionName, const IObservableVector<hstring>& availableActions) :
         _Keys{ keys },
         _KeyChordText{ Model::KeyChordSerialization::ToString(keys) },
@@ -84,18 +87,25 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
     void KeyBindingViewModel::AttemptAcceptChanges(hstring newKeyChordText)
     {
-        auto args{ make_self<ModifyKeyBindingEventArgs>(_Keys, _Keys, _CurrentAction, unbox_value<hstring>(_ProposedAction)) };
-
-        // Key Chord Text
         try
         {
-            // Attempt to convert the provided key chord text
-            const auto newKeyChord{ KeyChordSerialization::FromString(newKeyChordText) };
-            args->NewKeys(newKeyChord);
+            // empty string --> don't accept changes
+            if (newKeyChordText.empty())
+            {
+                return;
+            }
+
+            // ModifyKeyBindingEventArgs
+            const auto args{ make_self<ModifyKeyBindingEventArgs>(_Keys, // OldKeys
+                                                                  KeyChordSerialization::FromString(newKeyChordText), // NewKeys: Attempt to convert the provided key chord text
+                                                                  _IsNewlyAdded ? hstring{} : _CurrentAction, // OldAction
+                                                                  unbox_value<hstring>(_ProposedAction)) }; //
+            _ModifyKeyBindingRequestedHandlers(*this, *args);
         }
         catch (hresult_invalid_argument)
         {
-            // Converting the text into a key chord failed
+            // Converting the text into a key chord failed.
+            // Don't accept the changes.
             // TODO GH #6900:
             //  This is tricky. I still haven't found a way to reference the
             //  key chord text box. It's hidden behind the data template.
@@ -104,13 +114,25 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
             //  Alternatively, we want a full key chord editor/listener.
             //  If we implement that, we won't need this validation or error message.
         }
+    }
 
-        _ModifyKeyBindingRequestedHandlers(*this, *args);
+    void KeyBindingViewModel::CancelChanges()
+    {
+        if (_IsNewlyAdded)
+        {
+            _DeleteNewlyAddedKeyBindingHandlers(*this, nullptr);
+        }
+        else
+        {
+            ToggleEditMode();
+        }
     }
 
     Actions::Actions()
     {
         InitializeComponent();
+
+        Automation::AutomationProperties::SetName(AddNewButton(), RS_(L"Actions_AddNewTextBlock/Text"));
     }
 
     Automation::Peers::AutomationPeer Actions::OnCreateAutomationPeer()
@@ -149,10 +171,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         {
             // convert the cmd into a KeyBindingViewModel
             auto container{ make_self<KeyBindingViewModel>(keys, cmd.Name(), _AvailableActionAndArgs) };
-            container->PropertyChanged({ this, &Actions::_ViewModelPropertyChangedHandler });
-            container->DeleteKeyBindingRequested({ this, &Actions::_ViewModelDeleteKeyBindingHandler });
-            container->ModifyKeyBindingRequested({ this, &Actions::_ViewModelModifyKeyBindingHandler });
-            container->IsAutomationPeerAttached(_AutomationPeerAttached);
+            _RegisterEvents(container);
             keyBindingList.push_back(*container);
         }
 
@@ -183,11 +202,30 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         }
         else if (e.OriginalKey() == VirtualKey::Escape)
         {
-            kbdVM.ToggleEditMode();
+            kbdVM.CancelChanges();
             e.Handled(true);
         }
     }
 
+    void Actions::AddNew_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*eventArgs*/)
+    {
+        // Create the new key binding and register all of the event handlers.
+        auto kbdVM{ make_self<KeyBindingViewModel>(_AvailableActionAndArgs) };
+        _RegisterEvents(kbdVM);
+        kbdVM->DeleteNewlyAddedKeyBinding({ this, &Actions::_ViewModelDeleteNewlyAddedKeyBindingHandler });
+
+        // Manually add the editing background. This needs to be done in Actions not the view model.
+        // We also have to do this manually because it hasn't been added to the list yet.
+        kbdVM->IsInEditMode(true);
+        const auto& containerBackground{ Resources().Lookup(box_value(L"ActionContainerBackgroundEditing")).as<Windows::UI::Xaml::Media::Brush>() };
+        kbdVM->ContainerBackground(containerBackground);
+
+        // IMPORTANT: do this _after_ setting IsInEditMode. Otherwise, it'll get deleted immediately
+        //              by the PropertyChangedHandler below (where we delete any IsNewlyAdded items)
+        kbdVM->IsNewlyAdded(true);
+        _KeyBindingList.InsertAt(0, *kbdVM);
+    }
+
     void Actions::_ViewModelPropertyChangedHandler(const IInspectable& sender, const Windows::UI::Xaml::Data::PropertyChangedEventArgs& args)
     {
         const auto senderVM{ sender.as<Editor::KeyBindingViewModel>() };
@@ -198,8 +236,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
             {
                 // Ensure that...
                 // 1. we move focus to the edit mode controls
-                // 2. this is the only entry that is in edit mode
-                for (uint32_t i = 0; i < _KeyBindingList.Size(); ++i)
+                // 2. any actions that were newly added are removed
+                // 3. this is the only entry that is in edit mode
+                for (int32_t i = _KeyBindingList.Size() - 1; i >= 0; --i)
                 {
                     const auto& kbdVM{ _KeyBindingList.GetAt(i) };
                     if (senderVM == kbdVM)
@@ -210,6 +249,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
                         const auto& container{ KeyBindingsListView().ContainerFromIndex(i).try_as<ListViewItem>() };
                         container.Focus(FocusState::Programmatic);
                     }
+                    else if (kbdVM.IsNewlyAdded())
+                    {
+                        // Remove any actions that were newly added
+                        _KeyBindingList.RemoveAt(i);
+                    }
                     else
                     {
                         // Exit edit mode for all other containers
@@ -254,13 +298,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
     void Actions::_ViewModelModifyKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::ModifyKeyBindingEventArgs& args)
     {
+        const auto isNewAction{ !args.OldKeys() && args.OldActionName().empty() };
+
         auto applyChangesToSettingsModel = [=]() {
             // If the key chord was changed,
             // update the settings model and view model appropriately
-            if (args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey())
+            // NOTE: we still need to update the view model if we're working with a newly added action
+            if (isNewAction || args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey())
             {
-                // update settings model
-                _State.Settings().ActionMap().RebindKeys(args.OldKeys(), args.NewKeys());
+                if (!isNewAction)
+                {
+                    // update settings model
+                    _State.Settings().ActionMap().RebindKeys(args.OldKeys(), args.NewKeys());
+                }
 
                 // update view model
                 auto senderVMImpl{ get_self<KeyBindingViewModel>(senderVM) };
@@ -269,6 +319,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
             // If the action was changed,
             // update the settings model and view model appropriately
+            // NOTE: no need to check for "isNewAction" here. <empty_string> != <action name> already.
             if (args.OldActionName() != args.NewActionName())
             {
                 // convert the action's name into a view model.
@@ -280,13 +331,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
                 // update view model
                 auto senderVMImpl{ get_self<KeyBindingViewModel>(senderVM) };
                 senderVMImpl->CurrentAction(args.NewActionName());
+                senderVMImpl->IsNewlyAdded(false);
             }
         };
 
         // 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())
+        if (isNewAction || args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey())
         {
             const auto& conflictingCmd{ _State.Settings().ActionMap().GetActionByKeyChord(args.NewKeys()) };
             if (conflictingCmd)
@@ -342,6 +394,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         senderVM.ToggleEditMode();
     }
 
+    void Actions::_ViewModelDeleteNewlyAddedKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const IInspectable& /*args*/)
+    {
+        for (uint32_t i = 0; i < _KeyBindingList.Size(); ++i)
+        {
+            const auto& kbdVM{ _KeyBindingList.GetAt(i) };
+            if (kbdVM == senderVM)
+            {
+                _KeyBindingList.RemoveAt(i);
+                return;
+            }
+        }
+    }
+
     // Method Description:
     // - performs a search on KeyBindingList by key chord.
     // Arguments:
@@ -365,4 +430,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         //  to quickly search through the sorted list.
         return std::nullopt;
     }
+
+    void Actions::_RegisterEvents(com_ptr<KeyBindingViewModel>& kbdVM)
+    {
+        kbdVM->PropertyChanged({ this, &Actions::_ViewModelPropertyChangedHandler });
+        kbdVM->DeleteKeyBindingRequested({ this, &Actions::_ViewModelDeleteKeyBindingHandler });
+        kbdVM->ModifyKeyBindingRequested({ this, &Actions::_ViewModelModifyKeyBindingHandler });
+        kbdVM->IsAutomationPeerAttached(_AutomationPeerAttached);
+    }
 }

src/cascadia/TerminalSettingsEditor/Actions.h

@@ -38,6 +38,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
     struct KeyBindingViewModel : KeyBindingViewModelT<KeyBindingViewModel>, ViewModelHelper<KeyBindingViewModel>
     {
     public:
+        KeyBindingViewModel(const Windows::Foundation::Collections::IObservableVector<hstring>& availableActions);
         KeyBindingViewModel(const Control::KeyChord& keys, const hstring& name, const Windows::Foundation::Collections::IObservableVector<hstring>& availableActions);
 
         hstring Name() const { return _CurrentAction; }
@@ -60,6 +61,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         void DisableEditMode() { IsInEditMode(false); }
         void AttemptAcceptChanges();
         void AttemptAcceptChanges(hstring newKeyChordText);
+        void CancelChanges();
         void DeleteKeyBinding() { _DeleteKeyBindingRequestedHandlers(*this, _Keys); }
 
         // ProposedAction:   the entry selected by the combo box; may disagree with the settings model.
@@ -81,6 +83,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         VIEW_MODEL_OBSERVABLE_PROPERTY(Control::KeyChord, Keys, nullptr);
 
         VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsInEditMode, false);
+        VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsNewlyAdded, false);
         VIEW_MODEL_OBSERVABLE_PROPERTY(Windows::UI::Xaml::Controls::Flyout, AcceptChangesFlyout, nullptr);
         VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsAutomationPeerAttached, false);
         VIEW_MODEL_OBSERVABLE_PROPERTY(bool, IsHovered, false);
@@ -89,6 +92,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         VIEW_MODEL_OBSERVABLE_PROPERTY(Windows::UI::Xaml::Media::Brush, ContainerBackground, nullptr);
         TYPED_EVENT(ModifyKeyBindingRequested, Editor::KeyBindingViewModel, Editor::ModifyKeyBindingEventArgs);
         TYPED_EVENT(DeleteKeyBindingRequested, Editor::KeyBindingViewModel, Terminal::Control::KeyChord);
+        TYPED_EVENT(DeleteNewlyAddedKeyBinding, Editor::KeyBindingViewModel, IInspectable);
 
     private:
         hstring _KeyChordText{};
@@ -111,6 +115,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);
         Windows::UI::Xaml::Automation::Peers::AutomationPeer OnCreateAutomationPeer();
         void KeyChordEditor_KeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);
+        void AddNew_Click(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
 
         WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
         WINRT_PROPERTY(Editor::ActionsPageNavigationState, State, nullptr);
@@ -120,8 +125,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         void _ViewModelPropertyChangedHandler(const Windows::Foundation::IInspectable& senderVM, const Windows::UI::Xaml::Data::PropertyChangedEventArgs& args);
         void _ViewModelDeleteKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Control::KeyChord& args);
         void _ViewModelModifyKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Editor::ModifyKeyBindingEventArgs& args);
+        void _ViewModelDeleteNewlyAddedKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const IInspectable& args);
 
         std::optional<uint32_t> _GetContainerIndexByKeyChord(const Control::KeyChord& keys);
+        void _RegisterEvents(com_ptr<implementation::KeyBindingViewModel>& kbdVM);
 
         bool _AutomationPeerAttached{ false };
         Windows::Foundation::Collections::IObservableVector<hstring> _AvailableActionAndArgs;

src/cascadia/TerminalSettingsEditor/Actions.idl

@@ -22,6 +22,7 @@ namespace Microsoft.Terminal.Settings.Editor
         // UI side
         Boolean ShowEditButton { get; };
         Boolean IsInEditMode { get; };
+        Boolean IsNewlyAdded { get; };
         String ProposedKeys;
         Object ProposedAction;
         Windows.UI.Xaml.Controls.Flyout AcceptChangesFlyout;
@@ -40,6 +41,7 @@ namespace Microsoft.Terminal.Settings.Editor
         IObservableVector<String> AvailableActions { get; };
         void ToggleEditMode();
         void AttemptAcceptChanges();
+        void CancelChanges();
         void DeleteKeyBinding();
 
         event Windows.Foundation.TypedEventHandler<KeyBindingViewModel, ModifyKeyBindingEventArgs> ModifyKeyBindingRequested;

src/cascadia/TerminalSettingsEditor/Actions.xaml

@@ -286,7 +286,7 @@
                             <!--  Cancel editing the action  -->
                             <Button x:Uid="Actions_CancelButton"
                                     AutomationProperties.Name="{x:Bind CancelButtonName}"
-                                    Click="{x:Bind ToggleEditMode}"
+                                    Click="{x:Bind CancelChanges}"
                                     Style="{StaticResource EditButtonStyle}">
                                 <FontIcon FontSize="{StaticResource EditButtonIconSize}"
                                           Glyph="&#xE711;" />
@@ -307,7 +307,8 @@
                             <Button x:Uid="Actions_DeleteButton"
                                     Margin="8,0,0,0"
                                     AutomationProperties.Name="{x:Bind DeleteButtonName}"
-                                    Style="{StaticResource EditButtonStyle}">
+                                    Style="{StaticResource EditButtonStyle}"
+                                    Visibility="{x:Bind IsNewlyAdded, Mode=OneWay, Converter={StaticResource InvertedBooleanToVisibilityConverter}}">
                                 <Button.Content>
                                     <FontIcon FontSize="{StaticResource EditButtonIconSize}"
                                               Glyph="&#xE74D;" />
@@ -381,7 +382,21 @@
 
     <ScrollViewer>
         <StackPanel MaxWidth="600"
+                    Spacing="8"
                     Style="{StaticResource SettingsStackStyle}">
+            <!--  Add New Button  -->
+            <Button x:Name="AddNewButton"
+                    Click="AddNew_Click">
+                <Button.Content>
+                    <StackPanel Orientation="Horizontal">
+                        <FontIcon FontSize="{StaticResource StandardIconSize}"
+                                  Glyph="&#xE710;" />
+                        <TextBlock x:Uid="Actions_AddNewTextBlock"
+                                   Style="{StaticResource IconButtonTextBlockStyle}" />
+                    </StackPanel>
+                </Button.Content>
+            </Button>
+
             <!--  Keybindings  -->
             <ListView x:Name="KeyBindingsListView"
                       ItemTemplate="{StaticResource KeyBindingTemplate}"

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

@@ -1154,4 +1154,8 @@
     <value>Edit</value>
     <comment>Text label for a button that can be used to begin making changes to a key binding entry.</comment>
   </data>
+  <data name="Actions_AddNewTextBlock.Text" xml:space="preserve">
+    <value>Add new</value>
+    <comment>Button label that creates a new action on the actions page.</comment>
+  </data>
 </root>