nits from themes PR #13456
Description
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:
- 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_COPYdo 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. Weimplicit 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