Skip to content

Fix a crash for users without a tab theme#16046

Merged
zadjii-msft merged 1 commit intomainfrom
dev/migrie/b/msft-46714723-this-is-so-bad
Sep 28, 2023
Merged

Fix a crash for users without a tab theme#16046
zadjii-msft merged 1 commit intomainfrom
dev/migrie/b/msft-46714723-this-is-so-bad

Conversation

@zadjii-msft
Copy link
Member

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

{
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;
Copy link
Member

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

@DHowett
Copy link
Member

DHowett commented Sep 27, 2023

A crash if the Theme doesn't have a tab member.

We had the same for Window last time.

How do we remove this category of issues?

@zadjii-msft
Copy link
Member Author

How do we remove this category of issues

off the top of the dome:

we change MTSM_THEME_SETTINGS:

#define MTSM_THEME_SETTINGS(X) \
X(winrt::Microsoft::Terminal::Settings::Model::WindowTheme, Window, "window", nullptr) \
X(winrt::Microsoft::Terminal::Settings::Model::TabRowTheme, TabRow, "tabRow", nullptr) \
X(winrt::Microsoft::Terminal::Settings::Model::TabTheme, Tab, "tab", nullptr)

and instead make them all ctor to {}, so that we instantiate a *Theme object, instead of a null (when none exists)

@lhecker
Copy link
Member

lhecker commented Sep 27, 2023

FWIW that's not going to be particularly cheap with our current setup, because when the std::optional is empty it returns a new instance every time.

We can make it cheap by changing the implementation to store the WinRT type T as is (without wrapping it) and storing a bool _isDefaulted;. Then we can set the member T and use it as a cache the first time we're asked to return the default value.
(Or something along these lines.)

@zadjii-msft zadjii-msft merged commit cf19385 into main Sep 28, 2023
@zadjii-msft zadjii-msft deleted the dev/migrie/b/msft-46714723-this-is-so-bad branch September 28, 2023 14:34
DHowett pushed a commit that referenced this pull request Sep 29, 2023
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
@zadjii-msft zadjii-msft mentioned this pull request Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

3 participants