No longer serialize generated IDs

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -506,7 +506,7 @@ bool SettingsLoader::FixupUserSettings()
 
     // we need to generate an ID for a command in the user settings if it doesn't already have one
     auto actionMap{ winrt::get_self<ActionMap>(userSettings.globals->ActionMap()) };
-    fixedUp = actionMap->GenerateIDsForActions() || fixedUp;
+    actionMap->GenerateIDsForActions();
 
     return fixedUp;
 }

src/cascadia/TerminalSettingsModel/Command.cpp

@@ -129,6 +129,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty())
             {
                 _ID = generatedID;
+                _IDWasGenerated = true;
                 return true;
             }
         }
@@ -445,7 +446,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             Json::Value cmdJson{ Json::ValueType::objectValue };
             JsonUtils::SetValueForKey(cmdJson, IconKey, _iconPath);
             JsonUtils::SetValueForKey(cmdJson, NameKey, _name);
-            if (!_ID.empty())
+            if (!_ID.empty() && !_IDWasGenerated)
             {
                 JsonUtils::SetValueForKey(cmdJson, IDKey, _ID);
             }

src/cascadia/TerminalSettingsModel/Command.h

@@ -88,6 +88,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         std::vector<Control::KeyChord> _keyMappings;
         std::optional<std::wstring> _name;
         std::wstring _ID;
+        bool _IDWasGenerated{ false };
         std::optional<std::wstring> _iconPath;
         bool _nestedCommand{ false };
 

src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp

@@ -972,36 +972,17 @@ namespace SettingsModelUnitTests
             ]
         })" };
 
-        // Key differences: - the sendInput action now has a generated ID
-        //                  - this generated ID was created at the time of writing this test,
-        //                    and should remain robust (i.e. every time we hash the args we should get the same result)
-        static constexpr std::string_view newSettingsJson{ R"(
-        {
-            "actions": [
-                {
-                    "name": "foo",
-                    "command": { "action": "sendInput", "input": "just some input" },
-                    "keys": "ctrl+shift+w",
-                    "id" : "User.sendInput.)" SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH R"("
-                }
-            ]
-        })" };
+        implementation::SettingsLoader loader{ oldSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) };
+        loader.MergeInboxIntoUserSettings();
+        loader.FinalizeLayering();
+        loader.FixupUserSettings();
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+        const auto oldResult{ settings->ToJson() };
+        const auto sendInputCmd = settings->ActionMap().GetActionByKeyChord(KeyChord{ true, false, true, false, 87, 0 });
 
-        implementation::SettingsLoader oldLoader{ oldSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) };
-        oldLoader.MergeInboxIntoUserSettings();
-        oldLoader.FinalizeLayering();
-        VERIFY_IS_TRUE(oldLoader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
-        const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(oldLoader));
-        const auto oldResult{ oldSettings->ToJson() };
-
-        implementation::SettingsLoader newLoader{ newSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) };
-        newLoader.MergeInboxIntoUserSettings();
-        newLoader.FinalizeLayering();
-        newLoader.FixupUserSettings();
-        const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
-        const auto newResult{ newSettings->ToJson() };
+        std::string_view expectedID{ R"(User.sendInput.)" SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH };
 
-        VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult));
+        VERIFY_ARE_EQUAL(sendInputCmd.ID(), winrt::to_hstring(expectedID));
     }
 
     void SerializationTests::NoGeneratedIDsForIterableAndNestedCommands()
@@ -1068,17 +1049,20 @@ namespace SettingsModelUnitTests
         implementation::SettingsLoader loader1{ settingsJson1, implementation::LoadStringResource(IDR_DEFAULTS) };
         loader1.MergeInboxIntoUserSettings();
         loader1.FinalizeLayering();
-        VERIFY_IS_TRUE(loader1.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+        loader1.FixupUserSettings();
         const auto settings1 = winrt::make_self<implementation::CascadiaSettings>(std::move(loader1));
         const auto result1{ settings1->ToJson() };
 
         implementation::SettingsLoader loader2{ settingsJson2, implementation::LoadStringResource(IDR_DEFAULTS) };
         loader2.MergeInboxIntoUserSettings();
         loader2.FinalizeLayering();
-        VERIFY_IS_TRUE(loader2.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+        loader2.FixupUserSettings();
         const auto settings2 = winrt::make_self<implementation::CascadiaSettings>(std::move(loader2));
         const auto result2{ settings2->ToJson() };
 
-        VERIFY_ARE_EQUAL(toString(result1), toString(result2));
+        const auto sendInputCmd1 = settings1->ActionMap().GetActionByKeyChord(KeyChord{ true, false, true, false, 87, 0 });
+        const auto sendInputCmd2 = settings2->ActionMap().GetActionByKeyChord(KeyChord{ true, false, true, false, 87, 0 });
+
+        VERIFY_ARE_EQUAL(sendInputCmd1.ID(), sendInputCmd1.ID());
     }
 }