Let QuickFix run in UI, save editor, rebuild on error and wait for jobs

Fixing one warning might make the resulting markers either invalid or no
longer apply. Also quickfixes are often written to be run in the UI
thread and might schedule jobs during execution.

This now rebuilds the project when one marker was fixed and run them in
the UI while waiting for any jobs scheduled in the meantime.

tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/CleanUpMojo.java

@@ -62,10 +62,12 @@ protected void handleResult(CleanupResult result)
 			return;
 		}
 		MarkdownBuilder builder = new MarkdownBuilder(reportFileName);
-		builder.add("The following cleanups where applied:");
+		builder.h3("The following cleanups where applied:");
 		result.cleanups().forEach(cleanup -> {
 			builder.addListItem(cleanup);
 		});
+		builder.newLine();
+		builder.newLine();
 		builder.write();
 	}
 

tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFix.java

@@ -15,16 +15,23 @@
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.Comparator;
-import java.util.concurrent.atomic.AtomicReference;
+import java.util.LinkedHashSet;
 
 import org.eclipse.core.resources.IMarker;
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.resources.IResource;
+import org.eclipse.core.runtime.CoreException;
+import org.eclipse.core.runtime.ICoreRunnable;
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.Status;
+import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.tycho.eclipsebuild.AbstractEclipseBuild;
 import org.eclipse.ui.IMarkerResolution;
 import org.eclipse.ui.IMarkerResolution2;
 import org.eclipse.ui.IMarkerResolutionRelevance;
+import org.eclipse.ui.PlatformUI;
 import org.eclipse.ui.ide.IDE;
+import org.eclipse.ui.progress.UIJob;
 
 /**
  * Applies the QuickFix with the highest relevance to a warning, because
@@ -40,44 +47,81 @@ public class QuickFix extends AbstractEclipseBuild<QuickFixResult> {
 	@Override
 	protected QuickFixResult createResult(IProject project) throws Exception {
 		QuickFixResult result = new QuickFixResult();
-		IMarker[] markers = project.findMarkers(IMarker.PROBLEM, true, IResource.DEPTH_INFINITE);
-		result.setNumberOfMarker(markers.length);
+		while (fixOneMarker(project, result)) {
+			runInUI("Save Editors", m -> {
+				if (PlatformUI.isWorkbenchRunning()) {
+					PlatformUI.getWorkbench().saveAllEditors(false);
+				}
+			});
+			debug("### Perform build to update markers ###");
+			buildProject(project);
+		}
+		return result;
+	}
+
+	private boolean fixOneMarker(IProject project, QuickFixResult result) throws CoreException {
+
+		debug("### Check for marker with resolutions...");
+		IMarker[] markers = getCurrentMarker(project, true);
 		for (IMarker marker : markers) {
-			debug("Marker: " + marker);
-			try {
-				IMarkerResolution[] resolutions = IDE.getMarkerHelpRegistry().getResolutions(marker);
-				debug("Marker has " + resolutions.length + " resolutions");
-				IMarkerResolution resolution = Arrays.stream(resolutions)
-						.max(Comparator.comparingInt(r -> getRelevance(r))).orElse(null);
-				if (resolution != null) {
-					debug("Apply best resolution to marker: " + getInfo(resolution));
-					// must use an own thread to make sure it is not called as a job
-					AtomicReference<Throwable> error = new AtomicReference<Throwable>();
-					Thread thread = new Thread(new Runnable() {
-
-						@Override
-						public void run() {
-							try {
+			if (result.tryFix(marker)) {
+				debug("Check Marker: " + getInfo(marker));
+				try {
+					IMarkerResolution[] resolutions = IDE.getMarkerHelpRegistry().getResolutions(marker);
+					debug("\tMarker has " + resolutions.length + " resolutions");
+					IMarkerResolution resolution = Arrays.stream(resolutions)
+							.max(Comparator.comparingInt(r -> getRelevance(r))).orElse(null);
+					if (resolution != null) {
+						for (IMarkerResolution r : resolutions) {
+							debug(String.format("\t\t- (%d): %s", getRelevance(r), getInfo(r, false)));
+						}
+						LinkedHashSet<Job> jobs = new LinkedHashSet<>();
+						IStatus status = runInUI("fix marker " + getInfo(marker), m -> {
+							AbstractEclipseBuild.recordJobs(jobs, m, nil -> {
+								debug("\tApply best resolution to marker: " + getInfo(resolution, true));
 								resolution.run(marker);
-							} catch (Throwable t) {
-								error.set(t);
-							}
+							});
+						});
+						for (Job job : jobs) {
+							debug("Wait for Job '" + job.getName() + "' scheduled during marker resolution...");
+							job.join();
+						}
+						if (status.isOK()) {
+							String fix = buildFixMessage(marker);
+							debug("\t" + fix);
+							result.addFix(fix);
+							return true;
+						} else {
+							debug("\tMarker could not be applied!", status.getException());
 						}
-					});
-					thread.start();
-					thread.join();
-					Throwable t = error.get();
-					if (t == null) {
-						result.addFix(buildFixMessage(marker));
-					} else {
-						debug("Marker could not be applied!", t);
 					}
+				} catch (Throwable t) {
+					debug("\tMarker resolutions could not be computed!", t);
 				}
-			} catch (Throwable t) {
-				debug("Marker resolutions could not be computed!", t);
 			}
 		}
-		return result;
+		return false;
+	}
+
+	private String getInfo(IMarker marker) {
+		return marker.getAttribute(IMarker.MESSAGE, "") + " @ " + marker.getResource() + ":"
+				+ marker.getAttribute(IMarker.LINE_NUMBER, -1);
+	}
+
+	private IMarker[] getCurrentMarker(IProject project, boolean rebuildOnError) throws CoreException {
+		IMarker[] markers = project.findMarkers(IMarker.PROBLEM, true, IResource.DEPTH_INFINITE);
+		for (IMarker marker : markers) {
+			if (marker.getAttribute(IMarker.SEVERITY, -1) == IMarker.SEVERITY_ERROR) {
+				if (rebuildOnError) {
+					debug("Found error marker, try rebuild ...");
+					buildProject(project);
+					return getCurrentMarker(project, false);
+				}
+				debug("Found error, can't apply fixes: " + getInfo(marker));
+				return new IMarker[0];
+			}
+		}
+		return markers;
 	}
 
 	private String buildFixMessage(IMarker marker) {
@@ -95,8 +139,8 @@ private String buildFixMessage(IMarker marker) {
 		return sb.toString();
 	}
 
-	private String getInfo(IMarkerResolution resolution) {
-		if (resolution instanceof IMarkerResolution2 res2) {
+	private String getInfo(IMarkerResolution resolution, boolean withDescription) {
+		if (withDescription && resolution instanceof IMarkerResolution2 res2) {
 			return resolution.getClass().getName() + ": " + getLabel(resolution) + " // " + getDescription(res2);
 		} else {
 			return resolution.getClass().getName() + ": " + getLabel(resolution);
@@ -110,7 +154,7 @@ private int getRelevance(IMarkerResolution resolution) {
 			}
 		} catch (RuntimeException e) {
 		}
-		return -1;
+		return 0;
 	}
 
 	private String getDescription(IMarkerResolution2 markerResolution) {
@@ -129,4 +173,20 @@ private String getLabel(IMarkerResolution resolution) {
 		}
 	}
 
+	private static IStatus runInUI(String action, ICoreRunnable runnable) throws InterruptedException {
+		UIJob job = UIJob.create(action, m -> {
+			try {
+				runnable.run(m);
+			} catch (CoreException e) {
+				return e.getStatus();
+			} catch (Throwable e) {
+				return Status.error("Run failed", e);
+			}
+			return Status.OK_STATUS;
+		});
+		job.schedule();
+		job.join();
+		return job.getResult();
+	}
+
 }

tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixMojo.java

@@ -52,9 +52,15 @@ protected void handleResult(QuickFixResult result) throws MojoFailureException {
 		}
 		MarkdownBuilder builder = new MarkdownBuilder(reportFileName);
 		List<String> fixes = result.fixes().toList();
-		builder.add("The following " + (fixes.size() > 0 ? "warnings" : "warning") + " has been resolved:");
+		builder.h3("The following " + (fixes.size() > 0 ? "warnings" : "warning") + " has been resolved:");
+		builder.newLine();
 		fixes.forEach(fix -> builder.addListItem(fix));
+		builder.newLine();
+		builder.newLine();
 		builder.write();
+		for (String fix : fixes) {
+			getLog().info("Fixed: " + fix);
+		}
 	}
 
 	@Override

tycho-cleancode-plugin/src/main/java/org/eclipse/tycho/cleancode/QuickFixResult.java

@@ -13,13 +13,18 @@
 package org.eclipse.tycho.cleancode;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.stream.Stream;
 
+import org.eclipse.core.resources.IMarker;
+import org.eclipse.core.runtime.CoreException;
 import org.eclipse.tycho.eclipsebuild.EclipseBuildResult;
 
 public class QuickFixResult extends EclipseBuildResult {
 
+	private Set<String> tried = new HashSet<String>();
 	private List<String> fixed = new ArrayList<>();
 	private int markers;
 
@@ -31,16 +36,22 @@ public void addFix(String fix) {
 		fixed.add(fix);
 	}
 
-	public void setNumberOfMarker(int markers) {
-		this.markers = markers;
-	}
-
-	public int getMarkers() {
-		return markers;
-	}
-
 	public boolean isEmpty() {
 		return fixed.isEmpty();
 	}
 
+	public boolean tryFix(IMarker marker) {
+		String msg = marker.getAttribute(IMarker.MESSAGE, "");
+		String resource = marker.getResource().toString();
+		int line = marker.getAttribute(IMarker.LINE_NUMBER, -1);
+		String type;
+		try {
+			type = marker.getType();
+		} catch (CoreException e) {
+			type = "";
+		}
+		String key = type + " " + resource + ":" + line + " " + msg;
+		return tried.add(key);
+	}
+
 }

tycho-core/src/main/java/org/eclipse/tycho/core/MarkdownBuilder.java

@@ -57,4 +57,23 @@ public void write() throws MojoFailureException {
         }
     }
 
+    public void newLine() {
+        lines.add("");
+    }
+
+    public void h1(String string) {
+        lines.add("# " + string);
+        lines.add("");
+    }
+
+    public void h2(String string) {
+        lines.add("## " + string);
+        lines.add("");
+    }
+
+    public void h3(String string) {
+        lines.add("### " + string);
+        lines.add("");
+    }
+
 }

tycho-eclipse-plugin/src/main/java/org/eclipse/tycho/eclipsebuild/AbstractEclipseBuild.java

@@ -5,6 +5,8 @@
 import java.io.Serializable;
 import java.io.StringWriter;
 import java.nio.file.Path;
+import java.util.LinkedHashSet;
+import java.util.Set;
 import java.util.concurrent.Callable;
 
 import org.eclipse.core.resources.IMarker;
@@ -16,9 +18,12 @@
 import org.eclipse.core.resources.IncrementalProjectBuilder;
 import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.CoreException;
+import org.eclipse.core.runtime.ICoreRunnable;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.Platform;
+import org.eclipse.core.runtime.jobs.IJobChangeEvent;
+import org.eclipse.core.runtime.jobs.IJobChangeListener;
 import org.eclipse.core.runtime.jobs.Job;
 
 /**
@@ -54,10 +59,8 @@ public final Result call() throws Exception {
 	}
 
 	protected void buildProject(IProject project) throws CoreException {
-		project.build(IncrementalProjectBuilder.FULL_BUILD, this);
-		while (!Job.getJobManager().isIdle()) {
-			Thread.yield();
-		}
+		debug("Building...");
+		executeWithJobs(this, m -> project.build(IncrementalProjectBuilder.FULL_BUILD, m));
 	}
 
 	protected abstract Result createResult(IProject project) throws Exception;
@@ -69,6 +72,63 @@ static void disableAutoBuild() throws CoreException {
 		workspace.setDescription(desc);
 	}
 
+	public static void executeWithJobs(IProgressMonitor monitor, ICoreRunnable runnable)
+			throws CoreException {
+		Set<Job> scheduledJobs = recordJobs(new LinkedHashSet<>(), monitor, runnable);
+		for (Job job : scheduledJobs) {
+			if (monitor != null) {
+				monitor.subTask("Wait for job " + job.getName() + " to finish");
+			}
+			try {
+				job.join();
+			} catch (InterruptedException e) {
+				Thread.currentThread().interrupt();
+				return;
+			}
+		}
+	}
+
+	public static Set<Job> recordJobs(Set<Job> scheduledJobs, IProgressMonitor monitor, ICoreRunnable runnable)
+			throws CoreException {
+		IJobChangeListener listener = new IJobChangeListener() {
+
+			@Override
+			public void sleeping(IJobChangeEvent event) {
+
+			}
+
+			@Override
+			public void scheduled(IJobChangeEvent event) {
+				scheduledJobs.add(event.getJob());
+			}
+
+			@Override
+			public void running(IJobChangeEvent event) {
+
+			}
+
+			@Override
+			public void done(IJobChangeEvent event) {
+				scheduledJobs.remove(event.getJob());
+			}
+
+			@Override
+			public void awake(IJobChangeEvent event) {
+
+			}
+
+			@Override
+			public void aboutToRun(IJobChangeEvent event) {
+
+			}
+		};
+		Job.getJobManager().addJobChangeListener(listener);
+		IProgressMonitor safe = IProgressMonitor.nullSafe(monitor);
+		runnable.run(safe);
+		Job.getJobManager().removeJobChangeListener(listener);
+		return scheduledJobs;
+	}
+
 	private void deleteAllProjects() throws CoreException {
 		for (IProject project : ResourcesPlugin.getWorkspace().getRoot().getProjects()) {
 			project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT | IResource.FORCE, this);
@@ -94,6 +154,10 @@ protected void debug(String string) {
 	}
 
 	protected void debug(String string, Throwable t) {
+		if (t == null) {
+			debug(string);
+			return;
+		}
 		if (debug) {
 			StringWriter writer = new StringWriter();
 			t.printStackTrace(new PrintWriter(writer));