Add support for `tab.unfocusedBackground` by zadjii-msft · Pull Request #13689 · microsoft/terminal

zadjii-msft · 2022-08-05T17:53:46Z

Does what it sounds like on the label.

This is important, because when unset, the tab will use the active Background color to create an inactive BG, which maybe isn't what we want. Consider:

a bright cyan active BG,
and terminalBG for the tabRow bg.

Without an unfocusedBackground setting, all the tabs will still appear cyan when unfocused, which is extra gross.

As a judgement call, I made terminalBackground and accent use 30% opacity when set, to match the existing coloration.

See also #13554. If we want to make the default theme tab.background: terminalBackground, we should make tab.unfocusedBackground transparent (#00000000) by default. Otherwise, a Campell (#0c0c0c) tab on any tab row will still have a faint tab visible.

closes Inactive tab BG color (tab.inactiveBackground) #13684
closes Light tab color causes unreadably dark foreground (text) color when the tab is not focused #13246
tested manually

This also does a lot of code shuffling, to get SettingsUI tabs to behave sensibly. We want those tabs to have (#0c0c0c, #ffffff) colored BGs for terminalBackground (see mail thread).

We also don't want dark focused tabs colors, combined with light tab row colors, combined with transparent unfocused tabs, to result in unfocused tabs having white-on-white text. That's gross. So that's been added to this PR in b38b704.

…activeTabBg

… FG color, works sanely now

…/13684-inactiveTabBg

…activeTabBg

carlos-zamora

Welp, definitely reviewed this out of order haha. Didn't realize most of the new stuff in TabBase came straight out of TerminalTab. I've got a lot of comments, so I'll circle back to this PR after they're addressed.

src/cascadia/WinRTUtils/inc/Utils.h

carlos-zamora · 2022-08-29T17:22:16Z

src/cascadia/WinRTUtils/inc/Utils.h

+        // Look through the theme dictionaries we defined:
+        for (const auto& [dictionaryKey, dict] : dictionary.ThemeDictionaries())


So the heuristic is if a Source isn't defined, we assume it's ours? And we can guarantee that the Source will always be set by MUX?

Should we go the extra mile and set a source ourselves? Like "TerminalAppResources" or something like that? 🤷

Basically, yea. I don't think mu will ever not set that.

I honestly don't know the right way to set our own source. We're just looking it up out of the App.xaml resources, I'm not sure that gets to have a Source unless we move it to a separate file. idk.

src/cascadia/TerminalSettingsModel/Theme.idl

src/cascadia/TerminalApp/TabBase.cpp

carlos-zamora · 2022-08-29T17:56:57Z

src/cascadia/TerminalApp/TabBase.cpp

+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundSelected"), selectedTabBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundPointerOver"), hoverTabBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundPressed"), selectedTabBrush);
+
+        // Similarly, TabViewItem().Foreground()  sets the color for the text
+        // when the TabViewItem isn't selected, but not when it is hovered,
+        // pressed, dragged, or selected, so we'll need to just set them all
+        // anyways.
+        TabViewItem().Foreground(deselectedFontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderForeground"), deselectedFontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderForegroundSelected"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderForegroundPointerOver"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderForegroundPressed"), fontBrush);
+
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderCloseButtonForeground"), deselectedFontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderCloseButtonForegroundPressed"), secondaryFontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderCloseButtonForegroundPointerOver"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderPressedCloseButtonForeground"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderPointerOverCloseButtonForeground"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderSelectedCloseButtonForeground"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderCloseButtonBackgroundPressed"), subtleFillColorTertiaryBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderCloseButtonBackgroundPointerOver"), subtleFillColorSecondaryBrush);
+
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewButtonForegroundActiveTab"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewButtonForegroundPressed"), fontBrush);
+        TabViewItem().Resources().Insert(winrt::box_value(L"TabViewButtonForegroundPointerOver"), fontBrush);


Would there be enough benefit to magic static-ify all of these winrt::box_value(L"<resource name>")?

hmm... The strings also seem to be used below in _ClearTabBackgroundColor. Maybe we should static-ify the strings themselves? Idk if/when it's worth it though 🤷

meh we maybe could. This is all copypasta from TerminalTab and this hasn't ever seemed like an issue before.

src/cascadia/TerminalApp/TabBase.cpp

src/cascadia/TerminalApp/SettingsTab.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

…activeTabBg

carlos-zamora

👍👍

zadjii-msft · 2022-08-31T11:38:10Z

src/cascadia/TerminalApp/TabBase.cpp

+        return std::nullopt;
+    }
+
+    void TabBase::ThemeColor(const winrt::Microsoft::Terminal::Settings::Model::ThemeColor& focused,


note to reviewers: This mostly just came from TerminalTab.cpp so go ahead and hit that . and open the two SxS

src/inc/til/color.h

Co-authored-by: Leonard Hecker <lhecker@microsoft.com>

[11:48 AM] Leonard Hecker of course not - it's "alpha A inversed" [11:49 AM] Leonard Hecker why is that so hard to understand /s lmao

DHowett

👍🏻

The color of inactive tab text is incorrect since #13689 due to the introduction of `til::color::layer_over` which incorrectly calculated the RGB values. ## Validation Steps Performed * Added unit tests ✅ Co-authored-by: Leonard Hecker <lhecker@microsoft.com>

ghost · 2022-09-13T17:08:50Z

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

Handy links:

sometimes I do the code good

57e02b8

ghost added Area-Theming Anything related to the theming of elements of the window Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 5, 2022

zadjii-msft added 12 commits August 11, 2022 11:02

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13684-in…

fe5a86d

…activeTabBg

Allow the settings tab to also have a color set by the theme

bdfb015

default settings changes

4146d71

transparent inactive tabs on light tab row, when the tabs have a dark…

f911929

… FG color, works sanely now

comments

2964ebe

I can't tell you how much I hate this

37ce107

comments galore

25e09d6

Merge branch 'dev/migrie/b/13554-new-default-theme' into dev/migrie/b…

b38b704

…/13684-inactiveTabBg

Revert the changes I made to the default themes

48deba0

runformat

ff90631

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13684-in…

dfe40fa

…activeTabBg

Thanks austin mode

b040192

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) labels Aug 16, 2022

zadjii-msft added 5 commits August 18, 2022 16:48

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13684-in…

17262f7

…activeTabBg

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13684-in…

60d28f3

…activeTabBg

Merge branch 'main' into dev/migrie/b/13684-inactiveTabBg

4c7cfee

fixes the settings tab getting initialized with the wrong theme

9f7cbdc

This fixes the unfocused tab color weirdness

c5e0445

carlos-zamora reviewed Aug 29, 2022

View reviewed changes

zadjii-msft added 3 commits August 29, 2022 16:35

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13684-in…

2e8e802

…activeTabBg

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13684-in…

da56d48

…activeTabBg

nits from review

951ab2c

This comment has been minimized.

Sign in to view

okay

b92da58

carlos-zamora approved these changes Aug 30, 2022

View reviewed changes

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Aug 30, 2022

ghost requested review from DHowett, PankajBhojwani and lhecker August 30, 2022 20:24

zadjii-msft commented Aug 31, 2022

View reviewed changes

lhecker reviewed Aug 31, 2022

View reviewed changes

src/inc/til/color.h Outdated Show resolved Hide resolved

Update src/inc/til/color.h

81ef597

Co-authored-by: Leonard Hecker <lhecker@microsoft.com>

This comment has been minimized.

Sign in to view

lhecker approved these changes Aug 31, 2022

View reviewed changes

ugh "aai is not a word"

ec3ebb9

[11:48 AM] Leonard Hecker of course not - it's "alpha A inversed" [11:49 AM] Leonard Hecker why is that so hard to understand /s lmao

zadjii-msft merged commit f07b9e1 into main Aug 31, 2022

zadjii-msft deleted the dev/migrie/b/13684-inactiveTabBg branch August 31, 2022 17:48

DHowett reviewed Aug 31, 2022

View reviewed changes

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

zadjii-msft mentioned this pull request Sep 8, 2022

Fix til::color::layer_over #13946

Merged

This was referenced Sep 13, 2022

Light tab color causes unreadably dark foreground (text) color when the tab is not focused #13246

Closed

Inactive tab BG color (tab.inactiveBackground) #13684

Closed

zadjii-msft mentioned this pull request Oct 21, 2022

Tab color opacity #14026

Closed

		// Look through the theme dictionaries we defined:
		for (const auto& [dictionaryKey, dict] : dictionary.ThemeDictionaries())

Conversation

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

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

DHowett left a comment

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 Aug 5, 2022 •

edited

Loading