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

During quit, close unloaded windows #17873

Merged
merged 1 commit into from Aug 28, 2018

Conversation

Projects
None yet
3 participants
@stepanhruda
Contributor

stepanhruda commented Aug 16, 2018

Description of the Change

When quitting the app (e.g. by Cmd-Q on macOS), in before-quit Atom asks each open window to unload. Any one of them can resolve the promise with false (e.g. because user realized they didn't want to quit and hit Cancel on unsaved changes dialog), which stops the app from quitting.

When some of the windows end up unloading successfully, but the quit is stopped this way, these windows stay open, left in a weird unloaded state (Atom expects them to ultimately get closed by a quit, which doesn't happen). This adds logic to immediately close windows after they are successfully unloaded during a quit.

Alternate Designs

We could close the window in prepareToUnload()? We'd need to check that this makes sense for all callsites.

Why Should This Be In Core?

It's a bugfix.

Benefits

  1. No unloaded windows after an interrupted quit.
  2. Windows get closed as soon as they are unloaded - even during a successful quit, you could see the unloaded UI if one window's unload is significantly slower than others'.

Possible Drawbacks

State restoration on other platforms? Everything worked fine in my testing on macOS.

Verification Process

  1. Added a test.
  2. Had 2 windows, hit Cmd-Q. Window A returned false from prepareToUnload(). Window B still closed.

Applicable Issues

Don't know.

@stepanhruda stepanhruda requested review from maxbrunsfeld and as-cii Aug 16, 2018

@maxbrunsfeld

This looks great. Thanks for fixing this!

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Aug 16, 2018

Member

I noticed this test failure on VSTS:

2018-08-16T20:19:10.6236474Z   1) AtomApplication prevents quitting if user cancels when prompted to save an item:
2018-08-16T20:19:10.6236742Z      Error: timeout of 60000ms exceeded. Ensure the done() callback is being called in this test.
2018-08-16T20:19:10.6237013Z       at Timeout.<anonymous> (D:\a\1\s\node_modules\mocha\lib\runnable.js:226:19)

Not sure if this is caused by the change or a test flake so I've restarted the build to see if it goes green then next time.

Member

daviwil commented Aug 16, 2018

I noticed this test failure on VSTS:

2018-08-16T20:19:10.6236474Z   1) AtomApplication prevents quitting if user cancels when prompted to save an item:
2018-08-16T20:19:10.6236742Z      Error: timeout of 60000ms exceeded. Ensure the done() callback is being called in this test.
2018-08-16T20:19:10.6237013Z       at Timeout.<anonymous> (D:\a\1\s\node_modules\mocha\lib\runnable.js:226:19)

Not sure if this is caused by the change or a test flake so I've restarted the build to see if it goes green then next time.

@stepanhruda

This comment has been minimized.

Show comment
Hide comment
@stepanhruda

stepanhruda Aug 16, 2018

Contributor

I have only been testing on macOS, it's very possible that Windows behaves slightly differently 😡 I might need to debug this in a Windows environment

Contributor

stepanhruda commented Aug 16, 2018

I have only been testing on macOS, it's very possible that Windows behaves slightly differently 😡 I might need to debug this in a Windows environment

captbaritone added a commit to captbaritone/atom that referenced this pull request Aug 23, 2018

Remove unload-aborted event
This edge case will be handled by atom#17873
During quit, close unloaded windows
Released under CC0.

@stepanhruda stepanhruda merged commit 069d3ec into master Aug 28, 2018

3 checks passed

Atom Pull Requests 20180827.8 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stepanhruda stepanhruda deleted the fb-sh-close-on-unload branch Aug 28, 2018

daviwil added a commit that referenced this pull request Sep 18, 2018

Revert "Merge pull request #17873 from atom/fb-sh-close-on-unload"
This reverts commit 069d3ec, reversing
changes made to 5c74112.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment