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

Add an error dialog when we fail to parse settings #903

Merged
merged 11 commits into from
May 23, 2019

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

Add an error dialog when we fail to parse settings.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Sneak peek:
image

  • If we fail to load the settings, we'll temporarily use the default settings. These defaults won't get written back to the profiles.json file, so people don't need to worry about blowing their settings away with a simple typo.
  • This includes a bit of a XAML hack to make the button appear correctly when the app's theme is opposite that of the system theme. ContentDialog does a weird thing where it takes its child and moves that to the popup and leaves itself in the tree. That causes things like ElementTheme inheritance to break. So instead, we're theming the CloseButton manually, to make sure it's always visible.

  * Load messages from the Resources.resw file
  * Display a message when we fail to parse the settings on an initial parse, or
    on a reload.
    * Unfortunately, the dialog is styled incorrectly when the app theme is
      opposite the system theme. This can result in the button being invisible.
…or-dialog

# Conflicts:
#	src/cascadia/CascadiaPackage/Resources/en-US/Resources.resw
@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels May 20, 2019
@zadjii-msft zadjii-msft requested a review from a team May 20, 2019 17:38
<value>Ok</value>
</data>
<data name="ReloadJsonParseErrorText" xml:space="preserve">
<value>Settings could not be reloaded from file. Check for syntax errors, including trailing commas.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there space, time or brainpower to surface the exception text?
Will the exception text be useful or interesting?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could always make it a follow-up task to investigate

src/cascadia/TerminalApp/App.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/CustomContentDialog.xaml Outdated Show resolved Hide resolved
src/inc/DefaultSettings.h Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 20, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 20, 2019
@zadjii-msft
Copy link
Member Author

From #748
src/cascadia/TerminalApp/AppKeyBindings.cpp:288

perhaps pop the todo about error handling in here so a code sweep finds it

Make sure that keybindings serialization pops the error dialog correctly too

@zadjii-msft zadjii-msft mentioned this pull request May 20, 2019
5 tasks
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 21, 2019
@mdtauk
Copy link

mdtauk commented May 21, 2019

Will this dialog (and other error, confirmatory or feedback dialogs) be using a UWP ContentDialog, MessageDialog, or a Win32 MessageBox?

I vote for UWP (eventually WinUI 3.0) dialogs.

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 21, 2019

@mdtauk The screenshot looks exactly like a win32 MessageBoxW i think

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 22, 2019
@zadjii-msft zadjii-msft requested a review from DHowett-MSFT May 22, 2019 22:19
@zadjii-msft zadjii-msft requested a review from adiviness May 22, 2019 22:19
@zadjii-msft
Copy link
Member Author

@DHowett-MSFT Things I'm excited to see land:
This pull request

@zadjii-msft zadjii-msft merged commit 8dab297 into master May 23, 2019
@zadjii-msft zadjii-msft deleted the dev/migrie/f/gh-error-dialog branch May 23, 2019 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There should be an error dialog for incorrect settings instead of just crashing
6 participants