Immediate user feedback on failed push background jobs

With commit 09e0d369 we've eliminated asynchronous dialogs from
background jobs for pushing and fetching. Instead, we associate an
action to open the result dialog with the job; the user can invoke that
action from the progress view.

This works nicely, but makes it easy to overlook a failed push.

Try to improve that by making the job return an ERROR status when a
push failure has occurred. This makes the job/status framework notify
the user; the notification dialog will show the failure message and
additionally a button giving access to the job's action, which will
open the full push result dialog.

Bug: 498176
Change-Id: Ia12c8586a0bf3827fe8916d1fa5853cbfb1e7604
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

7 files changed, 191 insertions, 122 deletions

diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/UIText.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/UIText.java
index 77122751a4..05c1c0d93c 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/UIText.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/UIText.java
@@ -2176,7 +2176,10 @@ public class UIText extends NLS {
 	public static String ShowPushResultAction_name;
 
 	/** */
-	public static String PushWizard_cantConnectToAny;
+	public static String PushJob_cantConnectToAny;
+
+	/** */
+	public static String PushJob_unexpectedError;
 
 	/** */
 	public static String PushWizard_cantPrepareUpdatesMessage;
@@ -2200,9 +2203,6 @@ public class UIText extends NLS {
 	public static String PushWizard_missingRefsTitle;
 
 	/** */
-	public static String PushWizard_unexpectedError;
-
-	/** */
 	public static String PushWizard_windowTitleDefault;
 
 	/** */
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/jobs/RepositoryJob.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/jobs/RepositoryJob.java
index bd3bf3ad8f..4830ca5e2e 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/jobs/RepositoryJob.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/jobs/RepositoryJob.java
@@ -16,6 +16,7 @@ import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.egit.ui.Activator;
 import org.eclipse.egit.ui.internal.UIText;
 import org.eclipse.jface.action.IAction;
+import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.swt.widgets.Display;
 import org.eclipse.ui.PlatformUI;
 import org.eclipse.ui.progress.IProgressConstants;
@@ -59,8 +60,14 @@ public abstract class RepositoryJob extends Job {
 				} else {
 					setProperty(IProgressConstants.KEEP_PROPERTY, Boolean.TRUE);
 					setProperty(IProgressConstants.ACTION_PROPERTY, action);
-					return new Status(IStatus.OK, Activator.getPluginId(),
-							IStatus.OK, action.getText(), null);
+					IStatus finalStatus = getDeferredStatus();
+					String msg = finalStatus.getMessage();
+					if (msg == null || msg.isEmpty()) {
+						return new Status(finalStatus.getSeverity(),
+								finalStatus.getPlugin(), finalStatus.getCode(),
+								action.getText(), finalStatus.getException());
+					}
+					return finalStatus;
 				}
 			}
 			return status;
@@ -87,6 +94,18 @@ public abstract class RepositoryJob extends Job {
 	 */
 	abstract protected IAction getAction();
 
+	/**
+	 * Obtains an {@link IStatus} describing the final outcome of the operation.
+	 * This default implementation returns an {@link IStatus#OK OK} status.
+	 *
+	 * @return an {@link IStatus} describing the outcome of the job
+	 */
+	@NonNull
+	protected IStatus getDeferredStatus() {
+		return new Status(IStatus.OK, Activator.getPluginId(), IStatus.OK, "", //$NON-NLS-1$
+				null);
+	}
+
 	private boolean isModal() {
 		Boolean modal = (Boolean) getProperty(
 				IProgressConstants.PROPERTY_IN_DIALOG);
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushJob.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushJob.java
new file mode 100644
index 0000000000..0f034a1148
--- /dev/null
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushJob.java
@@ -0,0 +1,131 @@
+/*******************************************************************************
+ * Copyright (C) 2016, Thomas Wolf <thomas.wolf@paranor.ch>
+ *
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *******************************************************************************/
+package org.eclipse.egit.ui.internal.push;
+
+import java.lang.reflect.InvocationTargetException;
+
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.Status;
+import org.eclipse.egit.core.op.PushOperation;
+import org.eclipse.egit.core.op.PushOperationResult;
+import org.eclipse.egit.ui.Activator;
+import org.eclipse.egit.ui.JobFamilies;
+import org.eclipse.egit.ui.internal.UIText;
+import org.eclipse.egit.ui.internal.jobs.RepositoryJob;
+import org.eclipse.jface.action.IAction;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.eclipse.jgit.transport.URIish;
+import org.eclipse.osgi.util.NLS;
+
+/**
+ * Background job for pushing to repositories.
+ */
+public class PushJob extends RepositoryJob {
+
+	private final PushOperation operation;
+
+	private final PushOperationResult resultToCompare;
+
+	private final String destinationString;
+
+	private final Repository localDb;
+
+	private final boolean showConfigureButton;
+
+	private PushOperationResult operationResult;
+
+	/**
+	 * Creates a new {@link PushJob} that performs the given
+	 * {@link PushOperation} on the given {@link Repository}.
+	 *
+	 * @param name
+	 *            of the job
+	 * @param repository
+	 *            to push to
+	 * @param operation
+	 *            to perform
+	 * @param expectedResult
+	 *            of the operation
+	 * @param destinationString
+	 *            describing where to push to
+	 * @param showConfigureButton
+	 *            whether the result dialog should have a configuration button
+	 */
+	public PushJob(String name, Repository repository, PushOperation operation,
+			PushOperationResult expectedResult, String destinationString,
+			boolean showConfigureButton) {
+		super(name);
+		this.operation = operation;
+		this.resultToCompare = expectedResult;
+		this.destinationString = destinationString;
+		this.localDb = repository;
+		this.showConfigureButton = showConfigureButton;
+	}
+
+	@Override
+	protected IStatus performJob(final IProgressMonitor monitor) {
+		try {
+			operation.run(monitor);
+		} catch (final InvocationTargetException e) {
+			return new Status(IStatus.ERROR, Activator.getPluginId(),
+					UIText.PushJob_unexpectedError, e.getCause());
+		}
+
+		operationResult = operation.getOperationResult();
+		if (!operationResult.isSuccessfulConnectionForAnyURI()) {
+			return new Status(IStatus.ERROR, Activator.getPluginId(),
+					NLS.bind(UIText.PushJob_cantConnectToAny,
+							operationResult.getErrorStringForAllURis()));
+		}
+
+		return Status.OK_STATUS;
+	}
+
+	@Override
+	protected IAction getAction() {
+		Repository repo = localDb;
+		if (repo != null && (resultToCompare == null
+				|| !resultToCompare.equals(operationResult))) {
+			return new ShowPushResultAction(repo, operationResult,
+					destinationString, showConfigureButton);
+		}
+		return null;
+	}
+
+	@Override
+	protected IStatus getDeferredStatus() {
+		for (URIish uri : operationResult.getURIs()) {
+			PushResult outcome = operationResult.getPushResult(uri);
+			for (RemoteRefUpdate update : outcome.getRemoteUpdates()) {
+				switch (update.getStatus()) {
+				case NOT_ATTEMPTED:
+				case UP_TO_DATE:
+				case OK:
+					continue;
+				default:
+					return new Status(IStatus.ERROR, Activator.getPluginId(),
+							IStatus.ERROR, update.getMessage(), null);
+				}
+			}
+		}
+		return super.getDeferredStatus();
+	}
+
+	@Override
+	public boolean belongsTo(Object family) {
+		if (JobFamilies.PUSH.equals(family)) {
+			return true;
+		}
+		return super.belongsTo(family);
+	}
+
+}
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushOperationUI.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushOperationUI.java
index c948725f0d..f859481db8 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushOperationUI.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushOperationUI.java
@@ -19,19 +19,14 @@ import java.util.List;
 
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
-import org.eclipse.core.runtime.IStatus;
-import org.eclipse.core.runtime.Status;
 import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.egit.core.op.PushOperation;
 import org.eclipse.egit.core.op.PushOperationResult;
 import org.eclipse.egit.core.op.PushOperationSpecification;
 import org.eclipse.egit.ui.Activator;
-import org.eclipse.egit.ui.JobFamilies;
 import org.eclipse.egit.ui.UIPreferences;
 import org.eclipse.egit.ui.internal.UIText;
 import org.eclipse.egit.ui.internal.credentials.EGitCredentialsProvider;
-import org.eclipse.egit.ui.internal.jobs.RepositoryJob;
-import org.eclipse.jface.action.IAction;
 import org.eclipse.jgit.errors.NotSupportedException;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.CredentialsProvider;
@@ -218,48 +213,28 @@ public class PushOperationUI {
 	}
 
 	/**
-	 * Starts the operation asynchronously showing a confirmation dialog after
-	 * completion
+	 * Starts the operation asynchronously.
 	 */
 	public void start() {
 		final Repository repo = repository;
 		if (repo == null) {
 			return;
 		}
-		Job job = new RepositoryJob(NLS.bind(UIText.PushOperationUI_PushJobName,
-				destinationString)) {
-
-			private PushOperationResult result;
-
-			@Override
-			protected IStatus performJob(IProgressMonitor monitor) {
-				try {
-					result = execute(monitor);
-				} catch (CoreException e) {
-					return Activator.createErrorStatus(e.getStatus()
-							.getMessage(), e);
-				}
-				return Status.OK_STATUS;
-			}
-
-			@Override
-			protected IAction getAction() {
-				if (expectedResult == null || !expectedResult.equals(result)) {
-					return new ShowPushResultAction(repo, result,
-							destinationString, showConfigureButton);
-				}
-				return null;
-			}
-
-			@Override
-			public boolean belongsTo(Object family) {
-				if (JobFamilies.PUSH.equals(family)) {
-					return true;
-				}
-				return super.belongsTo(family);
-			}
-
-		};
+		try {
+			createPushOperation();
+		} catch (CoreException e) {
+			Activator.showErrorStatus(e.getLocalizedMessage(), e.getStatus());
+			return;
+		}
+		if (credentialsProvider != null) {
+			op.setCredentialsProvider(credentialsProvider);
+		} else {
+			op.setCredentialsProvider(new EGitCredentialsProvider());
+		}
+		Job job = new PushJob(
+				NLS.bind(UIText.PushOperationUI_PushJobName, destinationString),
+				repo, op, expectedResult, destinationString,
+				showConfigureButton);
 		job.setUser(true);
 		job.schedule();
 	}
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushWizard.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushWizard.java
index ebf1cd5b99..dcb5a8cdd9 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushWizard.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/PushWizard.java
@@ -12,13 +12,11 @@
 package org.eclipse.egit.ui.internal.push;
 
 import java.io.IOException;
-import java.lang.reflect.InvocationTargetException;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 
-import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.Status;
 import org.eclipse.core.runtime.jobs.Job;
@@ -27,7 +25,6 @@ import org.eclipse.egit.core.op.PushOperationResult;
 import org.eclipse.egit.core.op.PushOperationSpecification;
 import org.eclipse.egit.core.securestorage.UserPasswordCredentials;
 import org.eclipse.egit.ui.Activator;
-import org.eclipse.egit.ui.JobFamilies;
 import org.eclipse.egit.ui.UIPreferences;
 import org.eclipse.egit.ui.internal.SecureStoreUtils;
 import org.eclipse.egit.ui.internal.UIIcons;
@@ -36,8 +33,6 @@ import org.eclipse.egit.ui.internal.components.RefSpecPage;
 import org.eclipse.egit.ui.internal.components.RepositorySelection;
 import org.eclipse.egit.ui.internal.components.RepositorySelectionPage;
 import org.eclipse.egit.ui.internal.credentials.EGitCredentialsProvider;
-import org.eclipse.egit.ui.internal.jobs.RepositoryJob;
-import org.eclipse.jface.action.IAction;
 import org.eclipse.jface.dialogs.ErrorDialog;
 import org.eclipse.jface.wizard.IWizardPage;
 import org.eclipse.jface.wizard.Wizard;
@@ -165,12 +160,16 @@ public class PushWizard extends Wizard {
 			operation.setCredentialsProvider(new EGitCredentialsProvider(
 					credentials.getUser(), credentials.getPassword()));
 		final PushOperationResult resultToCompare;
-		if (confirmPage.isShowOnlyIfChangedSelected())
+		if (confirmPage.isShowOnlyIfChangedSelected()) {
 			resultToCompare = confirmPage.getConfirmedResult();
-		else
+		} else {
 			resultToCompare = null;
-		final Job job = new PushJob(localDb, operation, resultToCompare,
-				getDestinationString(repoPage.getSelection()));
+		}
+		final Job job = new PushJob(
+				NLS.bind(UIText.PushWizard_jobName,
+						getURIsString(operation.getSpecification().getURIs())),
+				localDb, operation, resultToCompare,
+				getDestinationString(repoPage.getSelection()), true);
 
 		job.setUser(true);
 		job.schedule();
@@ -272,65 +271,4 @@ public class PushWizard extends Wizard {
 		return destination;
 	}
 
-	static class PushJob extends RepositoryJob {
-		private final PushOperation operation;
-
-		private final PushOperationResult resultToCompare;
-
-		private final String destinationString;
-
-		private Repository localDb;
-
-		private PushOperationResult operationResult;
-
-		public PushJob(final Repository localDb, final PushOperation operation,
-				final PushOperationResult resultToCompare,
-				final String destinationString) {
-			super(NLS.bind(UIText.PushWizard_jobName, getURIsString(operation
-					.getSpecification().getURIs())));
-			this.operation = operation;
-			this.resultToCompare = resultToCompare;
-			this.destinationString = destinationString;
-			this.localDb = localDb;
-		}
-
-		@Override
-		protected IStatus performJob(final IProgressMonitor monitor) {
-			try {
-				operation.run(monitor);
-			} catch (final InvocationTargetException e) {
-				return new Status(IStatus.ERROR, Activator.getPluginId(),
-						UIText.PushWizard_unexpectedError, e.getCause());
-			}
-
-			operationResult = operation.getOperationResult();
-			if (!operationResult.isSuccessfulConnectionForAnyURI()) {
-				return new Status(IStatus.ERROR, Activator.getPluginId(),
-						NLS.bind(UIText.PushWizard_cantConnectToAny,
-								operationResult.getErrorStringForAllURis()));
-			}
-
-			return Status.OK_STATUS;
-		}
-
-		@Override
-		protected IAction getAction() {
-			Repository repo = localDb;
-			if (repo != null && (resultToCompare == null
-					|| !resultToCompare.equals(operationResult))) {
-				return new ShowPushResultAction(repo, operationResult,
-						destinationString, true);
-			}
-			return null;
-		}
-
-		@Override
-		public boolean belongsTo(Object family) {
-			if (JobFamilies.PUSH.equals(family)) {
-				return true;
-			}
-			return super.belongsTo(family);
-		}
-
-	}
 }
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/ShowPushResultAction.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/ShowPushResultAction.java
index b5e101cc4c..d103d2fe3b 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/ShowPushResultAction.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/push/ShowPushResultAction.java
@@ -12,6 +12,7 @@ import org.eclipse.egit.ui.internal.UIText;
 import org.eclipse.egit.ui.internal.jobs.RepositoryJobResultAction;
 import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.swt.SWT;
 import org.eclipse.swt.widgets.Shell;
 import org.eclipse.ui.PlatformUI;
 
@@ -49,12 +50,17 @@ public class ShowPushResultAction extends RepositoryJobResultAction {
 		this.showConfigure = showConfigureButton;
 	}
 
+	private boolean isModal(Shell shell) {
+		return (shell.getStyle() & (SWT.APPLICATION_MODAL | SWT.PRIMARY_MODAL
+				| SWT.SYSTEM_MODAL)) != 0;
+	}
+
 	@Override
-	protected void showResult(Repository repository) {
-		Shell shell = PlatformUI.getWorkbench().getActiveWorkbenchWindow()
+	protected void showResult(@NonNull Repository repository) {
+		Shell shell = PlatformUI.getWorkbench().getModalDialogShellProvider()
 				.getShell();
 		PushResultDialog dialog = new PushResultDialog(shell, repository,
-				operationResult, destination, false);
+				operationResult, destination, isModal(shell));
 		dialog.showConfigureButton(showConfigure);
 		dialog.open();
 	}
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/uitext.properties b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/uitext.properties
index 8dc34c3136..b4ba05a3d7 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/uitext.properties
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/uitext.properties
@@ -756,7 +756,8 @@ RepositoryJob_NullStatus=Internal error: job ''{0}'' returned a null status.
 RepositoryJobResultAction_RepositoryGone=Cannot find git repository ''{0}'' anymore: result cannot be shown.
 ShowPushResultAction_name=Show Push Result...
 
-PushWizard_cantConnectToAny=Can''t connect to any repository: {0}
+PushJob_cantConnectToAny=Can''t connect to any repository: {0}
+PushJob_unexpectedError=Unexpected error occurred.
 PushWizard_cantPrepareUpdatesMessage=Can't resolve ref specifications locally (local refs changed?) or create tracking ref update.
 PushWizard_cantPrepareUpdatesTitle=Preparing Ref Updates Error
 PushWizard_cantSaveMessage=Couldn't save specified specifications in configuration file.
@@ -764,7 +765,6 @@ PushWizard_cantSaveTitle=Configuration Storage Warning
 PushWizard_jobName=Pushing to {0}
 PushWizard_missingRefsMessage=Ref specifications don't match any source ref (local refs changed?).
 PushWizard_missingRefsTitle=Missing Refs Error
-PushWizard_unexpectedError=Unexpected error occurred.
 PushWizard_windowTitleDefault=Push to Another Repository
 PushWizard_windowTitleWithDestination=Push to: {0}