code review feedback fixes

src/cpp/session/SessionConsoleProcess.cpp

@@ -688,7 +688,7 @@ boost::shared_ptr<ConsoleProcess> ConsoleProcess::create(
  ProcTable::const_iterator pos = s_procs.find(terminalHandle);
  if (pos != s_procs.end())
  {
- return boost::shared_ptr<ConsoleProcess> (pos->second);
+ return pos->second;
  }
 
  options.terminateChildren = true;

src/gwt/src/org/rstudio/studio/client/common/console/ConsoleProcess.java

@@ -14,8 +14,6 @@
  */
 package org.rstudio.studio.client.common.console;
 
-import java.util.ArrayList;
-
 import org.rstudio.core.client.Debug;
 import org.rstudio.core.client.HandlerRegistrations;
 import org.rstudio.core.client.StringUtil;
@@ -33,7 +31,6 @@
 import org.rstudio.studio.client.workbench.events.SessionInitHandler;
 import org.rstudio.studio.client.workbench.model.Session;
 import org.rstudio.studio.client.workbench.views.console.model.ConsoleServerOperations;
-import org.rstudio.studio.client.workbench.views.terminal.events.TerminalSessionListEvent;
 import org.rstudio.studio.client.workbench.views.vcs.common.ConsoleProgressDialog;
 
 import com.google.gwt.core.client.JsArray;
@@ -75,13 +72,16 @@ public void onSessionInit(SessionInitEvent sie)
  JsArray<ConsoleProcessInfo> procs =
  session.getSessionInfo().getConsoleProcesses();
 
- final ArrayList<ConsoleProcess> terminalProcs = 
- new ArrayList<ConsoleProcess>();
-
  for (int i = 0; i < procs.length(); i++)
  {
  final ConsoleProcessInfo proc = procs.get(i);
-
+ if (!proc.isDialog())
+ {
+ // non-modal processes represent terminals and are handled
+ // by the terminal UI
+ continue;
+ }
+
  connectToProcess(
  proc,
  new ServerRequestCallback<ConsoleProcess>()
@@ -90,69 +90,61 @@ public void onSessionInit(SessionInitEvent sie)
  public void onResponseReceived(
  final ConsoleProcess cproc)
  {
- if (proc.isDialog())
+ assert(proc.isDialog());
+ // first determine whether to create and/or
+ // show the dialog immediately
+ boolean createDialog = false;
+ boolean showDialog = false;
+
+ // standard dialog -- always show it
+ if (!proc.getShowOnOutput())
  {
- // first determine whether to create and/or
- // show the dialog immediately
- boolean createDialog = false;
- boolean showDialog = false;
-
- // standard dialog -- always show it
- if (!proc.getShowOnOutput())
- {
- createDialog = true;
- showDialog = true;
- }
-
- // showOnOutput dialog that already has 
- // output -- make sure the user sees it
- //
- // NOTE: we have to trim the buffered output
- // for the comparison because when the password
- // manager provides a password the back-end
- // process sometimes echos a newline back to us
- //
- else if (proc.getBufferedOutput().trim().length() > 0)
- {
- createDialog = true;
- showDialog = true;
- }
-
- // showOnOutput dialog that has exited
- // and has no output -- reap it
- else if (proc.getExitCode() != null)
- {
- cproc.reap(new VoidServerRequestCallback());
- }
-
- // showOnOutput dialog with no output that is
- // still running -- create but don't show yet
- else
- {
- createDialog = true;
- }
-
- // take indicated actions
- if (createDialog)
- {
- ConsoleProgressDialog dlg = new ConsoleProgressDialog(
+ createDialog = true;
+ showDialog = true;
+ }
+
+ // showOnOutput dialog that already has 
+ // output -- make sure the user sees it
+ //
+ // NOTE: we have to trim the buffered output
+ // for the comparison because when the password
+ // manager provides a password the back-end
+ // process sometimes echos a newline back to us
+ //
+ else if (proc.getBufferedOutput().trim().length() > 0)
+ {
+ createDialog = true;
+ showDialog = true;
+ }
+
+ // showOnOutput dialog that has exited
+ // and has no output -- reap it
+ else if (proc.getExitCode() != null)
+ {
+ cproc.reap(new VoidServerRequestCallback());
+ }
+
+ // showOnOutput dialog with no output that is
+ // still running -- create but don't show yet
+ else
+ {
+ createDialog = true;
+ }
+
+ // take indicated actions
+ if (createDialog)
+ {
+ ConsoleProgressDialog dlg = new ConsoleProgressDialog(
  proc.getCaption(),
  cproc,
  proc.getBufferedOutput(),
  proc.getExitCode(),
  cryptoServer);
-
- if (showDialog)
- dlg.showModal();
- else
- dlg.showOnOutput();
- }
- }
- else
- {
- // Build up array of terminal-related processes,
- // in ascending order by terminal sequence.
- addTerminalProc(terminalProcs, cproc);
+
+ if (showDialog)
+ dlg.showModal();
+ else
+ dlg.showOnOutput();
  }
  }
 
@@ -163,11 +155,6 @@ public void onError(ServerError error)
  }
  });
  }
-
- if (terminalProcs.size() > 0)
- {
- eventBus_.fireEvent(new TerminalSessionListEvent(terminalProcs));
- }
  }
  });
 
@@ -256,62 +243,35 @@ public ConsoleProgressDialog showConsoleProgressDialog(
  }
 
  /**
- * Add process to list of processes, sorted in ascending order by
- * terminal sequence number. If duplicate sequence numbers are
- * encountered, all but the first will have the process killed.
- * 
- * @param terminalProcs (in/out) sorted list of terminal processes
- * @param cproc process to insert in the list
+ * Terminate and reap a process
+ * @param procInfo process to terminate and reap
  */
- private static void addTerminalProc(ArrayList<ConsoleProcess> terminalProcs,
- ConsoleProcess cproc)
+ public void reap(final ConsoleProcessInfo procInfo)
  {
- int newSequence = cproc.getProcessInfo().getTerminalSequence();
-
- if (newSequence < 1)
- {
- Debug.log("Invalid terminal sequence, killing unrecognized process");
- reap(cproc);
- return;
- }
-
- for (int i = 0; i < terminalProcs.size(); i++)
- {
- int currentSequence = 
- terminalProcs.get(i).getProcessInfo().getTerminalSequence();
-
- if (newSequence == currentSequence)
- {
- Debug.log("Duplicate terminal sequence, killing duplicate process");
- reap(cproc);
- return;
- }
+ connectToProcess(
+ procInfo,
+ new ServerRequestCallback<ConsoleProcess>()
+ {
+ public void onResponseReceived( final ConsoleProcess cproc)
+ {
+ cproc.interrupt(new SimpleRequestCallback<Void>()
+ {
+ @Override
+ public void onResponseReceived(Void response)
+ {
+ cproc.reap(new VoidServerRequestCallback());
+ }
+ }); 
+ }
 
- if (newSequence < currentSequence)
- {
- terminalProcs.add(i, cproc);
- return;
- }
- }
- terminalProcs.add(cproc);
+ @Override
+ public void onError(ServerError error)
+ {
+ Debug.logError(error);
+ }
+ });
  }
 
- /**
- * Terminate and reap a process.
- * @param cproc process to terminate and reap
- */
- private static void reap(final ConsoleProcess cproc)
- {
- cproc.interrupt(new SimpleRequestCallback<Void>()
- {
- @Override
- public void onResponseReceived(Void response)
- {
- cproc.reap(new VoidServerRequestCallback());
- }
- });
- }
-
  private final ConsoleServerOperations server_;
  private final CryptoServerOperations cryptoServer_;
  private final EventBus eventBus_;

src/gwt/src/org/rstudio/studio/client/workbench/views/terminal/TerminalPane.java

@@ -15,20 +15,20 @@
 
 package org.rstudio.studio.client.workbench.views.terminal;
 
+import java.util.ArrayList;
+
 import org.rstudio.core.client.Debug;
 import org.rstudio.core.client.theme.res.ThemeStyles;
 import org.rstudio.core.client.widget.Operation;
 import org.rstudio.core.client.widget.Toolbar;
 import org.rstudio.studio.client.application.events.EventBus;
 import org.rstudio.studio.client.common.GlobalDisplay;
-import org.rstudio.studio.client.common.console.ConsoleProcess;
 import org.rstudio.studio.client.common.console.ConsoleProcessInfo;
 import org.rstudio.studio.client.common.shell.ShellSecureInput;
 import org.rstudio.studio.client.workbench.ui.WorkbenchPane;
 import org.rstudio.studio.client.workbench.views.terminal.TerminalList.TerminalMetadata;
 import org.rstudio.studio.client.workbench.views.terminal.events.SwitchToTerminalEvent;
 import org.rstudio.studio.client.workbench.views.terminal.events.TerminalCaptionEvent;
-import org.rstudio.studio.client.workbench.views.terminal.events.TerminalSessionListEvent;
 import org.rstudio.studio.client.workbench.views.terminal.events.TerminalSessionStartedEvent;
 import org.rstudio.studio.client.workbench.views.terminal.events.TerminalSessionStoppedEvent;
 
@@ -157,21 +157,21 @@ public void startTerminal(int sequence, String terminalHandle)
  }
 
  @Override
- public void repopulateTerminals(TerminalSessionListEvent event)
+ public void repopulateTerminals(ArrayList<ConsoleProcessInfo> procList)
  {
  // Expect to receive this after a browser reset, so if we already have
  // terminals in the cache, something is, to be technical, busted.
  if (terminals_.terminalCount() > 0 || getLoadedTerminalCount() > 0 )
  {
- Debug.log("Received terminal list from server when terminals already loaded. Ignoring.");
+ Debug.logWarning("Received terminal list from server when terminals " + 
+ "already loaded. Ignoring.");
  return;
  }
 
  // add terminal to the dropdown's cache; terminals aren't actually
  // loaded until selected via the dropdown
- for (ConsoleProcess proc : event.getTerminalList())
+ for (ConsoleProcessInfo procInfo : procList)
  {
- ConsoleProcessInfo procInfo = proc.getProcessInfo();
  terminals_.addTerminal(new TerminalMetadata(procInfo.getTerminalHandle(),
  procInfo.getCaption(), 
  procInfo.getTerminalSequence()));
@@ -258,6 +258,7 @@ public void onTerminalSessionStopped(TerminalSessionStoppedEvent event)
  public void onSwitchToTerminal(SwitchToTerminalEvent event)
  {
  String handle = event.getTerminalHandle();
+
  // If terminal was already loaded, just make it visible
  TerminalSession terminal = terminalWithHandle(handle);
  if (terminal != null)
@@ -266,16 +267,16 @@ public void onSwitchToTerminal(SwitchToTerminalEvent event)
  setFocusOnVisible();
  return;
  }
- 
+
  // Reconnect to server?
  TerminalMetadata existingTerminal = terminals_.getMetadataForHandle(handle);
  if (existingTerminal != null)
  {
  startTerminal(existingTerminal.getSequence(), handle);
  return;
  }
- 
- Debug.log("Tried to switch to unknown terminal");
+
+ Debug.logWarning("Tried to switch to unknown terminal handle");
  }
 
  @Override

src/gwt/src/org/rstudio/studio/client/workbench/views/terminal/TerminalTab.java

@@ -21,11 +21,12 @@
 import org.rstudio.studio.client.application.events.EventBus;
 import org.rstudio.studio.client.workbench.commands.Commands;
 import org.rstudio.studio.client.workbench.events.BusyHandler;
+import org.rstudio.studio.client.workbench.events.SessionInitEvent;
+import org.rstudio.studio.client.workbench.events.SessionInitHandler;
 import org.rstudio.studio.client.workbench.model.Session;
 import org.rstudio.studio.client.workbench.ui.DelayLoadTabShim;
 import org.rstudio.studio.client.workbench.ui.DelayLoadWorkbenchTab;
 import org.rstudio.studio.client.workbench.views.terminal.events.CreateTerminalEvent;
-import org.rstudio.studio.client.workbench.views.terminal.events.TerminalSessionListEvent;
 
 import com.google.inject.Inject;
 
@@ -38,7 +39,7 @@ public abstract static class Shim
  extends DelayLoadTabShim<TerminalTabPresenter, TerminalTab>
  implements ProvidesBusy,
  CreateTerminalEvent.Handler,
- TerminalSessionListEvent.Handler
+ SessionInitHandler
  {
  @Handler
  public abstract void onActivateTerminal();
@@ -61,7 +62,7 @@ public TerminalTab(Shim shim,
 
  binder.bind(commands, shim_);
  events.addHandler(CreateTerminalEvent.TYPE, shim_);
- events.addHandler(TerminalSessionListEvent.TYPE, shim_);
+ events.addHandler(SessionInitEvent.TYPE, shim_);
 
  shim_.initialize();
  }