No longer serialize generated IDs (#17145)

We will no longer serialize IDs that we generated for the user.
This change is being made so that we can release user action IDs at the
same time as the features that require them!

Validation: Generated IDs do not get written to the json, user-made IDs
still do

Closes #17109

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());
  }
 }