A pile of Theming nits fixes

src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp

@@ -47,6 +47,10 @@ namespace SettingsModelLocalTests
         {
             return Core::Color{ r, g, b, 255 };
         }
+        static Core::Color rgba(uint8_t r, uint8_t g, uint8_t b, uint8_t a) noexcept
+        {
+            return Core::Color{ r, g, b, a };
+        }
     };
 
     void ThemeTests::ParseSimpleTheme()
@@ -56,7 +60,7 @@ namespace SettingsModelLocalTests
             "tabRow":
             {
                 "background": "#FFFF8800",
-                "unfocusedBackground": "#FF884400"
+                "unfocusedBackground": "#FF8844"
             },
             "window":
             {
@@ -72,7 +76,8 @@ namespace SettingsModelLocalTests
         VERIFY_IS_NOT_NULL(theme->TabRow());
         VERIFY_IS_NOT_NULL(theme->TabRow().Background());
         VERIFY_ARE_EQUAL(Settings::Model::ThemeColorType::Color, theme->TabRow().Background().ColorType());
-        VERIFY_ARE_EQUAL(rgb(0xff, 0x88, 0x00), theme->TabRow().Background().Color());
+        VERIFY_ARE_EQUAL(rgba(0xff, 0xff, 0x88, 0x00), theme->TabRow().Background().Color());
+        VERIFY_ARE_EQUAL(rgba(0xff, 0x88, 0x44, 0xff), theme->TabRow().UnfocusedBackground().Color());
 
         VERIFY_IS_NOT_NULL(theme->Window());
         VERIFY_ARE_EQUAL(winrt::Windows::UI::Xaml::ElementTheme::Light, theme->Window().RequestedTheme());
@@ -101,7 +106,7 @@ namespace SettingsModelLocalTests
             "name": "noWindow",
             "tabRow":
             {
-                "background": "#FF112233",
+                "background": "#112233",
                 "unfocusedBackground": "#FF884400"
             },
         })" };
@@ -126,7 +131,7 @@ namespace SettingsModelLocalTests
             "name": "nullWindow",
             "tabRow":
             {
-                "background": "#FF112233",
+                "background": "#112233",
                 "unfocusedBackground": "#FF884400"
             },
             "window": null

src/cascadia/TerminalApp/AppLogic.cpp

@@ -958,9 +958,16 @@ namespace winrt::TerminalApp::implementation
     }
     CATCH_LOG()
 
+    // Method Description:
+    // - Update the current theme of the application. This will trigger our
+    //   RequestedThemeChanged event, to have our host change the theme of the
+    //   root of the application.
+    // Arguments:
+    // - newTheme: The ElementTheme to apply to our elements.
     void AppLogic::_RefreshThemeRoutine()
     {
-        _ApplyTheme(GetRequestedTheme());
+        // Propagate the event to the host layer, so it can update its own UI
+        _RequestedThemeChangedHandlers(*this, Theme());
     }
 
     // Function Description:
@@ -1069,18 +1076,6 @@ namespace winrt::TerminalApp::implementation
         return _settings;
     }
 
-    // Method Description:
-    // - Update the current theme of the application. This will trigger our
-    //   RequestedThemeChanged event, to have our host change the theme of the
-    //   root of the application.
-    // Arguments:
-    // - newTheme: The ElementTheme to apply to our elements.
-    void AppLogic::_ApplyTheme(const Windows::UI::Xaml::ElementTheme& newTheme)
-    {
-        // Propagate the event to the host layer, so it can update its own UI
-        _RequestedThemeChangedHandlers(*this, newTheme);
-    }
-
     UIElement AppLogic::GetRoot() noexcept
     {
         return _root.as<winrt::Windows::UI::Xaml::Controls::Control>();

src/cascadia/TerminalApp/AppLogic.h

@@ -133,10 +133,15 @@ namespace winrt::TerminalApp::implementation
 
         // -------------------------------- WinRT Events ---------------------------------
         // PropertyChanged is surprisingly not a typed event, so we'll define that one manually.
+        // Usually we'd just do
+        //    WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
+        //
+        // But what we're doing here is exposing the Page's PropertyChanged _as
+        // our own event_. It's a FORWARDED_CALLBACK, essentially.
         winrt::event_token PropertyChanged(Windows::UI::Xaml::Data::PropertyChangedEventHandler const& handler) { return _root->PropertyChanged(handler); }
         void PropertyChanged(winrt::event_token const& token) { _root->PropertyChanged(token); }
 
-        TYPED_EVENT(RequestedThemeChanged, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::ElementTheme);
+        TYPED_EVENT(RequestedThemeChanged, winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Settings::Model::Theme);
         TYPED_EVENT(SettingsChanged, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable);
         TYPED_EVENT(SystemMenuChangeRequested, winrt::Windows::Foundation::IInspectable, winrt::TerminalApp::SystemMenuChangeArgs);
 
@@ -191,8 +196,6 @@ namespace winrt::TerminalApp::implementation
         void _ReloadSettings();
         void _OpenSettingsUI();
 
-        void _ApplyTheme(const Windows::UI::Xaml::ElementTheme& newTheme);
-
         bool _hasCommandLineArguments{ false };
         bool _hasSettingsStartupActions{ false };
         std::vector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> _warnings;

src/cascadia/TerminalApp/AppLogic.idl

@@ -121,7 +121,7 @@ namespace TerminalApp
         event Windows.Foundation.TypedEventHandler<Object, Windows.UI.Xaml.UIElement> SetTitleBarContent;
         event Windows.Foundation.TypedEventHandler<Object, String> TitleChanged;
         event Windows.Foundation.TypedEventHandler<Object, LastTabClosedEventArgs> LastTabClosed;
-        event Windows.Foundation.TypedEventHandler<Object, Windows.UI.Xaml.ElementTheme> RequestedThemeChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Microsoft.Terminal.Settings.Model.Theme> RequestedThemeChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> FocusModeChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> FullscreenChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> ChangeMaximizeRequested;

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -4092,7 +4092,6 @@ namespace winrt::TerminalApp::implementation
 
         if (_settings.GlobalSettings().UseAcrylicInTabRow())
         {
-            const til::color backgroundColor = backgroundSolidBrush.Color();
             const auto acrylicBrush = Media::AcrylicBrush();
             acrylicBrush.BackgroundSource(Media::AcrylicBackgroundSource::HostBackdrop);
             acrylicBrush.FallbackColor(bgColor);
@@ -4101,27 +4100,25 @@ namespace winrt::TerminalApp::implementation
 
             TitlebarBrush(acrylicBrush);
         }
-        else if (theme.TabRow())
+        else if (const auto tabRowBg{ _activated ? theme.TabRow().Background() :
+                                                   theme.TabRow().UnfocusedBackground() };
+                 tabRowBg != nullptr && theme.TabRow() != nullptr)
         {
-            if (const auto tabRowBg{ _activated ? theme.TabRow().Background() :
-                                                  theme.TabRow().UnfocusedBackground() })
-            {
-                const auto terminalBrush = [this]() -> Media::Brush {
-                    if (const auto& control{ _GetActiveControl() })
-                    {
-                        return control.BackgroundBrush();
-                    }
-                    else if (auto settingsTab = _GetFocusedTab().try_as<TerminalApp::SettingsTab>())
-                    {
-                        return settingsTab.Content().try_as<Settings::Editor::MainPage>().BackgroundBrush();
-                    }
-                    return nullptr;
-                }();
+            const auto terminalBrush = [this]() -> Media::Brush {
+                if (const auto& control{ _GetActiveControl() })
+                {
+                    return control.BackgroundBrush();
+                }
+                else if (auto settingsTab = _GetFocusedTab().try_as<TerminalApp::SettingsTab>())
+                {
+                    return settingsTab.Content().try_as<Settings::Editor::MainPage>().BackgroundBrush();
+                }
+                return nullptr;
+            }();
 
-                const auto themeBrush{ tabRowBg.Evaluate(res, terminalBrush, true) };
-                bgColor = ThemeColor::ColorFromBrush(themeBrush);
-                TitlebarBrush(themeBrush);
-            }
+            const auto themeBrush{ tabRowBg.Evaluate(res, terminalBrush, true) };
+            bgColor = ThemeColor::ColorFromBrush(themeBrush);
+            TitlebarBrush(themeBrush);
         }
         else
         {

src/cascadia/TerminalApp/TitlebarControl.cpp

@@ -8,6 +8,8 @@
 
 #include "TitlebarControl.h"
 
+#include "ColorHelper.h"
+
 #include "TitlebarControl.g.cpp"
 
 namespace winrt::TerminalApp::implementation
@@ -21,6 +23,25 @@ namespace winrt::TerminalApp::implementation
         MinMaxCloseControl().MinimizeClick({ this, &TitlebarControl::Minimize_Click });
         MinMaxCloseControl().MaximizeClick({ this, &TitlebarControl::Maximize_Click });
         MinMaxCloseControl().CloseClick({ this, &TitlebarControl::Close_Click });
+
+        // Listen for changes to the Background. If the Background changes,
+        // we'll want to manually adjust the RequestedTheme of our caption
+        // buttons, so the foreground stands out against whatever BG color was
+        // selected for us.
+        //
+        // This is how you register a PropertyChanged event for the Background
+        // property of a Grid. The Background property is defined in the base
+        // class Panel.
+        const auto bgProperty{ winrt::Windows::UI::Xaml::Controls::Panel::BackgroundProperty() };
+        RegisterPropertyChangedCallback(bgProperty, [weakThis = get_weak(), bgProperty](auto& /*sender*/, auto& e) {
+            if (auto self{ weakThis.get() })
+            {
+                if (e == bgProperty)
+                {
+                    self->_backgroundChanged(self->Background());
+                }
+            }
+        });
     }
 
     double TitlebarControl::CaptionButtonWidth()
@@ -144,4 +165,26 @@ namespace winrt::TerminalApp::implementation
         MinMaxCloseControl().ReleaseButtons();
     }
 
+    void TitlebarControl::_backgroundChanged(winrt::Windows::UI::Xaml::Media::Brush brush)
+    {
+        // Loosely cribbed from TerminalPage::_SetNewTabButtonColor
+        til::color c;
+        if (auto acrylic = brush.try_as<winrt::Windows::UI::Xaml::Media::AcrylicBrush>())
+        {
+            c = acrylic.TintColor();
+        }
+        else if (auto solidColor = brush.try_as<winrt::Windows::UI::Xaml::Media::SolidColorBrush>())
+        {
+            c = solidColor.Color();
+        }
+        else
+        {
+            return;
+        }
+
+        const auto isBrightColor = ColorHelper::IsBrightColor(c);
+        MinMaxCloseControl().RequestedTheme(isBrightColor ? winrt::Windows::UI::Xaml::ElementTheme::Light :
+                                                            winrt::Windows::UI::Xaml::ElementTheme::Dark);
+    }
+
 }

src/cascadia/TerminalApp/TitlebarControl.h

@@ -31,6 +31,8 @@ namespace winrt::TerminalApp::implementation
     private:
         void _OnMaximizeOrRestore(byte flag);
         HWND _window{ nullptr }; // non-owning handle; should not be freed in the dtor.
+
+        void _backgroundChanged(winrt::Windows::UI::Xaml::Media::Brush brush);
     };
 }
 

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -41,6 +41,10 @@ static constexpr std::string_view ProfilesListKey{ "list" };
 static constexpr std::string_view SchemesKey{ "schemes" };
 static constexpr std::string_view ThemesKey{ "themes" };
 
+constexpr std::wstring_view systemThemeName{ L"system" };
+constexpr std::wstring_view darkThemeName{ L"dark" };
+constexpr std::wstring_view lightThemeName{ L"light" };
+
 static constexpr std::wstring_view jsonExtension{ L".json" };
 static constexpr std::wstring_view FragmentsSubDirectory{ L"\\Fragments" };
 static constexpr std::wstring_view FragmentsPath{ L"\\Microsoft\\Windows Terminal\\Fragments" };
@@ -538,7 +542,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
             if (const auto theme = Theme::FromJson(themeJson))
             {
                 if (origin != OriginTag::InBox &&
-                    (theme->Name() == L"system" || theme->Name() == L"light" || theme->Name() == L"dark"))
+                    (theme->Name() == systemThemeName || theme->Name() == lightThemeName || theme->Name() == darkThemeName))
                 {
                     // If the theme didn't come from the in-box themes, and its
                     // name was one of the reserved names, then just ignore it.
@@ -1119,7 +1123,7 @@ Json::Value CascadiaSettings::ToJson() const
         // Ignore the built in themes, when serializing the themes back out. We
         // don't want to re-include them in the user settings file.
         const auto theme{ winrt::get_self<Theme>(entry.Value()) };
-        if (theme->Name() == L"system" || theme->Name() == L"light" || theme->Name() == L"dark")
+        if (theme->Name() == systemThemeName || theme->Name() == lightThemeName || theme->Name() == darkThemeName)
         {
             continue;
         }

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

@@ -558,18 +558,21 @@ JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage)
 template<>
 struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt::Microsoft::Terminal::Settings::Model::ThemeColor>
 {
+    static constexpr std::string_view accentString{ "accent" };
+    static constexpr std::string_view terminalBackgroundString{ "terminalBackground" };
+
     winrt::Microsoft::Terminal::Settings::Model::ThemeColor FromJson(const Json::Value& json)
     {
         if (json == Json::Value::null)
         {
             return nullptr;
         }
         const auto string{ Detail::GetStringView(json) };
-        if (string == "accent")
+        if (string == accentString)
         {
             return winrt::Microsoft::Terminal::Settings::Model::ThemeColor::FromAccent();
         }
-        else if (string == "terminalBackground")
+        else if (string == terminalBackgroundString)
         {
             return winrt::Microsoft::Terminal::Settings::Model::ThemeColor::FromTerminalBackground();
         }
@@ -592,8 +595,8 @@ struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt:
 
         const auto string{ Detail::GetStringView(json) };
         const auto isColorSpec = (string.length() == 9 || string.length() == 7 || string.length() == 4) && string.front() == '#';
-        const auto isAccent = string == "accent";
-        const auto isTerminalBackground = string == "terminalBackground";
+        const auto isAccent = string == accentString;
+        const auto isTerminalBackground = string == terminalBackgroundString;
         return isColorSpec || isAccent || isTerminalBackground;
     }
 
@@ -624,7 +627,7 @@ struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt:
 
     std::string TypeDescription() const
     {
-        return "ThemeColor (#rrggbb, #rgb, #aarrggbb, accent, terminalBackground)";
+        return "ThemeColor (#rrggbb, #rgb, #rrggbbaa, accent, terminalBackground)";
     }
 };
 

src/cascadia/TerminalSettingsModel/Theme.cpp

@@ -266,21 +266,6 @@ winrt::com_ptr<Theme> Theme::FromJson(const Json::Value& json)
 {
     auto result = winrt::make_self<Theme>();
 
-    if (json.isString())
-    {
-        // We found a string, not an object. Just secretly promote that string
-        // to a theme object with just the applicationTheme set from that value.
-        JsonUtils::GetValue(json, result->_Name);
-        winrt::WUX::ElementTheme requestedTheme{ winrt::WUX::ElementTheme::Default };
-        JsonUtils::GetValue(json, requestedTheme);
-
-        auto window{ winrt::make_self<implementation::WindowTheme>() };
-        window->RequestedTheme(requestedTheme);
-        result->_Window = *window;
-
-        return result;
-    }
-
     JsonUtils::GetValueForKey(json, NameKey, result->_Name);
 
     // This will use each of the ConversionTrait's from above to quickly parse the sub-objects

src/cascadia/TerminalSettingsModel/Theme.h

@@ -96,6 +96,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
 #undef THEME_SETTINGS_INITIALIZE
 #undef THEME_SETTINGS_COPY
+#undef COPY_THEME_OBJECT
 #undef THEME_OBJECT
 }
 

src/cascadia/WindowsTerminal/AppHost.cpp

@@ -719,7 +719,8 @@ void AppHost::_UpdateTitleBarContent(const winrt::Windows::Foundation::IInspecta
 // - arg: the ElementTheme to use as the new theme for the UI
 // Return Value:
 // - <none>
-void AppHost::_UpdateTheme(const winrt::Windows::Foundation::IInspectable&, const winrt::Windows::UI::Xaml::ElementTheme& /*arg*/)
+void AppHost::_UpdateTheme(const winrt::Windows::Foundation::IInspectable&,
+                           const winrt::Microsoft::Terminal::Settings::Model::Theme& /*arg*/)
 {
     _updateTheme();
 }

src/cascadia/WindowsTerminal/AppHost.h

@@ -44,7 +44,7 @@ class AppHost
     void _UpdateTitleBarContent(const winrt::Windows::Foundation::IInspectable& sender,
                                 const winrt::Windows::UI::Xaml::UIElement& arg);
     void _UpdateTheme(const winrt::Windows::Foundation::IInspectable&,
-                      const winrt::Windows::UI::Xaml::ElementTheme& arg);
+                      const winrt::Microsoft::Terminal::Settings::Model::Theme& arg);
     void _FocusModeChanged(const winrt::Windows::Foundation::IInspectable& sender,
                            const winrt::Windows::Foundation::IInspectable& arg);
     void _FullscreenChanged(const winrt::Windows::Foundation::IInspectable& sender,