-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a crash for users without a tab
theme
#16046
Conversation
@@ -176,7 +176,9 @@ namespace winrt::TerminalApp::implementation | |||
{ | |||
if (!profile.Icon().empty()) | |||
{ | |||
newTabImpl->UpdateIcon(profile.Icon(), _settings.GlobalSettings().CurrentTheme().Tab().IconStyle()); | |||
const auto theme = _settings.GlobalSettings().CurrentTheme(); | |||
const auto iconStyle = (theme && theme.Tab()) ? theme.Tab().IconStyle() : IconStyle::Default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const auto tab = theme ? theme.Tab() : nullptr;
const auto iconStyle = tab ? tab. IconStyle() : IconStyle::Default;
…for the Tab() call deduplication (untested code). Not really an issue tho. Might not even be worth a "nit".
We had the same for Window last time. How do we remove this category of issues? |
off the top of the dome: we change terminal/src/cascadia/TerminalSettingsModel/MTSMSettings.h Lines 138 to 141 in 310814b
and instead make them all ctor to |
FWIW that's not going to be particularly cheap with our current setup, because when the We can make it cheap by changing the implementation to store the WinRT type |
One day into 1.19, and there's a LOT of hits here (**76.25%** of our ~300 crashes). A crash if the Theme doesn't have a `tab` member. Regressed in #15948 Closes MSFT:46714723 (cherry picked from commit cf19385) Service-Card-Id: 90670731 Service-Version: 1.19
One day into 1.19, and there's a LOT of hits here (76.25% of our ~300 crashes). A crash if the Theme doesn't have a
tab
member.Regressed in #15948
Closes MSFT:46714723