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

Prevent closing the app while it's updating #15416

Merged
merged 20 commits into from
Oct 26, 2022
Merged

Conversation

sergiou87
Copy link
Member

@sergiou87 sergiou87 commented Oct 5, 2022

Closes #7055
Closes #5197

Description

After digging into some complaints from users about the app leaving temporary files after updates, and with a hint from one of the said users, I noticed how closing GitHub Desktop while it's being updated can cause either a corrupted installation (unable to open the next time) or just cluttering up the user's harddrive.

This PR attempts to prevent users from closing Desktop while an update is in progress by showing a dialog when the user tries to quit the app. From that dialog, the user might choose to quit the app anyway. Otherwise, the app will quit automatically when the update is completed.

Screenshots

Screen.Recording.2022-10-05.at.17.41.22.mov

Release notes

Notes: [Fixed] Prevent closing the GitHub Desktop while it's being updated

@sergiou87 sergiou87 requested a review from niik October 5, 2022 12:05
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

First look at this seems like a decent approach. It does feel a bit weird that a dialog is responsible for quitting the app but I think I can learn to live with it 😄

app/src/ui/lib/update-store.ts Outdated Show resolved Hide resolved
app/src/main-process/app-window.ts Outdated Show resolved Hide resolved
app/src/lib/ipc-shared.ts Outdated Show resolved Hide resolved
@sergiou87
Copy link
Member Author

It does feel a bit weird that a dialog is responsible for quitting the app but I think I can learn to live with it 😄

@niik I intend to go back to the dispatcher -> app store for that, which should be a bit cleaner and more in line with the rest of our dialogs. This is just kind of a PoC, so as long as you don't see any holes in this logic (knowing closing/quitting are different actions on macOS), I'm happy 😂 Thanks for the quick review!! 🙌

@sergiou87
Copy link
Member Author

Changed it to look like this:

Screen.Recording.2022-10-10.at.13.37.15.mov

@sergiou87
Copy link
Member Author

After a few more changes, it finally looks like this:
image

@sergiou87 sergiou87 marked this pull request as ready for review October 11, 2022 11:30
@sergiou87 sergiou87 requested a review from niik October 11, 2022 11:42
Co-Authored-By: Maximilian Sachs <15086212+masachs@users.noreply.github.com>
@sergiou87
Copy link
Member Author

I just added a feature flag, but other than that this should be ready for another review @niik 🙏

Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Only a stylistic request. One other thing though, should we perhaps call cancelQuittingApp from the unmount callback of the InstallingUpdate dialog if the dialog is being unmounted without an explicit choice either way from the user? Like if the user clicks on an open in Desktop link on GitHub.com (i.e. open x-github-desktop-dev-auth://openRepo//) while the dialog is open?

app/src/ui/installing-update/installing-update.tsx Outdated Show resolved Hide resolved
Co-authored-by: Markus Olsson <j.markus.olsson@gmail.com>
@sergiou87
Copy link
Member Author

That's a very good point!

Co-Authored-By: Markus Olsson <634063+niik@users.noreply.github.com>
@sergiou87 sergiou87 requested a review from niik October 25, 2022 13:21
@sergiou87 sergiou87 merged commit fafd8b0 into development Oct 26, 2022
@sergiou87 sergiou87 deleted the update-before-closing branch October 26, 2022 13:10
@sergiou87 sergiou87 restored the update-before-closing branch October 27, 2022 08:32
@sergiou87 sergiou87 deleted the update-before-closing branch October 27, 2022 08:33
@sergiou87 sergiou87 mentioned this pull request Jan 23, 2023
3 tasks
@steveward steveward mentioned this pull request Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants