Show progress when pre-calculating the AST during save #2414

[fedejeanne]

What it does

Fixes #2414

Show a progress dialog (after a delay) if the precalculation of the AST during a save takes too long.

Requires May be tested together with

• Show ProgressMonitorJobsDialog after delay in ProgressManager.run eclipse-platform/eclipse.platform.ui#3147
• Fix/progress service run eclipse-platform/eclipse.platform.ui#4203

The aforementioned PRs must not necessarily be merged before this one, but it would be nice to have them since they would avoid the "flickering" of the progress dialog showing up and immediately disappearing when the calculations of the AST and/or the cleanup changes take just a few milliseconds.

How to test

• Activate a save action that requires an AST, e.g. "Organize imports"
• Open a big class and make a modification
• Save

Expected outcome
If the AST takes a while to calculate then you should see a (non-cancelable) progress dialog with a moving progress bar (IProgressMonitor.UNKNOWN).

---

Also, if calculating the changes takes a while, you should see another (non-cancelable) progress dialog with a moving progress bar (IProgressMonitor.UNKNOWN).

---

A small hack to help testing

You can simulate these long-running operations from the issue...

... by adding a delay like this one...

for (int i=0; i<5;i++) {
	try {
		Thread.sleep(1000);
	} catch (InterruptedException e) {
		e.printStackTrace();
	}
}

...to org.eclipse.jdt.internal.corext.fix.CleanUpPostSaveListener.createAst(ICompilationUnit, Map<String, String>, IProgressMonitor) (1) and another delay in org.eclipse.jdt.core.dom.ASTParser.createAST(IProgressMonitor) (2). Technically, the delay in the ASTParser should be enough to simulate both scenarios, but since the call to SharedASTProviderCore.getAST(...); inside CleanUpPostSaveListener.createAst(...) passes SharedASTProviderCore.WAIT_NO as parameter, it is really unlikely that the call will reach as deep as ASTParser::createAST.

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[fedejeanne]

Test failures are unrelated. I ran them all and they pass locally.

[carstenartur]

Test failures are unrelated. I ran them all and they pass locally.

In general you cannot conclude that there is no issue when tests run through locally. It might be right in this case but is not in general.
See
#2213 (comment)

[fedejeanne]

@carstenartur the test failures are unrelated and they are documented in #2423.

Since that issue has been fixed last week (see https://ci.eclipse.org/jdt/job/eclipse.jdt.ui-github/job/master/474/testReport/), I rebased on master. That should get rid of the errors.

[fedejeanne]

I'm drafting this PR since I have no intention of merging it before M1

[fedejeanne]

This PR is ready to be reviewed.

Even if the progress bar doesn't move (which is outside of the control of this PR), showing the progress dialog will IMO still improve the UX since the user at least will know that something is being done in the background. Better than a UI freeze.

[fedejeanne]

I used m.beginTask(..., IProgressMonitor.UNKNOWN); in order to show indefinite progress in the dialog and I also added a second call to the newly added runUsingProgressService(...) so that both expensive calls from the issue show a progress dialog if necessary.

I updated the description of this PR to reflect these changes.

This PR is ready to be reviewed.