diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index 83d061dbaba..87b6f4eebfb 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -45,6 +45,8 @@ namespace TerminalAppLocalTests TEST_METHOD(TestAllValidationsWithNullGuids); TEST_METHOD(TestReorderWithNullGuids); TEST_METHOD(TestReorderingWithoutGuid); + TEST_METHOD(TestLayeringNameOnlyProfiles); + TEST_METHOD(TestExplodingNameOnlyProfiles); TEST_CLASS_SETUP(ClassSetup) { @@ -967,4 +969,160 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(L"Ubuntu", settings._profiles.at(2)._name); VERIFY_ARE_EQUAL(L"Windows PowerShell", settings._profiles.at(3)._name); } + + void SettingsTests::TestLayeringNameOnlyProfiles() + { + // This is a test discovered during GH#2782. When we add a name-only + // profile, it should only layer with other name-only profiles with the + // _same name_ + + const std::string settings0String{ R"( + { + "defaultProfile" : "{00000000-0000-5f56-a8ff-afceeeaa6101}", + "profiles": [ + { + "guid" : "{00000000-0000-5f56-a8ff-afceeeaa6101}", + "name" : "ThisProfileIsGood" + + }, + { + "name" : "ThisProfileShouldNotLayer" + }, + { + "name" : "NeitherShouldThisOne" + } + ] + })" }; + + const auto settings0Json = VerifyParseSucceeded(settings0String); + + CascadiaSettings settings; + settings._ParseJsonString(DefaultJson, true); + settings.LayerJson(settings._defaultSettings); + VERIFY_ARE_EQUAL(2u, settings._profiles.size()); + VERIFY_IS_TRUE(settings._profiles.at(0)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(1)._guid.has_value()); + VERIFY_ARE_EQUAL(L"Windows PowerShell", settings._profiles.at(0)._name); + VERIFY_ARE_EQUAL(L"cmd", settings._profiles.at(1)._name); + + Log::Comment(NoThrowString().Format( + L"Parse the user settings")); + settings._ParseJsonString(settings0String, false); + settings.LayerJson(settings._userSettings); + + VERIFY_ARE_EQUAL(5u, settings._profiles.size()); + VERIFY_IS_TRUE(settings._profiles.at(0)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(1)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(2)._guid.has_value()); + VERIFY_IS_FALSE(settings._profiles.at(3)._guid.has_value()); + VERIFY_IS_FALSE(settings._profiles.at(4)._guid.has_value()); + VERIFY_ARE_EQUAL(L"Windows PowerShell", settings._profiles.at(0)._name); + VERIFY_ARE_EQUAL(L"cmd", settings._profiles.at(1)._name); + VERIFY_ARE_EQUAL(L"ThisProfileIsGood", settings._profiles.at(2)._name); + VERIFY_ARE_EQUAL(L"ThisProfileShouldNotLayer", settings._profiles.at(3)._name); + VERIFY_ARE_EQUAL(L"NeitherShouldThisOne", settings._profiles.at(4)._name); + } + + void SettingsTests::TestExplodingNameOnlyProfiles() + { + // This is a test for GH#2782. When we add a name-only profile, we'll + // generate a GUID for it. We should make sure that we don't re-append + // that profile to the list of profiles. + + const std::string settings0String{ R"( + { + "defaultProfile" : "{00000000-0000-5f56-a8ff-afceeeaa6101}", + "profiles": [ + { + "guid" : "{00000000-0000-5f56-a8ff-afceeeaa6101}", + "name" : "ThisProfileIsGood" + + }, + { + "name" : "ThisProfileShouldNotDuplicate" + }, + { + "name" : "NeitherShouldThisOne" + } + ] + })" }; + + const auto settings0Json = VerifyParseSucceeded(settings0String); + + CascadiaSettings settings; + settings._ParseJsonString(DefaultJson, true); + settings.LayerJson(settings._defaultSettings); + VERIFY_ARE_EQUAL(2u, settings._profiles.size()); + VERIFY_IS_TRUE(settings._profiles.at(0)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(1)._guid.has_value()); + VERIFY_ARE_EQUAL(L"Windows PowerShell", settings._profiles.at(0)._name); + VERIFY_ARE_EQUAL(L"cmd", settings._profiles.at(1)._name); + + Log::Comment(NoThrowString().Format( + L"Parse the user settings")); + settings._ParseJsonString(settings0String, false); + settings.LayerJson(settings._userSettings); + + VERIFY_ARE_EQUAL(5u, settings._profiles.size()); + VERIFY_IS_TRUE(settings._profiles.at(0)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(1)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(2)._guid.has_value()); + VERIFY_IS_FALSE(settings._profiles.at(3)._guid.has_value()); + VERIFY_IS_FALSE(settings._profiles.at(4)._guid.has_value()); + VERIFY_ARE_EQUAL(L"Windows PowerShell", settings._profiles.at(0)._name); + VERIFY_ARE_EQUAL(L"cmd", settings._profiles.at(1)._name); + VERIFY_ARE_EQUAL(L"ThisProfileIsGood", settings._profiles.at(2)._name); + VERIFY_ARE_EQUAL(L"ThisProfileShouldNotDuplicate", settings._profiles.at(3)._name); + VERIFY_ARE_EQUAL(L"NeitherShouldThisOne", settings._profiles.at(4)._name); + + Log::Comment(NoThrowString().Format( + L"Pretend like we're checking to append dynamic profiles to the " + L"user's settings file. We absolutely _shouldn't_ be adding anything here.")); + bool const needToWriteFile = settings._AppendDynamicProfilesToUserSettings(); + VERIFY_IS_FALSE(needToWriteFile); + VERIFY_ARE_EQUAL(settings0String.size(), settings._userSettingsString.size()); + + Log::Comment(NoThrowString().Format( + L"Re-parse the settings file. We should have the _same_ settings as before.")); + Log::Comment(NoThrowString().Format( + L"Do this to a _new_ settings object, to make sure it turns out the same.")); + { + CascadiaSettings settings2; + settings2._ParseJsonString(DefaultJson, true); + settings2.LayerJson(settings2._defaultSettings); + VERIFY_ARE_EQUAL(2u, settings2._profiles.size()); + // Initialize the second settings object from the first settings + // object's settings string, the one that we synthesized. + const auto firstSettingsString = settings._userSettingsString; + settings2._ParseJsonString(firstSettingsString, false); + settings2.LayerJson(settings2._userSettings); + VERIFY_ARE_EQUAL(5u, settings2._profiles.size()); + VERIFY_IS_TRUE(settings2._profiles.at(0)._guid.has_value()); + VERIFY_IS_TRUE(settings2._profiles.at(1)._guid.has_value()); + VERIFY_IS_TRUE(settings2._profiles.at(2)._guid.has_value()); + VERIFY_IS_FALSE(settings2._profiles.at(3)._guid.has_value()); + VERIFY_IS_FALSE(settings2._profiles.at(4)._guid.has_value()); + VERIFY_ARE_EQUAL(L"Windows PowerShell", settings2._profiles.at(0)._name); + VERIFY_ARE_EQUAL(L"cmd", settings2._profiles.at(1)._name); + VERIFY_ARE_EQUAL(L"ThisProfileIsGood", settings2._profiles.at(2)._name); + VERIFY_ARE_EQUAL(L"ThisProfileShouldNotDuplicate", settings2._profiles.at(3)._name); + VERIFY_ARE_EQUAL(L"NeitherShouldThisOne", settings2._profiles.at(4)._name); + } + + Log::Comment(NoThrowString().Format( + L"Validate the settings. All the profiles we have should be valid.")); + settings._ValidateSettings(); + + VERIFY_ARE_EQUAL(5u, settings._profiles.size()); + VERIFY_IS_TRUE(settings._profiles.at(0)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(1)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(2)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(3)._guid.has_value()); + VERIFY_IS_TRUE(settings._profiles.at(4)._guid.has_value()); + VERIFY_ARE_EQUAL(L"ThisProfileIsGood", settings._profiles.at(0)._name); + VERIFY_ARE_EQUAL(L"ThisProfileShouldNotDuplicate", settings._profiles.at(1)._name); + VERIFY_ARE_EQUAL(L"NeitherShouldThisOne", settings._profiles.at(2)._name); + VERIFY_ARE_EQUAL(L"Windows PowerShell", settings._profiles.at(3)._name); + VERIFY_ARE_EQUAL(L"cmd", settings._profiles.at(4)._name); + } } diff --git a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp index 7c593ee3aa9..782503aec84 100644 --- a/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp @@ -282,6 +282,9 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings() { return true; } + // If the profileJson doesn't have a GUID, then it might be in + // the file still. We returned false because it shouldn't be + // layered, but it might be a name-only profile. } } return false; @@ -298,6 +301,20 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings() for (const auto& profile : _profiles) { + if (!profile.HasGuid()) + { + // If the profile doesn't have a guid, it's a name-only profile. + // During validation, we'll generate a GUID for the profile, but + // validation occurs after this. We should ignore these types of + // profiles. + // If a dynamic profile was generated _without_ a GUID, we also + // don't want it serialized here. The first check in + // Profile::ShouldBeLayered checks that the profile has a guid. For a + // dynamic profile without a GUID, that'll _never_ be true, so it + // would be impossible to be layered. + continue; + } + // Skip profiles that are in the user settings or the default settings. if (isInJsonObj(profile, _userSettings) || isInJsonObj(profile, _defaultSettings)) { diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp index aa1b0156177..fe07983c08b 100644 --- a/src/cascadia/TerminalApp/Profile.cpp +++ b/src/cascadia/TerminalApp/Profile.cpp @@ -119,6 +119,16 @@ Profile::~Profile() { } +bool Profile::HasGuid() const noexcept +{ + return _guid.has_value(); +} + +bool Profile::HasSource() const noexcept +{ + return _source.has_value(); +} + GUID Profile::GetGuid() const noexcept { // This can throw if we never had our guid set to a legitimate value. diff --git a/src/cascadia/TerminalApp/Profile.h b/src/cascadia/TerminalApp/Profile.h index 5e16f642799..eabba4f3aeb 100644 --- a/src/cascadia/TerminalApp/Profile.h +++ b/src/cascadia/TerminalApp/Profile.h @@ -55,6 +55,8 @@ class TerminalApp::Profile final void LayerJson(const Json::Value& json); static bool IsDynamicProfileObject(const Json::Value& json); + bool HasGuid() const noexcept; + bool HasSource() const noexcept; GUID GetGuid() const noexcept; void SetSource(std::wstring_view sourceNamespace) noexcept; std::wstring_view GetName() const noexcept;