Skip to content

Commit

Permalink
Don't explode name-only profiles (#2789)
Browse files Browse the repository at this point in the history 
* Add a test for #2782

* Attempt to do something weird with _GenerateStub

  I was thinking maybe we have the stubs have a GUID included. I like that less though I think. That would mean that DPGs would always have the GUID generated for them, even if the DPG doesn't specify a GUID. I guess that's fine though. No DPG's _aren't_ generating names now so this shouldn't change anything.

* Add some more notes on why this was a bad idea

* Actually fix the issue at hand

  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 hasa guid. For a
  dynamic profile without a GUID, that'll _never_ be true, so it
  would be impossible to be layered.

* Revert "Add some more notes on why this was a bad idea"

This reverts commit 85b8b8a.

* Revert "Attempt to do something weird with _GenerateStub"

This reverts commit f204b98.

* Little test fixes

* Update src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

Co-Authored-By: Dustin L. Howett (MSFT) <[email protected]>
  • Loading branch information
@zadjii-msft @DHowett
zadjii-msft and DHowett authored Sep 17, 2019
1 parent fe6fa04 commit c30ef6d
Show file tree
Hide file tree
Showing 4 changed files with 187 additions and 0 deletions.
158 changes: 158 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
}
}
17 changes: 17 additions & 0 deletions src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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))
{
Expand Down
10 changes: 10 additions & 0 deletions src/cascadia/TerminalApp/Profile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/Profile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit c30ef6d

Please sign in to comment.