Add titlebar acrylic by matthew4850 · Pull Request #10864 · microsoft/terminal

matthew4850 · 2021-08-03T22:47:42Z

Add support for acrylic in the titlebar

PR Checklist

CLA signed

Detailed Description of the Pull Request / Additional comments

This seems to be a highly requested feature and seeing as #5772 was closed I thought it made sense to make a PR for this.

Validation Steps Performed

Checked that acrylic works in both dark and light modes and switching between them still works. Also checked that acrylic in the tab row still works when tabs in titlebar is disabled.

zadjii-msft · 2021-08-04T13:25:44Z

Okay so I love this, and is almost certainly exactly how I'd implement it.

I might have not been the most clear when I closed #5772. I do plan on coming back to XAML theming (#3327). It's my personal passion project, probably one of the features that I'm most excited about. I unfortunately have other priorities that are more important right now (mainly #5000), so I'm pretty confident that #3327 isn't going to land in the 2.0 timeframe. I know the team wasn't going to have the time to come through and review that spec in any short order (it's already 12 months old haha). Plus, the spec needed a decent amount of additional work to reconcile it with Mica. I thought it best to close the spec PR for now and help clear out the PR backlog, and re-open when the team actually will have a chance to review.

I don't want to be the kind of person who turns away valid and valuable community contributions in the name of "the big picture". I especially don't want to do that when I have no idea when the big picture will work itself out. So I'm gonna give this a think for a while to see if there's a reasonable way to accept this now and reconcile with themes later.

matthew4850 · 2021-08-07T13:02:34Z

Okay so I love this, and is almost certainly exactly how I'd implement it.

Thank you so much for this, I am new to C++/WinRT so it really means a lot.

I might have not been the most clear when I closed #5772. I do plan on coming back to XAML theming (#3327). It's my personal passion project, probably one of the features that I'm most excited about. I unfortunately have other priorities that are more important right now (mainly #5000), so I'm pretty confident that #3327 isn't going to land in the 2.0 timeframe. I know the team wasn't going to have the time to come through and review that spec in any short order (it's already 12 months old haha). Plus, the spec needed a decent amount of additional work to reconcile it with Mica. I thought it best to close the spec PR for now and help clear out the PR backlog, and re-open when the team actually will have a chance to review.

I don't want to be the kind of person who turns away valid and valuable community contributions in the name of "the big picture". I especially don't want to do that when I have no idea when the big picture will work itself out. So I'm gonna give this a think for a while to see if there's a reasonable way to accept this now and reconcile with themes later.

I hope it shouldn't be too hard to reconcile with themes, when necessary I think it should be possible to just remove this code and have some kind of auto-upgrade for the settings to something compliant with themes.

zadjii-msft

Okay, team consensus was that we're okay doing this for now, and migrating it to themes when themes arrive in ~3.0. We're not going to take any of the other titlebar theming settings in 2.0.

(We might take a fix for following the system setting for "use accent color in the titlebar", but with an opt-out setting (that again, would be migrated to themes in 3.0). The rest of the theming asks though will have to wait.)

I'm okay with

this setting not hot-reloading right now. The team can always follow up and add that if needed
~~the team can come through and link this up to the settings UI in a follow-up, no need to delay this PR any longer.~~ It's already in the SUI!

Thanks again for the contribution here!

src/cascadia/TerminalApp/TerminalPage.cpp

matthew4850 · 2021-08-10T15:17:45Z

the team can come through and link this up to the settings UI in a follow-up, no need to delay this PR any longer.

I might be misunderstanding but the PR already contains a setting UI toggle.

zadjii-msft · 2021-08-10T16:24:44Z

I might be misunderstanding but the PR already contains a setting UI toggle.

omfg it does. I forgot how easy it is to add settings to the SUI sometimes, that I completely forgot that I had read that code. Ignore me!

carlos-zamora

This looks great! A few things before I sign off:

Please add useAcrylicInTabRow to the schema
Please add this to the docs repo
Please file a follow-up issue for this to be hot-reloadable

carlos-zamora · 2021-08-10T20:20:58Z

src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp

+        const auto backgroundSolidBrush = backgroundBrush.try_as<Media::SolidColorBrush>();
+        const auto backgroundAcrylicBrush = backgroundBrush.try_as<Media::AcrylicBrush>();
+
+        til::color backgroundColor = Colors::Black();
+        if (backgroundSolidBrush)
+        {
+            backgroundColor = backgroundSolidBrush.Color();
+        }
+        else if (backgroundAcrylicBrush)
+        {
+            backgroundColor = backgroundAcrylicBrush.FallbackColor();
+        }


Suggested change

const auto backgroundSolidBrush = backgroundBrush.try_as<Media::SolidColorBrush>();

const auto backgroundAcrylicBrush = backgroundBrush.try_as<Media::AcrylicBrush>();

til::color backgroundColor = Colors::Black();

if (backgroundSolidBrush)

{

backgroundColor = backgroundSolidBrush.Color();

}

else if (backgroundAcrylicBrush)

{

backgroundColor = backgroundAcrylicBrush.FallbackColor();

}

til::color backgroundColor = Colors::Black();

if (const auto backgroundSolidBrush = backgroundBrush.try_as<Media::SolidColorBrush>())

{

backgroundColor = backgroundSolidBrush.Color();

}

else if (const auto backgroundAcrylicBrush = backgroundBrush.try_as<Media::AcrylicBrush>())

{

backgroundColor = backgroundAcrylicBrush.FallbackColor();

}

Totally optional. This'll make it so that the brushes are available in a smaller scope.

carlos-zamora · 2021-08-10T21:10:23Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <comment>Header for a control to toggle whether acrylic shows in the tab row. Changing this setting requires the user to relaunch the app.</comment>
+  </data>
+  <data name="Globals_AcrylicTabRow.HelpText" xml:space="preserve">
+    <value>When checked, the tab row will have the acrylic material.</value>


Suggested change

<value>When checked, the tab row will have the acrylic material.</value>

<value>When enabled, the tab row will have the acrylic material.</value>

ghost · 2021-08-18T00:02:43Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

zadjii-msft · 2021-08-20T23:26:15Z

@carlos-zamora Mind if we do those in post? Not sure I want to necessarily force an external contributor to comply with the fullness of our bureaucracy 😜

zadjii-msft · 2021-08-21T09:20:06Z

Ah shoot, there are merge conflicts though - @matthew4850 if you want to fix those, otherwise I might just try on monday

matthew4850 · 2021-08-21T10:36:32Z

@carlos-zamora apologise for not getting round to your requested changes, I was away and only just got back.

matthew4850 · 2021-08-21T11:13:26Z

Ah shoot, there are merge conflicts though - @matthew4850 if you want to fix those, otherwise I might just try on monday

Resolved 🙂

carlos-zamora · 2021-08-23T16:39:49Z

@carlos-zamora apologise for not getting round to your requested changes, I was away and only just got back.

No worries. Thank YOU for doing this! 🙂

ghost · 2021-08-23T16:40:10Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-08-31T17:14:24Z

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

Handy links:

`useAcrylicInTabRow` was missing from the JSON schema, so I added it in. ## References #10864 ## PR Checklist * [x] Closes #11087 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Schema updated.

This has been a saga. Basically, any resources in `App.xaml` aren't going to be able to reference other theme-aware resources. We can't change the theme of the app at runtime, only elements within the app. So we can't use `ApplicationPageBackgroundThemeBrush` in app.xaml, because it will ALWAYS be evaluated as the OS theme version of that brush. * regressed in #12326 * See also #10864 * #3917 CANNOT be fixed in the same way. We're lucky here that the TabView uses a `{ThemeResource TabViewBackground}` in markup to set the bg. We're not similarly lucky with the Pane one. * [x] closes #12356 * [x] Tested manually. You can confirm, my eyes are bleeding from the OS-wide light mode

This has been a saga. Basically, any resources in `App.xaml` aren't going to be able to reference other theme-aware resources. We can't change the theme of the app at runtime, only elements within the app. So we can't use `ApplicationPageBackgroundThemeBrush` in app.xaml, because it will ALWAYS be evaluated as the OS theme version of that brush. * regressed in #12326 * See also #10864 * #3917 CANNOT be fixed in the same way. We're lucky here that the TabView uses a `{ThemeResource TabViewBackground}` in markup to set the bg. We're not similarly lucky with the Pane one. * [x] closes #12356 * [x] Tested manually. You can confirm, my eyes are bleeding from the OS-wide light mode (cherry picked from commit 5ba0d61)

This has been a saga. Basically, any resources in `App.xaml` aren't going to be able to reference other theme-aware resources. We can't change the theme of the app at runtime, only elements within the app. So we can't use `ApplicationPageBackgroundThemeBrush` in app.xaml, because it will ALWAYS be evaluated as the OS theme version of that brush. * regressed in #12326 * See also #10864 * #3917 CANNOT be fixed in the same way. We're lucky here that the TabView uses a `{ThemeResource TabViewBackground}` in markup to set the bg. We're not similarly lucky with the Pane one. * [x] closes #12356 * [x] Tested manually. You can confirm, my eyes are bleeding from the OS-wide light mode

matthew4850 added 2 commits August 3, 2021 23:36

Add titlebar acrylic.

7f09333

Fix formatting

9138979

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 5, 2021

zadjii-msft approved these changes Aug 10, 2021

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

zadjii-msft added Area-Theming Anything related to the theming of elements of the window Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal. and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 10, 2021

ghost requested review from DHowett, PankajBhojwani, carlos-zamora, leonMSFT, lhecker and miniksa August 10, 2021 13:03

carlos-zamora requested changes Aug 10, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 10, 2021

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 18, 2021

ghost removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 20, 2021

carlos-zamora approved these changes Aug 20, 2021

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 21, 2021

Merge branch 'main' into main

2e777e3

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 23, 2021

ghost merged commit ed7c716 into microsoft:main Aug 23, 2021

cinnamon-msft mentioned this pull request Aug 31, 2021

useAcrylicInTabRow missing from JSON schema #11087

Closed

cinnamon-msft mentioned this pull request Sep 1, 2021

Add useAcrylicInTabRow to JSON schema #11117

Merged

6 tasks

zadjii-msft mentioned this pull request Feb 10, 2022

Manually set the colors of the TabViewBackground #12460

Merged

2 tasks

zadjii-msft mentioned this pull request Jun 13, 2022

New features #13280

Closed

This pull request was closed.

	<value>When checked, the tab row will have the acrylic material.</value>
	<value>When enabled, the tab row will have the acrylic material.</value>

Conversation

matthew4850 commented Aug 3, 2021

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

zadjii-msft commented Aug 4, 2021

Uh oh!

matthew4850 commented Aug 7, 2021

Uh oh!

zadjii-msft left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthew4850 commented Aug 10, 2021

Uh oh!

zadjii-msft commented Aug 10, 2021

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 18, 2021

Uh oh!

zadjii-msft commented Aug 20, 2021

Uh oh!

zadjii-msft commented Aug 21, 2021

Uh oh!

matthew4850 commented Aug 21, 2021

Uh oh!

matthew4850 commented Aug 21, 2021

Uh oh!

carlos-zamora commented Aug 23, 2021

Uh oh!

ghost commented Aug 23, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zadjii-msft left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.