maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Don't explode name-only profiles (#2789)

Browse source 
* 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 85b8b8a53c.

* Revert "Attempt to do something weird with _GenerateStub"

This reverts commit f204b98177.

* Little test fixes

* Update src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
This commit is contained in:
Mike Griese 2019-09-17 10:20:52 -07:00 committed by GitHub
parent fe6fa04f15
commit c30ef6d30b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 187 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

158
src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file

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

17
src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp
View file

@ -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))
{

10
src/cascadia/TerminalApp/Profile.cpp
View file

@ -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.

2
src/cascadia/TerminalApp/Profile.h
View file

@ -55,6 +55,8 @@ public:
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;