Polish code for actions page (#10173)

Applies feedback from #9949 (review)

Highlights include:
- bugfix: make all edit buttons stay visible if the user is using assistive technology
- rename a few functions and resources to match the correct naming scheme
- update the localized text for a conflicting key chord being assigned
- provide better comments throughout the actions page code

## References
#9949 - Original PR
Closes #10168

src/cascadia/TerminalSettingsEditor/Actions.cpp

@@ -103,6 +103,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
     Automation::Peers::AutomationPeer Actions::OnCreateAutomationPeer()
     {
+        _AutomationPeerAttached = true;
         for (const auto& kbdVM : _KeyBindingList)
         {
             // To create a more accessible experience, we want the "edit" buttons to _always_
@@ -135,7 +136,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         _KeyBindingList = single_threaded_observable_vector(std::move(keyBindingList));
     }
 
-    void Actions::KeyChordEditor_PreviewKeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e)
+    void Actions::KeyChordEditor_KeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e)
     {
         const auto& senderTB{ sender.as<TextBox>() };
         const auto& kbdVM{ senderTB.DataContext().as<Editor::KeyBindingViewModel>() };
@@ -192,35 +193,36 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
                     }
                 }
 
-                const auto& containerBackground{ Resources().Lookup(box_value(L"EditModeContainerBackground")).as<Windows::UI::Xaml::Media::Brush>() };
+                const auto& containerBackground{ Resources().Lookup(box_value(L"ActionContainerBackgroundEditing")).as<Windows::UI::Xaml::Media::Brush>() };
                 get_self<KeyBindingViewModel>(senderVM)->ContainerBackground(containerBackground);
             }
             else
             {
                 // Focus on the list view item
                 KeyBindingsListView().ContainerFromItem(senderVM).as<Controls::Control>().Focus(FocusState::Programmatic);
 
-                const auto& containerBackground{ Resources().Lookup(box_value(L"NonEditModeContainerBackground")).as<Windows::UI::Xaml::Media::Brush>() };
+                const auto& containerBackground{ Resources().Lookup(box_value(L"ActionContainerBackground")).as<Windows::UI::Xaml::Media::Brush>() };
                 get_self<KeyBindingViewModel>(senderVM)->ContainerBackground(containerBackground);
             }
         }
     }
 
-    void Actions::_ViewModelDeleteKeyBindingHandler(const Editor::KeyBindingViewModel& /*senderVM*/, const Control::KeyChord& keys)
+    void Actions::_ViewModelDeleteKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const Control::KeyChord& keys)
     {
         // Update the settings model
         _State.Settings().ActionMap().DeleteKeyBinding(keys);
 
         // Find the current container in our list and remove it.
         // This is much faster than rebuilding the entire ActionMap.
-        if (const auto index{ _GetContainerIndexByKeyChord(keys) })
+        uint32_t index;
+        if (_KeyBindingList.IndexOf(senderVM, index))
         {
-            _KeyBindingList.RemoveAt(*index);
+            _KeyBindingList.RemoveAt(index);
 
             // Focus the new item at this index
             if (_KeyBindingList.Size() != 0)
             {
-                const auto newFocusedIndex{ std::clamp(*index, 0u, _KeyBindingList.Size() - 1) };
+                const auto newFocusedIndex{ std::clamp(index, 0u, _KeyBindingList.Size() - 1) };
                 KeyBindingsListView().ContainerFromIndex(newFocusedIndex).as<Controls::Control>().Focus(FocusState::Programmatic);
             }
         }

src/cascadia/TerminalSettingsEditor/Actions.h

@@ -48,10 +48,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
         void EnterHoverMode() { IsHovered(true); };
         void ExitHoverMode() { IsHovered(false); };
-        void FocusContainer() { IsContainerFocused(true); };
-        void UnfocusContainer() { IsContainerFocused(false); };
-        void FocusEditButton() { IsEditButtonFocused(true); };
-        void UnfocusEditButton() { IsEditButtonFocused(false); };
+        void ActionGotFocus() { IsContainerFocused(true); };
+        void ActionLostFocus() { IsContainerFocused(false); };
+        void EditButtonGettingFocus() { IsEditButtonFocused(true); };
+        void EditButtonLosingFocus() { IsEditButtonFocused(false); };
         bool ShowEditButton() const noexcept;
         void ToggleEditMode();
         void DisableEditMode() { IsInEditMode(false); }
@@ -92,7 +92,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_PreviewKeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);
+        void KeyChordEditor_KeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e);
 
         WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
         WINRT_PROPERTY(Editor::ActionsPageNavigationState, State, nullptr);

src/cascadia/TerminalSettingsEditor/Actions.idl

@@ -30,10 +30,10 @@ namespace Microsoft.Terminal.Settings.Editor
 
         void EnterHoverMode();
         void ExitHoverMode();
-        void FocusContainer();
-        void UnfocusContainer();
-        void FocusEditButton();
-        void UnfocusEditButton();
+        void ActionGotFocus();
+        void ActionLostFocus();
+        void EditButtonGettingFocus();
+        void EditButtonLosingFocus();
         void ToggleEditMode();
         void AttemptAcceptChanges();
         void DeleteKeyBinding();

src/cascadia/TerminalSettingsEditor/Actions.xaml

@@ -9,7 +9,6 @@
       xmlns:local="using:Microsoft.Terminal.Settings.Editor"
       xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
       xmlns:model="using:Microsoft.Terminal.Settings.Model"
-      xmlns:muxc="using:Microsoft.UI.Xaml.Controls"
       mc:Ignorable="d">
 
     <Page.Resources>
@@ -164,9 +163,9 @@
             <!--  Converters & Misc.  -->
             <model:IconPathConverter x:Key="IconSourceConverter" />
             <local:InvertedBooleanToVisibilityConverter x:Key="InvertedBooleanToVisibilityConverter" />
-            <SolidColorBrush x:Key="EditModeContainerBackground"
+            <SolidColorBrush x:Key="ActionContainerBackgroundEditing"
                              Color="{ThemeResource SystemListMediumColor}" />
-            <SolidColorBrush x:Key="NonEditModeContainerBackground"
+            <SolidColorBrush x:Key="ActionContainerBackground"
                              Color="Transparent" />
 
             <!--  Templates  -->
@@ -175,8 +174,8 @@
                 <ListViewItem AutomationProperties.AcceleratorKey="{x:Bind KeyChordText, Mode=OneWay}"
                               AutomationProperties.Name="{x:Bind Name, Mode=OneWay}"
                               Background="{x:Bind ContainerBackground, Mode=OneWay}"
-                              GotFocus="{x:Bind FocusContainer}"
-                              LostFocus="{x:Bind UnfocusContainer}"
+                              GotFocus="{x:Bind ActionGotFocus}"
+                              LostFocus="{x:Bind ActionLostFocus}"
                               PointerEntered="{x:Bind EnterHoverMode}"
                               PointerExited="{x:Bind ExitHoverMode}"
                               Style="{StaticResource KeyBindingContainerStyle}">
@@ -218,7 +217,7 @@
                         <!--  Edit Mode: Key Chord Text Box  -->
                         <TextBox Grid.Column="1"
                                  DataContext="{x:Bind Mode=OneWay}"
-                                 PreviewKeyDown="KeyChordEditor_PreviewKeyDown"
+                                 KeyDown="KeyChordEditor_KeyDown"
                                  Style="{StaticResource KeyChordEditorStyle}"
                                  Text="{x:Bind ProposedKeys, Mode=TwoWay}"
                                  Visibility="{x:Bind IsInEditMode, Mode=OneWay}" />
@@ -229,8 +228,8 @@
                                 AutomationProperties.Name="{x:Bind EditButtonName}"
                                 Background="Transparent"
                                 Click="{x:Bind ToggleEditMode}"
-                                GettingFocus="{x:Bind FocusEditButton}"
-                                LosingFocus="{x:Bind UnfocusEditButton}"
+                                GettingFocus="{x:Bind EditButtonGettingFocus}"
+                                LosingFocus="{x:Bind EditButtonLosingFocus}"
                                 Style="{StaticResource EditButtonStyle}"
                                 Visibility="{x:Bind ShowEditButton, Mode=OneWay}">
                             <Button.Content>

src/cascadia/TerminalSettingsEditor/CommonResources.xaml

@@ -49,7 +49,7 @@
     <Style x:Key="SettingsStackStyle"
            TargetType="StackPanel">
         <Setter Property="HorizontalAlignment" Value="Left" />
-        <Setter Property="Margin" Value="13,0,13,48" />
+        <Setter Property="Margin" Value="13,0,0,48" />
     </Style>
 
     <!--  Used to stack a group of settings inside a pivot  -->

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

@@ -1119,8 +1119,8 @@
     <comment>Error message displayed when a key chord that is already in use is input by the user. The name of the conflicting key chord is displayed after this message.</comment>
   </data>
   <data name="Actions_RenameConflictConfirmationQuestion" xml:space="preserve">
-    <value>Would you like to overwrite that key binding?</value>
-    <comment>Confirmation message displayed when a key chord that is already in use is input by the user. This is intended to ask the user if they wish to delete the conflicting key binding, and assign the current key chord (or binding) instead.</comment>
+    <value>Would you like to overwrite it?</value>
+    <comment>Confirmation message displayed when a key chord that is already in use is input by the user. This is intended to ask the user if they wish to delete the conflicting key binding, and assign the current key chord (or binding) instead. This is presented in the context of Actions_RenameConflictConfirmationMessage. The subject of this sentence is the object of that one.</comment>
   </data>
   <data name="Actions_UnnamedCommandName" xml:space="preserve">
     <value>&lt;unnamed command&gt;</value>

src/cascadia/TerminalSettingsEditor/ViewModelHelpers.h

@@ -74,7 +74,8 @@ public:                                                   \
     _BASE_OBSERVABLE_PROJECTED_SETTING(target, name)
 
 // Defines a basic observable property that uses the _NotifyChanges
-// system from ViewModelHelper.
+// system from ViewModelHelper. This is very similar to WINRT_OBSERVABLE_PROPERTY
+// except it leverages _NotifyChanges.
 #define VIEW_MODEL_OBSERVABLE_PROPERTY(type, name, ...) \
 public:                                                 \
     type name() const noexcept { return _##name; };     \

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -234,6 +234,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         // Update KeyBindingsMap with our current layer
         for (const auto& [keys, actionID] : _KeyMap)
         {
+            // Get the action our KeyMap maps to.
+            // This _cannot_ be nullopt because KeyMap can only map to
+            //   actions in this layer.
+            // This _can_ be nullptr because nullptr means it was
+            //   explicitly unbound ( "command": "unbound", "keys": "ctrl+c" ).
             const auto cmd{ _GetActionByID(actionID).value() };
             if (cmd)
             {
@@ -258,7 +263,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             }
         }
 
-        // Update KeyBindingsMap and visitedKeyChords with our parents
+        // Update keyBindingsMap and unboundKeys with our parents
         FAIL_FAST_IF(_parents.size() > 1);
         for (const auto& parent : _parents)
         {