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

Show uncaughtException as an internal error and quit the app #626

Merged
merged 8 commits into from
Nov 6, 2017

Conversation

yuya-oc
Copy link
Contributor

@yuya-oc yuya-oc commented Oct 17, 2017

Before submitting, please confirm you've

Please provide the following information:

Summary
When an uncaught error occurred, the error log is saved to AppData\Roaming\Mattermost\uncaughtException-*.txt. Then a dialog would pop up to notice it.

error_dialog

When the user click "Show detail", the file is opened in the default text editor.

Application: Mattermost 3.7.1
Platform: Windows_NT 10.0.15063 x64
Error: test error
    at Timeout.setTimeout [as _onTimeout] (C:\Users\yuya-oc\git\mattermost-desktop\src\main_bundle.js:7781:11)
    at ontimeout (timers.js:386:14)
    at tryOnTimeout (timers.js:250:5)
    at Timer.listOnTimeout (timers.js:214:5)

Usually the error is unrecoverable, so the application should finish soon.
https://nodejs.org/api/process.html#process_warning_using_uncaughtexception_correctly

Issue link
#625

Test Cases

  • General use cases. The error should NOT appear.
  • Check text messages.

Additional Notes
CircleCI is delaying...

@yuya-oc yuya-oc added the All Platforms null label Oct 17, 2017
@yuya-oc yuya-oc added this to the v3.8.0 milestone Oct 17, 2017
@jasonblais
Copy link
Contributor

@yuya-oc Do you want me to test something here? Also, do we currently show this error dialog, or is this adding it?

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Oct 20, 2017

@jasonblais Like Electron upgrading, please use for several days. The dialog should not appear frequently. https://circleci.com/gh/yuya-oc/desktop/438#artifacts

Currently the application shows such errors only in command line console. So users can't know them even when a critical error happens. Then the application continues to work at such bad situation without quitting.

@jasonblais
Copy link
Contributor

Sounds good, I'll use the app and post here if there are any issues.

What, if any, can we change for the dialog design and text?

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Oct 20, 2017

We can change texts, button labels, icon and misc things (see dialog.showMessageBox()). The event handler must behave as a synchronous function. In this case, only dialog.showMessageBox() and dialog.showErrorBox() are applicable.

Copy link
Contributor

@MusikPolice MusikPolice left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I think we should make the visual design a little less windows-centric. Seeing as this is a cross platform application, we may have to design our own look and feel so that the dialog appears natural on all platforms.

@yuya-oc yuya-oc force-pushed the show-internal-error branch 2 times, most recently from 0b0ef4f to 04e1432 Compare October 24, 2017 13:44
@jasonblais
Copy link
Contributor

@MusikPolice Yeah, we'll have Asaad's help with UI mockups for this, #609 and #611

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Oct 24, 2017

Thanks for reviewing. Electron's dialog.showMessageBox() uses native modal dialog for each platform. Samples are below.

macOS:
macos

Ubuntu 16.04
ubuntu1604

@MusikPolice
Copy link
Contributor

@yuya-oc the native modal looks perfect.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Oct 30, 2017

@jasonblais Are there any feedback?

@jasonblais
Copy link
Contributor

@yuya-oc is there a way for the user to reopen the app via this dialog? E.g. something like attached

image

Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

@yuya-oc, here are the proposed text changes:

Title: Mattermost
Text: The Mattermost app quit unexpectedly. Click “Show Details” to learn more. \n\n Internal error: {error}
Buttons: “Show Details” / “OK”

where \n represents a new line

  1. For Windows, can we have "Show Details" as a button next to "OK"?

  2. What is the (O) on the Linux screenshot next to OK, is that standard?

  3. Can the app be reopened via this dialog? (Not needed now, wondering if this can be accomplished later)


After the changes, it would be great to have a screenshot for Windows and Mac and I'll get final sign-off from our UI team.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Oct 31, 2017

  1. A whitespace exists after \n\n. Is it intended?
  2. Yes.
  3. (O) is added by Electron, so I think it's commonly used. I will check other apps.
  4. Possible.

Unfortunately I found an important issue for this. On Windows, it seems the app can't show the dialog when a problem actually happens. However strangely, if the app is launched from command line, the dialog properly appears. So I couldn't notice the problem in dev mode. I need to inspect Electron.

@jasonblais
Copy link
Contributor

@yuya-oc

1 - Unintentional. Should read The Mattermost app quit unexpectedly. Click “Show Details” to learn more.\n\n Internal error: {error}

4 - Let me know and I can create a ticket for later

Unfortunately I found an important issue for this. On Windows, it seems the app can't show the dialog when a problem actually happens. However strangely, if the app is launched from command line, the dialog properly appears. So I couldn't notice the problem in dev mode. I need to inspect Electron.

I see. We can also create a separate ticket for Windows for later.

@yuya-oc yuya-oc force-pushed the show-internal-error branch from acb42ae to 19ced85 Compare November 1, 2017 15:59
@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 1, 2017

@jasonblais Updated texts. https://circleci.com/gh/yuya-oc/desktop/467#artifacts

  1. I checked Gedit, the default text editor of Ubuntu desktop. It shows brackets for each button. Unfortunately Electron doesn't do that automatically.

  2. It's easy by calling app.relaunch() when the button is pressed.

I didn't write in the previous comment though, the problem means that the app silently finishes if a problem actually happened. Is it acceptable? Of course, the report file is written in the data directory.

@jasonblais
Copy link
Contributor

1/2: Great! Thanks Yuya
3: Okay. I think this is okay for now, since fewer people use Linux.
4: > It's easy by calling app.relaunch() when the button is pressed.

Is this something we want to consider for this PR, or do I create an improvement ticket for later?


I didn't write in the previous comment though, the problem means that the app silently finishes if a problem actually happened. Is it acceptable? Of course, the report file is written in the data directory.

Will the user see the dialog?

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 5, 2017

@jasonblais
4. We can consider it in this PR. However in that case, we need to consider the order of buttons. For example, Show detail, OK, Reopen like macOS?

I didn't write in the previous comment though, the problem means that the app silently finishes if a problem actually happened. Is it acceptable? Of course, the report file is written in the data directory.

It was my misunderstanding. The actual problem was to call fs.writeFileSync(2). So I removed that statement. Now this PR works on all platforms.

@jasonblais
Copy link
Contributor

@yuya-oc
4. Yeah, that would be great. Agree on order, let's have it Show Details / OK / Reopen. (Note that both words in Show Details should be capitalized.)

If we add it, propose we modify the text to The Mattermost app quit unexpectedly. Click “Show Details” to learn more or "Reopen" to open the application again.\n\n Internal error: {error}

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 6, 2017

Updated: https://circleci.com/gh/yuya-oc/desktop/477#artifacts

Test build: https://circleci.com/gh/yuya-oc/desktop/480#artifacts This build shows a test error 10 seconds later.

macOS:
osx-error

Ubuntu:
ubuntu-error

Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @yuya-oc! Good to merge

@yuya-oc yuya-oc merged commit e14ff7b into mattermost:master Nov 6, 2017
@yuya-oc yuya-oc deleted the show-internal-error branch November 6, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants