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

Change the ControlCore layer to own a copy of its settings #11619

Merged
62 commits merged into from
Dec 1, 2021
Merged

Change the ControlCore layer to own a copy of its settings #11619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
43ec102
blindly make control settings read-only, there's only 2 build breaks?
zadjii-msft Sep 23, 2021
dad065e
make this a macro in case we want to reuse this
zadjii-msft Sep 23, 2021
981d8cc
This is a crazy idea but if it works, it'll be a game changer
zadjii-msft Sep 23, 2021
d6989ec
this is wild
zadjii-msft Sep 23, 2021
4b18bb4
missed one
zadjii-msft Sep 23, 2021
7fc2f10
move to a common header
zadjii-msft Sep 23, 2021
c7536ed
This says there's only one build error in Terminal.Control.Lib but th…
zadjii-msft Sep 28, 2021
c26dd6b
This works better than it has any business doing. Plemty of bugs, but…
zadjii-msft Sep 28, 2021
4a1baf0
I think I needed this to get it to build. Or I started breaknig somet…
zadjii-msft Sep 30, 2021
fbba74e
I think we're going to have to cut this
zadjii-msft Oct 14, 2021
1413d01
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Oct 14, 2021
31e7998
this builds at least
zadjii-msft Oct 19, 2021
4f7e883
This crashes substantially less frequently
zadjii-msft Oct 19, 2021
a9de82e
saving the settings works again
zadjii-msft Oct 20, 2021
70b9f8c
This is the start of fixing the opacity roundtripping weirdness
zadjii-msft Oct 20, 2021
0371194
okay this seems to fix opacity & cleartype in all cases
zadjii-msft Oct 20, 2021
4912b65
mostly cleanup, 7 TODOs remain
zadjii-msft Oct 20, 2021
888e157
more cleanup, 5 todo!s left
zadjii-msft Oct 20, 2021
17829f4
This is how I wanted to solve the color scheme setting, previewing, b…
zadjii-msft Oct 21, 2021
1aa2849
you've seen WINRT_PROPERTY now get ready for RUNTIME_PROPERTY
zadjii-msft Oct 21, 2021
e3a50cf
turns out we didn't need all them fancy overrides
zadjii-msft Oct 21, 2021
9160966
This almost works for previewing, but I fudged something up with the …
zadjii-msft Oct 26, 2021
282c03c
whoop, it works!
zadjii-msft Oct 26, 2021
6babb4e
cleanup
zadjii-msft Oct 26, 2021
94f4ef5
lots of cleanup
zadjii-msft Oct 26, 2021
28cbad1
get the tests compiling again
zadjii-msft Oct 26, 2021
634b685
fix a bug with toggling the retro effects from the palette
zadjii-msft Oct 26, 2021
48ca704
More cleanup. Forgot to lock here.
zadjii-msft Oct 26, 2021
217742c
Some of these calls were duplicates. Some were redundant. Overall cle…
zadjii-msft Oct 26, 2021
4a700cd
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Oct 26, 2021
01cef2f
fix control unit tets
zadjii-msft Oct 26, 2021
8c7ce77
fix the local tests too
zadjii-msft Oct 26, 2021
7c28851
this fixes the weird lock that we really really don't need anymore
zadjii-msft Oct 26, 2021
bf9bf0e
cleanup two of the remaining TODOs
zadjii-msft Oct 26, 2021
f087dd8
no more todos. Bot, tell me what I typod
zadjii-msft Oct 26, 2021
f111c6d
thanks spell bot
zadjii-msft Oct 26, 2021
51486a4
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Oct 28, 2021
6da5d79
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Nov 1, 2021
83e7aea
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Nov 2, 2021
d51c2cf
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Nov 10, 2021
9eeea4a
The simple PR feedback
zadjii-msft Nov 10, 2021
5173ea3
much of this appearance code was unneeded
zadjii-msft Nov 10, 2021
147101f
Whoops, needed this earlier
zadjii-msft Nov 10, 2021
5cd8096
Revert 5173ea30f58cc90e92bfc31d5ee5e0254eca38d9
zadjii-msft Nov 10, 2021
713f72e
5/6 transparency cases seem to work. Previewing with unfocused apprea…
zadjii-msft Nov 10, 2021
59c193c
okay, so it's just vintage(100)+cleartype that doesn't work. Everythi…
zadjii-msft Nov 10, 2021
6e8a2ad
MIKE YOU HAVE TEST CASES HERE
zadjii-msft Nov 10, 2021
88f2e64
Transparency is hard
zadjii-msft Nov 10, 2021
ae833a7
notes, because it's 5pm here
zadjii-msft Nov 10, 2021
2a18d7d
This is a checkpoint
zadjii-msft Nov 11, 2021
a9e706c
fix previewing, again
zadjii-msft Nov 11, 2021
1a7649c
turns out this _was_ overkill
zadjii-msft Nov 11, 2021
5adb327
rename this one too
zadjii-msft Nov 11, 2021
a338ca1
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Nov 11, 2021
8532dd6
fix conflicts with #11625
zadjii-msft Nov 11, 2021
7bb8975
more todos
zadjii-msft Nov 11, 2021
efdc090
fix the tests
zadjii-msft Nov 11, 2021
8313987
tests are hard
zadjii-msft Nov 11, 2021
a1bfa33
this was a nit
zadjii-msft Nov 11, 2021
b421ee6
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Nov 16, 2021
d675fd8
figure this out
zadjii-msft Nov 16, 2021
3acb374
Merge remote-tracking branch 'origin/main' into dev/migrie/oop/ragnarok
zadjii-msft Nov 30, 2021
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
8 changes: 6 additions & 2 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,12 @@ namespace SettingsModelLocalTests
TerminalSettings settings;
VERIFY_IS_NOT_NULL(settings);
auto oldFontSize = settings.FontSize();
settings.FontSize(oldFontSize + 5);
auto newFontSize = settings.FontSize();

auto selfSettings = winrt::make_self<implementation::TerminalSettings>();
VERIFY_ARE_EQUAL(oldFontSize, selfSettings->FontSize());

selfSettings->FontSize(oldFontSize + 5);
auto newFontSize = selfSettings->FontSize();
VERIFY_ARE_NOT_EQUAL(oldFontSize, newFontSize);
}

Expand Down
108 changes: 23 additions & 85 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ namespace TerminalAppLocalTests
TEST_METHOD(TestWindowRenameSuccessful);
TEST_METHOD(TestWindowRenameFailure);

TEST_METHOD(TestControlSettingsHasParent);
TEST_METHOD(TestPreviewCommitScheme);
TEST_METHOD(TestPreviewDismissScheme);
TEST_METHOD(TestPreviewSchemeWhilePreviewing);
Expand Down Expand Up @@ -134,10 +133,6 @@ namespace TerminalAppLocalTests
// Just creating it is enough to know that everything is working.
TerminalSettings settings;
VERIFY_IS_NOT_NULL(settings);
auto oldFontSize = settings.FontSize();
settings.FontSize(oldFontSize + 5);
auto newFontSize = settings.FontSize();
VERIFY_ARE_NOT_EQUAL(oldFontSize, newFontSize);
}

void TabTests::TryCreateConnectionType()
Expand Down Expand Up @@ -1297,25 +1292,6 @@ namespace TerminalAppLocalTests
});
}

void TabTests::TestControlSettingsHasParent()
{
Log::Comment(L"Ensure that when we create a control, it always has a parent TerminalSettings");

auto page = _commonSetup();
VERIFY_IS_NOT_NULL(page);

TestOnUIThread([&page]() {
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& originalSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);
});
}

void TabTests::TestPreviewCommitScheme()
{
Log::Comment(L"Preview a color scheme. Make sure it's applied, then committed accordingly");
Expand All @@ -1327,12 +1303,9 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& originalSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground());
});

Expand All @@ -1346,17 +1319,12 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& previewSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(previewSettings);

const auto& originalSettings = previewSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

Log::Comment(L"Color should be changed to the preview");
VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground());

// And we should have stored a function to revert the change.
VERIFY_ARE_EQUAL(1u, page->_restorePreviewFuncs.size());
});
Expand All @@ -1373,20 +1341,22 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& originalSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

const auto& grandparentSettings = originalSettings.GetParent();
VERIFY_IS_NULL(grandparentSettings);

Log::Comment(L"Color should be changed");
VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground());

// After preview there should be no more restore functions to execute.
VERIFY_ARE_EQUAL(0u, page->_restorePreviewFuncs.size());
});

Log::Comment(L"Sleep to let events propagate");
// If you don't do this, we will _sometimes_ crash as we're tearing down
// the control from this test as we start the next one. We crash
// somewhere in the CursorPositionChanged handler. It's annoying, but
// this works.
Sleep(250);
}

void TabTests::TestPreviewDismissScheme()
Expand All @@ -1400,12 +1370,9 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& originalSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground());
});

Expand All @@ -1419,15 +1386,9 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& previewSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(previewSettings);

const auto& originalSettings = previewSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

Log::Comment(L"Color should be changed to the preview");
VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground());
});
Expand All @@ -1441,18 +1402,14 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& originalSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

const auto& grandparentSettings = originalSettings.GetParent();
VERIFY_IS_NULL(grandparentSettings);

Log::Comment(L"Color should be the same as it originally was");
VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground());
});
Log::Comment(L"Sleep to let events propagate");
Sleep(250);
}

void TabTests::TestPreviewSchemeWhilePreviewing()
Expand All @@ -1468,12 +1425,9 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& originalSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

VERIFY_ARE_EQUAL(til::color{ 0xff0c0c0c }, controlSettings.DefaultBackground());
});

Expand All @@ -1487,15 +1441,9 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& previewSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(previewSettings);

const auto& originalSettings = previewSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

Log::Comment(L"Color should be changed to the preview");
VERIFY_ARE_EQUAL(til::color{ 0xff000000 }, controlSettings.DefaultBackground());
});
Expand All @@ -1510,15 +1458,9 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& previewSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(previewSettings);

const auto& originalSettings = previewSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

Log::Comment(L"Color should be changed to the preview");
VERIFY_ARE_EQUAL(til::color{ 0xffFAFAFA }, controlSettings.DefaultBackground());
});
Expand All @@ -1535,18 +1477,14 @@ namespace TerminalAppLocalTests
const auto& activeControl{ page->_GetActiveControl() };
VERIFY_IS_NOT_NULL(activeControl);

const auto& controlSettings = activeControl.Settings().as<TerminalSettings>();
const auto& controlSettings = activeControl.Settings();
VERIFY_IS_NOT_NULL(controlSettings);

const auto& originalSettings = controlSettings.GetParent();
VERIFY_IS_NOT_NULL(originalSettings);

const auto& grandparentSettings = originalSettings.GetParent();
VERIFY_IS_NULL(grandparentSettings);

Log::Comment(L"Color should be changed");
VERIFY_ARE_EQUAL(til::color{ 0xffFAFAFA }, controlSettings.DefaultBackground());
});
Log::Comment(L"Sleep to let events propagate");
Sleep(250);
}

void TabTests::TestClampSwitchToTab()
Expand Down
63 changes: 13 additions & 50 deletions src/cascadia/TerminalApp/ActionPreviewHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,12 @@ namespace winrt::TerminalApp::implementation
// - <none>
void TerminalPage::_EndPreviewColorScheme()
{
for (const auto& f : _restorePreviewFuncs)
// Apply the reverts in reverse order - If we had multiple previews
// stacked on top of each other, then this will ensure the first one in
// is the last one out.
for (auto i{ _restorePreviewFuncs.rbegin() }; i < _restorePreviewFuncs.rend(); i++)
{
auto f = *i;
Copy link
Member

Choose a reason for hiding this comment

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

nit: this does a copy whereas f->operator()() does not

f();
}
_restorePreviewFuncs.clear();
Expand All @@ -90,59 +94,18 @@ namespace winrt::TerminalApp::implementation
{
if (const auto& scheme{ _settings.GlobalSettings().ColorSchemes().TryLookup(args.SchemeName()) })
{
// Clear the saved preview funcs because we don't need to add a restore each time
// the preview color changes, we only need to be able to restore the last one.
_restorePreviewFuncs.clear();

_ApplyToActiveControls([&](const auto& control) {
// Get the settings of the focused control and stash them
const auto& controlSettings = control.Settings().as<TerminalSettings>();
// Make sure to recurse up to the root - if you're doing
// this while you're currently previewing a SetColorScheme
// action, then the parent of the control's settings is _the
// last preview TerminalSettings we inserted! We don't want
// to save that one!
auto originalSettings = controlSettings.GetParent();
while (originalSettings.GetParent() != nullptr)
{
originalSettings = originalSettings.GetParent();
}
// Create a new child for those settings
TerminalSettingsCreateResult fake{ originalSettings };
const auto& childStruct = TerminalSettings::CreateWithParent(fake);
// Modify the child to have the applied color scheme
childStruct.DefaultSettings().ApplyColorScheme(scheme);
// Stash a copy of the current scheme.
auto originalScheme{ control.ColorScheme() };

// Insert that new child as the parent of the control's settings
controlSettings.SetParent(childStruct.DefaultSettings());
control.UpdateSettings();
// Apply the new scheme.
control.ColorScheme(scheme.ToCoreScheme());

// Take a copy of the inputs, since they are pointers anyways.
// Each control will emplace a revert into the
// _restorePreviewFuncs for itself.
_restorePreviewFuncs.emplace_back([=]() {
// Get the runtime settings of the focused control
const auto& controlSettings{ control.Settings().as<TerminalSettings>() };

// Get the control's root settings, the ones that we actually
// assigned to it.
auto parentSettings{ controlSettings.GetParent() };
while (parentSettings.GetParent() != nullptr)
{
parentSettings = parentSettings.GetParent();
}

// If the root settings are the same as the ones we stashed,
// then reset the parent of the runtime settings to the stashed
// settings. This condition might be false if the settings
// hot-reloaded while the palette was open. In that case, we
// don't want to reset the settings to what they were _before_
// the hot-reload.
if (originalSettings == parentSettings)
{
// Set the original settings as the parent of the control's settings
control.Settings().as<TerminalSettings>().SetParent(originalSettings);
}

control.UpdateSettings();
// On dismiss, restore the original scheme.
control.ColorScheme(originalScheme);
});
});
}
Expand Down
23 changes: 1 addition & 22 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,28 +451,7 @@ namespace winrt::TerminalApp::implementation
if (const auto scheme = _settings.GlobalSettings().ColorSchemes().TryLookup(realArgs.SchemeName()))
{
const auto res = _ApplyToActiveControls([&](auto& control) {
// Start by getting the current settings of the control
auto controlSettings = control.Settings().as<TerminalSettings>();
auto parentSettings = controlSettings;
// Those are the _runtime_ settings however. What we
// need to do is:
//
// 1. Blow away any colors set in the runtime settings.
// 2. Apply the color scheme to the parent settings.
//
// 1 is important to make sure that the effects of
// something like `colortool` are cleared when setting
// the scheme.
if (controlSettings.GetParent() != nullptr)
{
parentSettings = controlSettings.GetParent();
}

// ApplyColorScheme(nullptr) will clear the old color scheme.
controlSettings.ApplyColorScheme(nullptr);
parentSettings.ApplyColorScheme(scheme);

control.UpdateSettings();
control.ColorScheme(scheme.ToCoreScheme());
});
args.Handled(res);
}
Expand Down
Loading