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

The "Error opening app" dialog #5610

Merged
merged 6 commits into from May 24, 2016

Conversation

Projects
None yet
4 participants
@bigtimebuddy
Contributor

bigtimebuddy commented May 19, 2016

The current "Error opening app" message is rather harsh.

screen shot 2016-05-19 at 10 19 30 am

Changes to:

Unable to open or find an Electron app. 
The docs on how to write an app can be found here:
@paulcbetts

This comment has been minimized.

Contributor

paulcbetts commented May 19, 2016

I think the current copy is alright, thanks though!

@@ -255,7 +255,7 @@ function loadApplicationPackage (packagePath) {
app.focus()
dialog.showErrorBox(
'Error opening app',
'The app provided is not a valid Electron app, please read the docs on how to write one:\n' +
'Unable to open or find an Electron app. The docs on how to write an app can be found here:\n' +

This comment has been minimized.

@kevinsawicki

kevinsawicki May 19, 2016

Contributor

I think this is an improvement.

URLs in dialogs are less than ideal since they can't be clicked or copied.

What do you think about adding another button in the dialog that says "View Guide" or "View Docs" or "Learn More" or something and when clicked opens the docs? And then we can remove the URL completely from the message.

This comment has been minimized.

@sindresorhus

sindresorhus May 20, 2016

Contributor

Learn More

👍

@bigtimebuddy

This comment has been minimized.

Contributor

bigtimebuddy commented May 20, 2016

Added "Learn More" button. Good suggestion.

screen shot 2016-05-20 at 11 03 23 am

message: 'Error opening app',
detail: 'Unable to open or find an Electron app. Click to learn more on how to write an app.\n\n' +
`${e.toString()}`,
buttons: ['Learn More', 'OK']

This comment has been minimized.

@kevinsawicki

kevinsawicki May 20, 2016

Contributor

I think you'll want to have to OK button be the default selected button instead of Learn More, seems like a more common pattern since the person might know what to do just by seeing the error message and might not want to always learn more.

)
dialog.showMessageBox({
message: 'Error opening app',
detail: 'Unable to open or find an Electron app. Click to learn more on how to write an app.\n\n' +

This comment has been minimized.

@kevinsawicki

kevinsawicki May 20, 2016

Contributor

I think you can now drop the Click to learn more on how to write an app. piece of this message.

buttons: ['Learn More', 'OK']
}, (response) => {
if (response === 0) {
shell.openExternal(`https://github.com/electron/electron/tree/v${process.versions.electron}/docs`)

This comment has been minimized.

@kevinsawicki

kevinsawicki May 20, 2016

Contributor

I think it might make sense to change this URL to http://electron.atom.io/docs now

@bigtimebuddy

This comment has been minimized.

Contributor

bigtimebuddy commented May 20, 2016

Thanks @kevinsawicki!

screen shot 2016-05-20 at 1 31 47 pm

)
dialog.showMessageBox({
message: 'Error opening app',
detail: 'Unable to open or find an Electron app.\n\n' +

This comment has been minimized.

@kevinsawicki

kevinsawicki May 20, 2016

Contributor

Do you think it could be worth include the value of packagePath in the dialog so people know what path was attempted?

@bigtimebuddy

This comment has been minimized.

Contributor

bigtimebuddy commented May 23, 2016

@kevinsawicki Another thought... what actually might be more useful here are better messaging for common start-up errors. Saying the app won't open probably isn't as useful as saying what specifically went wrong. Checking for these errors first might help to give some better feedback to the developer:

  1. package.json doesn't exist
  2. package.json JSON parsed incorrectly
  3. package.json main is empty
  4. package.json main doesn't exist
  5. all other errors

@bigtimebuddy bigtimebuddy changed the title from The "Error opening app" dialog, now with less attitude to The "Error opening app" dialog May 23, 2016

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 23, 2016

Checking for these errors first might help to give some better feedback to the developer

Yeah, handing these with better error messages sounds great 👍

@bigtimebuddy

This comment has been minimized.

Contributor

bigtimebuddy commented May 23, 2016

Added some additional error checking for the app startup:

  • Checks that package.json JSON is parsed correctly
  • Checks that package.json has a main field
  • Checks that the packagePath is a resolved module
  • All other errors are handled with a "Unable to open Electron app" message
packageJson = JSON.parse(fs.readFileSync(packageJsonPath))
} catch (e) {
showErrorMessage('Unable to parse package.json, it contains errors.\n\n' +
`${e.toString()} in ${packageJsonPath}`)

This comment has been minimized.

@kevinsawicki

kevinsawicki May 23, 2016

Contributor

I don't think we should say it contains errors here since the error caught here could be thrown from both JSON.parse and fs.readFileSync so it might have failed to read but actually be valid JSON.

if (packageJson.version) app.setVersion(packageJson.version)
if (packageJson.productName) {
app.setName(packageJson.productName)
} else if (packageJson.name) {
app.setName(packageJson.name)
}
if (!packageJson.main) {

This comment has been minimized.

@kevinsawicki

kevinsawicki May 23, 2016

Contributor

I think main is optional and defaults to index.js when not present so we might not want this check here and just catch the main being an invalid path down below where the package path is resolved.

@bigtimebuddy

This comment has been minimized.

Contributor

bigtimebuddy commented May 23, 2016

@kevinsawicki Sounds good.

@kevinsawicki kevinsawicki merged commit 4d56437 into electron:master May 24, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 24, 2016

Thanks for this 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment