A pile of Theming nits fixes #13465
A pile of Theming nits fixes #13465
Conversation
ghost
added
Area-CodeHealth
| til::color c; | ||
| if (auto acrylic = brush.try_as<winrt::Windows::UI::Xaml::Media::AcrylicBrush>()) | ||
| { | ||
| c = acrylic.TintColor(); | ||
| } | ||
| else if (auto solidColor = brush.try_as<winrt::Windows::UI::Xaml::Media::SolidColorBrush>()) | ||
| { | ||
| c = solidColor.Color(); | ||
| } |
There was a problem hiding this comment.
what will c be initialized to if it's neither Acrylic nor SolidColor?
| const auto bgProperty{ winrt::Windows::UI::Xaml::Controls::Panel::BackgroundProperty() }; | ||
| RegisterPropertyChangedCallback(bgProperty, [weakThis = get_weak(), bgProperty](auto& /*sender*/, auto& e) { | ||
| if (auto self{ weakThis.get() }) |
There was a problem hiding this comment.
In this very narrow case, you can capture this directly. You will not be getting property change callbacks after you get destructed, afterall :)
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the 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 (
|
ghost
merged commit 
| @@ -4101,27 +4100,25 @@ namespace winrt::TerminalApp::implementation | |||
|
|
|||
| TitlebarBrush(acrylicBrush); | |||
| } | |||
| else if (theme.TabRow()) | |||
| else if (const auto tabRowBg{ _activated ? theme.TabRow().Background() : | |||
There was a problem hiding this comment.
This call is causing a crash on startup in the current build of main with the error Access violation reading location 0x0000000000000000 (just did a git pull, clean and tried to run)
There was a problem hiding this comment.
Ah, I think the theme.TabRow() != nullptr check should come before we try to access the Background?
|
🎉 Handy links: |
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)