Avoid flashing shutdown dialog when save is fast#3929
Avoid flashing shutdown dialog when save is fast#3929vogella merged 4 commits intoeclipse-platform:masterfrom
Conversation
| status.merge(e.getStatus()); | ||
| } | ||
| }; | ||
| final Display display = PlatformUI.getWorkbench().getDisplay(); |
There was a problem hiding this comment.
Are you sure workbench is still there at this time?
|
Using this now in a custom SDK builds, it is please to close the IDE with this "non-flashing" patch. Thanks @HeikoKlare for the implementation suggestions. |
| } | ||
| }); | ||
| } finally { | ||
| display.timerExec(-1, openDialog); |
There was a problem hiding this comment.
display.timerExec(-1, openDialog);
Why timerExec and why "-1"? The code will be not executed on Linux!
There was a problem hiding this comment.
timerExec(-1, openDialogLater) is the documented SWT idiom for cancelling a previously scheduled runnable, and it behaves the same on all three platforms. The javadoc states "If milliseconds is less than zero, the runnable is not executed," and each Display implementation matches the Runnable by reference in timerList and cancels the native timer before returning: GTK calls g_source_remove, win32 calls OS.KillTimer, and cocoa calls timer.invalidate() / release(). That is exactly the intent here: schedule openDialogLater to open the dialog after the long-operation threshold, and cancel it in finally if the save already finished, so no dialog appears on fast shutdowns. I pushed a small rename (openDialog -> openDialogLater) and a one-line comment on the cancel to make the schedule/cancel pairing explicit.
There was a problem hiding this comment.
Pull request overview
Reduces UI flicker during fast shutdown by delaying the shutdown save progress dialog until the long-operation threshold is exceeded, while preserving correct workspace-save behavior and adding regression coverage for slow saves.
Changes:
- Replace the eager/cancelable shutdown save dialog with a delayed-open
ProgressMonitorJobsDialog+ busy cursor approach. - Remove now-unused cancellation/history-compaction message keys and their NLS fields.
- Add a shutdown regression test that forces a slow workspace save to exercise the delayed-open path.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/org.eclipse.ui.ide.application.tests/src/org/eclipse/ui/internal/ide/application/IDEWorkbenchAdvisorTest.java | Adds a regression test that simulates a slow save participant during shutdown. |
| bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/messages.properties | Removes obsolete message keys tied to the removed cancel/history-compaction path. |
| bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/IDEWorkbenchMessages.java | Removes corresponding unused NLS fields. |
| bundles/org.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEWorkbenchAdvisor.java | Implements delayed dialog opening during workspace disconnect/save at shutdown and removes broken cancel support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final Runnable openDialog = () -> { | ||
| if (ProgressManagerUtil.safeToOpen(dialog, null)) { | ||
| dialog.open(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
openDialog calls ProgressManagerUtil.safeToOpen(...), which internally reaches through PlatformUI.getWorkbench() (see org.eclipse.ui.internal.progress.ProgressManagerUtil#getModalShellExcluding). Since disconnectFromWorkspace() is invoked from postShutdown(), the workbench may already be stopped/tearing down, and this can throw (or behave unpredictably) during shutdown. Consider either guarding the callback with PlatformUI.isWorkbenchRunning() (and skipping the dialog when false) or avoiding ProgressManagerUtil here by checking for modal shells via the already-captured Display (e.g., display.getShells()) before calling dialog.open().
vogella
force-pushed
the
fix-shutdown-dialog-flash
branch
from
3a6ea3a to
5b053da
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Avoid calling PlatformUI.getWorkbench() from disconnectFromWorkspace(), which runs from postShutdown() and may execute (via the workspace-locked timerExec retry) after the workbench has been torn down. The display is fetched via Display.getCurrent(), matching the existing pattern in postShutdown(), and the long-operation time is captured in initialize().
Review feedback: during postShutdown() the workbench may already be gone, so reaching through PlatformUI via ProgressManagerUtil.safeToOpen is unsafe. Scan display.getShells() for modal shells directly instead, rename the scheduled runnable to openDialogLater to make the schedule/cancel pairing obvious, and document the timerExec(-1, ...) cancel call.
vogella
force-pushed
the
fix-shutdown-dialog-flash
branch
from
5b053da to
e1a5ece
Compare
The shutdown test hard-coded a 1200ms sleep based on the current 800ms default of IProgressService#getLongOperationTime(). Read the live value in postStartup() and add a fixed margin so the test still exercises the delayed-open path if the platform default changes.
Fixes #1269. The IDE's shutdown save dialog currently pops up and vanishes in a blink on fast machines. This change replaces the eager
CancelableProgressMonitorJobsDialogwith a plainProgressMonitorJobsDialogthat opens only afterIProgressService#getLongOperationTimehas elapsed; fast shutdowns now just show a busy cursor, slow ones still show a real progress dialog.The cancel path that went away was effectively broken anyway. Its button only cleared a sub-task label, and the
CancelableProgressMonitorWrapperused hard-coded work-amount math (Math.abs(total - 4.0)) to toggle cancellation around history pruning, an assumption that had drifted out of sync withSaveManager. Removing it also drops three no-longer-referenced message keys.A new
IDEWorkbenchAdvisorTest#testShutdownWithSlowSaveexercises the delayed-open path via a save participant that sleeps past the long-operation threshold, asserting that the save still completes and hooks fire.