Fix for some profiles.defaults settings not working (#9764)

Fix for profiles.defaults.colorScheme not working Fix for background image only showing up after a settings reload Closes #9761
microsoft · Apr 12, 2021 · 7df4b3c · 7df4b3c
1 parent 8f79f7c
commit 7df4b3c
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 16 deletions.
diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp
@@ -306,13 +306,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // terminal.
         co_await winrt::resume_foreground(Dispatcher());
 
-        _UpdateSettingsFromUIThread(_settings);
+        // Take the lock before calling the helper functions to update the settings and appearance
+        auto lock = _terminal->LockForWriting();
+
+        _UpdateSettingsFromUIThreadUnderLock(_settings);
+
         auto appearance = _settings.try_as<IControlAppearance>();
         if (!_focused && _UnfocusedAppearance)
         {
             appearance = _UnfocusedAppearance;
         }
-        _UpdateAppearanceFromUIThread(appearance);
+        _UpdateAppearanceFromUIThreadUnderLock(appearance);
     }
 
     // Method Description:
@@ -323,7 +327,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     {
         // Dispatch a call to the UI thread
         co_await winrt::resume_foreground(Dispatcher());
-        _UpdateAppearanceFromUIThread(newAppearance);
+
+        // Take the lock before calling the helper function to update the appearance
+        auto lock = _terminal->LockForWriting();
+        _UpdateAppearanceFromUIThreadUnderLock(newAppearance);
     }
 
     // Method Description:
@@ -366,17 +373,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     //   issue that causes one of our hstring -> wstring_view conversions to result in garbage,
     //   but only from a coroutine context. See GH#8723.
     // - INVARIANT: This method must be called from the UI thread.
+    // - INVARIANT: This method can only be called if the caller has the writing lock on the terminal.
     // Arguments:
     // - newSettings: the new settings to set
-    void TermControl::_UpdateSettingsFromUIThread(IControlSettings newSettings)
+    void TermControl::_UpdateSettingsFromUIThreadUnderLock(IControlSettings newSettings)
     {
         if (_closing)
         {
             return;
         }
 
-        auto lock = _terminal->LockForWriting();
-
         // Update our control settings
         _ApplyUISettings(_settings);
 
@@ -420,10 +426,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
     // Method Description:
     // - Updates the appearance
-    // - This should only be called from the UI thread
+    // - INVARIANT: This method must be called from the UI thread.
+    // - INVARIANT: This method can only be called if the caller has the writing lock on the terminal.
     // Arguments:
     // - newAppearance: the new appearance to set
-    void TermControl::_UpdateAppearanceFromUIThread(IControlAppearance newAppearance)
+    void TermControl::_UpdateAppearanceFromUIThreadUnderLock(IControlAppearance newAppearance)
     {
         if (_closing)
         {
@@ -472,7 +479,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         TSFInputControl().Foreground(foregroundBrush);
 
         // Update the terminal core with its new Core settings
-        auto lock = _terminal->LockForWriting();
         _terminal->UpdateAppearance(newAppearance);
 
         // Update DxEngine settings under the lock
@@ -800,9 +806,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             const auto viewInPixels = Viewport::FromDimensions({ 0, 0 }, windowSize);
             LOG_IF_FAILED(dxEngine->SetWindowSize({ viewInPixels.Width(), viewInPixels.Height() }));
 
-            // Update DxEngine's SelectionBackground
-            dxEngine->SetSelectionBackground(til::color{ _settings.SelectionBackground() });
-
             const auto vp = dxEngine->GetViewportInCharacters(viewInPixels);
             const auto width = vp.Width();
             const auto height = vp.Height();
@@ -820,8 +823,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             // the first paint will be ignored!
             dxEngine->SetWarningCallback(std::bind(&TermControl::_RendererWarning, this, std::placeholders::_1));
 
-            dxEngine->SetRetroTerminalEffect(_settings.RetroTerminalEffect());
-            dxEngine->SetPixelShaderPath(_settings.PixelShaderPath());
             dxEngine->SetForceFullRepaintRendering(_settings.ForceFullRepaintRendering());
             dxEngine->SetSoftwareRendering(_settings.SoftwareRendering());
 
@@ -910,6 +911,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             //      becomes a no-op.
             this->Focus(FocusState::Programmatic);
 
+            // Now that the renderer is set up, update the appearance for initialization
+            _UpdateAppearanceFromUIThreadUnderLock(_settings);
+
             _initializedTerminal = true;
         } // scope for TerminalLock
 

diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h
@@ -198,8 +198,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;
 
-        void _UpdateSettingsFromUIThread(IControlSettings newSettings);
-        void _UpdateAppearanceFromUIThread(IControlAppearance newAppearance);
+        void _UpdateSettingsFromUIThreadUnderLock(IControlSettings newSettings);
+        void _UpdateAppearanceFromUIThreadUnderLock(IControlAppearance newAppearance);
         bool _isReadOnly{ false };
 
         void _ApplyUISettings(const IControlSettings&);

diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp
@@ -381,6 +381,9 @@ void Profile::_FinalizeInheritance()
 {
     if (auto defaultAppearanceImpl = get_self<AppearanceConfig>(_DefaultAppearance))
     {
+        // Clear any existing parents first, we don't want duplicates from any previous
+        // calls to this function
+        defaultAppearanceImpl->ClearParents();
         for (auto& parent : _parents)
         {
             if (auto parentDefaultAppearanceImpl = parent->_DefaultAppearance.try_as<AppearanceConfig>())