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.

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

Make Global and Profile settings inheritable #7923

Merged
31 commits merged into from
Oct 27, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
62b2a90
Make Global and Profile settings inheritable
carlos-zamora Oct 12, 2020
570afa2
Update IDL. Fix Generate Stub. More inheritance
carlos-zamora Oct 15, 2020
374ad80
address PR Comments; inherit schemes/actions
carlos-zamora Oct 15, 2020
ae97064
fix BI Alignment bug
carlos-zamora Oct 16, 2020
cb59ec1
Merge branch 'master' into dev/cazamor/tsm/inheritance
carlos-zamora Oct 16, 2020
d1ddab5
fix StartingDirectory
carlos-zamora Oct 16, 2020
2866b4e
Introduce Multi-Parent Inheritance (LOGIC ERROR)
carlos-zamora Oct 20, 2020
d74f236
fix logic error (needed see-thru guid)
carlos-zamora Oct 20, 2020
9685108
startingDirectory is just a setting
carlos-zamora Oct 20, 2020
298636b
fix up some tests
carlos-zamora Oct 20, 2020
49b98b3
polish
carlos-zamora Oct 20, 2020
e026f32
first draft of CloneInheritanceGraph
carlos-zamora Oct 21, 2020
dd8a2cb
fix inheritance linker error; optimize inheritance tree
carlos-zamora Oct 21, 2020
07351bb
fix inheritance copy
carlos-zamora Oct 21, 2020
6d34980
fix remaining PR comments
carlos-zamora Oct 21, 2020
6e1849b
fix startingDirectory
carlos-zamora Oct 21, 2020
80e234b
polish
carlos-zamora Oct 21, 2020
7150e52
move guid generation to getter
carlos-zamora Oct 21, 2020
0a806fd
address more of Dustin's comments
carlos-zamora Oct 22, 2020
401efb8
adios comment
carlos-zamora Oct 22, 2020
1292633
fix failing tests
carlos-zamora Oct 22, 2020
0c549f0
Merge branch 'main' into dev/cazamor/tsm/inheritance
carlos-zamora Oct 22, 2020
5c594f3
fix tests
carlos-zamora Oct 22, 2020
01e2c49
address Zadji PR comments
carlos-zamora Oct 23, 2020
e0e3943
make ConnectionType Inheritable
carlos-zamora Oct 23, 2020
b4ec4da
identify connectiontype-ed profiles
carlos-zamora Oct 23, 2020
2716fa0
fix defaultProfile
carlos-zamora Oct 26, 2020
36211ba
add optimization: don't inherit PD if it's empty
carlos-zamora Oct 26, 2020
b55856e
dynamic generator guids should have their own namespace
carlos-zamora Oct 26, 2020
6f55ae5
code format
carlos-zamora Oct 26, 2020
4cf3b1c
remove a few comments
carlos-zamora Oct 27, 2020
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
65 changes: 65 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
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 @@ -2457,4 +2458,68 @@ 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->LayerJson(settings->_userSettings);
settings->_ValidateSettings();

DebugBreak();
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());

// 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_NOT_EQUAL(settings->_userDefaultProfileSettings->HasName(), copyImpl->_userDefaultProfileSettings->HasName());
VERIFY_ARE_NOT_EQUAL(settings->_userDefaultProfileSettings->Name(), copyImpl->_userDefaultProfileSettings->Name());
}
}
25 changes: 20 additions & 5 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
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 @@ -86,6 +81,26 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
settings->_userSettings = _userSettings;
settings->_defaultSettings = _defaultSettings;
settings->_userDefaultProfileSettings = _userDefaultProfileSettings;

// 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, and
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
auto dummyRootSource{ winrt::make_self<Profile>() };
for (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>() };
Profile::CloneInheritanceGraph(dummyRootSource, dummyRootClone);

auto cloneParents{ dummyRootClone->ExportParents() };
for (auto profile : cloneParents)
{
settings->_profiles.Append(*profile);
}

return *settings;
}

Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsModel/IInheritable.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
_parents.insert(pos, parent);
}

std::vector<com_ptr<T>> ExportParents()
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
return _parents;
}

protected:
std::vector<com_ptr<T>> _parents{};

Expand Down
136 changes: 91 additions & 45 deletions src/cascadia/TerminalSettingsModel/Profile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,60 +70,106 @@ Profile::Profile(guid guid) :
{
}

winrt::com_ptr<Profile> Profile::Copy() const
winrt::com_ptr<Profile> Profile::_CopyMembers(winrt::com_ptr<Profile> source)
{
// copy the private members
auto profile{ winrt::make_self<Profile>() };
profile->_Guid = _Guid;
profile->_Name = _Name;
profile->_Source = _Source;
profile->_Hidden = _Hidden;
profile->_Icon = _Icon;
profile->_CloseOnExit = _CloseOnExit;
profile->_TabTitle = _TabTitle;
profile->_TabColor = _TabColor;
profile->_SuppressApplicationTitle = _SuppressApplicationTitle;
profile->_UseAcrylic = _UseAcrylic;
profile->_AcrylicOpacity = _AcrylicOpacity;
profile->_ScrollState = _ScrollState;
profile->_FontFace = _FontFace;
profile->_FontSize = _FontSize;
profile->_FontWeight = _FontWeight;
profile->_Padding = _Padding;
profile->_Commandline = _Commandline;
profile->_StartingDirectory = _StartingDirectory;
profile->_BackgroundImagePath = _BackgroundImagePath;
profile->_BackgroundImageOpacity = _BackgroundImageOpacity;
profile->_BackgroundImageStretchMode = _BackgroundImageStretchMode;
profile->_AntialiasingMode = _AntialiasingMode;
profile->_RetroTerminalEffect = _RetroTerminalEffect;
profile->_ForceFullRepaintRendering = _ForceFullRepaintRendering;
profile->_SoftwareRendering = _SoftwareRendering;
profile->_ColorSchemeName = _ColorSchemeName;
profile->_Foreground = _Foreground;
profile->_Background = _Background;
profile->_SelectionBackground = _SelectionBackground;
profile->_CursorColor = _CursorColor;
profile->_HistorySize = _HistorySize;
profile->_SnapOnInput = _SnapOnInput;
profile->_AltGrAliasing = _AltGrAliasing;
profile->_CursorShape = _CursorShape;
profile->_CursorHeight = _CursorHeight;
profile->_BellStyle = _BellStyle;
profile->_Guid = source->_Guid;
profile->_Name = source->_Name;
profile->_Source = source->_Source;
profile->_Hidden = source->_Hidden;
profile->_Icon = source->_Icon;
profile->_CloseOnExit = source->_CloseOnExit;
profile->_TabTitle = source->_TabTitle;
profile->_TabColor = source->_TabColor;
profile->_SuppressApplicationTitle = source->_SuppressApplicationTitle;
profile->_UseAcrylic = source->_UseAcrylic;
profile->_AcrylicOpacity = source->_AcrylicOpacity;
profile->_ScrollState = source->_ScrollState;
profile->_FontFace = source->_FontFace;
profile->_FontSize = source->_FontSize;
profile->_FontWeight = source->_FontWeight;
profile->_Padding = source->_Padding;
profile->_Commandline = source->_Commandline;
profile->_StartingDirectory = source->_StartingDirectory;
profile->_BackgroundImagePath = source->_BackgroundImagePath;
profile->_BackgroundImageOpacity = source->_BackgroundImageOpacity;
profile->_BackgroundImageStretchMode = source->_BackgroundImageStretchMode;
profile->_AntialiasingMode = source->_AntialiasingMode;
profile->_RetroTerminalEffect = source->_RetroTerminalEffect;
profile->_ForceFullRepaintRendering = source->_ForceFullRepaintRendering;
profile->_SoftwareRendering = source->_SoftwareRendering;
profile->_ColorSchemeName = source->_ColorSchemeName;
profile->_Foreground = source->_Foreground;
profile->_Background = source->_Background;
profile->_SelectionBackground = source->_SelectionBackground;
profile->_CursorColor = source->_CursorColor;
profile->_HistorySize = source->_HistorySize;
profile->_SnapOnInput = source->_SnapOnInput;
profile->_AltGrAliasing = source->_AltGrAliasing;
profile->_CursorShape = source->_CursorShape;
profile->_CursorHeight = source->_CursorHeight;
profile->_BellStyle = source->_BellStyle;

// copy settings that did not use the GETSET macro
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
profile->_BackgroundImageAlignment = _BackgroundImageAlignment;
profile->_ConnectionType = _ConnectionType;

// TODO CARLOS: copy parents instead of parent
//if (_parent)
//{
// profile->_parent = _parent->Copy();
//}
profile->_BackgroundImageAlignment = source->_BackgroundImageAlignment;
profile->_ConnectionType = source->_ConnectionType;

return profile;
}

// Method Description:
// - Creates a copy of the inheritance graph by performing a depth-first traversal recursively.
// Profiles are recorded as visited via the "visited" parameter.
// Unvisited Profiles are copied into the "cloneGraph" parameter, then marked as visited.
// Arguments:
// - sourceGraph - the graph of Profile's we're cloning
// - cloneGraph - the clone of sourceGraph that is being constructed
// - visited - a map of which Profiles have been visited, and, if so, a reference to the Profile's clone
// Return Value:
// - a clone in both inheritance structure and Profile values of sourceGraph
winrt::com_ptr<Profile> Profile::CloneInheritanceGraph(winrt::com_ptr<Profile> sourceGraph, winrt::com_ptr<Profile> cloneGraph, std::unordered_map<void*, winrt::com_ptr<Profile>> visited)
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
// If this is an unexplored Profile
// and we have parents...
if (visited.find(sourceGraph.get()) == visited.end() && !sourceGraph->_parents.empty())
{
// iterate through all of our parents to copy them
for (auto sourceParent : sourceGraph->_parents)
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
// If we visited this Profile already...
auto kv{ visited.find(sourceParent.get()) };
if (visited.find(sourceParent.get()) != visited.end())
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
// add this Profile's clone as a parent
cloneGraph->InsertParent(kv->second);
}
else
{
// We have not visited this Profile yet,
// copy contents of sourceParent to clone
winrt::com_ptr<Profile> clone{ sourceParent->Copy() };

// add the new copy to the cloneGraph
cloneGraph->InsertParent(clone);

// copy the sub-graph at "clone"
CloneInheritanceGraph(sourceParent, clone, visited);

// mark clone as "visited"
// save it to the map in case somebody else references it
visited[sourceParent.get()] = clone;
}

// if clone is empty, this is the end of the inheritance line
// otherwise, it is populated with the contents of its parent
}
}

// we have no more to explore down this path.
return cloneGraph;
}

// Method Description:
// - Generates a Json::Value which is a "stub" of this profile. This stub will
// have enough information that it could be layered with this profile.
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsModel/Profile.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Profile();
Profile(guid guid);
com_ptr<Profile> Copy() const;
static com_ptr<Profile> CloneInheritanceGraph(com_ptr<Profile> oldProfile, com_ptr<Profile> newProfile, std::unordered_map<void*, com_ptr<Profile>> visited = {});

Json::Value GenerateStub() const;
static com_ptr<Profile> FromJson(const Json::Value& json);
Expand Down Expand Up @@ -147,6 +148,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

static guid _GenerateGuidForProfile(const hstring& name, const hstring& source) noexcept;

static com_ptr<Profile> _CopyMembers(com_ptr<Profile> source);

friend class TerminalAppLocalTests::SettingsTests;
friend class TerminalAppLocalTests::ProfileTests;
friend class TerminalAppUnitTests::JsonTests;
Expand Down