Skip to content

Commit

Permalink
Change the ControlCore layer to own a copy of its settings (#11619)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request

Currently, the TermControl and ControlCore recieve a settings object that implements `IControlSettings`. They use for this for both reading the settings they should use, and also storing some runtime overrides to those settings (namely, `Opacity`). The object they recieve currently is a `T.S.M.TerminalSettings` object, as well as another `TerminalSettings` object if the user wants to have an `unfocusedAppearance`. All these are all hosted in the same process, so everything is fine and dandy. 

With the upcoming move to having the Terminal split into multiple processes, this will no longer work. If the `ControlCore` in the Content Process is given a pointer to a `TerminalSettings` in a certain Window Process, and that control is subsequently moved to another window, then there's no guarantee that the original `TerminalSettings` object continues to exist. In this scenario, when window 1 is closed, now the Core is unable to read any settings, because the process that owned that object no longer exists. 

The solution to this issue is to have the `ControlCore`'s own their own copy of the settings they were created with. that way, they can be confident those settings will always exist. Enter `ControlSettings`, a dumb struct for just storing all the contents of the Settings. I used x-macros for this, so that we don't need to copy-paste into this file every time we add a setting. 

Changing this has all sorts of other fallout effects:
* Previewing a scheme/anything is a tad bit more annoying. Before, we could just sneak the previewed scheme into a `TerminalSettings` that lived between the settings we created the control with, and the settings they were actually using, and it would _just work_. Even explaining that here, it sounds like magic, because it was. However, now, the TermControl can't use a layered `TerminalSettings` for the settings anymore. Now we need to actually read out the current color table, and set the whole scheme when we change it. So now there's also a `Microsoft.Terminal.Core.Scheme` _struct_ for holding that data. 
  - Why a `struct`? Because that will go across the process boundary as a blob, rather than as a pointer to an object in the other process. That way we can transit the whole struct from window to core safely. 
* A TermControl doesn't have a `IControlSettings` at all anymore - it initalizes itself via the settings in the `Core`. This will be useful for tear-out, when we need to have the `TermControl` initialize itself from just a `ControlCore`, without being able to rebuild the settings from scratch.
* The `TabTests` that were written under the assumption that the Control had a layered `TerminalSettings` obviously broke, as they were designed to. They've been modified to reflect the new reality.
* When we initialize the Control, we give it the settings and the `UnfocusedAppearance` all at once. If we don't give it an `unfocusedAppearance`, it will just use the focused appearance as the unfocused appearance.
* The Control no longer can _write_ settings to the `ControlSettings`. We don't want to be storing things in there. Pretty much everything we set in the control, we store somewhere other than in the settings object itself. However, `opacity` and `useAcrylic`, we need to store in a handy new `RUNTIME_SETTING` property. We can write those runtime overrides to those properties.  
* We no longer store the color scheme for a pane in the persisted state. I'm tracking that in #9800. I don't think it's too hard to add back, but I wanted this in front of eyes sooner than later.

## References

* #1256
* #5000
* #9794 has the scheme previewing in it.
* #9818 is WAY more possible now.

## PR Checklist
* [x] Surprisingly there wasn't ever a card or issue for this one. This was only ever a bullet point in #5000. 
* A bunch of these issues were fixed along the way, though I never intended to fix them:
  * [x] Closes #11571
  * [x] Closes #11586
  * [x] Closes #7219
  * [x] Closes #11067
  * [x] I think #11623 actually ended up resolving this one, but I'm double tapping on it here: Closes #5703
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Along the way I tried to clean up code where possible, but not too agressively. 

I didn't end up converting the various `MockTerminalSettings` classes used in tests to the x macros quite yet. I wanted to merge this with #11416 in `main` before I went too crazy.

## Validation Steps Performed

* [x] Scheme previewing works
* [x] Adjusting the font size works
* [x] focused/unfocused appearances still work
* [x] mouse-wheeling opacity still works
* [x] acrylic & cleartype still does the right thing
* [x] saving the settings still works
* [x] going wild on sliding the opacity slider in the settings doesn't crash the terminal
* [x] toggling retro effects with a keybinding still works
* [x] toggling retro effects with the command palette works
* [x] The matrix of (`useAcrylic(true,false)`)x(`opacity(50,100)`)x(`antialiasingMode(cleartype, grayscale)`) works as expected. Slightly changed, falls back to grayscale more often, but looks more right.
  • Loading branch information
@zadjii-msft
zadjii-msft authored Dec 1, 2021
1 parent 6f3464d commit 094273b
Show file tree
Hide file tree
Showing 39 changed files with 871 additions and 500 deletions.
8 changes: 6 additions & 2 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
View file
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
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
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;
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
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

0 comments on commit 094273b

Please sign in to comment.