Initial Theme support by zadjii-msft · Pull Request #12992 · microsoft/terminal

zadjii-msft · 2022-04-27T21:09:45Z

⚠️ targeting 1.15

Summary of the Pull Request

Adds support for Themes, a new type of customization for the Terminal. Themes allow the user to customize elements of the Terminal window itself. In this first iteration, this PR adds support for two main properties:

enabling Mica as the window backdrop
changing the tab row color (read: changing the titelbar color)

These represent the most important asks of theming in the Terminal. The properties added in this PR are:

Theme color variants:
- "#rrggbb" or "#aarrggbb"
- "accent"
- "terminalBackground"
Properties (listed here in dot notation, but implemented as sub-objects)
- tabRow.background: accepts a ThemeColor (above)
- window.applicationTheme: accepts one of {"system", "light", "dark"}
- window.useMica: accepts a boolean, defaults to false.

References

As first described in Add comprehensive XAML "theming" functionality #3327
spec'd in Draft spec for theming, Mica #12530

PR Checklist

Sorta enables Add support for the Mica material #10509, but doesn't close it. That'll need more comprehensive changes to the titlebar code.
- update: I totally disabled mica, but left the serialization code. It just seems silly without Add support for the Mica material #10509.
Closes Add a setting to enable the using the Accent Color for the titlebar #1963
Closes Changing the title bar color in accordance with the terminal's background. #3774
Closes Bring back the old titlebar color #12939
Does the bulk of the Add comprehensive XAML "theming" functionality #3327 work, but I'm going to leave that open since that's become my megathread for everything related to theming.
I work here
Tests added/passed
Requires documentation to be updated - SURE DOES

Detailed Description of the Pull Request / Additional comments

--> GO READ #12530 <--

Seriously.

These themes aren't customizable in the SUI currently. You can change the active theme, and the UI will show all of the defined themes, but they're not editable there.

They don't layer. You'll need to define your own themes.

Thay can't come from fragments. This is a really cool future idea, but not implemented in this v0.

The sub objects have some gnarly macros to generate a lot of the serialization code for you.

TODOs

I still have yet to establish what the accent color algorithm is. This might be proprietary and require a ThemeHelpers workaround.
Make sure terminalBackground & the SUI result in something sensible
Make sure runtime BG changes work with terminalBackground. One time, they didn't. printf "\x1b]11;rgb:ff/00/ff\x07"
Acrylic Terminal BG's look weird, like, the opacity is always 50% or something. And the tab row looks all wrong then.

Validation Steps Performed

This is the blob I've been testing with:

Details

    // "useAcrylicInTabRow": true,
    "theme": "my dark",
    // "theme": "Edge",
    "theme": "orangey",
    "theme": "WHITE",
    // "theme": "terminal",
    "themes": [
        {
            "name": "my dark",
            "window": {
                "applicationTheme": "dark",
                "useMica": true,
            },
            "tabRow": {
                "background": "#00000000",
            },
        },
        {
            "name": "Edge",
            "tabRow": { "background": "accent" },
            "window": { "applicationTheme": "system" }
        },
        {
            "name": "orangey",

            "window": {
                "applicationTheme": "light",
                "useMica": true,
            },
            "tabRow": {
                "background": "#ff8800",
            },
        },
        {
            "name": "WHITE",
            "window": {
                "applicationTheme": "dark",
                "useMica": true,
            },
            "tabRow": {
                "background": "#FFFFFF",
            },
        },
        {
            "name": "terminal",

            "window": {
                "applicationTheme": "dark",
                "useMica": false,
            },
            "tabRow": {
                "background": "terminalBackground",
            },
        },
    ]

https://stackoverflow.com/questions/64694722/changing-themeresources-dynamically-in-uwp That post looked SUPER promising. Problem is though, I CANNOT for the life of me get that to work. Like, I can't get anything to `{Binding Brush, Mode=TwoWay, Source={StaticResource TerminalBackground}}` to the `TerminalBackground` thing I made there. I thought that was so clever. I wanted an easy way to just change the value of a resource and have it update the Titlebar, but since the Titlebar isn't a child of the TerminalPage, and this binding thing didn't work, I think I'm at a dead end.

…e brush in there

…g-2022-prototype

…dev/migrie/fhl/theming-2022-prototype

…ses are NOT optional

…to be sub-properties of the Theme

zadjii-msft · 2022-06-08T10:42:53Z

shhh bot, I was cleaning up pee all of last week

…g-2022-prototype

carlos-zamora

YOLO. Best to get this in at the beginning of the cycle and start selfhosting this. :)

carlos-zamora · 2022-07-06T23:30:21Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        }
+
+        const auto theme = _settings.GlobalSettings().CurrentTheme();
+        auto requestedTheme{ theme.RequestedTheme() };


Suggested change

auto requestedTheme{ theme.RequestedTheme() };

const auto requestedTheme{ theme.RequestedTheme() };

I think?

carlos-zamora · 2022-07-06T23:45:37Z

src/cascadia/TerminalApp/TerminalPage.cpp

+
+        til::color bgColor = backgroundSolidBrush.Color();
+
+        if (_settings.GlobalSettings().UseAcrylicInTabRow())


Long-term, should we move useAcrylicInTabRow to be a part of the theme itself? If that's the case, (in a follow up) we should probably add deserialization logic to take this setting and write it as a part of themes (kinda like we did with the font object change).

FWIW if we do that, we need some way to emit a completely new theme for the user, because they are not allowed to edit the system themes

carlos-zamora · 2022-07-06T23:50:05Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        else if (theme.TabRow() && theme.TabRow().Background())
+        {
+            const auto tabRowBg = theme.TabRow().Background();
+            const auto terminalBrush = [this]() -> Media::Brush {


General question: when should we capture this vs &?

typically I've found it best to try and capture as little as possible. There's a lot going on in this method, but the only thing we really need is this. If there were a bunch of other locals we needed in here too, then just go for &. That's not really based on any specific insight, just my own preference.

In anything that's gonna happen async, both this and & are bad.

## Summary of the Pull Request Adds support for the `tab.background` property to themes. This is also a ThemeColor, so it accepts, colors, `accent`, and `terminalBackground`, just like everything else. ## References * See #3327 * ⚠️ targets #12992 ⚠️ ## PR Checklist * [x] Closes #702 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated - YUP ## Detailed Description of the Pull Request / Additional comments I apparently left behing an optional color in TerminalTab for theme colors some time ago, just never used it. Crazy, huh? ## Validation Steps Performed gifs below

Adds `tabRow.unfocusedBackground` to the theme properties. When provided, the window will use this ThemeColor as the color of the tab row when the window is inactive. When omitted, the window will fall back to the default tab row color, `{"key": "TabViewBackground"}` from our App.xaml. * [ ] tests added. * [x] Closes #4862 * [ ] Needs a whole pile of docs updates, which we'll do at the end here. This actually helped validate #12992 quite a bit. I found a bunch of bugs concerning null colors, null objects. Json parsing is hard 😛

DHowett · 2022-07-07T17:51:47Z

src/types/utils.cpp

+        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

DHowett · 2022-07-07T17:55:27Z

src/cascadia/TerminalSettingsModel/Theme.idl

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

DHowett · 2022-07-07T17:57:23Z

src/cascadia/TerminalSettingsModel/Theme.h

+    };
+
+#undef THEME_SETTINGS_INITIALIZE
+#undef THEME_SETTINGS_COPY


do we need to undef COPY_THEME...?

DHowett · 2022-07-07T17:58:34Z

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

+
+    std::string TypeDescription() const
+    {
+        return "ThemeColor (#rrggbb, #rgb, #aarrggbb, accent, terminalBackground)";


isn't it rrggbbaa?

DHowett · 2022-07-07T18:00:16Z

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

+            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

DHowett · 2022-07-07T18:18:49Z

src/cascadia/TerminalApp/TerminalPage.cpp

+
+        if (_settings.GlobalSettings().UseAcrylicInTabRow())
+        {
+            const til::color backgroundColor = backgroundSolidBrush.Color();


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

DHowett · 2022-07-07T18:22:49Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        _activated = activated;
+        _updateTabRowColors();
+    }


so _activated is ignored?

ah, this guy snuck into the parent branch. It was used in the unfocused titlebar branch.

DHowett · 2022-07-07T18:23:54Z

src/cascadia/TerminalSettingsModel/Theme.cpp

+    return val;
+}
+
+static til::color _getAccentColorForTitlebar()


do we reload this when the accent color changes?

AMAZINGLY, YES WE DO

DHowett · 2022-07-07T18:26:07Z

src/cascadia/TerminalSettingsModel/Theme.cpp

+                                                                                               \
+        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

DHowett · 2022-07-07T18:27:31Z

src/cascadia/TerminalSettingsModel/Theme.cpp

+{
+    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?

@DHowett

The main fix here is for the caption button colors. If you had a dark OS/app theme, and a light titlebar, we'd end up with light glyphs, so the caption buttons would be impossible to find. There's also a pile of nits from #12992 in here. Probably enough to close #13456 out, but I'll let Dustin be the judge. Filing today, to get in for 1.16 selfhost (@DHowett)

ghost · 2022-09-13T17:09:23Z

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 30 commits March 24, 2022 06:31

Settings project builds at least

15e4fd9

this fixes the App bits as well

7e0ae35

on reload, this changes the BG of the titlebar, but not the tab view

00c135b

this is better. Apphost is gonna need to be involved anyways so fuck it

b4cd7b3

okay so i bet that resource changed. Whatever, we'll work with it.

9627483

dumbly accept alpha channel in colors

84805f5

mica works, but the titlebar looks DUMB

9670d13

this did seem to work, but will it work for acrylic?

c555e6e

okay so you can't change a solid brush to an acylic one at runtime

01f07be

i feel like I'm getting closer

9eff0a2

really, no good progress here. It's rough

44112fc

We can't be clever here. Just make an observable property and slam th…

f92724d

…e brush in there

guess what, it works

cd0012a

change color based on if the window is activated or now

ac49459

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/themin…

03211c8

…g-2022-prototype

Merge remote-tracking branch 'origin/dev/migrie/titebar-colors' into …

1d0a5d7

…dev/migrie/fhl/theming-2022-prototype

accept accent color as a ThemeColor

e22046f

this actually works now. Remember kids, break statements in switch/ca…

99dc75d

…ses are NOT optional

cleanup, works on start and when unset

a7ce860

Lookup themes from a list of themes, use that one to style the window

4f42661

This is horrifying and yet I'm totally shipping it. Move the objects …

5081118

…to be sub-properties of the Theme

better, better,

fe26114

wow

7fcfda3

stragglers

df0efff

hey this is easier

7243603

this is like 99% of the work here

606db76

switches when a tab does, not the pane

de22df0

I think that's everything

01d6907

Some minor todos

39b6fdd

This comment was marked as resolved.

Sign in to view

zadjii-msft removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Jun 8, 2022

zadjii-msft modified the milestones: Terminal v1.15, Terminal v1.16 Jun 8, 2022

zadjii-msft added 5 commits June 16, 2022 10:11

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/themin…

8d86538

…g-2022-prototype

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/themin…

ee4ae62

…g-2022-prototype

just disable mica for now

9806d64

This is what happens when a PR has been open for ages

4245964

nits across the board

9ab99b3

zadjii-msft assigned carlos-zamora and unassigned zadjii-msft Jul 6, 2022

carlos-zamora approved these changes Jul 7, 2022

View reviewed changes

runformat

ab81648

zadjii-msft unassigned carlos-zamora Jul 7, 2022

zadjii-msft merged commit 07d58a8 into main Jul 7, 2022

zadjii-msft deleted the dev/migrie/fhl/theming-2022-prototype branch July 7, 2022 11:54

DHowett reviewed Jul 7, 2022

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Jul 7, 2022

This was referenced Jul 7, 2022

nits from themes PR #13456

Closed

A pile of Theming nits fixes #13465

Merged

This was referenced Sep 13, 2022

Bring back the old titlebar color #12939

Closed

Changing the title bar color in accordance with the terminal's background. #3774

Closed

Add a setting to enable the using the Accent Color for the titlebar #1963

Closed

zadjii-msft mentioned this pull request Jan 26, 2023

Explain the dark mode default in the FAQ MicrosoftDocs/terminal#635

Closed

	auto requestedTheme{ theme.RequestedTheme() };
	const auto requestedTheme{ theme.RequestedTheme() };


		til::color bgColor = backgroundSolidBrush.Color();

		if (_settings.GlobalSettings().UseAcrylicInTabRow())

Conversation

zadjii-msft commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ targeting 1.15

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

--> GO READ #12530 <--

TODOs

Validation Steps Performed

Uh oh!

This comment was marked as resolved.

zadjii-msft commented Jun 8, 2022

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zadjii-msft commented Apr 27, 2022 •

edited

Loading