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.h

@@ -133,6 +133,11 @@ 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); }
 

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

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/NonClientIslandWindow.cpp

@@ -505,8 +505,8 @@ void NonClientIslandWindow::_OnMaximizeChange() noexcept
         const auto isIconified = WI_IsFlagSet(windowStyle, WS_ICONIC);
 
         const auto state = _isMaximized ? winrt::TerminalApp::WindowVisualState::WindowVisualStateMaximized :
-                           isIconified  ? winrt::TerminalApp::WindowVisualState::WindowVisualStateIconified :
-                                          winrt::TerminalApp::WindowVisualState::WindowVisualStateNormal;
+                                          isIconified ? winrt::TerminalApp::WindowVisualState::WindowVisualStateIconified :
+                                                        winrt::TerminalApp::WindowVisualState::WindowVisualStateNormal;
 
         try
         {