Repository: microsoft/terminal (language: cpp)

Issue:
The "Overwrite key binding" warning is very broken
(from bug bash 1.22)

Action Editor
🛑 "Overwrite key binding" warning is very broken

![image](https://github.com/user-attachments/assets/0cdbab01-b1d4-4963-b04e-9691b9ae2000)
_(thats right, I had to take a screenshot of the image in Loop, thanks Loop)_

I can't say "no". Dismissing it is like saying "yes". Also it scrolls horizontally.



Discussion / hints:
/cc @PankajBhojwani  since I believe you were the last one to touch Actions
I'm somewhat certain this is the way it worked before my changes as well (I see this same behaviour in Stable)
> I see this same behaviour in Stable

I do not. Dismissing the flyout does not "confirm" and overwrite the binding in Stable.
Dismissing the flyout does confirm for me in 1.21. So maybe it regressed in 20->21?
I wonder if it's something more subtle in XAML somewhere. Cause like, on 1.20, when you **hit the checkbox button**, the action stays in the "editing" state, with the textbox and the buttons. But in 1.21, the button immediately exits the editing state, then pops the "are you sure"? 
Much more likely that it was from #14292, which rewrote the action view model (and it should be the viewmodel that keeps track of states like this.)

Fix patch:
diff --git a/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp
index 1d59f7a6c7c..a8f22f0212f 100644
--- a/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp
+++ b/src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp
@@ -278,11 +278,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         // Check for this special case:
         //  we're changing the key chord,
         //  but the new key chord is already in use
+        bool conflictFound{ false };
         if (isNewAction || args.OldKeys().Modifiers() != args.NewKeys().Modifiers() || args.OldKeys().Vkey() != args.NewKeys().Vkey())
         {
             const auto& conflictingCmd{ _Settings.ActionMap().GetActionByKeyChord(args.NewKeys()) };
             if (conflictingCmd)
             {
+                conflictFound = true;
                 // We're about to overwrite another key chord.
                 // Display a confirmation dialog.
                 TextBlock errorMessageTB{};
@@ -324,13 +326,18 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
             }
         }
 
-        // update settings model and view model
-        applyChangesToSettingsModel();
+        // if there was a conflict, the flyout we created will handle whether changes need to be propagated
+        // otherwise, go ahead and apply the changes
+        if (!conflictFound)
+        {
+            // update settings model and view model
+            applyChangesToSettingsModel();
 
-        // We NEED to toggle the edit mode here,
-        // so that if nothing changed, we still exit
-        // edit mode.
-        senderVM.ToggleEditMode();
+            // We NEED to toggle the edit mode here,
+            // so that if nothing changed, we still exit
+            // edit mode.
+            senderVM.ToggleEditMode();
+        }
     }
 
     void ActionsViewModel::_KeyBindingViewModelDeleteNewlyAddedKeyBindingHandler(const Editor::KeyBindingViewModel& senderVM, const IInspectable& /*args*/)


Test patch: