diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index ea603c942..c20a9a853 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -7,7 +7,9 @@ #include "../TerminalSettingsModel/CascadiaSettings.h" #include "JsonTestClass.h" #include "TestUtils.h" + #include +#include using namespace Microsoft::Console; using namespace WEX::Logging; @@ -70,6 +72,7 @@ namespace SettingsModelLocalTests TEST_METHOD(TestCloneInheritanceTree); TEST_METHOD(TestValidDefaults); TEST_METHOD(TestInheritedCommand); + TEST_METHOD(LoadFragmentsWithMultipleUpdates); private: static winrt::com_ptr createSettings(const std::string_view& userJSON) @@ -1979,4 +1982,34 @@ namespace SettingsModelLocalTests VERIFY_IS_NULL(actualKeyChord); } } + + // This test ensures GH#11597 doesn't regress. + void DeserializationTests::LoadFragmentsWithMultipleUpdates() + { + static constexpr std::wstring_view fragmentSource{ L"fragment" }; + static constexpr std::string_view fragmentJson{ R"({ + "profiles": [ + { + "updates": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}", + "cursorShape": "filledBox" + }, + { + "updates": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}", + "cursorShape": "filledBox" + }, + { + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "commandline": "cmd.exe" + } + ] + })" }; + + implementation::SettingsLoader loader{ std::string_view{}, DefaultJson }; + loader.MergeInboxIntoUserSettings(); + loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson); + loader.FinalizeLayering(); + + VERIFY_IS_FALSE(loader.duplicateProfile); + VERIFY_ARE_EQUAL(3u, loader.userSettings.profiles.size()); + } } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index b96e299ff..f74eb90d1 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -57,6 +57,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void ApplyRuntimeInitialSettings(); void MergeInboxIntoUserSettings(); void FindFragmentsAndMergeIntoUserSettings(); + void MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content); void FinalizeLayering(); bool DisableDeletedProfiles(); @@ -82,7 +83,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _parseFragment(const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings); static JsonSettings _parseJson(const std::string_view& content); static winrt::com_ptr _parseProfile(const OriginTag origin, const winrt::hstring& source, const Json::Value& profileJson); - void _appendProfile(winrt::com_ptr&& profile, ParsedSettings& settings); + void _appendProfile(winrt::com_ptr&& profile, const winrt::guid& guid, ParsedSettings& settings); static void _addParentProfile(const winrt::com_ptr& profile, ParsedSettings& settings); void _executeGenerator(const IDynamicProfileGenerator& generator); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 83f0952c2..aa8402af1 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -207,26 +207,6 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings() { const auto content = ReadUTF8File(fragmentExt.path()); _parseFragment(source, content, fragmentSettings); - - for (const auto& fragmentProfile : fragmentSettings.profiles) - { - if (const auto updates = fragmentProfile->Updates(); updates != winrt::guid{}) - { - if (const auto it = userSettings.profilesByGuid.find(updates); it != userSettings.profilesByGuid.end()) - { - it->second->InsertParent(0, fragmentProfile); - } - } - else - { - _addParentProfile(fragmentProfile, userSettings); - } - } - - for (const auto& kv : fragmentSettings.globals->ColorSchemes()) - { - userSettings.globals->AddColorScheme(kv.Value()); - } } CATCH_LOG(); } @@ -289,6 +269,15 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings() } } +// See FindFragmentsAndMergeIntoUserSettings. +// This function does the same, but for a single given JSON blob and source +// and at the time of writing is used for unit tests only. +void SettingsLoader::MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content) +{ + ParsedSettings fragmentSettings; + _parseFragment(source, content, fragmentSettings); +} + // Call this method before passing SettingsLoader to the CascadiaSettings constructor. // It layers all remaining objects onto each other (those that aren't covered // by MergeInboxIntoUserSettings/FindFragmentsAndMergeIntoUserSettings). @@ -475,7 +464,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source // GH#9962: Discard Guid-less, Name-less profiles. if (profile->HasGuid()) { - _appendProfile(std::move(profile), settings); + _appendProfile(std::move(profile), profile->Guid(), settings); } } } @@ -517,14 +506,37 @@ void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::str auto profile = _parseProfile(OriginTag::Fragment, source, profileJson); // GH#9962: Discard Guid-less, Name-less profiles, but... // allow ones with an Updates field, as those are special for fragments. - if (profile->HasGuid() || profile->Updates() != winrt::guid{}) + // We need to make sure to only call Guid() if HasGuid() is true, + // as Guid() will dynamically generate a return value otherwise. + const auto guid = profile->HasGuid() ? profile->Guid() : profile->Updates(); + if (guid != winrt::guid{}) { - _appendProfile(std::move(profile), settings); + _appendProfile(std::move(profile), guid, settings); } } CATCH_LOG() } } + + for (const auto& fragmentProfile : settings.profiles) + { + if (const auto updates = fragmentProfile->Updates(); updates != winrt::guid{}) + { + if (const auto it = userSettings.profilesByGuid.find(updates); it != userSettings.profilesByGuid.end()) + { + it->second->InsertParent(0, fragmentProfile); + } + } + else + { + _addParentProfile(fragmentProfile, userSettings); + } + } + + for (const auto& kv : settings.globals->ColorSchemes()) + { + userSettings.globals->AddColorScheme(kv.Value()); + } } SettingsLoader::JsonSettings SettingsLoader::_parseJson(const std::string_view& content) @@ -564,11 +576,11 @@ winrt::com_ptr SettingsLoader::_parseProfile(const OriginTag origin, co // Adds a profile to the ParsedSettings instance. Takes ownership of the profile. // It ensures no duplicate GUIDs are added to the ParsedSettings instance. -void SettingsLoader::_appendProfile(winrt::com_ptr&& profile, ParsedSettings& settings) +void SettingsLoader::_appendProfile(winrt::com_ptr&& profile, const winrt::guid& guid, ParsedSettings& settings) { // FYI: The static_cast ensures we don't move the profile into // `profilesByGuid`, even though we still need it later for `profiles`. - if (settings.profilesByGuid.emplace(profile->Guid(), static_cast&>(profile)).second) + if (settings.profilesByGuid.emplace(guid, static_cast&>(profile)).second) { settings.profiles.emplace_back(profile); }