Closing Eclipse IDE shows flashing popup if closing is really fast

[vogella]

These days Eclipse closes really fast for me, I assume due to https://github.com/eclipse-platform/eclipse.platform.ui/issues/1084 and other performance enhancements.

It looks like we still show the "Shutting down" dialog, at least a dialog flashes up, but it vanishes so fast again, that I cannot read it. Do avoid such flashing dialogs it would be nicer, if we only show the dialog if shutting down takes longer than for example 1000 ms.

@HeikoKlare WDYT?

[HeikoKlare]

I think it a good general pattern to show progress dialogs only if they are (expected to be) open for a relevant amount of time, so it would also make sense when shutting down. Just to be sure: Do you mean the progress information dialog for saving the workbench state shown during shutdown or is there some further dialog?

I am not sure where this dialog is spawned. But I could image that it is shown by the busy indicator that is created during workbench shutdown: https://github.com/eclipse-platform/eclipse.platform.ui/blob/728f0760e1fb1b641ebe2ffa5c275a23e5762d58/bundles/org.eclipse.ui.workbench/Eclipse%20UI/org/eclipse/ui/internal/Workbench.java#L1390-L1395

In general, to avoid flashing progress dialogs, there is already the longOperationTime defined in the IProgressService: https://github.com/eclipse-platform/eclipse.platform.ui/blob/728f0760e1fb1b641ebe2ffa5c275a23e5762d58/bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/IProgressService.java#L67

It may be an option to use that functionality during shutdown. But it may also be the case that this functionality is already applied and the dialog flashes up for a short period of time, because the progress took slightly longer than the defined longOperationTime. That's something we should check. Anyway, if you are not able to estimate how long a progress will take, showing a dialog when the progress takes longer than a defined amount of time will always result in the dialog flashing for only a moment whenever the progress takes slightly longer than that amount of time.

[vogella]

Looks to me that the popup comes up, after the advisor.postShutdown() is called.

https://github.com/eclipse-platform/eclipse.platform.ui/blob/728f0760e1fb1b641ebe2ffa5c275a23e5762d58/bundles/org.eclipse.ui.workbench/Eclipse%20UI/org/eclipse/ui/internal/Workbench.java#L3012

[vogella]

IDEWorkbenchAdvisor#disconnectFromWorkspace opens a CancelableProgressMonitorJobsDialog. The whole code looks fishy to me, the cancel button seem not to do anything, expect updating the subTaskLabel to "". @HeikoKlare does the code make sense to you? It looks like this dialog "should" allow to cancel history pruning but it does not seem to do this, at least as far as I can see.

[HeikoKlare]

Thanks for the clarification! At a first glance, the code does not make sense to me either. The cancel event does not seem to be handled properly, as it only sets the label text and the canceled state of the monitor, which slightly changes the processing of the progress update in CancelableProgressMonitorWrapper: https://github.com/eclipse-platform/eclipse.platform.ui/blob/728f0760e1fb1b641ebe2ffa5c275a23e5762d58/bundles/org.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEWorkbenchAdvisor.java#L497-L500

However, the monitor is also passed to the save operation: https://github.com/eclipse-platform/eclipse.platform.ui/blob/728f0760e1fb1b641ebe2ffa5c275a23e5762d58/bundles/org.eclipse.ui.ide.application/src/org/eclipse/ui/internal/ide/application/IDEWorkbenchAdvisor.java#L553-L553

And this save operation actually interprets the cancel state and seems to cancel history pruning in case the monitor is canceled:

monitor.ignoreCancelState(false);
workspace.getFileSystemManager().getHistoryStore().clean(Policy.subMonitorFor(monitor, 1));
monitor.ignoreCancelState(keepConsistencyWhenCanceled);

I can imagine that it is either hard to press the cancel button in exactly that (potentially rather short) time period in which the pruning happens, as only then the cancel event is actually processed in the SaveManager. I would expect most of the time of the save operation to be taken for actual saving and not for history pruning. Or the CancelableProgressMonitorWrapper is or became incorrent: it uses hard coded work amounts (Math.abs(total - 4.0)) to identify the history pruning phase, but maybe the SaveManager changed the amount of work it processes, so that the cancel button is not enabled in the pruning phase but some other phase,.

Either way, this whole implementation feels very weird and is far from comprehensible. I am not completely sure whether the code is somehow "correct" or not, but I think this behavior should be adapted anyway as it does not really make sense. Even if canceling the history pruning step works properly, it does not feel intuitive that you can press a cancel button in a progress dialog for an operation, but this only cancels a subtask and the whole progress keeps running for quite some time.

[vogella]

Thanks for the detailed analysis @HeikoKlare. As the current behavior is frequently incorrect and the cancel behavior questionable I suggest that we remove the whole strange dialog and simply save. I push up a draft for you to comment on, if you have time. That would be something for the next release.

[laeubi]

Closing Eclipse IDE shows flashing popup if closing is really fast

I thing the important part is the one in bold, if it is not fast the user won't notice that and probably wondering why restart does not happen or workspace is locked.

I think we have discussed this before, but actually this should be the feature of the progress dialog itself that it should not popup before a certain amount of time is passed by.

[merks]

Yes, I have many IDEs that close very slowly where lack of an indicator that it's busy closing but taking 10 seconds would be disconcerting at best.

[HeikoKlare]

I agree that we should probably not completely remove the progress but only remove the confusing cancellation option. I order to have a progress dialog that opens after a specific amount of time, can't we simply replace the whole complex logic with usage of the IProgresService#busyCursorWhile implemented by the ProgressManager, i.e.:

ProgressManager.getInstance().busyCursorWhile(monitor -> {
    try {
        status.merge(((Workspace) ResourcesPlugin.getWorkspace()).save(true, true, monitor));
    } catch (CoreException e) {
        status.merge(e.getStatus());
    }
});

More precisely, this will show a progress dialog if the operation takes longer than 800ms. Before, only a busy cursor is shown.