Display a warning for when we fail to write to the settings file #7950

PankajBhojwani · 2020-10-16T19:22:15Z

We wrap the call to _WriteSettings in
CascadiaSettingsSerialization.cpp in a try/catch block, and if we
catch an error we append a warning telling the user to check the
permissions on their settings file.

Closes #7727

PankajBhojwani · 2020-10-16T19:35:35Z

Alternative implementation:

We wrap the call to _WriteSettings in CascadiaSettingsSerialization.cpp in a try/catch block, and if we catch an error we throw a SettingsTypedDeserializationException with a message about what happened.

This would mean we should update the InitialJsonParseErrorText resource string to not talk about syntax errors though.

miniksa · 2020-10-19T16:35:19Z

Alternative implementation:

We wrap the call to _WriteSettings in CascadiaSettingsSerialization.cpp in a try/catch block, and if we catch an error we throw a SettingsTypedDeserializationException with a message about what happened.

This would mean we should update the InitialJsonParseErrorText resource string to not talk about syntax errors though.

I kind of like this better. Right now my concern with how you have it written is that you're only keying off the one E_ACCESSDENIED exception. I feel like there are at least half a dozen errors that could come out of just reading the file and one or more of those could possibly overlap with the deserialization/json library's set of errors.

It might just be nicer to adjust that existing json error message to take a format substring inside it and let the exception, as you describe, provide the details as it throws it up the stack from either the file load or the json work itself.

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

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

DHowett · 2020-10-21T02:02:44Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

+            }
+            catch (...)
+            {
+                throw SettingsTypedDeserializationException{ winrt::to_string((RS_(L"WriteSettingsFailed"))) };


I'm not in love with this one -- SettingsTypedDeserializationException taking a string is because of laziness, not to signal that it is the right way to emit settings error text. I'd rather surface it through the existing mechanisms for settings warnings (which we do have!), if at all possible. Warnings are allocated resource strings in TerminalApp instead of TerminalSettingsModel, admittedly, but there's already code that handles displaying a dialog with a number of warnings in it 😄

I know this conflicts with what @miniksa asked for, but we've got a long tradition of swallowing exceptions and presenting them to the user as "well something weird happened" here on Terminal. At least sending it up as a warning code (not an error!) DOES NOT discard the entire settings load operation!

I agree with the above. Failing to write the file doesn't seem like the kind of thing that makes the entire settings unusable - we've got perfectly valid settings in fact, we just can't update the dynamic profiles. That's not world-ending, we can still run with that.

Changed it to a warning now!

zadjii-msft

This looks like how I'd do it, thanks!

DHowett

Love it

ghost · 2020-10-23T00:20:26Z

Hello @DHowett!

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.

We wrap the call to `_WriteSettings` in `CascadiaSettingsSerialization.cpp` in a try/catch block, and if we catch an error we append a warning telling the user to check the permissions on their settings file. Closes #7727 (cherry picked from commit 16b8ea1)

ghost · 2020-11-11T18:05:44Z

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

Handy links:

ghost · 2020-11-11T18:09:38Z

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

Handy links:

inform user about permissions on their settings file

bb22a92

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Oct 16, 2020

alternative implementation

a8728c6

miniksa reviewed Oct 20, 2020

View reviewed changes

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

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

resource string updates

9f8a6c8

DHowett requested changes Oct 21, 2020

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 Oct 21, 2020

warning instead of error

b569215

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 21, 2020

format

65f545b

PankajBhojwani changed the title ~~Display a more appropriate error message for when we fail to write to the settings file~~ Display a more appropriate warning for when we fail to write to the settings file Oct 21, 2020

PankajBhojwani changed the title ~~Display a more appropriate warning for when we fail to write to the settings file~~ Display a warning for when we fail to write to the settings file Oct 21, 2020

zadjii-msft approved these changes Oct 21, 2020

View reviewed changes

zadjii-msft assigned DHowett Oct 21, 2020

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27

DHowett approved these changes Oct 23, 2020

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 23, 2020

ghost merged commit 16b8ea1 into main Oct 23, 2020

ghost deleted the dev/pabhoj/read_only_settings branch October 23, 2020 00:21

ghost mentioned this pull request Nov 11, 2020

Terminal shouldn't treat read-only settings as an error reading settings #7727

Closed

zadjii-msft mentioned this pull request Feb 10, 2022

Settings could not be loaded from file. Element not found (when running unpackaged) #12305

Closed

zadjii-msft mentioned this pull request Aug 3, 2022

If we fail to open the settings file, display a message if it existed #1328

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display a warning for when we fail to write to the settings file #7950

Display a warning for when we fail to write to the settings file #7950

PankajBhojwani commented Oct 16, 2020 •

edited by DHowett

Loading

PankajBhojwani commented Oct 16, 2020

miniksa commented Oct 19, 2020

DHowett Oct 21, 2020

DHowett Oct 21, 2020

zadjii-msft Oct 21, 2020

PankajBhojwani Oct 21, 2020

zadjii-msft left a comment

DHowett left a comment

ghost commented Oct 23, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

Display a warning for when we fail to write to the settings file #7950

Display a warning for when we fail to write to the settings file #7950

Conversation

PankajBhojwani commented Oct 16, 2020 • edited by DHowett Loading

PankajBhojwani commented Oct 16, 2020

miniksa commented Oct 19, 2020

DHowett Oct 21, 2020

Choose a reason for hiding this comment

DHowett Oct 21, 2020

Choose a reason for hiding this comment

zadjii-msft Oct 21, 2020

Choose a reason for hiding this comment

PankajBhojwani Oct 21, 2020

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Oct 23, 2020

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 Nov 11, 2020

ghost commented Nov 11, 2020

PankajBhojwani commented Oct 16, 2020 •

edited by DHowett

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.