Fix a crash on settings reload

[lhecker]

07d58a8 contains a regression where the settings' Themes() property is
accessed without checking whether it's a nullptr. This can happen because
the invalid settings modal is shown with a empty settings model object.

This commit fixes the issue by deferring the update of _settings until
after we ensured that the _settings object is valid (besides warnings).

Closes #13543

Validation Steps Performed

• Replace any string value with 123
• Application doesn't crash ✅

[zadjii-msft]

Huh. This is clever

[carlos-zamora]

Mainly blocking over the question on the removed FAILED check. Just wanna make sure we have our bases covered.

[lhecker]

@zadjii-msft @carlos-zamora With the issue Carlos found, I've chosen a new approach which should also work. This slightly changes the behavior of WT however: From now on we'll not reload any settings until after the settings.json could be successfully parsed.

[carlos-zamora]

@zadjii-msft @carlos-zamora With the issue Carlos found, I've chosen a new approach which should also work. This slightly changes the behavior of WT however: From now on we'll not reload any settings until after the settings.json could be successfully parsed.

Hmm... That's not noticeable right? A user generally experiences the following behavior after updating the settings.json...

• no errors in the JSON --> hot reload
• lingering comma --> we're smart and we ignore the comma --> hot reload
• valid JSON, but wrong type for a setting --> ignore the setting
• JSON cannot be read --> fall back to defaults.json

[carlos-zamora]

Yeah, I think this works just fine. Thanks!

[zadjii-msft]

huh. Another clever solution. Neato, thanks!

[ghost]

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.

[lhecker]

Hmm... That's not noticeable right? [...]

• valid JSON, but wrong type for a setting --> ignore the setting
• JSON cannot be read --> fall back to defaults.json

I think the only reason it's generally not very noticeable is because we call _root->SetSettings(_settings, true) only if the settings reload was successful.

But if you check AppLogic.cpp you can see how _settings is used in a lot of other places as well. I get the feeling that this PR might fix a number of subtle bugs and not just #13543. For instance AppLogic::FindTargetWindow depends on _settings, but before this PR _settings might have been invalid.

[DHowett]

Thanks!

[ghost]

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

Handy links:

• Release Notes
• Store Download