-
Notifications
You must be signed in to change notification settings - Fork 59
Show quit confirmation dialog when sites are running #2314
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
Changes from 2 commits
8139cd0
1b74ae7
f6cada3
35e4b2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import { | |
| import path from 'path'; | ||
| import { pathToFileURL } from 'url'; | ||
| import * as Sentry from '@sentry/electron/main'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { __, _n, sprintf } from '@wordpress/i18n'; | ||
| import { PROTOCOL_PREFIX } from 'common/constants'; | ||
| import { | ||
| bumpStat, | ||
|
|
@@ -45,8 +45,14 @@ import { renameLaunchUniquesStat } from 'src/migrations/rename-launch-uniques-st | |
| import { startSiteWatcher, stopSiteWatcher } from 'src/modules/cli/lib/execute-site-watch-command'; | ||
| import { updateWindowsCliVersionedPathIfNeeded } from 'src/modules/cli/lib/windows-installation-manager'; | ||
| import { setupWPServerFiles, updateWPServerFiles } from 'src/setup-wp-server-files'; | ||
| import { stopAllServersOnQuit } from 'src/site-server'; | ||
| import { loadUserData, lockAppdata, saveUserData, unlockAppdata } from 'src/storage/user-data'; | ||
| import { getRunningSiteCount, stopAllServersOnQuit } from 'src/site-server'; | ||
| import { | ||
| loadUserData, | ||
| lockAppdata, | ||
| saveUserData, | ||
| unlockAppdata, | ||
| updateAppdata, | ||
| } from 'src/storage/user-data'; | ||
| import { setupUpdates } from 'src/updates'; | ||
| // eslint-disable-next-line import/order | ||
| import packageJson from '../package.json'; | ||
|
|
@@ -95,6 +101,8 @@ const isInInstaller = require( 'electron-squirrel-startup' ); | |
| const gotTheLock = app.requestSingleInstanceLock(); | ||
|
|
||
| let finishedInitialization = false; | ||
| let isQuittingConfirmed = false; | ||
| let shouldStopSitesOnQuit = true; | ||
|
|
||
| if ( gotTheLock && ! isInInstaller ) { | ||
| void appBoot(); | ||
|
|
@@ -396,46 +404,99 @@ async function appBoot() { | |
| } ); | ||
|
|
||
| app.on( 'before-quit', ( event ) => { | ||
| if ( ! hasActiveSyncOperations() ) { | ||
| if ( isQuittingConfirmed ) { | ||
| return; | ||
| } | ||
|
|
||
| const QUIT_APP_BUTTON_INDEX = 0; | ||
| const CANCEL_BUTTON_INDEX = 1; | ||
| if ( hasActiveSyncOperations() ) { | ||
| const QUIT_APP_BUTTON_INDEX = 0; | ||
| const CANCEL_BUTTON_INDEX = 1; | ||
|
|
||
| const messageInformation: Pick< MessageBoxSyncOptions, 'message' | 'detail' | 'type' > = | ||
| hasUploadingPushOperations() | ||
| ? { | ||
| message: __( 'Sync is in progress' ), | ||
| detail: __( | ||
| "There's a sync operation in progress. Quitting the app will abort that operation. Are you sure you want to quit?" | ||
| ), | ||
| type: 'warning', | ||
| } | ||
| : { | ||
| message: __( 'Sync will continue' ), | ||
| detail: __( | ||
| 'The sync process will continue running remotely after you quit Studio. We will send you an email once it is complete.' | ||
| ), | ||
| type: 'info', | ||
| }; | ||
|
|
||
| const clickedButtonIndex = dialog.showMessageBoxSync( { | ||
| message: messageInformation.message, | ||
| detail: messageInformation.detail, | ||
| type: messageInformation.type, | ||
| buttons: [ __( 'Yes, quit the app' ), __( 'No, take me back' ) ], | ||
| cancelId: CANCEL_BUTTON_INDEX, | ||
| defaultId: QUIT_APP_BUTTON_INDEX, | ||
| } ); | ||
|
|
||
| const messageInformation: Pick< MessageBoxSyncOptions, 'message' | 'detail' | 'type' > = | ||
| hasUploadingPushOperations() | ||
| ? { | ||
| message: __( 'Sync is in progress' ), | ||
| detail: __( | ||
| "There's a sync operation in progress. Quitting the app will abort that operation. Are you sure you want to quit?" | ||
| ), | ||
| type: 'warning', | ||
| } | ||
| : { | ||
| message: __( 'Sync will continue' ), | ||
| detail: __( | ||
| 'The sync process will continue running remotely after you quit Studio. We will send you an email once it is complete.' | ||
| ), | ||
| type: 'info', | ||
| }; | ||
|
|
||
| const clickedButtonIndex = dialog.showMessageBoxSync( { | ||
| message: messageInformation.message, | ||
| detail: messageInformation.detail, | ||
| type: messageInformation.type, | ||
| buttons: [ __( 'Yes, quit the app' ), __( 'No, take me back' ) ], | ||
| cancelId: CANCEL_BUTTON_INDEX, | ||
| defaultId: QUIT_APP_BUTTON_INDEX, | ||
| } ); | ||
| if ( clickedButtonIndex === CANCEL_BUTTON_INDEX ) { | ||
| event.preventDefault(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if ( clickedButtonIndex === CANCEL_BUTTON_INDEX ) { | ||
| const runningSiteCount = getRunningSiteCount(); | ||
| if ( runningSiteCount > 0 ) { | ||
| event.preventDefault(); | ||
|
|
||
| void ( async () => { | ||
| const userData = await loadUserData(); | ||
|
|
||
| if ( userData.stopSitesOnQuit !== undefined ) { | ||
| shouldStopSitesOnQuit = userData.stopSitesOnQuit; | ||
| isQuittingConfirmed = true; | ||
| app.quit(); | ||
| return; | ||
| } | ||
|
|
||
| const STOP_SITES_BUTTON_INDEX = 0; | ||
| const LEAVE_RUNNING_BUTTON_INDEX = 1; | ||
|
|
||
| const { response, checkboxChecked } = await dialog.showMessageBox( { | ||
| type: 'question', | ||
| message: _n( 'You have a running site', 'You have running sites', runningSiteCount ), | ||
| detail: sprintf( | ||
| _n( | ||
| '%d site is currently running. Do you want to stop it before quitting?', | ||
| '%d sites are currently running. Do you want to stop them before quitting?', | ||
| runningSiteCount | ||
| ), | ||
| runningSiteCount | ||
| ), | ||
| buttons: [ __( 'Stop sites' ), __( 'Leave running' ) ], | ||
| checkboxLabel: __( 'Remember my choice' ), | ||
|
||
| cancelId: LEAVE_RUNNING_BUTTON_INDEX, | ||
| defaultId: STOP_SITES_BUTTON_INDEX, | ||
| } ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if we go with adding this option, let's ensure that the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to this one, I think we should provide some way for the user to get out of this screen.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestions, I added a |
||
|
|
||
| const stopSites = response === STOP_SITES_BUTTON_INDEX; | ||
|
|
||
| if ( checkboxChecked ) { | ||
| await updateAppdata( { stopSitesOnQuit: stopSites } ); | ||
| } | ||
|
|
||
| shouldStopSitesOnQuit = stopSites; | ||
| isQuittingConfirmed = true; | ||
| app.quit(); | ||
| } )(); | ||
|
|
||
| return; | ||
| } | ||
| } ); | ||
|
|
||
| app.on( 'quit', () => { | ||
| void stopAllServersOnQuit(); | ||
| if ( shouldStopSitesOnQuit ) { | ||
| void stopAllServersOnQuit(); | ||
| } | ||
| stopUserDataWatcher(); | ||
| stopSiteWatcher(); | ||
| } ); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine, but I'm assuming the AI wrote these tests, and I'm starting to question the value of tests written entirely by AI. This is a very testable function, so the tests are easy to read, but I know I'll be trying the inverse approach soon: TDD and letting the AI write the implementation based on my tests. |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional, but I would consider moving these variable declarations inside
appBoot, closer to where they're actually used.