Change the ControlCore layer to own a copy of its settings

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp

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

src/cascadia/LocalTests_TerminalApp/TabTests.cpp

@@ -90,7 +90,6 @@ namespace TerminalAppLocalTests
         TEST_METHOD(TestWindowRenameSuccessful);
         TEST_METHOD(TestWindowRenameFailure);
 
-        TEST_METHOD(TestControlSettingsHasParent);
         TEST_METHOD(TestPreviewCommitScheme);
         TEST_METHOD(TestPreviewDismissScheme);
         TEST_METHOD(TestPreviewSchemeWhilePreviewing);
@@ -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()
@@ -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");
@@ -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());
         });
 
@@ -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());
         });
@@ -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()
@@ -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());
         });
 
@@ -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());
         });
@@ -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()
@@ -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());
         });
 
@@ -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());
         });
@@ -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());
         });
@@ -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()

src/cascadia/TerminalApp/ActionPreviewHandlers.cpp

@@ -95,54 +95,15 @@ namespace winrt::TerminalApp::implementation
             _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 original 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.
                 _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);
                 });
             });
         }

src/cascadia/TerminalApp/AppActionHandlers.cpp

@@ -452,28 +452,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);
             }

src/cascadia/TerminalApp/Pane.cpp

@@ -125,7 +125,7 @@ NewTerminalArgs Pane::GetTerminalArgsForPane() const
     assert(_IsLeaf());
 
     NewTerminalArgs args{};
-    auto controlSettings = _control.Settings().as<TerminalSettings>();
+    auto controlSettings = _control.Settings();
 
     args.Profile(controlSettings.ProfileName());
     // If we know the user's working directory use it instead of the profile.
@@ -156,16 +156,13 @@ NewTerminalArgs Pane::GetTerminalArgsForPane() const
         args.TabColor(winrt::Windows::Foundation::IReference<winrt::Windows::UI::Color>(c));
     }
 
-    if (controlSettings.AppliedColorScheme())
-    {
-        auto name = controlSettings.AppliedColorScheme().Name();
-        // Only save the color scheme if it is different than the profile color
-        // scheme to not override any other profile appearance choices.
-        if (_profile.DefaultAppearance().ColorSchemeName() != name)
-        {
-            args.ColorScheme(name);
-        }
-    }
+    // TODO:GH#9800 - we used to be able to persist the color scheme that a
+    // TermControl was initialized with, by name. With the change to having the
+    // control own its own copy of its settings, this isn't possible anymore.
+    //
+    // We may be able to get around this by storing the Name in the Core::Scheme
+    // object. That would work for schemes set by the Terminal, but not ones set
+    // by VT, but that seems good enough.
 
     return args;
 }
@@ -1459,21 +1456,8 @@ void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const Pr
     assert(_IsLeaf());
 
     _profile = profile;
-    auto controlSettings = _control.Settings().as<TerminalSettings>();
-    // Update the parent of the control's settings object (and not the object itself) so
-    // that any overrides made by the control don't get affected by the reload
-    controlSettings.SetParent(settings.DefaultSettings());
-    auto unfocusedSettings{ settings.UnfocusedSettings() };
-    if (unfocusedSettings)
-    {
-        // Note: the unfocused settings needs to be entirely unchanged _except_ we need to
-        // set its parent to the settings object that lives in the control. This is because
-        // the overrides made by the control live in that settings object, so we want to make
-        // sure the unfocused settings inherit from that.
-        unfocusedSettings.SetParent(controlSettings);
-    }
-    _control.UnfocusedAppearance(unfocusedSettings);
-    _control.UpdateSettings();
+
+    _control.UpdateControlSettings(settings.DefaultSettings(), settings.UnfocusedSettings());
 }
 
 // Method Description: