Avoid flashing shutdown dialog when save is fast

Replace the CancelableProgressMonitorJobsDialog used during workspace
shutdown with a plain ProgressMonitorJobsDialog that opens only after
IProgressService#getLongOperationTime has elapsed. During a fast save
only a busy cursor is shown, which fixes issue 1269.

The previous implementation eagerly opened the dialog and wired a cancel
button whose only effect was to clear a sub-task label. Its
CancelableProgressMonitorWrapper relied on hard-coded work-amount math
to toggle cancellation around the history-pruning phase, an assumption
that drifted out of sync with SaveManager. That plumbing is removed
along with its now-unused messages, since cancelling shutdown is no
longer offered.

Author: vogella

bundles/org.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEWorkbenchAdvisor.java

@@ -42,7 +42,6 @@
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.MultiStatus;
 import org.eclipse.core.runtime.Platform;
-import org.eclipse.core.runtime.ProgressMonitorWrapper;
 import org.eclipse.core.runtime.Status;
 import org.eclipse.core.runtime.jobs.ISchedulingRule;
 import org.eclipse.core.runtime.jobs.Job;
@@ -58,13 +57,10 @@
 import org.eclipse.jface.util.Policy;
 import org.eclipse.jface.window.Window;
 import org.eclipse.swt.SWT;
-import org.eclipse.swt.events.SelectionAdapter;
-import org.eclipse.swt.events.SelectionEvent;
+import org.eclipse.swt.custom.BusyIndicator;
 import org.eclipse.swt.graphics.Resource;
-import org.eclipse.swt.widgets.Composite;
 import org.eclipse.swt.widgets.Display;
 import org.eclipse.swt.widgets.Listener;
-import org.eclipse.swt.widgets.Shell;
 import org.eclipse.ui.PlatformUI;
 import org.eclipse.ui.application.IWorkbenchConfigurer;
 import org.eclipse.ui.application.IWorkbenchWindowConfigurer;
@@ -83,6 +79,7 @@
 import org.eclipse.ui.internal.ide.IDEWorkbenchMessages;
 import org.eclipse.ui.internal.ide.IDEWorkbenchPlugin;
 import org.eclipse.ui.internal.ide.undo.WorkspaceUndoMonitor;
+import org.eclipse.ui.internal.progress.ProgressManagerUtil;
 import org.eclipse.ui.internal.progress.ProgressMonitorJobsDialog;
 import org.eclipse.ui.progress.IProgressService;
 import org.eclipse.ui.statushandlers.AbstractStatusHandler;
@@ -464,119 +461,61 @@ public IStatus runInWorkspace(IProgressMonitor monitor) throws CoreException {
 		}
 	}
 
-	protected static class CancelableProgressMonitorWrapper extends
-			ProgressMonitorWrapper {
-		private double total = 0;
-		private final ProgressMonitorJobsDialog dialog;
-
-		CancelableProgressMonitorWrapper(IProgressMonitor monitor,
-				ProgressMonitorJobsDialog dialog) {
-			super(monitor);
-			this.dialog = dialog;
-		}
-
-		@Override
-		public void internalWorked(double work) {
-			super.internalWorked(work);
-			total += work;
-			updateProgressDetails();
-		}
-
-		@Override
-		public void worked(int work) {
-			super.worked(work);
-			total += work;
-			updateProgressDetails();
-		}
-
-		@Override
-		public void beginTask(String name, int totalWork) {
-			super.beginTask(name, totalWork);
-			subTask(IDEWorkbenchMessages.IDEWorkbenchAdvisor_preHistoryCompaction);
-		}
-
-		private void updateProgressDetails() {
-			if (!isCanceled() && Math.abs(total - 4.0) < 0.0001 /* right before history compacting */) {
-				subTask(IDEWorkbenchMessages.IDEWorkbenchAdvisor_cancelHistoryPruning);
-				dialog.setCancelable(true);
-			}
-			if (Math.abs(total - 5.0) < 0.0001 /* history compacting finished */) {
-				subTask(IDEWorkbenchMessages.IDEWorkbenchAdvisor_postHistoryCompaction);
-				dialog.setCancelable(false);
-			}
-		}
-	}
-
-	protected static class CancelableProgressMonitorJobsDialog extends
-			ProgressMonitorJobsDialog {
-
-		public CancelableProgressMonitorJobsDialog(Shell parent) {
-			super(parent);
-		}
-
-		@Override
-		protected void createButtonsForButtonBar(Composite parent) {
-			super.createButtonsForButtonBar(parent);
-			registerCancelButtonListener();
-		}
-
-		public void registerCancelButtonListener() {
-			cancel.addSelectionListener(new SelectionAdapter() {
-				@Override
-				public void widgetSelected(SelectionEvent e) {
-					subTaskLabel.setText(""); //$NON-NLS-1$
-				}
-			});
-		}
-	}
-
 	/**
 	 * Disconnect from the core workspace.
 	 *
+	 * Shows the progress dialog only if the save operation takes longer than
+	 * the {@link IProgressService#getLongOperationTime() long operation time};
+	 * otherwise a busy cursor is shown while the save runs on a worker thread.
+	 * This avoids a flashing dialog for fast shutdowns.
+	 *
 	 * Locks workspace in a background thread, should not be called while
 	 * holding any workspace locks.
 	 */
 	protected void disconnectFromWorkspace() {
-		// save the workspace
 		final MultiStatus status = new MultiStatus(IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
 				IDEWorkbenchMessages.ProblemSavingWorkbench);
-		try {
-			final ProgressMonitorJobsDialog p = new CancelableProgressMonitorJobsDialog(
-					null);
 
-			final boolean applyPolicy = ResourcesPlugin.getWorkspace()
-					.getDescription().isApplyFileStatePolicy();
+		final ProgressMonitorJobsDialog dialog = new ProgressMonitorJobsDialog(null);
+		dialog.setOpenOnRun(false);
 
-			IRunnableWithProgress runnable = monitor -> {
-				try {
-					if (applyPolicy) {
-						monitor = new CancelableProgressMonitorWrapper(monitor, p);
-					}
+		IRunnableWithProgress runnable = monitor -> {
+			try {
+				status.merge(((Workspace) ResourcesPlugin.getWorkspace()).save(true, true, monitor));
+			} catch (CoreException e) {
+				status.merge(e.getStatus());
+			}
+		};
 
-					status.merge(((Workspace) ResourcesPlugin.getWorkspace()).save(true, true, monitor));
-				} catch (CoreException e) {
-					status.merge(e.getStatus());
-				}
-			};
+		final Display display = PlatformUI.getWorkbench().getDisplay();
+		final int longOperationTime = PlatformUI.getWorkbench().getProgressService().getLongOperationTime();
+		final Runnable openDialog = () -> {
+			if (ProgressManagerUtil.safeToOpen(dialog, null)) {
+				dialog.open();
+			}
+		};
+		display.timerExec(longOperationTime, openDialog);
 
-			p.run(true, false, runnable);
-		} catch (InvocationTargetException e) {
-			status
-					.merge(new Status(IStatus.ERROR,
-							IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
-							IDEWorkbenchMessages.InternalError, e
-									.getTargetException()));
-		} catch (InterruptedException e) {
-			status.merge(new Status(IStatus.ERROR,
-					IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
-					IDEWorkbenchMessages.InternalError, e));
+		try {
+			BusyIndicator.showWhile(display, () -> {
+				try {
+					dialog.run(true, false, runnable);
+				} catch (InvocationTargetException e) {
+					status.merge(new Status(IStatus.ERROR, IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
+							IDEWorkbenchMessages.InternalError, e.getTargetException()));
+				} catch (InterruptedException e) {
+					status.merge(new Status(IStatus.ERROR, IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
+							IDEWorkbenchMessages.InternalError, e));
+				}
+			});
+		} finally {
+			display.timerExec(-1, openDialog);
 		}
+
 		if (!status.isOK()) {
-			ErrorDialog.openError(null,
-					IDEWorkbenchMessages.ProblemsSavingWorkspace, null, status,
+			ErrorDialog.openError(null, IDEWorkbenchMessages.ProblemsSavingWorkspace, null, status,
 					IStatus.ERROR | IStatus.WARNING);
-			IDEWorkbenchPlugin.log(
-					IDEWorkbenchMessages.ProblemsSavingWorkspace, status);
+			IDEWorkbenchPlugin.log(IDEWorkbenchMessages.ProblemsSavingWorkspace, status);
 		}
 	}
 

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/IDEWorkbenchMessages.java

@@ -37,9 +37,6 @@ public class IDEWorkbenchMessages extends NLS {
 	// package: org.eclipse.ui.ide
 
 	public static String IDEWorkbenchAdvisor_noPerspective;
-	public static String IDEWorkbenchAdvisor_cancelHistoryPruning;
-	public static String IDEWorkbenchAdvisor_preHistoryCompaction;
-	public static String IDEWorkbenchAdvisor_postHistoryCompaction;
 
 	public static String IDE_noFileEditorSelectedUserCanceled;
 	public static String IDE_noFileEditorFound;

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/messages.properties

@@ -41,9 +41,6 @@
 # package: org.eclipse.ui.ide
 
 IDEWorkbenchAdvisor_noPerspective=No perspectives are open. To open a perspective, press this button:
-IDEWorkbenchAdvisor_cancelHistoryPruning=Cancel to skip compacting local history.
-IDEWorkbenchAdvisor_preHistoryCompaction=Saving workbench state.
-IDEWorkbenchAdvisor_postHistoryCompaction=Disconnecting from workspace.
 
 IDE_noFileEditorSelectedUserCanceled = No editor selected, operation canceled by user.
 IDE_noFileEditorFound = No editor found to edit the file resource.

tests/org.eclipse.ui.ide.application.tests/src/org/eclipse/ui/internal/ide/application/IDEWorkbenchAdvisorTest.java

@@ -41,6 +41,7 @@ public class IDEWorkbenchAdvisorTest {
 	private static final String PLUGIN_ID = "org.eclipse.ui.ide.application.tests";
 	private Display display = null;
 	private ISchedulingRule rule;
+	private final IWorkspace workspace = ResourcesPlugin.getWorkspace();
 
 	private static final class SaveHook implements ISaveParticipant, Closeable {
 		public ISaveContext saving = null;
@@ -200,6 +201,64 @@ public void postShutdown() {
 		}
 	}
 
+	/**
+	 * Workbench shutdown should complete cleanly when the workspace save takes
+	 * longer than the progress service's long operation time. In that case the
+	 * progress dialog is expected to open after the delay, but the save must
+	 * still run to completion and the save hooks must fire.
+	 *
+	 * Regression test for issue 1269 (flashing shutdown dialog).
+	 */
+	@Test
+	public void testShutdownWithSlowSave() throws CoreException {
+		try (SaveHook saveHook = new SaveHook()) {
+			// Exceeds the default IProgressService#getLongOperationTime (800ms),
+			// so the new code path schedules and opens the progress dialog.
+			final long sleepMillis = 1200L;
+			workspace.addSaveParticipant(PLUGIN_ID + ".slow", new ISaveParticipant() {
+				@Override
+				public void saving(ISaveContext context) {
+					try {
+						Thread.sleep(sleepMillis);
+					} catch (InterruptedException e) {
+						Thread.currentThread().interrupt();
+					}
+				}
+
+				@Override
+				public void prepareToSave(ISaveContext context) {
+				}
+
+				@Override
+				public void doneSaving(ISaveContext context) {
+				}
+
+				@Override
+				public void rollback(ISaveContext context) {
+				}
+			});
+			try {
+				IDEWorkbenchAdvisor advisor = new IDEWorkbenchAdvisor() {
+					@Override
+					public void postStartup() {
+						super.postStartup();
+						display.asyncExec(() -> PlatformUI.getWorkbench().close());
+					}
+				};
+				int returnCode = PlatformUI.createAndRunWorkbench(display, advisor);
+				assertEquals(PlatformUI.RETURN_OK, returnCode);
+				dispatchDisplay();
+
+				assertNotNull(saveHook.prepareToSave);
+				assertNotNull(saveHook.saving);
+				assertNotNull(saveHook.doneSaving);
+				assertNull(saveHook.rollback);
+			} finally {
+				workspace.removeSaveParticipant(PLUGIN_ID + ".slow");
+			}
+		}
+	}
+
 	/**
 	 * Workbench shutdown should disconnect workspace if it is not locked
 	 *