Add Warnings during settings load #2422

zadjii-msft · 2019-08-13T19:18:43Z

Summary of the Pull Request

Adds support for two things: more discrete settings errors that we can detect on load, and discrete warnings we can detect on load.

Errors are things that we might find that cause the settings to be totally invalid. At the moment, there are two such cases:

Failing to parse the JSON at all
Failing to find any profiles. We need some profiles to be able to run.

When we find such an error case, we'll display more specific text in the popup:

Warnings are things that aren't necessarily fatal, but they certainly aren't good. I focused on two cases:

Failing to find the default profile in the list of profiles
Finding duplicate profiles in the list of profiles.

When we find a warning, we'll try to mitigate it, and keep running. We won't save the settings on load, because we'll have likely changed them internally. We'll also display all the warnings to the user, so they can see what they did wrong.

PR Checklist

Closes Display a specific error for not finding the default profile #1348
Closes Possibly expose exception text on error dialog #990
Closes the issue where the json being empty crashes the app
CLA signed. If not, go over here and sign the CLA
(☞ﾟヮﾟ)☞Tests added/passed ☜(ﾟヮﾟ☜)
[N/A] Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

I added tests.
I tried loading a totally empty profiles.json
I messed with the settingonce the app was open to see if this would work during reload
I tried removing the profiles entirely
I tried having 2 warnings simultaneously
I tried having invalid json

The start of work on #1348

…ettings-warnings

* Add a ton of tests * Polish the _GetMessageText bits * Add code to check for duplicate profiles * Verify that many warnings work at the same time * comments y'all

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/CascadiaSettings.cpp

src/cascadia/TerminalApp/CascadiaSettings.h

DHowett-MSFT · 2019-08-13T19:28:55Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+</value>
+  </data>
+  <data name="DuplicateProfileText" xml:space="preserve">
+    <value>Found multiple profiles with the same guid in your profiles - temporarily ignoring duplicates. Make sure each profile's guid is unique.


should we capitalize GUID?

Usually we do, but at the same time, the key is guid not GUID, so IDK. Maybe we should have a wordsmith's opinion @bitcrazed @cinnamon-msft

Most of the projects I've seen follow much of the guidance in Google's JSON Style Guide which states that property names should be camel cased:

{ "thisPropertyIsAnIdentifier": "identifier value" }

sooooooooo you're saying we should call it gUID?

@bitcrazed This is not a question about our json schema. This is a question about a user-facing error message

Ohhhhh - apologies!

In human-text, acronyms formed from the initial letter of each word should be all-caps - GUID.

I agree with GUID

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/App.xaml

src/cascadia/TerminalApp/App.cpp

DHowett-MSFT

Does this properly propagate error texts up from jsoncpp?

zadjii-msft · 2019-08-14T22:24:26Z

Does this properly propagate error texts up from jsoncpp?

you better believe it does :D

DHowett-MSFT · 2019-08-14T22:26:39Z

This is a real failure in x86.

packages\Taef.Redist.Wlk.10.38.190610001-uapadmin\build\Include\Verify.h(539,31): Error C4389:  '==': signed/unsigned mismatch (compiling source file SettingsTests.cpp)
Process 'msbuild.exe' exited with code '1'.

carlos-zamora

(1) if I change the defaultProfile to an invalid guid, I get "No profiles were found in your settings.
". Not sure if you want to address that in this PR or open a new issue.

(2) In our dialog box, we should explicitly mention that we're temporarily using the "Windows Terminal default settings" or something to that nature. That way users know what is happening when there is an error. That should probably address my "temporarily" issue below.

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

src/cascadia/TerminalApp/CascadiaSettings.cpp

src/cascadia/TerminalApp/App.cpp

…ettings-warnings # Conflicts: # src/cascadia/TerminalApp/App.cpp

* Fix x86 build break * Add a bit on "using the defaults" when we encountering an exception * remove a redundant variable

src/cascadia/TerminalApp/CascadiaSettings.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

miniksa · 2019-08-15T21:17:53Z

src/cascadia/TerminalApp/TerminalWarnings.h

+
+        // We don't use the what() method - we want to be able to display
+        // localizable error messages. Catchers of this exception should use
+        // _GetErrorText to get the localized exception string.


Where is this _GetErrorText? Also, wouldn't providing a what() at least help someone figure out what's going on in the debugger moreso than providing nothing even if it's not localized?

I added some placeholder text. I f you really want, we could file a task to try and make that text specific to the exception value

src/cascadia/TerminalApp/TerminalWarnings.h

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp

carlos-zamora · 2019-08-16T16:51:18Z

Text feels a bit redundant now. Like...
"Failed to load settings" and "Settings cound not be loaded from file"
"temporarily using the default settings" and "Temporarily using the Windows Terminal default settings."

Just something for us to be on the lookout for. Since we'll be defaulting to the Windows Terminal default settings, this means that the user may be working with profiles that don't exist AFTER the proper settings are imported. For example...

reate a tab from the WT default settings
import your custom settings
duplicate the created tab
CRASH!
I'd bet that this is because we usually use "FindProfile()" (or something like that) and we just don't handle that properly. Want to go through and fix it now or should I submit an issue?

zadjii-msft · 2019-08-16T19:20:13Z

@carlos-zamora yea that is pretty redundant now. I'll try and update that.

As far as the reload/duplicate crash, let's file a new issue, since that's not necessarily worse because of this change

cinnamon-msft · 2019-08-16T20:49:02Z

Nit: Could we change "in your profiles" to "in your profiles.json"? (I'm using an old screenshot, but the wording is still there)

DHowett-MSFT · 2019-08-16T20:50:59Z

We may want to go less specific and just say “in your configuration file”?

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

ghost · 2019-08-16T21:04:42Z

Hello @zadjii-msft!

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.

ghost · 2019-08-27T16:09:06Z

🎉Windows Terminal Preview v0.4.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 4 commits August 6, 2019 17:47

Warn the user when their settings are bad

80c18fb

The start of work on #1348

Merge remote-tracking branch 'origin/master' into dev/migrie/f/1348-s…

7a2f0e6

…ettings-warnings

Display an error dialog for errors during validation

7d6fd68

Polish for PR

3c28f8b

* Add a ton of tests * Polish the _GetMessageText bits * Add code to check for duplicate profiles * Verify that many warnings work at the same time * comments y'all

zadjii-msft self-assigned this Aug 13, 2019

zadjii-msft requested review from DHowett-MSFT, miniksa and carlos-zamora August 13, 2019 19:19

DHowett-MSFT suggested changes Aug 13, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 13, 2019

DHowett-MSFT reviewed Aug 13, 2019

View reviewed changes

src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

DHowett-MSFT reviewed Aug 13, 2019

View reviewed changes

src/cascadia/TerminalApp/App.cpp Show resolved Hide resolved

Apply fixes for dustin's thoughts from PR

566d6ba

DHowett-MSFT suggested changes Aug 14, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 14, 2019

Add a proper exception type, use an array instead of a map

13a8100

DHowett-MSFT reviewed Aug 14, 2019

View reviewed changes

src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved

DHowett-MSFT approved these changes Aug 14, 2019

View reviewed changes

carlos-zamora requested changes Aug 14, 2019

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 14, 2019

Merge remote-tracking branch 'origin/master' into dev/migrie/f/1348-s…

0b2215a

…ettings-warnings # Conflicts: # src/cascadia/TerminalApp/App.cpp

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 15, 2019

PR Fixes

d149177

* Fix x86 build break * Add a bit on "using the defaults" when we encountering an exception * remove a redundant variable

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Aug 15, 2019

carlos-zamora mentioned this pull request Aug 15, 2019

Bug Report: Invalid DefaultProfile returns wrong warning #2440

Closed

guid->GUID

6f816f1

miniksa reviewed Aug 15, 2019

View reviewed changes

Address Michael's PR comments

cf2a198

mauve reviewed Aug 16, 2019

View reviewed changes

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp Outdated Show resolved Hide resolved

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp Outdated Show resolved Hide resolved

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp Outdated Show resolved Hide resolved

miniksa added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 16, 2019

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 16, 2019

carlos-zamora mentioned this pull request Aug 16, 2019

Bug Report: Cannot find profile --> Crash #2455

Closed

Clean up this error text, and catch exceptions better

96c9784

zadjii-msft requested a review from carlos-zamora August 16, 2019 19:58

carlos-zamora approved these changes Aug 16, 2019

View reviewed changes

DHowett-MSFT approved these changes Aug 16, 2019

View reviewed changes

miniksa approved these changes Aug 16, 2019

View reviewed changes

zadjii-msft commented Aug 16, 2019

View reviewed changes

src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

Update src/cascadia/TerminalApp/Resources/en-US/Resources.resw

704359b

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 16, 2019

ghost merged commit d7d96f7 into master Aug 16, 2019

ghost deleted the dev/migrie/f/1348-settings-warnings branch August 16, 2019 21:21

SJang1 mentioned this pull request Aug 20, 2019

Terminal couldn't be opened if profile's commandline couldn't be opened (re-open) #2484

Closed

This was referenced Aug 27, 2019

Possibly expose exception text on error dialog #990

Closed

Display a specific error for not finding the default profile #1348

Closed

zadjii-msft mentioned this pull request Oct 2, 2019

Bug Report - Default profile not handled #2167

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Warnings during settings load #2422

Add Warnings during settings load #2422

zadjii-msft commented Aug 13, 2019

DHowett-MSFT Aug 13, 2019

zadjii-msft Aug 14, 2019

bitcrazed Aug 14, 2019

DHowett-MSFT Aug 14, 2019

DHowett-MSFT Aug 14, 2019

bitcrazed Aug 14, 2019

cinnamon-msft Aug 15, 2019

DHowett-MSFT left a comment

zadjii-msft commented Aug 14, 2019

DHowett-MSFT commented Aug 14, 2019

carlos-zamora left a comment

miniksa Aug 15, 2019

zadjii-msft Aug 16, 2019

carlos-zamora commented Aug 16, 2019

zadjii-msft commented Aug 16, 2019

cinnamon-msft commented Aug 16, 2019 •

edited

Loading

DHowett-MSFT commented Aug 16, 2019

ghost commented Aug 16, 2019

ghost commented Aug 27, 2019

Add Warnings during settings load #2422

Add Warnings during settings load #2422

Conversation

zadjii-msft commented Aug 13, 2019

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

zadjii-msft commented Aug 14, 2019

DHowett-MSFT commented Aug 14, 2019

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Aug 16, 2019

zadjii-msft commented Aug 16, 2019

cinnamon-msft commented Aug 16, 2019 • edited Loading

DHowett-MSFT commented Aug 16, 2019

ghost commented Aug 16, 2019

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.

ghost commented Aug 27, 2019

cinnamon-msft commented Aug 16, 2019 •

edited

Loading

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.