Skip to content

Commit

Permalink
Make Global and Profile settings inheritable (#7923)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request
Introduces `IInheritable` as an interface that helps move cascading settings into the Terminal Settings Model. `GlobalAppSettings` and `Profile` both are now `IInheritable`. `CascadiaSettings` was updated to `CreateChild()` for globals and each profile when we are loading the JSON data.

IInheritable does most of the heavy lifting. It introduces a two new macros and the interface. The macros help implement the fallback functionality for nullable and non-nullable settings.

## References
#7876 - Spec Addendum
#6904 - TSM Spec
#1564 - Settings UI

#7876 - `Copy()` needs to be updated to include _parent
  • Loading branch information
@carlos-zamora
carlos-zamora authored Oct 27, 2020
1 parent 7e86001 commit b603929
Show file tree
Hide file tree
Showing 20 changed files with 1,050 additions and 296 deletions.
1 change: 1 addition & 0 deletions .github/actions/spell-check/dictionary/apis.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ ICustom
IDialog
IDirect
IExplorer
IInheritable
IMap
IObject
IStorage
Expand Down
142 changes: 132 additions & 10 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ namespace SettingsModelLocalTests
TEST_METHOD(TestRebindNestedCommand);

TEST_METHOD(TestCopy);
TEST_METHOD(TestCloneInheritanceTree);

TEST_CLASS_SETUP(ClassSetup)
{
Expand Down Expand Up @@ -831,7 +832,7 @@ namespace SettingsModelLocalTests
const auto serialized0Profile = profile0->GenerateStub();
const auto profile1 = implementation::Profile::FromJson(serialized0Profile);
VERIFY_IS_FALSE(profile0->HasGuid());
VERIFY_IS_FALSE(profile1->HasGuid());
VERIFY_IS_TRUE(profile1->HasGuid());

auto settings = winrt::make_self<implementation::CascadiaSettings>();
settings->_profiles.Append(*profile1);
Expand Down Expand Up @@ -1355,6 +1356,7 @@ namespace SettingsModelLocalTests
const winrt::guid guid1{ ::Microsoft::Console::Utils::GuidFromString(L"{6239a42c-6666-49a3-80bd-e8fdd045185c}") };
const winrt::guid guid2{ ::Microsoft::Console::Utils::GuidFromString(L"{2C4DE342-38B7-51CF-B940-2309A097F518}") };
const winrt::guid fakeGuid{ ::Microsoft::Console::Utils::GuidFromString(L"{FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}") };
const winrt::guid autogeneratedGuid{ implementation::Profile::_GenerateGuidForProfile(name3, L"") };
const std::optional<winrt::guid> badGuid{};

VerifyParseSucceeded(settings0String);
Expand All @@ -1366,7 +1368,7 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(guid0, settings->_GetProfileGuidByName(name0));
VERIFY_ARE_EQUAL(guid1, settings->_GetProfileGuidByName(name1));
VERIFY_ARE_EQUAL(guid2, settings->_GetProfileGuidByName(name2));
VERIFY_ARE_EQUAL(badGuid, settings->_GetProfileGuidByName(name3));
VERIFY_ARE_EQUAL(autogeneratedGuid, settings->_GetProfileGuidByName(name3));
VERIFY_ARE_EQUAL(badGuid, settings->_GetProfileGuidByName(badName));

auto prof0{ settings->FindProfile(guid0) };
Expand Down Expand Up @@ -1521,9 +1523,9 @@ namespace SettingsModelLocalTests
{
auto settings = winrt::make_self<implementation::CascadiaSettings>(false);
settings->_ParseJsonString(settings0String, false);
VERIFY_IS_TRUE(settings->_userDefaultProfileSettings == Json::Value::null);
VERIFY_IS_NULL(settings->_userDefaultProfileSettings);
settings->_ApplyDefaultsFromUserSettings();
VERIFY_IS_FALSE(settings->_userDefaultProfileSettings == Json::Value::null);
VERIFY_IS_NOT_NULL(settings->_userDefaultProfileSettings);
settings->LayerJson(settings->_userSettings);

VERIFY_ARE_EQUAL(guid1String, settings->_globals->UnparsedDefaultProfile());
Expand Down Expand Up @@ -1573,9 +1575,9 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(2u, settings->_profiles.Size());

settings->_ParseJsonString(settings0String, false);
VERIFY_IS_TRUE(settings->_userDefaultProfileSettings == Json::Value::null);
VERIFY_IS_NULL(settings->_userDefaultProfileSettings);
settings->_ApplyDefaultsFromUserSettings();
VERIFY_IS_FALSE(settings->_userDefaultProfileSettings == Json::Value::null);
VERIFY_IS_NOT_NULL(settings->_userDefaultProfileSettings);

Log::Comment(NoThrowString().Format(
L"Ensure that cmd and powershell don't get their GUIDs overwritten"));
Expand Down Expand Up @@ -1692,10 +1694,6 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(L"Terminal.App.UnitTest.1", settings->_profiles.GetAt(1).Source());
VERIFY_ARE_EQUAL(L"Terminal.App.UnitTest.1", settings->_profiles.GetAt(2).Source());

VERIFY_IS_TRUE(settings->_profiles.GetAt(0).HasGuid());
VERIFY_IS_TRUE(settings->_profiles.GetAt(1).HasGuid());
VERIFY_IS_TRUE(settings->_profiles.GetAt(2).HasGuid());

VERIFY_ARE_EQUAL(guid1, settings->_profiles.GetAt(0).Guid());
VERIFY_ARE_EQUAL(guid1, settings->_profiles.GetAt(1).Guid());
VERIFY_ARE_EQUAL(guid2, settings->_profiles.GetAt(2).Guid());
Expand Down Expand Up @@ -2256,6 +2254,7 @@ namespace SettingsModelLocalTests
settings->_ParseJsonString(settings1Json, false);
settings->LayerJson(settings->_userSettings);
settings->_ValidateSettings();
commands = settings->_globals->Commands();
_logCommandNames(commands);
VERIFY_ARE_EQUAL(0u, settings->_warnings.Size());
VERIFY_ARE_EQUAL(0u, commands.Size());
Expand Down Expand Up @@ -2350,6 +2349,7 @@ namespace SettingsModelLocalTests
settings->_ParseJsonString(settings1Json, false);
settings->LayerJson(settings->_userSettings);
settings->_ValidateSettings();
commands = settings->_globals->Commands();
_logCommandNames(commands);
VERIFY_ARE_EQUAL(0u, settings->_warnings.Size());
VERIFY_ARE_EQUAL(1u, commands.Size());
Expand Down Expand Up @@ -2460,4 +2460,126 @@ namespace SettingsModelLocalTests
copyImpl->_globals->WordDelimiters(L"changed value");
VERIFY_ARE_NOT_EQUAL(settings->_globals->WordDelimiters(), copyImpl->_globals->WordDelimiters());
}

void DeserializationTests::TestCloneInheritanceTree()
{
const std::string settingsJson{ R"(
{
"defaultProfile": "{61c54bbd-1111-5271-96e7-009a87ff44bf}",
"profiles":
{
"defaults": {
"name": "PROFILE DEFAULTS"
},
"list": [
{
"guid": "{61c54bbd-1111-5271-96e7-009a87ff44bf}",
"name": "CMD"
},
{
"guid": "{61c54bbd-2222-5271-96e7-009a87ff44bf}",
"name": "PowerShell"
},
{
"guid": "{61c54bbd-3333-5271-96e7-009a87ff44bf}"
}
]
}
})" };

VerifyParseSucceeded(settingsJson);

auto settings{ winrt::make_self<implementation::CascadiaSettings>() };
settings->_ParseJsonString(settingsJson, false);
settings->_ApplyDefaultsFromUserSettings();
settings->LayerJson(settings->_userSettings);
settings->_ValidateSettings();

const auto copy{ settings->Copy() };
const auto copyImpl{ winrt::get_self<implementation::CascadiaSettings>(copy) };

// test globals
VERIFY_ARE_EQUAL(settings->_globals->DefaultProfile(), copyImpl->_globals->DefaultProfile());

// test profiles
VERIFY_ARE_EQUAL(settings->_profiles.Size(), copyImpl->_profiles.Size());
VERIFY_ARE_EQUAL(settings->_profiles.GetAt(0).Name(), copyImpl->_profiles.GetAt(0).Name());
VERIFY_ARE_EQUAL(settings->_profiles.GetAt(1).Name(), copyImpl->_profiles.GetAt(1).Name());
VERIFY_ARE_EQUAL(settings->_profiles.GetAt(2).Name(), copyImpl->_profiles.GetAt(2).Name());
VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->Name(), copyImpl->_userDefaultProfileSettings->Name());

// Modifying profile.defaults should...
VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName());
copyImpl->_userDefaultProfileSettings->Name(L"changed value");

// ...keep the same name for the first two profiles
VERIFY_ARE_EQUAL(settings->_profiles.Size(), copyImpl->_profiles.Size());
VERIFY_ARE_EQUAL(settings->_profiles.GetAt(0).Name(), copyImpl->_profiles.GetAt(0).Name());
VERIFY_ARE_EQUAL(settings->_profiles.GetAt(1).Name(), copyImpl->_profiles.GetAt(1).Name());

// ...but change the name for the one that inherited it from profile.defaults
VERIFY_ARE_NOT_EQUAL(settings->_profiles.GetAt(2).Name(), copyImpl->_profiles.GetAt(2).Name());

// profile.defaults should be different between the two graphs
VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName());
VERIFY_ARE_NOT_EQUAL(settings->_userDefaultProfileSettings->Name(), copyImpl->_userDefaultProfileSettings->Name());

Log::Comment(L"Test empty profiles.defaults");
const std::string emptyPDJson{ R"(
{
"defaultProfile": "{61c54bbd-1111-5271-96e7-009a87ff44bf}",
"profiles":
{
"defaults": {
},
"list": [
{
"guid": "{61c54bbd-2222-5271-96e7-009a87ff44bf}",
"name": "PowerShell"
}
]
}
})" };

const std::string missingPDJson{ R"(
{
"defaultProfile": "{61c54bbd-1111-5271-96e7-009a87ff44bf}",
"profiles":
[
{
"guid": "{61c54bbd-2222-5271-96e7-009a87ff44bf}",
"name": "PowerShell"
}
]
})" };

auto verifyEmptyPD = [this](const std::string json) {
VerifyParseSucceeded(json);

auto settings{ winrt::make_self<implementation::CascadiaSettings>() };
settings->_ParseJsonString(json, false);
settings->_ApplyDefaultsFromUserSettings();
settings->LayerJson(settings->_userSettings);
settings->_ValidateSettings();

const auto copy{ settings->Copy() };
const auto copyImpl{ winrt::get_self<implementation::CascadiaSettings>(copy) };

// test optimization: if we don't have profiles.defaults, don't add it to the tree
VERIFY_IS_NULL(settings->_userDefaultProfileSettings);
VERIFY_ARE_EQUAL(settings->_userDefaultProfileSettings, copyImpl->_userDefaultProfileSettings);

VERIFY_ARE_EQUAL(settings->Profiles().Size(), 1u);
VERIFY_ARE_EQUAL(settings->Profiles().Size(), copyImpl->Profiles().Size());

// so we should only have one parent, instead of two
auto srcProfile{ winrt::get_self<implementation::Profile>(settings->Profiles().GetAt(0)) };
auto copyProfile{ winrt::get_self<implementation::Profile>(copyImpl->Profiles().GetAt(0)) };
VERIFY_ARE_EQUAL(srcProfile->Parents().size(), 0u);
VERIFY_ARE_EQUAL(srcProfile->Parents().size(), copyProfile->Parents().size());
};

verifyEmptyPD(emptyPDJson);
verifyEmptyPD(missingPDJson);
}
}
44 changes: 23 additions & 21 deletions src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ namespace SettingsModelLocalTests
// A profile _can_ be layered with itself, though what's the point?
VERIFY_IS_FALSE(profile3->ShouldBeLayered(profile1Json));
VERIFY_IS_FALSE(profile3->ShouldBeLayered(profile2Json));
VERIFY_IS_FALSE(profile3->ShouldBeLayered(profile3Json));
VERIFY_IS_TRUE(profile3->ShouldBeLayered(profile3Json));
}

void ProfileTests::LayerProfileProperties()
Expand Down Expand Up @@ -132,39 +132,41 @@ namespace SettingsModelLocalTests

Log::Comment(NoThrowString().Format(
L"Layering profile1 on top of profile0"));
profile0->LayerJson(profile1Json);
auto profile1{ profile0->CreateChild() };
profile1->LayerJson(profile1Json);

VERIFY_IS_NOT_NULL(profile0->Foreground());
VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile0->Foreground().Value() });
VERIFY_IS_NOT_NULL(profile1->Foreground());
VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile1->Foreground().Value() });

VERIFY_IS_NOT_NULL(profile0->Background());
VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile0->Background().Value() });
VERIFY_IS_NOT_NULL(profile1->Background());
VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile1->Background().Value() });

VERIFY_IS_NOT_NULL(profile0->Background());
VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile0->Background().Value() });
VERIFY_IS_NOT_NULL(profile1->Background());
VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile1->Background().Value() });

VERIFY_ARE_EQUAL(L"profile1", profile0->Name());
VERIFY_ARE_EQUAL(L"profile1", profile1->Name());

VERIFY_IS_FALSE(profile0->StartingDirectory().empty());
VERIFY_ARE_EQUAL(L"C:/", profile0->StartingDirectory());
VERIFY_IS_FALSE(profile1->StartingDirectory().empty());
VERIFY_ARE_EQUAL(L"C:/", profile1->StartingDirectory());

Log::Comment(NoThrowString().Format(
L"Layering profile2 on top of (profile0+profile1)"));
profile0->LayerJson(profile2Json);
auto profile2{ profile1->CreateChild() };
profile2->LayerJson(profile2Json);

VERIFY_IS_NOT_NULL(profile0->Foreground());
VERIFY_ARE_EQUAL(til::color(3, 3, 3), til::color{ profile0->Foreground().Value() });
VERIFY_IS_NOT_NULL(profile2->Foreground());
VERIFY_ARE_EQUAL(til::color(3, 3, 3), til::color{ profile2->Foreground().Value() });

VERIFY_IS_NOT_NULL(profile0->Background());
VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile0->Background().Value() });
VERIFY_IS_NOT_NULL(profile2->Background());
VERIFY_ARE_EQUAL(til::color(1, 1, 1), til::color{ profile2->Background().Value() });

VERIFY_IS_NOT_NULL(profile0->SelectionBackground());
VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile0->SelectionBackground().Value() });
VERIFY_IS_NOT_NULL(profile2->SelectionBackground());
VERIFY_ARE_EQUAL(til::color(2, 2, 2), til::color{ profile2->SelectionBackground().Value() });

VERIFY_ARE_EQUAL(L"profile2", profile0->Name());
VERIFY_ARE_EQUAL(L"profile2", profile2->Name());

VERIFY_IS_FALSE(profile0->StartingDirectory().empty());
VERIFY_ARE_EQUAL(L"C:/", profile0->StartingDirectory());
VERIFY_IS_FALSE(profile2->StartingDirectory().empty());
VERIFY_ARE_EQUAL(L"C:/", profile2->StartingDirectory());
}

void ProfileTests::LayerProfileIcon()
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,7 @@ namespace TerminalAppLocalTests

const std::string settings0String{ R"(
{
"defaultProfile": "profile5",
"profiles": [
{
"name" : "profile0",
Expand Down
11 changes: 2 additions & 9 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -756,17 +756,10 @@ namespace winrt::TerminalApp::implementation

TerminalConnection::ITerminalConnection connection{ nullptr };

winrt::guid connectionType{};
winrt::guid connectionType = profile.ConnectionType();
winrt::guid sessionGuid{};

const auto hasConnectionType = profile.HasConnectionType();
if (hasConnectionType)
{
connectionType = profile.ConnectionType();
}

if (hasConnectionType &&
connectionType == TerminalConnection::AzureConnection::ConnectionType() &&
if (connectionType == TerminalConnection::AzureConnection::ConnectionType() &&
TerminalConnection::AzureConnection::IsAzureConnectionAvailable())
{
// TODO GH#4661: Replace this with directly using the AzCon when our VT is better
Expand Down
5 changes: 1 addition & 4 deletions src/cascadia/TerminalApp/TerminalSettings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,7 @@ namespace winrt::TerminalApp::implementation

_Commandline = profile.Commandline();

if (!profile.StartingDirectory().empty())
{
_StartingDirectory = profile.EvaluatedStartingDirectory();
}
_StartingDirectory = profile.EvaluatedStartingDirectory();

// GH#2373: Use the tabTitle as the starting title if it exists, otherwise
// use the profile name
Expand Down
51 changes: 45 additions & 6 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,6 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
// dynamic profile generators added by default
auto settings{ winrt::make_self<CascadiaSettings>() };
settings->_globals = _globals->Copy();
for (const auto profile : _profiles)
{
auto profImpl{ winrt::get_self<Profile>(profile) };
settings->_profiles.Append(*profImpl->Copy());
}
for (auto warning : _warnings)
{
settings->_warnings.Append(warning);
Expand All @@ -85,10 +80,54 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
settings->_userSettingsString = _userSettingsString;
settings->_userSettings = _userSettings;
settings->_defaultSettings = _defaultSettings;
settings->_userDefaultProfileSettings = _userDefaultProfileSettings;

_CopyProfileInheritanceTree(settings);

return *settings;
}

// Method Description:
// - Copies the inheritance tree for profiles and hooks them up to a clone CascadiaSettings
// Arguments:
// - cloneSettings: the CascadiaSettings we're copying the inheritance tree to
// Return Value:
// - <none>
void CascadiaSettings::_CopyProfileInheritanceTree(winrt::com_ptr<CascadiaSettings>& cloneSettings) const
{
// Our profiles inheritance graph doesn't have a formal root.
// However, if we create a dummy Profile, and set _profiles as the parent,
// we now have a root. So we'll do just that, then copy the inheritance graph
// from the dummyRoot.
auto dummyRootSource{ winrt::make_self<Profile>() };
for (const auto& profile : _profiles)
{
winrt::com_ptr<Profile> profileImpl;
profileImpl.copy_from(winrt::get_self<Profile>(profile));
dummyRootSource->InsertParent(profileImpl);
}

auto dummyRootClone{ winrt::make_self<Profile>() };
std::unordered_map<void*, winrt::com_ptr<Profile>> visited{};

if (_userDefaultProfileSettings)
{
// profile.defaults must be saved to CascadiaSettings
// So let's do that manually first, and add that to visited
cloneSettings->_userDefaultProfileSettings = Profile::CopySettings(_userDefaultProfileSettings);
visited[_userDefaultProfileSettings.get()] = cloneSettings->_userDefaultProfileSettings;
}

Profile::CloneInheritanceGraph(dummyRootSource, dummyRootClone, visited);

// All of the parents of the dummy root clone are _profiles.
// Get the parents and add them to the settings clone.
const auto cloneParents{ dummyRootClone->Parents() };
for (const auto& profile : cloneParents)
{
cloneSettings->_profiles.Append(*profile);
}
}

// Method Description:
// - Finds a profile that matches the given GUID. If there is no profile in this
// settings object that matches, returns nullptr.
Expand Down
Loading

0 comments on commit b603929

Please sign in to comment.