Skip to content
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

Merged
merged 1 commit into from Sep 28, 2023

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

@zadjii-msft zadjii-msft added this to To Cherry Pick in 1.19 Servicing Pipeline via automation Sep 27, 2023
@@ -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;
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
17 checks passed
@zadjii-msft zadjii-msft deleted the dev/migrie/b/msft-46714723-this-is-so-bad branch September 28, 2023 14:34
@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.19 Servicing Pipeline Sep 29, 2023
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
@DHowett DHowett moved this from Cherry Picked to Shipped in 1.19 Servicing Pipeline Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants