Bug 489533 - NullPointerException in PushImageCommandHandler$1.run

Prevent NPE by checking the connection is not null in the
 command handler
Disable the command handler in the toolbar if there is no selection
Refactored the ImagePushPage to use the same createControl() code
 style as in the ImageTagPage. Also, page is immediately valid
 if it displays a combo (ie, there is at least one image name/tag)

Change-Id: I77254c691a00344c40555811bc25d0ae2e6b1deb
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Reviewed-on: https://git.eclipse.org/r/68541
Tested-by: Hudson CI
Reviewed-by: Jeff Johnston <jjohnstn@redhat.com>
Reviewed-on: https://git.eclipse.org/r/68707

5 files changed, 58 insertions, 69 deletions

diff --git a/containers/org.eclipse.linuxtools.docker.ui/plugin.xml b/containers/org.eclipse.linuxtools.docker.ui/plugin.xml
index 7ca20e0c3f..64ac04ea80 100644
--- a/containers/org.eclipse.linuxtools.docker.ui/plugin.xml
+++ b/containers/org.eclipse.linuxtools.docker.ui/plugin.xml
@@ -448,6 +448,20 @@
 		<handler
               commandId="org.eclipse.linuxtools.docker.ui.commands.pushImage"
               class="org.eclipse.linuxtools.internal.docker.ui.commands.PushImageCommandHandler">
+              <enabledWhen>
+	               <with
+	                   variable="selection">
+	               <count
+	                      value="1">
+	                </count>
+	                <iterate
+	                      ifEmpty="false"
+	                      operator="and">
+	                 <instanceof
+	                       value="org.eclipse.linuxtools.docker.core.IDockerImage" />
+	                </iterate>
+	               </with>
+	            </enabledWhen>
      	</handler>
 		<handler
               commandId="org.eclipse.linuxtools.docker.ui.commands.tagImage"
diff --git a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/BaseImagesCommandHandler.java b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/BaseImagesCommandHandler.java
index d907240bfc..58d6b952ee 100644
--- a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/BaseImagesCommandHandler.java
+++ b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/BaseImagesCommandHandler.java
@@ -30,7 +30,7 @@ import org.eclipse.ui.IWorkbenchPart;
 import org.eclipse.ui.handlers.HandlerUtil;
 
 /**
- * Command handler to kill all the selected {@link IDockerImage}
+ * Base handler for commands on {@link IDockerImage}
  * 
  * @author jjohnstn
  *
diff --git a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/PushImageCommandHandler.java b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/PushImageCommandHandler.java
index 716696413a..9f6a6e1df3 100644
--- a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/PushImageCommandHandler.java
+++ b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/commands/PushImageCommandHandler.java
@@ -22,21 +22,22 @@ import org.eclipse.linuxtools.docker.core.IDockerConnection;
 import org.eclipse.linuxtools.docker.core.IDockerImage;
 import org.eclipse.linuxtools.internal.docker.core.DockerConnection;
 import org.eclipse.linuxtools.internal.docker.ui.views.DVMessages;
-import org.eclipse.linuxtools.internal.docker.ui.views.DockerImagesView;
 import org.eclipse.linuxtools.internal.docker.ui.views.ImagePushProgressHandler;
 import org.eclipse.linuxtools.internal.docker.ui.wizards.ImagePush;
 import org.eclipse.swt.widgets.Display;
 import org.eclipse.ui.IWorkbenchPart;
 import org.eclipse.ui.handlers.HandlerUtil;
 
+/**
+ * Command handler to push a given image to the registry
+ */
 public class PushImageCommandHandler extends AbstractHandler {
 
 	private final static String PUSH_IMAGE_JOB_TITLE = "ImagePush.title"; //$NON-NLS-1$
 	private final static String PUSH_IMAGE_JOB_TASK = "ImagePush.msg"; //$NON-NLS-1$
 	private static final String ERROR_PUSHING_IMAGE = "ImagePushError.msg"; //$NON-NLS-1$
+	private static final String NO_CONNECTION = "NoConnection.error"; //$NON-NLS-1$
 	
-	private IDockerConnection connection;
-
 	@Override
 	public Object execute(final ExecutionEvent event) {
 		final IWorkbenchPart activePart = HandlerUtil.getActivePart(event);
@@ -46,16 +47,27 @@ public class PushImageCommandHandler extends AbstractHandler {
 		final boolean pushImage = CommandUtils.openWizard(wizard,
 				HandlerUtil.getActiveShell(event));
 		if (pushImage) {
-			if (activePart instanceof DockerImagesView) {
-				connection = ((DockerImagesView) activePart)
-						.getConnection();
-			}
-			performPushImage(wizard);
+			final IDockerConnection connection = CommandUtils
+					.getCurrentConnection(activePart);
+			performPushImage(wizard, connection);
 		}
 		return null;
 	}
 	
-	private void performPushImage(final ImagePush wizard) {
+	private void performPushImage(final ImagePush wizard,
+			final IDockerConnection connection) {
+		if (connection == null) {
+			Display.getDefault().syncExec(new Runnable() {
+
+				@Override
+				public void run() {
+					MessageDialog.openError(
+							Display.getDefault().getActiveShell(), "Error",
+							DVMessages.getFormattedString(NO_CONNECTION));
+				}
+			});
+			return;
+		}
 		final Job pushImageJob = new Job(DVMessages.getFormattedString(
 				PUSH_IMAGE_JOB_TITLE, wizard.getImageTag())) {
 
@@ -64,7 +76,7 @@ public class PushImageCommandHandler extends AbstractHandler {
 				final String tag = wizard.getImageTag();
 				monitor.beginTask(DVMessages.getString(PUSH_IMAGE_JOB_TASK),
 						IProgressMonitor.UNKNOWN);
-				// pull the image and let the progress
+				// push the image and let the progress
 				// handler refresh the images when done
 				try {
 					((DockerConnection) connection).pushImage(tag,
@@ -92,9 +104,7 @@ public class PushImageCommandHandler extends AbstractHandler {
 			}
 
 		};
-
 		pushImageJob.schedule();
-
 	}
 
 
diff --git a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/views/DVMessages.properties b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/views/DVMessages.properties
index ea9c093ee8..ebcd1b103e 100644
--- a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/views/DVMessages.properties
+++ b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/views/DVMessages.properties
@@ -10,6 +10,7 @@
 ###############################################################################
 
 NoConnection.label=<a>No connection to a Docker daemon is available. Click this link to create a new connection...</a>
+NoConnection.error=No connection to a Docker daemon is available.
 
 Refresh.label=Refresh View
 Pull.label=Pull Image...
diff --git a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/wizards/ImagePushPage.java b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/wizards/ImagePushPage.java
index 241e489830..8dde47cf54 100644
--- a/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/wizards/ImagePushPage.java
+++ b/containers/org.eclipse.linuxtools.docker.ui/src/org/eclipse/linuxtools/internal/docker/ui/wizards/ImagePushPage.java
@@ -10,21 +10,17 @@
  *******************************************************************************/
 package org.eclipse.linuxtools.internal.docker.ui.wizards;
 
-import java.util.List;
-
+import org.eclipse.jface.layout.GridDataFactory;
+import org.eclipse.jface.layout.GridLayoutFactory;
 import org.eclipse.jface.wizard.WizardPage;
 import org.eclipse.linuxtools.docker.core.IDockerImage;
 import org.eclipse.linuxtools.internal.docker.ui.SWTImagesFactory;
 import org.eclipse.swt.SWT;
 import org.eclipse.swt.events.ModifyEvent;
 import org.eclipse.swt.events.ModifyListener;
-import org.eclipse.swt.graphics.Point;
-import org.eclipse.swt.layout.FormAttachment;
-import org.eclipse.swt.layout.FormData;
-import org.eclipse.swt.layout.FormLayout;
+import org.eclipse.swt.layout.GridLayout;
 import org.eclipse.swt.widgets.Combo;
 import org.eclipse.swt.widgets.Composite;
-import org.eclipse.swt.widgets.Control;
 import org.eclipse.swt.widgets.Label;
 import org.eclipse.swt.widgets.Text;
 
@@ -68,19 +64,15 @@ public class ImagePushPage extends WizardPage {
 	private void validate() {
 		boolean complete = true;
 		boolean error = false;
-
 		String name = null;
-
 		if (nameText != null) {
 			name = nameText.getText();
 		} else {
 			name = nameCombo.getText();
 		}
-
 		if (name.length() == 0) {
 			complete = false;
 		}
-
 		if (!error) {
 			setErrorMessage(null);
 			tag = name;
@@ -89,62 +81,34 @@ public class ImagePushPage extends WizardPage {
 	}
 
 	@Override
-	public void createControl(Composite parent) {
-		final Composite container = new Composite(parent, SWT.NULL);
-		FormLayout layout = new FormLayout();
-		layout.marginHeight = 5;
-		layout.marginWidth = 5;
-		container.setLayout(layout);
-
-		Label label = new Label(container, SWT.NULL);
-
-		Label nameLabel = new Label(container, SWT.NULL);
+	public void createControl(final Composite parent) {
+		parent.setLayout(new GridLayout());
+		final Composite container = new Composite(parent, SWT.NONE);
+		GridLayoutFactory.fillDefaults().numColumns(2).margins(6, 6)
+				.applyTo(container);
+		GridDataFactory.fillDefaults().align(SWT.FILL, SWT.CENTER).span(1, 1)
+				.grab(true, false).applyTo(container);
+		final Label nameLabel = new Label(container, SWT.NULL);
 		nameLabel.setText(WizardMessages.getString(NAME_LABEL));
-
-		// If we are given an image, use its tags for choices in a combo box,
-		// otherwise,
-		// allow the user to enter an existing tag.
-		Control c = null;
+		GridDataFactory.fillDefaults().align(SWT.FILL, SWT.CENTER)
+				.grab(false, false).applyTo(nameLabel);
 		if (image == null || image.repoTags().size() == 0) {
 			nameText = new Text(container, SWT.BORDER | SWT.SINGLE);
 			nameText.addModifyListener(Listener);
 			nameText.setToolTipText(WizardMessages.getString(NAME_TOOLTIP));
-			c = nameText;
+			GridDataFactory.fillDefaults().align(SWT.FILL, SWT.CENTER)
+					.grab(true, false).applyTo(nameText);
 		} else {
 			nameCombo = new Combo(container, SWT.DROP_DOWN | SWT.READ_ONLY);
 			nameCombo.addModifyListener(Listener);
 			nameCombo.setToolTipText(WizardMessages.getString(NAME_TOOLTIP));
-			List<String> repoTags = image.repoTags();
-			nameCombo.setItems(repoTags.toArray(new String[0]));
-			nameCombo.setText(repoTags.get(0));
-			c = nameCombo;
+			nameCombo.setItems(image.repoTags().toArray(new String[0]));
+			nameCombo.setText(image.repoTags().get(0));
+			GridDataFactory.fillDefaults().align(SWT.FILL, SWT.CENTER)
+					.grab(true, false).applyTo(nameCombo);
 		}
-
-		Point p1 = label.computeSize(SWT.DEFAULT, SWT.DEFAULT);
-		Point p2;
-		if (nameText != null)
-			p2 = nameText.computeSize(SWT.DEFAULT, SWT.DEFAULT);
-		else
-			p2 = nameCombo.computeSize(SWT.DEFAULT, SWT.DEFAULT);
-		int centering = (p2.y - p1.y + 1) / 2;
-
-		FormData f = new FormData();
-		f.top = new FormAttachment(0);
-		label.setLayoutData(f);
-
-		f = new FormData();
-		f.top = new FormAttachment(label, 11 + centering);
-		f.left = new FormAttachment(0, 0);
-		nameLabel.setLayoutData(f);
-
-		f = new FormData();
-		f.top = new FormAttachment(label, 11 + centering);
-		f.left = new FormAttachment(nameLabel, 5);
-		f.right = new FormAttachment(100);
-		c.setLayoutData(f);
-
 		setControl(container);
-		setPageComplete(nameText == null);
+		validate();
 	}
 
 }