Improve progress reporting for download jobs

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/Messages.java

@@ -18,6 +18,9 @@
 public class Messages extends NLS {
 	private static final String BUNDLE_NAME = "org.eclipse.equinox.internal.p2.artifact.repository.messages"; //$NON-NLS-1$
 
+	public static String DownloadJob_initial;
+	public static String DownloadJob_current_artifact;
+
 	public static String artifact_not_found;
 	public static String available_already_in;
 	public static String no_location;

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/messages.properties

@@ -66,4 +66,6 @@ retryRequest=Download of {0} failed on repository {1}. Retrying.
 error_copying_local_file=An error occurred copying file {0}.
 
 onlyInsecureDigestAlgorithmUsed = The digest algorithms ({0}) used to verify {1} have severely compromised security. Please report this concern to the artifact provider.
-noDigestAlgorithmToVerifyDownload = No digest algorithm is available to verify download of {0} from repository {1}.
+noDigestAlgorithmToVerifyDownload = No digest algorithm is available to verify download of {0} from repository {1}.
+DownloadJob_initial=Downloading Software
+DownloadJob_current_artifact=Downloading {0}

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/DownloadJob.java

@@ -15,26 +15,33 @@
 package org.eclipse.equinox.internal.p2.artifact.repository.simple;
 
 import java.util.LinkedList;
+import java.util.function.Consumer;
 import org.eclipse.core.runtime.*;
 import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.equinox.internal.p2.artifact.repository.Messages;
+import org.eclipse.equinox.p2.metadata.IArtifactKey;
 import org.eclipse.equinox.p2.repository.artifact.IArtifactRequest;
 
 public class DownloadJob extends Job {
+	private static final int SUB_TICKS = 1000;
+
 	static final Object FAMILY = new Object();
 
 	private final LinkedList<IArtifactRequest> requestsPending;
 	private final SimpleArtifactRepository repository;
-	private final IProgressMonitor masterMonitor;
-	private final MultiStatus overallStatus;
+
+	private Consumer<IStatus> resultConsumer;
+
+	private Consumer<String> messageConsumer;
 
 	DownloadJob(String name, SimpleArtifactRepository repository, LinkedList<IArtifactRequest> requestsPending,
-			IProgressMonitor masterMonitor, MultiStatus overallStatus) {
+			Consumer<IStatus> resultConsumer, Consumer<String> messageConsumer) {
 		super(name);
+		this.resultConsumer = resultConsumer;
+		this.messageConsumer = messageConsumer;
 		setSystem(true);
 		this.repository = repository;
 		this.requestsPending = requestsPending;
-		this.masterMonitor = masterMonitor;
-		this.overallStatus = overallStatus;
 	}
 
 	@Override
@@ -44,28 +51,97 @@ public boolean belongsTo(Object family) {
 
 	@Override
 	protected IStatus run(IProgressMonitor jobMonitor) {
-		jobMonitor.beginTask("Downloading software", IProgressMonitor.UNKNOWN); //$NON-NLS-1$
+		SubMonitor monitor = SubMonitor.convert(jobMonitor, Messages.DownloadJob_initial, 1_000_000);
 		do {
-			// get the request we are going to process
 			IArtifactRequest request;
 			synchronized (requestsPending) {
-				if (requestsPending.isEmpty())
-					break;
+				int totalDownloadWork = requestsPending.size();
+				if (totalDownloadWork == 0) {
+					return Status.OK_STATUS;
+				}
+				monitor.setWorkRemaining(totalDownloadWork * SUB_TICKS);
 				request = requestsPending.removeFirst();
 			}
-			if (masterMonitor.isCanceled())
-				return Status.CANCEL_STATUS;
-			// process the actual request
-			SubMonitor subMonitor = SubMonitor.convert(masterMonitor.slice(1), 1);
-			IStatus status = repository.getArtifact(request, subMonitor);
-			if (!status.isOK()) {
-				synchronized (overallStatus) {
-					overallStatus.add(status);
+			IArtifactKey key = request.getArtifactKey();
+			String currentArtifact = String.format("%s %s", key.getId(), key.getVersion()); //$NON-NLS-1$
+			monitor.setTaskName(currentArtifact);
+			SubMonitor split = monitor.split(SUB_TICKS, SubMonitor.SUPPRESS_NONE);
+			IStatus status = repository.getArtifact(request, new IProgressMonitor() {
+
+				private volatile boolean canceled;
+				private String taskName;
+				private String subTaskName;
+				private String lastMessage;
+
+				@Override
+				public void worked(int work) {
+					split.worked(work);
 				}
-			}
-		} while (true);
 
-		jobMonitor.done();
-		return Status.OK_STATUS;
+				@Override
+				public void subTask(String name) {
+					this.subTaskName = name;
+					if (messageConsumer != null) {
+						messageConsumer.accept(name);
+					}
+					updateTaskName();
+				}
+
+				@Override
+				public void setTaskName(String name) {
+					this.taskName = name;
+					updateTaskName();
+				}
+
+				@Override
+				public void setCanceled(boolean canceled) {
+					this.canceled = canceled;
+				}
+
+				@Override
+				public boolean isCanceled() {
+					return canceled;
+				}
+
+				@Override
+				public void internalWorked(double work) {
+					split.internalWorked(work);
+				}
+
+				@Override
+				public void done() {
+					split.done();
+
+				}
+
+				@Override
+				public void beginTask(String name, int totalWork) {
+					monitor.beginTask(name, totalWork);
+					this.taskName = name;
+					updateTaskName();
+				}
+
+				private void updateTaskName() {
+					StringBuilder sb = new StringBuilder();
+					if (taskName != null && !taskName.isBlank()) {
+						sb.append(taskName);
+					}
+					if (subTaskName != null && !subTaskName.isBlank()) {
+						if (sb.length() > 0) {
+							sb.append(" - "); //$NON-NLS-1$
+						}
+						sb.append(subTaskName);
+					}
+					String message = sb.toString();
+					if (message.length() > 0 && !message.equals(lastMessage)) {
+						lastMessage = message;
+						monitor.subTask(message);
+					}
+				}
+			});
+			resultConsumer.accept(status);
+		} while (!monitor.isCanceled());
+		return Status.CANCEL_STATUS;
 	}
+
 }

bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java

@@ -24,6 +24,8 @@
 import java.net.URISyntaxException;
 import java.util.*;
 import java.util.Map.Entry;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.function.Consumer;
 import java.util.jar.JarEntry;
 import java.util.jar.JarOutputStream;
 import org.eclipse.core.runtime.*;
@@ -887,27 +889,30 @@ public File getArtifactFile(IArtifactKey key) {
 	}
 
 	@Override
-	public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monitor) {
-		monitor = IProgressMonitor.nullSafe(monitor);
+	public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor unsafeMonitor) {
+		IProgressMonitor monitor = IProgressMonitor.nullSafe(unsafeMonitor);
 		if (!holdsLock() && URIUtil.isFileURI(getLocation())) {
 			load(new NullProgressMonitor());
 		}
 		if (monitor.isCanceled())
 			return Status.CANCEL_STATUS;
 
-		final MultiStatus overallStatus = new MultiStatus(Activator.ID, IStatus.OK, NLS.bind(Messages.message_problemReadingArtifact, getLocation()), null);
+		final MultiStatus overallStatus = new MultiStatus(Activator.ID, IStatus.OK,
+				NLS.bind(Messages.message_problemReadingArtifact, getLocation()), null);
 		LinkedList<IArtifactRequest> requestsPending = new LinkedList<>(Arrays.asList(requests));
 
 		int numberOfJobs = Math.min(requests.length, getMaximumThreads());
 		if (numberOfJobs <= 1 || (!isForceThreading() && isLocal())) {
 			SubMonitor subMonitor = SubMonitor.convert(monitor, requests.length);
 			try {
 				for (IArtifactRequest request : requests) {
-					if (monitor.isCanceled())
+					if (monitor.isCanceled()) {
 						return Status.CANCEL_STATUS;
+					}
 					IStatus result = getArtifact(request, subMonitor.newChild(1));
-					if (!result.isOK())
+					if (!result.isOK()) {
 						overallStatus.add(result);
+					}
 				}
 			} finally {
 				subMonitor.done();
@@ -917,28 +922,49 @@ public IStatus getArtifacts(IArtifactRequest[] requests, IProgressMonitor monito
 			monitor.beginTask(NLS.bind(Messages.sar_downloading, Integer.toString(requests.length)), requests.length);
 			try {
 				DownloadJob jobs[] = new DownloadJob[numberOfJobs];
+				List<IStatus> jobStatus = new CopyOnWriteArrayList<>();
+				Consumer<IStatus> resultConsumer = result -> {
+					synchronized (monitor) {
+						monitor.worked(1);
+					}
+					if (!result.isOK()) {
+						jobStatus.add(result);
+					}
+				};
+				Consumer<String> messageConsumer = jobMsg -> {
+					synchronized (monitor) {
+						// last message wins
+						monitor.subTask(jobMsg);
+					}
+				};
 				for (int i = 0; i < numberOfJobs; i++) {
-					jobs[i] = new DownloadJob(Messages.sar_downloadJobName + i, this, requestsPending, monitor,
-							overallStatus);
+					jobs[i] = new DownloadJob(Messages.sar_downloadJobName + i, this, requestsPending, resultConsumer,
+							messageConsumer);
 					jobs[i].schedule();
 				}
 				// wait for all the jobs to complete
 				try {
 					Job.getJobManager().join(DownloadJob.FAMILY, null);
 				} catch (InterruptedException e) {
-					//ignore
+					// ignore
 				}
+				overallStatus.addAll(overallStatus);
 			} finally {
 				monitor.done();
 			}
 		}
 
-		if (monitor.isCanceled())
+		if (monitor.isCanceled()) {
 			return Status.CANCEL_STATUS;
-		else if (overallStatus.isOK())
+		} else if (overallStatus.isOK()) {
 			return Status.OK_STATUS;
-		else
+		} else {
+			IStatus[] children = overallStatus.getChildren();
+			if (children.length == 1) {
+				return children[0];
+			}
 			return overallStatus;
+		}
 	}
 
 	public synchronized IArtifactDescriptor getCompleteArtifactDescriptor(IArtifactKey key) {

bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/artifact/repository/CompositeArtifactRepositoryTest.java

@@ -595,7 +595,8 @@ public void testGetArtifactsFromRequests() {
 	}
 
 	public void testGetArtifactsWithErrorInChild() throws Exception {
-		repositoryURI = getTestData("1", "/testData/artifactRepo/composite/errorInChild").toURI();
+		File testData = getTestData("1", "/testData/artifactRepo/composite/errorInChild");
+		repositoryURI = testData.toURI();
 		IArtifactRepository repo = getArtifactRepositoryManager().loadRepository(repositoryURI, null);
 
 		IArtifactRequest[] requests = new IArtifactRequest[] {new ArtifactRequest(new ArtifactKey("osgi.bundle", "plugin", Version.parseVersion("1.0.0")), null) {
@@ -611,8 +612,10 @@ public void perform(IArtifactRepository sourceRepository, IProgressMonitor monit
 		assertThat(status, is(statusWithMessageWhich(containsString("while reading artifacts from child repositories"))));
 
 		// bug 391400: status should point to repository with problem
-		String brokenChildURI = repositoryURI.toString() + "child";
-		assertThat(Arrays.asList(status.getChildren()), hasItem(statusWithMessageWhich(containsString(brokenChildURI))));
+		assertThat(Arrays.asList(status.getChildren()),
+				hasItem(statusWithMessageWhich(containsString("An error occurred copying file"))));
+		assertThat(Arrays.asList(status.getChildren()),
+				hasItem(statusWithMessageWhich(containsString(testData.getAbsolutePath()))));
 	}
 
 	public void testLoadingRepositoryRemote() {