starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

nits from themes PR #17883

New Issue
Closed
opened 2026-01-31 05:57:21 +00:00 by claunia · 5 comments
claunia commented 2026-01-31 05:57:21 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Jul 7, 2022).

see #12992

  • In src/cascadia/TerminalSettingsModel/Theme.idl:
> +        Color,
+        TerminalBackground
+    };
+
+    runtimeclass ThemeColor
+    {
+        ThemeColor();
+        static ThemeColor FromColor(Microsoft.Terminal.Core.Color color);
+        static ThemeColor FromAccent();
+        static ThemeColor FromTerminalBackground();
+
+        Microsoft.Terminal.Core.Color Color { get; };
+        ThemeColorType ColorType;
+
+        static Microsoft.Terminal.Core.Color ColorFromBrush(Windows.UI.Xaml.Media.Brush brush);
+        Windows.UI.Xaml.Media.Brush Evaluate(Windows.UI.Xaml.ResourceDictionary res,

XAML in the settings model -- this will cause trouble for a consumer like the shell extension in the future.

  • In src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp:
> +        // Surprisingly, though this is called every time we navigate to the page,
+        // the list does not keep growing on each navigation.

This comment has a VERY threatening aura! I am almost certain it will break at some point...

  • In src/cascadia/TerminalApp/AppLogic.cpp:
> @@ -964,7 +960,7 @@ namespace winrt::TerminalApp::implementation
 
     void AppLogic::_RefreshThemeRoutine()
     {
-        _ApplyTheme(_settings.GlobalSettings().Theme());
+        _ApplyTheme(GetRequestedTheme());

I feel like ApplyTheme should take a Theme ;P

  • In src/cascadia/TerminalApp/TerminalPage.cpp:
> +        //
+        // This helper allows us to instead lookup the value of a resource
+        // specified by `key` for the given `requestedTheme`, from the
+        // dictionaries in App.xaml. Make sure the value is actually there!
+        // Otherwise this'll throw like any other Lookup for a resource that
+        // isn't there.
+        static const auto lookup = [](auto& res, auto& requestedTheme, auto& key) {
+            // You want the Default version of the resource? Great, the App is
+            // always in the OS theme. Just look it up and be done.
+            if (requestedTheme == ElementTheme::Default)
+            {
+                return res.Lookup(key);
+            }
+            static const auto lightKey = winrt::box_value(L"Light");
+            static const auto darkKey = winrt::box_value(L"Dark");
+            // There isn't an ElementTheme::HighContrast.

so... what exactly happens in HC then?

  • In src/cascadia/TerminalSettingsModel/Theme.cpp:
> +        {                                                                                      \
+            return json.isObject();                                                            \
+        }                                                                                      \
+                                                                                               \
+        Json::Value ToJson(const nameSpace::name& val)                                         \
+        {                                                                                      \
+            if (val == nullptr)                                                                \
+                return Json::Value::null;                                                      \
+            Json::Value json{ Json::ValueType::objectValue };                                  \
+            macro(THEME_SETTINGS_TO_JSON);                                                     \
+            return json;                                                                       \
+        }                                                                                      \
+                                                                                               \
+        std::string TypeDescription() const                                                    \
+        {                                                                                      \
+            return "name (You should never see this)";                                         \

lol this will actually say the word "name" since it's not in a macro-stringify expression

resolved ________________________________________ * [x] Does this work with background images?

Yep, sure does.

  • It looks insane to have a light terminal BG titlebar, and in dark OS theme - the caption btns are also white:

image

  • In src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp:
> +                "background": "#FFFF8800",
+                "unfocusedBackground": "#FF884400"

wait but utils parses RGBA
how is this test passing?

wait, i am so confused

  • In src/cascadia/TerminalSettingsModel/Theme.h:
> +
+        static com_ptr<Theme> FromJson(const Json::Value& json);
+        void LayerJson(const Json::Value& json);
+        Json::Value ToJson() const;
+
+        winrt::Windows::UI::Xaml::ElementTheme RequestedTheme() const noexcept;
+
+        WINRT_PROPERTY(winrt::hstring, Name);
+
+        MTSM_THEME_SETTINGS(THEME_SETTINGS_INITIALIZE)
+
+    private:
+    };
+
+#undef THEME_SETTINGS_INITIALIZE
+#undef THEME_SETTINGS_COPY

do we need to undef COPY_THEME...?

  • In src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h:
> +        }
+        case winrt::Microsoft::Terminal::Settings::Model::ThemeColorType::Color:
+        {
+            return til::u16u8(til::color{ val.Color() }.ToHexString(false));
+        }
+        case winrt::Microsoft::Terminal::Settings::Model::ThemeColorType::TerminalBackground:
+        {
+            return "terminalBackground";
+        }
+        }
+        return til::u16u8(til::color{ val.Color() }.ToHexString(false));
+    }
+
+    std::string TypeDescription() const
+    {
+        return "ThemeColor (#rrggbb, #rgb, #aarrggbb, accent, terminalBackground)";

isn't it rrggbbaa?

  • In src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h:
> @@ -547,6 +547,79 @@ JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage)
     };
 };
 
+template<>
+struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt::Microsoft::Terminal::Settings::Model::ThemeColor>
+{
+    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")

nit: prefer declaring these magic constant values in the class body and referencing them later

  • In src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp:
> @@ -1092,6 +1113,20 @@ Json::Value CascadiaSettings::ToJson() const
     }
     json[JsonKey(SchemesKey)] = schemes;
 
+    Json::Value themes{ Json::ValueType::arrayValue };
+    for (const auto& entry : _globals->Themes())
+    {
+        // Ignore the built in themes, when serializing the themes back out. We

implicit contract: users cannot overwrite system themes

  • In src/cascadia/TerminalApp/AppLogic.h:
>          // -------------------------------- WinRT Events ---------------------------------
+        // PropertyChanged is surprisingly not a typed event, so we'll define that one manually.

W A T

do we do that everywhere? if not, why not? Should we have done here what we do elsewhere?

  • In src/cascadia/TerminalSettingsEditor/GlobalAppearance.idl:
> @@ -21,7 +21,8 @@ namespace Microsoft.Terminal.Settings.Editor
         IInspectable CurrentLanguage;
 
         IInspectable CurrentTheme;
-        Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Editor.EnumEntry> ThemeList { get; };
+        static String ThemeNameConverter(Microsoft.Terminal.Settings.Model.Theme theme);
+        Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Model.Theme> ThemeList { get; };

@PankajBhojwani FYI since this will impact MVVM -- this is a raw exposure of a settings model object

  • In src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp:
> +    constexpr std::wstring_view systemThemeName{ L"system" };
+    constexpr std::wstring_view darkThemeName{ L"dark" };
+    constexpr std::wstring_view lightThemeName{ L"light" };

these became global constants in TSE, but not in TSM -- curious!

  • In src/cascadia/TerminalApp/TerminalPage.cpp:
> +        _activated = activated;
+        _updateTabRowColors();
+    }

so _activated is ignored?

  • In src/cascadia/TerminalSettingsModel/Theme.cpp:
> +    HKEY hKey{ nullptr };
+    if (RegOpenKeyEx(HKEY_CURRENT_USER, RegKeyDwm, 0, KEY_READ, &hKey) == ERROR_SUCCESS)
+    {
+        return wil::unique_hkey{ hKey };
+    }
+    return nullptr;
+}
+static DWORD readDwmSubValue(const wil::unique_hkey& dwmRootKey, const wchar_t* key)
+{
+    DWORD val{ 0 };
+    DWORD size{ sizeof(val) };
+    LOG_IF_FAILED(RegQueryValueExW(dwmRootKey.get(), key, nullptr, nullptr, reinterpret_cast<BYTE*>(&val), &size));
+    return val;
+}
+
+static til::color _getAccentColorForTitlebar()

do we reload this when the accent color changes?

  • In src/cascadia/TerminalSettingsModel/Theme.cpp:
> +    }
+
+    return theme;
+}
+
+// Method Description:
+// - Create a new instance of this class from a serialized JsonObject.
+// Arguments:
+// - json: an object which should be a serialization of a ColorScheme object.
+// Return Value:
+// - Returns nullptr for invalid JSON.
+winrt::com_ptr<Theme> Theme::FromJson(const Json::Value& json)
+{
+    auto result = winrt::make_self<Theme>();
+
+    if (json.isString())

we control the entire themes array, from its inception to its death. Who would put a string in it?

  • In src/cascadia/TerminalApp/TerminalPage.cpp:
> +                    if (winrt::unbox_value<winrt::hstring>(dictionaryKey) !=
+                        winrt::unbox_value<winrt::hstring>(requestedThemeKey))

but do we need users to crash if this somehow happens in production?

  • In src/cascadia/TerminalApp/TerminalPage.cpp:
> +                }
+            }
+
+            // We didn't find it in the requested dict, fall back to the default dictionary.
+            return res.Lookup(key);
+        };
+
+        // Use our helper to lookup the theme-aware version of the resource.
+        const auto tabViewBackgroundKey = winrt::box_value(L"TabViewBackground");
+        const auto backgroundSolidBrush = lookup(res, requestedTheme, tabViewBackgroundKey).as<Media::SolidColorBrush>();
+
+        til::color bgColor = backgroundSolidBrush.Color();
+
+        if (_settings.GlobalSettings().UseAcrylicInTabRow())
+        {
+            const til::color backgroundColor = backgroundSolidBrush.Color();

You marked it as resolved... but the line of code is still here.

follow ups ________________________________________ * [ ] In `src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp`: ```diff > @@ -531,6 +532,25 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source } }
  • {
  •    for (const auto& themeJson : json.themes)

  •    {

  •        if (const auto theme = Theme::FromJson(themeJson))

  •        {

  •            if (origin != OriginTag::InBox &&

follow up: we should add an origin tag to themes, after #12800 lands.
________________________________________
* [ ] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml`:
```diff
>                            ItemsSource="{x:Bind ThemeList, Mode=OneWay}"
                           SelectedItem="{x:Bind CurrentTheme, Mode=TwoWay}"
-                          Style="{StaticResource ComboBoxSettingStyle}" />
+                          Style="{StaticResource ComboBoxSettingStyle}">
+                    <ComboBox.ItemTemplate>
+                        <DataTemplate x:DataType="model:Theme">
+                            <TextBlock Text="{x:Bind local:GlobalAppearance.ThemeNameConverter((model:Theme)), Mode=OneWay}" />

@carlos-zamora, can you make sure this is still accessible? we have had issues with putting custom data templates into combo boxes.

  • In src/types/utils.cpp:
>      }
-    else
+    else if (str.size() == 7)
+    {
+        rStr = std::string(&str.at(1), 2);
+        gStr = std::string(&str.at(3), 2);
+        bStr = std::string(&str.at(5), 2);
+        aStr = "ff";

I don't love that we force a string parser to parse ff just to get 255... ;P

Originally created by @zadjii-msft on GitHub (Jul 7, 2022). see #12992 ________________________________________ * [ ] In `src/cascadia/TerminalSettingsModel/Theme.idl`: ```diff > + Color, + TerminalBackground + }; + + runtimeclass ThemeColor + { + ThemeColor(); + static ThemeColor FromColor(Microsoft.Terminal.Core.Color color); + static ThemeColor FromAccent(); + static ThemeColor FromTerminalBackground(); + + Microsoft.Terminal.Core.Color Color { get; }; + ThemeColorType ColorType; + + static Microsoft.Terminal.Core.Color ColorFromBrush(Windows.UI.Xaml.Media.Brush brush); + Windows.UI.Xaml.Media.Brush Evaluate(Windows.UI.Xaml.ResourceDictionary res, ``` XAML in the settings model -- this will cause trouble for a consumer like the shell extension in the future. * [ ] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp`: ```diff > + // Surprisingly, though this is called every time we navigate to the page, + // the list does not keep growing on each navigation. ``` This comment has a VERY threatening aura! I am almost certain it will break at some point... _____________ * [x] In `src/cascadia/TerminalApp/AppLogic.cpp`: ```diff > @@ -964,7 +960,7 @@ namespace winrt::TerminalApp::implementation void AppLogic::_RefreshThemeRoutine() { - _ApplyTheme(_settings.GlobalSettings().Theme()); + _ApplyTheme(GetRequestedTheme()); ``` I feel like ApplyTheme should take a Theme ;P ________________________________________ * [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`: ```diff > + // + // This helper allows us to instead lookup the value of a resource + // specified by `key` for the given `requestedTheme`, from the + // dictionaries in App.xaml. Make sure the value is actually there! + // Otherwise this'll throw like any other Lookup for a resource that + // isn't there. + static const auto lookup = [](auto& res, auto& requestedTheme, auto& key) { + // You want the Default version of the resource? Great, the App is + // always in the OS theme. Just look it up and be done. + if (requestedTheme == ElementTheme::Default) + { + return res.Lookup(key); + } + static const auto lightKey = winrt::box_value(L"Light"); + static const auto darkKey = winrt::box_value(L"Dark"); + // There isn't an ElementTheme::HighContrast. ``` so... what exactly happens in HC then? ________________________________________ * [ ] In `src/cascadia/TerminalSettingsModel/Theme.cpp`: ```diff > + { \ + return json.isObject(); \ + } \ + \ + Json::Value ToJson(const nameSpace::name& val) \ + { \ + if (val == nullptr) \ + return Json::Value::null; \ + Json::Value json{ Json::ValueType::objectValue }; \ + macro(THEME_SETTINGS_TO_JSON); \ + return json; \ + } \ + \ + std::string TypeDescription() const \ + { \ + return "name (You should never see this)"; \ ``` lol this will actually say the word "name" since it's not in a macro-stringify expression <details> <summary>resolved</summary> ________________________________________ * [x] Does this work with background images? Yep, sure does. ________________________________________ * [x] It looks insane to have a light terminal BG titlebar, and in dark OS theme - the caption btns are also white: ![image](https://user-images.githubusercontent.com/18356694/177865865-8c1fc66f-05fd-4680-9c27-93e7f63bfac8.png) ________________________________________ * [x] In `src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp`: ```diff > + "background": "#FFFF8800", + "unfocusedBackground": "#FF884400" ``` wait but utils parses RGBA how is this test passing? wait, i am so confused ___________________________ * [x] In `src/cascadia/TerminalSettingsModel/Theme.h`: ```diff > + + static com_ptr<Theme> FromJson(const Json::Value& json); + void LayerJson(const Json::Value& json); + Json::Value ToJson() const; + + winrt::Windows::UI::Xaml::ElementTheme RequestedTheme() const noexcept; + + WINRT_PROPERTY(winrt::hstring, Name); + + MTSM_THEME_SETTINGS(THEME_SETTINGS_INITIALIZE) + + private: + }; + +#undef THEME_SETTINGS_INITIALIZE +#undef THEME_SETTINGS_COPY ``` do we need to undef COPY_THEME...? ________________________________________ * [x] In `src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h`: ```diff > + } + case winrt::Microsoft::Terminal::Settings::Model::ThemeColorType::Color: + { + return til::u16u8(til::color{ val.Color() }.ToHexString(false)); + } + case winrt::Microsoft::Terminal::Settings::Model::ThemeColorType::TerminalBackground: + { + return "terminalBackground"; + } + } + return til::u16u8(til::color{ val.Color() }.ToHexString(false)); + } + + std::string TypeDescription() const + { + return "ThemeColor (#rrggbb, #rgb, #aarrggbb, accent, terminalBackground)"; ``` isn't it rrggbbaa? ________________________________________ * [x] In `src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h`: ```diff > @@ -547,6 +547,79 @@ JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage) }; }; +template<> +struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt::Microsoft::Terminal::Settings::Model::ThemeColor> +{ + 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") ``` nit: prefer declaring these magic constant values in the class body and referencing them later ________________________________________ * [x] In `src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp`: ```diff > @@ -1092,6 +1113,20 @@ Json::Value CascadiaSettings::ToJson() const } json[JsonKey(SchemesKey)] = schemes; + Json::Value themes{ Json::ValueType::arrayValue }; + for (const auto& entry : _globals->Themes()) + { + // Ignore the built in themes, when serializing the themes back out. We ``` implicit contract: users cannot overwrite system themes ________________________________________ * [x] In `src/cascadia/TerminalApp/AppLogic.h`: ```diff > // -------------------------------- WinRT Events --------------------------------- + // PropertyChanged is surprisingly not a typed event, so we'll define that one manually. ``` W A T do we do that everywhere? if not, why not? Should we have done here what we do elsewhere? ________________________________________ * [x] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.idl`: ```diff > @@ -21,7 +21,8 @@ namespace Microsoft.Terminal.Settings.Editor IInspectable CurrentLanguage; IInspectable CurrentTheme; - Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Editor.EnumEntry> ThemeList { get; }; + static String ThemeNameConverter(Microsoft.Terminal.Settings.Model.Theme theme); + Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Model.Theme> ThemeList { get; }; ``` @PankajBhojwani FYI since this will impact MVVM -- this is a raw exposure of a settings model object ________________________________________ * [x] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp`: ```diff > + constexpr std::wstring_view systemThemeName{ L"system" }; + constexpr std::wstring_view darkThemeName{ L"dark" }; + constexpr std::wstring_view lightThemeName{ L"light" }; ``` these became global constants in TSE, but not in TSM -- curious! ________________________________________ * [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`: ```diff > + _activated = activated; + _updateTabRowColors(); + } ``` so _activated is ignored? ________________________________________ * [x] In `src/cascadia/TerminalSettingsModel/Theme.cpp`: ```diff > + HKEY hKey{ nullptr }; + if (RegOpenKeyEx(HKEY_CURRENT_USER, RegKeyDwm, 0, KEY_READ, &hKey) == ERROR_SUCCESS) + { + return wil::unique_hkey{ hKey }; + } + return nullptr; +} +static DWORD readDwmSubValue(const wil::unique_hkey& dwmRootKey, const wchar_t* key) +{ + DWORD val{ 0 }; + DWORD size{ sizeof(val) }; + LOG_IF_FAILED(RegQueryValueExW(dwmRootKey.get(), key, nullptr, nullptr, reinterpret_cast<BYTE*>(&val), &size)); + return val; +} + +static til::color _getAccentColorForTitlebar() ``` do we reload this when the accent color changes? ________________________________________ ________________________________________ * [x] In `src/cascadia/TerminalSettingsModel/Theme.cpp`: ```diff > + } + + return theme; +} + +// Method Description: +// - Create a new instance of this class from a serialized JsonObject. +// Arguments: +// - json: an object which should be a serialization of a ColorScheme object. +// Return Value: +// - Returns nullptr for invalid JSON. +winrt::com_ptr<Theme> Theme::FromJson(const Json::Value& json) +{ + auto result = winrt::make_self<Theme>(); + + if (json.isString()) ``` we control the entire themes array, from its inception to its death. Who would put a string in it? ________________________________________ * [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`: ```diff > + if (winrt::unbox_value<winrt::hstring>(dictionaryKey) != + winrt::unbox_value<winrt::hstring>(requestedThemeKey)) ``` but do we need users to crash if this somehow happens in production? ________________________________________ * [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`: ```diff > + } + } + + // We didn't find it in the requested dict, fall back to the default dictionary. + return res.Lookup(key); + }; + + // Use our helper to lookup the theme-aware version of the resource. + const auto tabViewBackgroundKey = winrt::box_value(L"TabViewBackground"); + const auto backgroundSolidBrush = lookup(res, requestedTheme, tabViewBackgroundKey).as<Media::SolidColorBrush>(); + + til::color bgColor = backgroundSolidBrush.Color(); + + if (_settings.GlobalSettings().UseAcrylicInTabRow()) + { + const til::color backgroundColor = backgroundSolidBrush.Color(); ``` You marked it as resolved... but the line of code is still here. </details> <details> <summary>follow ups</summary> ________________________________________ * [ ] In `src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp`: ```diff > @@ -531,6 +532,25 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source } } + { + for (const auto& themeJson : json.themes) + { + if (const auto theme = Theme::FromJson(themeJson)) + { + if (origin != OriginTag::InBox && ``` follow up: we should add an origin tag to themes, after #12800 lands. ________________________________________ * [ ] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml`: ```diff > ItemsSource="{x:Bind ThemeList, Mode=OneWay}" SelectedItem="{x:Bind CurrentTheme, Mode=TwoWay}" - Style="{StaticResource ComboBoxSettingStyle}" /> + Style="{StaticResource ComboBoxSettingStyle}"> + <ComboBox.ItemTemplate> + <DataTemplate x:DataType="model:Theme"> + <TextBlock Text="{x:Bind local:GlobalAppearance.ThemeNameConverter((model:Theme)), Mode=OneWay}" /> ``` @carlos-zamora, can you make sure this is still accessible? we have had issues with putting custom data templates into combo boxes. ________________________________________ * [ ] In `src/types/utils.cpp`: ```diff > } - else + else if (str.size() == 7) + { + rStr = std::string(&str.at(1), 2); + gStr = std::string(&str.at(3), 2); + bStr = std::string(&str.at(5), 2); + aStr = "ff"; ``` I don't love that we force a string parser to parse ff just to get 255... ;P ________________________________________ </details>
claunia commented 2026-01-31 05:57:23 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Jul 7, 2022):

but do we need users to crash if this somehow happens in production?

That one's interesting. I REALLY don't think that should ever be happening - your install has got to be all sorts of fucked up to have a ResourceDictionary, in the Terminal's resources, under a key that's not an hstring. IDK to me, if you're a user that hit that, you were probably playing stupid games.

(inb4 I regret this comment in like, 8 weeks)

@zadjii-msft commented on GitHub (Jul 7, 2022): > but do we need users to crash if this somehow happens in production? That one's interesting. I REALLY don't think that should ever be happening - your install has got to be all sorts of fucked up to have a ResourceDictionary, in the Terminal's resources, under a key that's _not_ an hstring. IDK to me, if you're a user that hit that, you were probably playing stupid games. (inb4 I regret this comment in like, 8 weeks)
claunia commented 2026-01-31 05:57:23 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Jul 8, 2022):

XAML in the settings model -- this will cause trouble for a consumer like the shell extension in the future

This one's annoying, but probably fine for now. It wouldn't be impossible for the body of ThemeColor::Evaluate to move to some helper. It doesn't rely on any private implementation details of ThemeColor, so we could move that inside TerminalApp, or Terminal.Settings.Editor (if we needed it there for some reason).

I am however gonna just leave that for now, since there's not a clear and present danger of the shell extension reading the settings any time soon (😢)

@zadjii-msft commented on GitHub (Jul 8, 2022): > XAML in the settings model -- this will cause trouble for a consumer like the shell extension in the future This one's annoying, but probably fine for now. It wouldn't be impossible for the body of `ThemeColor::Evaluate` to move to some helper. It doesn't rely on any private implementation details of `ThemeColor`, so we could move that inside `TerminalApp`, or `Terminal.Settings.Editor` (if we needed it there for some reason). I am however gonna just leave that for now, since there's not a clear and present danger of the shell extension reading the settings any time soon (😢)
claunia commented 2026-01-31 05:57:24 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jul 11, 2022):

Still curious about the signature of ApplyTheme and what happens in High Contrast mode for the theme resource looker-upper!

@DHowett commented on GitHub (Jul 11, 2022): Still curious about the signature of `ApplyTheme` and what happens in High Contrast mode for the theme resource looker-upper!
claunia commented 2026-01-31 05:57:24 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Jul 12, 2022):

what happens in High Contrast mode for the theme resource looker-upper

In HC mode, in "default" app theme, we just do a normal resource lookup for the key, and that gets us the HC resource. In other app themes, I think we'll look up the one from the theme dictionary before the HC one.

the signature of ApplyTheme

Meh, maybe that should just be cleaned even more? The event arg is basically unused anyways, as one of those "raise an event, and when handled, look up the current value" style events. meh. I got rid of that too.

@zadjii-msft commented on GitHub (Jul 12, 2022): > what happens in High Contrast mode for the theme resource looker-upper In HC mode, in "default" app theme, we just do a normal resource lookup for the key, and that gets us the HC resource. In other **app** themes, I think we'll look up the one from the theme dictionary before the HC one. > the signature of `ApplyTheme` Meh, maybe that should just be cleaned even more? The event arg is basically unused anyways, as one of those "raise an event, and when handled, look up the current value" style events. meh. I got rid of that too.
claunia commented 2026-01-31 05:57:24 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (Sep 13, 2022):

:tada:This issue was addressed in #13465, which has now been successfully released as Windows Terminal Preview v1.16.252.🎉

Handy links:

@ghost commented on GitHub (Sep 13, 2022): :tada:This issue was addressed in #13465, which has now been successfully released as `Windows Terminal Preview v1.16.252`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.16.252) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-CodeHealth Area-Theming Issue-Bug Product-Terminal Resolution-Fix-Committed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#17883
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?