tmf-remote: Bug 460842: clean-up ICommandShell and ICommandResult APIs

Change-Id: I0fe8cc51d6eb395c27c6387e7141e2caaa64c278
Signed-off-by: Bernd Hufmann <Bernd.Hufmann@ericsson.com>
Reviewed-on: https://git.eclipse.org/r/42539
Reviewed-by: Hudson CI
Reviewed-by: Alexandre Montplaisir <alexmonthy@voxpopuli.im>
Tested-by: Alexandre Montplaisir <alexmonthy@voxpopuli.im>

9 files changed, 116 insertions, 75 deletions

diff --git a/org.eclipse.tracecompass.tmf.remote.core/META-INF/MANIFEST.MF b/org.eclipse.tracecompass.tmf.remote.core/META-INF/MANIFEST.MF
index 64e7222f64..4e165e9e54 100644
--- a/org.eclipse.tracecompass.tmf.remote.core/META-INF/MANIFEST.MF
+++ b/org.eclipse.tracecompass.tmf.remote.core/META-INF/MANIFEST.MF
@@ -11,10 +11,12 @@ Bundle-RequiredExecutionEnvironment: JavaSE-1.7
 Require-Bundle: org.eclipse.core.runtime,
  org.eclipse.core.commands,
  org.eclipse.remote.core,
- org.eclipse.remote.ui
+ org.eclipse.remote.ui,
+ org.eclipse.tracecompass.common.core
 Export-Package: org.eclipse.tracecompass.internal.tmf.remote.core;x-internal:=true,
  org.eclipse.tracecompass.internal.tmf.remote.core.messages;x-internal:=true,
  org.eclipse.tracecompass.internal.tmf.remote.core.preferences;x-friends:="org.eclipse.tracecompass.tmf.remote.ui",
+ org.eclipse.tracecompass.internal.tmf.remote.core.shell;x-internal:=true,
  org.eclipse.tracecompass.tmf.remote.core.proxy,
  org.eclipse.tracecompass.tmf.remote.core.shell
 Import-Package: com.google.common.base,
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/CommandResult.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/CommandResult.java
index 0421d8e55b..86793ce7d1 100644
--- a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/CommandResult.java
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/CommandResult.java
@@ -9,14 +9,19 @@
  * Contributors:
  *   Bernd Hufmann - Initial API and implementation
  **********************************************************************/
-package org.eclipse.tracecompass.tmf.remote.core.shell;
+package org.eclipse.tracecompass.internal.tmf.remote.core.shell;
 
-import java.util.Arrays;
+import static org.eclipse.tracecompass.common.core.NonNullUtils.checkNotNull;
+import static org.eclipse.tracecompass.common.core.NonNullUtils.nullToEmptyString;
+
+import java.util.List;
+
+import org.eclipse.tracecompass.tmf.remote.core.shell.ICommandResult;
+
+import com.google.common.collect.ImmutableList;
 
 /**
- * <p>
  * Class containing command result of remote command execution.
- * </p>
  *
  * @author Bernd Hufmann
  */
@@ -25,16 +30,14 @@ public class CommandResult implements ICommandResult {
     // ------------------------------------------------------------------------
     // Attributes
     // ------------------------------------------------------------------------
-    /**
-     * The result of the command. 0 if successful else > 0
-     */
-    private int fResult;
+    /** The result of the command. 0 if successful else > 0 */
+    private final int fResult;
 
-    /**
-     * The output as String array.
-     */
-    private String[] fOutput = new String[0];
-    private String[] fErrorOutput = new String[0];
+    /** The output as list of Strings. */
+    private final List<String> fOutput;
+
+    /** The error stream output as list of Strings. */
+    private final List<String> fErrorOutput;
 
     // ------------------------------------------------------------------------
     // Constructor
@@ -52,12 +55,8 @@ public class CommandResult implements ICommandResult {
      */
     public CommandResult(int result, String[] output, String[] errorOutput) {
         fResult = result;
-        if (output != null) {
-            fOutput = Arrays.copyOf(output, output.length);
-        }
-        if (errorOutput != null) {
-            fErrorOutput = Arrays.copyOf(errorOutput, errorOutput.length);
-        }
+        fOutput = checkNotNull(ImmutableList.copyOf(output));
+        fErrorOutput = checkNotNull(ImmutableList.copyOf(errorOutput));
     }
 
     // ------------------------------------------------------------------------
@@ -71,11 +70,27 @@ public class CommandResult implements ICommandResult {
 
     @Override
     public String[] getOutput() {
-        return Arrays.copyOf(fOutput, fOutput.length);
+        return checkNotNull(fOutput.toArray(new String[fOutput.size()]));
     }
 
     @Override
     public String[] getErrorOutput() {
-        return Arrays.copyOf(fErrorOutput, fErrorOutput.length);
+        return checkNotNull(fErrorOutput.toArray(new String[fErrorOutput.size()]));
+    }
+
+    @Override
+    public String toString() {
+        StringBuffer ret = new StringBuffer();
+        ret.append("Error Output:\n"); //$NON-NLS-1$
+        for (String string : fErrorOutput) {
+            ret.append(string).append("\n"); //$NON-NLS-1$
+        }
+        ret.append("Return Value: "); //$NON-NLS-1$
+        ret.append(fResult);
+        ret.append("\n"); //$NON-NLS-1$
+        for (String string : fOutput) {
+            ret.append(string).append("\n"); //$NON-NLS-1$
+        }
+        return nullToEmptyString(ret.toString());
     }
 }
\ No newline at end of file
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/CommandShell.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/CommandShell.java
index 8410ecede4..228c29e624 100644
--- a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/CommandShell.java
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/CommandShell.java
@@ -12,7 +12,9 @@
  *   Markus Schorn - Bug 448058: Use org.eclipse.remote in favor of RSE
  *   Bernd Hufmann - Update to org.eclipse.remote API 2.0
  **********************************************************************/
-package org.eclipse.tracecompass.tmf.remote.core.shell;
+package org.eclipse.tracecompass.internal.tmf.remote.core.shell;
+
+import static org.eclipse.tracecompass.common.core.NonNullUtils.checkNotNull;
 
 import java.io.IOException;
 import java.util.List;
@@ -25,12 +27,16 @@ import java.util.concurrent.TimeoutException;
 
 import org.eclipse.core.commands.ExecutionException;
 import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.core.runtime.OperationCanceledException;
+import org.eclipse.jdt.annotation.Nullable;
 import org.eclipse.remote.core.IRemoteConnection;
 import org.eclipse.remote.core.IRemoteProcess;
 import org.eclipse.remote.core.IRemoteProcessService;
 import org.eclipse.tracecompass.internal.tmf.remote.core.messages.Messages;
 import org.eclipse.tracecompass.internal.tmf.remote.core.preferences.TmfRemotePreferences;
+import org.eclipse.tracecompass.tmf.remote.core.shell.ICommandResult;
+import org.eclipse.tracecompass.tmf.remote.core.shell.ICommandShell;
 
 /**
  * <p>
@@ -45,8 +51,8 @@ public class CommandShell implements ICommandShell {
     // ------------------------------------------------------------------------
     // Attributes
     // ------------------------------------------------------------------------
-    private IRemoteConnection fConnection = null;
-    private final ExecutorService fExecutor = Executors.newFixedThreadPool(1);
+    private final IRemoteConnection fConnection;
+    private final ExecutorService fExecutor = checkNotNull(Executors.newFixedThreadPool(1));
 
     // ------------------------------------------------------------------------
     // Constructors
@@ -66,29 +72,29 @@ public class CommandShell implements ICommandShell {
     // ------------------------------------------------------------------------
 
     @Override
-    public void connect() throws ExecutionException {
-    }
-
-    @Override
-    public void disconnect() {
+    public void dispose() {
         fExecutor.shutdown();
     }
 
     @Override
-    public ICommandResult executeCommand(final List<String> command, final IProgressMonitor monitor) throws ExecutionException {
+    public ICommandResult executeCommand(final List<String> command, @Nullable final IProgressMonitor aMonitor) throws ExecutionException {
         if (fConnection.isOpen()) {
             FutureTask<CommandResult> future = new FutureTask<>(new Callable<CommandResult>() {
                 @Override
                 public CommandResult call() throws IOException, InterruptedException {
-                    if (monitor == null || !monitor.isCanceled()) {
+                    IProgressMonitor monitor = aMonitor;
+                    if (monitor == null) {
+                        monitor = new NullProgressMonitor();
+                    }
+                    if (!monitor.isCanceled()) {
                         IRemoteProcess process = fConnection.getService(IRemoteProcessService.class).getProcessBuilder(command).start();
-                        InputReader stdout = new InputReader(process.getInputStream());
-                        InputReader stderr = new InputReader(process.getErrorStream());
+                        InputReader stdout = new InputReader(checkNotNull(process.getInputStream()));
+                        InputReader stderr = new InputReader(checkNotNull(process.getErrorStream()));
 
                         try {
                             stdout.waitFor(monitor);
                             stderr.waitFor(monitor);
-                            if (monitor == null || !monitor.isCanceled()) {
+                            if (!monitor.isCanceled()) {
                                 return createResult(process.waitFor(), stdout.toString(), stderr.toString());
                             }
                         } catch (OperationCanceledException e) {
@@ -107,14 +113,15 @@ public class CommandShell implements ICommandShell {
             fExecutor.execute(future);
 
             try {
-                return future.get(TmfRemotePreferences.getCommandTimeout(), TimeUnit.SECONDS);
-            } catch (java.util.concurrent.ExecutionException ex) {
-                throw new ExecutionException(Messages.TraceControl_ExecutionFailure, ex);
+                return checkNotNull(future.get(TmfRemotePreferences.getCommandTimeout(), TimeUnit.SECONDS));
             } catch (InterruptedException ex) {
                 throw new ExecutionException(Messages.TraceControl_ExecutionCancelled, ex);
             } catch (TimeoutException ex) {
                 throw new ExecutionException(Messages.TraceControl_ExecutionTimeout, ex);
-            } finally {
+            } catch (Exception ex) {
+                throw new ExecutionException(Messages.TraceControl_ExecutionFailure, ex);
+            }
+            finally {
                 future.cancel(true);
             }
         }
@@ -137,10 +144,7 @@ public class CommandShell implements ICommandShell {
     }
 
     private static String[] splitLines(String output) {
-        if (output == null) {
-            return null;
-        }
-        return output.split("\\r?\\n"); //$NON-NLS-1$
+        return checkNotNull(output.split("\\r?\\n")); //$NON-NLS-1$
     }
 
 }
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/InputReader.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/InputReader.java
index c6eb30d538..34f2c167a1 100644
--- a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/InputReader.java
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/InputReader.java
@@ -9,7 +9,9 @@
  * Contributors:
  *   Markus Schorn - Initial API and implementation
  **********************************************************************/
-package org.eclipse.tracecompass.tmf.remote.core.shell;
+package org.eclipse.tracecompass.internal.tmf.remote.core.shell;
+
+import static org.eclipse.tracecompass.common.core.NonNullUtils.nullToEmptyString;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -18,7 +20,6 @@ import java.io.InputStreamReader;
 import org.eclipse.core.runtime.IProgressMonitor;
 
 class InputReader {
-
     private static final int JOIN_TIMEOUT = 300;
     private static final int BYTES_PER_KB = 1024;
 
@@ -47,7 +48,7 @@ class InputReader {
     }
 
     public void waitFor(IProgressMonitor monitor) throws InterruptedException {
-        while (fThread.isAlive() && (monitor == null || !monitor.isCanceled())) {
+        while (fThread.isAlive() && (!monitor.isCanceled())) {
             fThread.join(JOIN_TIMEOUT);
         }
     }
@@ -59,7 +60,7 @@ class InputReader {
 
     @Override
     public String toString() {
-        return fResult.toString();
+        return nullToEmptyString(fResult.toString());
     }
 
 }
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/package-info.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/package-info.java
new file mode 100644
index 0000000000..8d7a45d9cd
--- /dev/null
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/internal/tmf/remote/core/shell/package-info.java
@@ -0,0 +1,14 @@
+/*******************************************************************************
+ * Copyright (c) 2015 Ericsson
+ *
+ * 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
+ *
+ * Contributors:
+ *    Bernd Hufmann - Initial API and implementation
+ *******************************************************************************/
+
+@org.eclipse.jdt.annotation.NonNullByDefault
+package org.eclipse.tracecompass.internal.tmf.remote.core.shell;
\ No newline at end of file
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/proxy/RemoteSystemProxy.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/proxy/RemoteSystemProxy.java
index 0209d376f5..6e6a5bba71 100644
--- a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/proxy/RemoteSystemProxy.java
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/proxy/RemoteSystemProxy.java
@@ -27,7 +27,7 @@ import org.eclipse.remote.core.IRemoteServicesManager;
 import org.eclipse.remote.core.RemoteConnectionChangeEvent;
 import org.eclipse.remote.core.exception.RemoteConnectionException;
 import org.eclipse.tracecompass.internal.tmf.remote.core.Activator;
-import org.eclipse.tracecompass.tmf.remote.core.shell.CommandShell;
+import org.eclipse.tracecompass.internal.tmf.remote.core.shell.CommandShell;
 import org.eclipse.tracecompass.tmf.remote.core.shell.ICommandShell;
 
 import com.google.common.base.Optional;
@@ -142,13 +142,9 @@ public class RemoteSystemProxy implements IRemoteConnectionChangeListener {
      * Creates a command shell.
      *
      * @return the command shell implementation
-     * @throws ExecutionException
-     *             If the command fails
      */
-    public ICommandShell createCommandShell() throws ExecutionException {
-        ICommandShell shell = new CommandShell(fHost);
-        shell.connect();
-        return shell;
+    public ICommandShell createCommandShell() {
+        return new CommandShell(fHost);
     }
 
     /**
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandResult.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandResult.java
index f0dab82f7b..4b249dab24 100644
--- a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandResult.java
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandResult.java
@@ -11,10 +11,9 @@
  *********************************************************************/
 package org.eclipse.tracecompass.tmf.remote.core.shell;
 
+
 /**
- * <p>
  * Interface for providing command execution result.
- * </p>
  *
  * @author Bernd Hufmann
  */
@@ -27,14 +26,20 @@ public interface ICommandResult {
     int getResult();
 
     /**
-     * @return returns the command output.
+     * Return the command output.
+     *
+     * It should not be null, but could be empty.
+     *
+     * @return the command output.
      */
     String[] getOutput();
 
     /**
-     * The error output of the command.
+     * Return the command error output.
+     *
+     * It should not be null, but could be empty.
      *
-     * @return returns the command error output.
+     * @return the command error output.
      */
     String[] getErrorOutput();
 }
\ No newline at end of file
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandShell.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandShell.java
index 0c7c01632c..f469d2b0a7 100644
--- a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandShell.java
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/ICommandShell.java
@@ -16,40 +16,30 @@ import java.util.List;
 
 import org.eclipse.core.commands.ExecutionException;
 import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.jdt.annotation.Nullable;
 
 /**
- * <p>
  * Interface for a command shell implementation
- * </p>
  *
  * @author Bernd Hufmann
  */
 public interface ICommandShell {
 
     /**
-     * Method to connect the command shell.
-     *
-     * @throws ExecutionException
-     *             If the command fails
-     */
-    void connect() throws ExecutionException;
-
-    /**
      * Method to disconnect the command shell.
      */
-    void disconnect();
+    void dispose();
 
     /**
      * Method to execute a command on the command shell.
      *
      * @param command
-     *            - the command to executed
+     *            The command to executed
      * @param monitor
-     *            - a progress monitor
-     * @return the command result
+     *            A progress monitor
+     * @return a {@link ICommandResult} instance
      * @throws ExecutionException
      *             If the command fails
      */
-    ICommandResult executeCommand(List<String> command,
-            IProgressMonitor monitor) throws ExecutionException;
+    ICommandResult executeCommand(List<String> command, @Nullable IProgressMonitor monitor) throws ExecutionException;
 }
diff --git a/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/package-info.java b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/package-info.java
new file mode 100644
index 0000000000..198d4f277e
--- /dev/null
+++ b/org.eclipse.tracecompass.tmf.remote.core/src/org/eclipse/tracecompass/tmf/remote/core/shell/package-info.java
@@ -0,0 +1,14 @@
+/*******************************************************************************
+ * Copyright (c) 2015 Ericsson
+ *
+ * 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
+ *
+ * Contributors:
+ *    Bernd Hufmann - Initial API and implementation
+ *******************************************************************************/
+
+@org.eclipse.jdt.annotation.NonNullByDefault
+package org.eclipse.tracecompass.tmf.remote.core.shell;
\ No newline at end of file