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.

Sign up for GitHub

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

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

Merged
5 commits merged into from
Oct 23, 2020
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb22a92
inform user about permissions on their settings file
PankajBhojwani Oct 16, 2020
a8728c6
alternative implementation
PankajBhojwani Oct 20, 2020
9f8a6c8
resource string updates
PankajBhojwani Oct 20, 2020
b569215
warning instead of error
PankajBhojwani Oct 21, 2020
65f545b
format
PankajBhojwani Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="InitialJsonParseErrorText" xml:space="preserve">
<value>Settings could not be loaded from file. Check for syntax errors, including trailing commas.</value>
<value>There was an error when loading the settings.</value>
</data>
<data name="MissingDefaultProfileText" xml:space="preserve">
<value>Could not find your default profile in your list of profiles - using the first profile. Check to make sure the "defaultProfile" matches the GUID of one of your profiles.</value>
Expand Down
9 changes: 8 additions & 1 deletion src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,14 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
// We should re-parse, but not re-layer
resultPtr->_ParseJsonString(resultPtr->_userSettingsString, false);

_WriteSettings(resultPtr->_userSettingsString);
try
{
_WriteSettings(resultPtr->_userSettingsString);
}
catch (...)
{
throw SettingsTypedDeserializationException{ winrt::to_string((RS_(L"WriteSettingsFailed"))) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to a warning now!

}
}

// If this throws, the app will catch it and use the default settings
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,4 +324,7 @@
<data name="ToggleRetroEffectCommandKey" xml:space="preserve">
<value>Toggle retro terminal effect</value>
</data>
<data name="WriteSettingsFailed" xml:space="preserve">
<value>We could not write to your settings file. Check the permissions on that file to ensure that the read-only flag is not set and that write access is granted.</value>
</data>
</root>
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettingsModel/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,5 @@ TRACELOGGING_DECLARE_PROVIDER(g_hSettingsModelProvider);

// Manually include til after we include Windows.Foundation to give it winrt superpowers
#include "til.h"

#include "LibraryResources.h"