Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't explode name-only profiles #2789

Merged
merged 8 commits into from
Sep 17, 2019
Merged

Don't explode name-only profiles #2789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
907ebcd
Add a test for #2782
zadjii-msft Sep 17, 2019
f204b98
Attempt to do something weird with _GenerateStub
zadjii-msft Sep 17, 2019
85b8b8a
Add some more notes on why this was a bad idea
zadjii-msft Sep 17, 2019
3feaefe
Actually fix the issue at hand
zadjii-msft Sep 17, 2019
30c0bc3
Revert "Add some more notes on why this was a bad idea"
zadjii-msft Sep 17, 2019
1ef2074
Revert "Attempt to do something weird with _GenerateStub"
zadjii-msft Sep 17, 2019
8ae7dee
Little test fixes
zadjii-msft Sep 17, 2019
be1d305
Update src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
zadjii-msft Sep 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 158 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this test need to parse the defaults and validate them? it seems extraneous, but idk

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
Open in desktop
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
Open in desktop
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
Open in desktop
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