Merge pull request #5711 from jasongrout/foreign

Move console foreign handler to its own plugin

packages/console-extension/package.json

@@ -41,6 +41,7 @@
     "@jupyterlab/rendermime": "^0.19.1",
     "@phosphor/algorithm": "^1.1.2",
     "@phosphor/coreutils": "^1.3.0",
+    "@phosphor/properties": "^1.1.2",
     "@phosphor/widgets": "^1.6.0"
   },
   "devDependencies": {

packages/console-extension/src/foreign.ts

@@ -0,0 +1,107 @@
+// Copyright (c) Jupyter Development Team.
+// Distributed under the terms of the Modified BSD License.
+
+import { JupyterLabPlugin, JupyterLab } from '@jupyterlab/application';
+
+import { ICommandPalette } from '@jupyterlab/apputils';
+
+import {
+  CodeConsole,
+  ConsolePanel,
+  IConsoleTracker,
+  ForeignHandler
+} from '@jupyterlab/console';
+
+import { AttachedProperty } from '@phosphor/properties';
+
+import { ReadonlyJSONObject } from '@phosphor/coreutils';
+
+/**
+ * The console widget tracker provider.
+ */
+export const foreign: JupyterLabPlugin<void> = {
+  id: '@jupyterlab/console-extension:foreign',
+  requires: [IConsoleTracker, ICommandPalette],
+  activate: activateForeign,
+  autoStart: true
+};
+
+export default foreign;
+
+function activateForeign(
+  app: JupyterLab,
+  tracker: IConsoleTracker,
+  palette: ICommandPalette
+) {
+  tracker.widgetAdded.connect((sender, panel) => {
+    const console = panel.console;
+
+    const handler = new ForeignHandler({
+      session: console.session,
+      parent: console
+    });
+    Private.foreignHandlerProperty.set(console, handler);
+    console.disposed.connect(() => {
+      handler.dispose();
+    });
+  });
+
+  const { commands, shell } = app;
+  const category = 'Console';
+  const toggleShowAllActivity = 'console:toggle-show-all-kernel-activity';
+
+  // Get the current widget and activate unless the args specify otherwise.
+  function getCurrent(args: ReadonlyJSONObject): ConsolePanel | null {
+    let widget = tracker.currentWidget;
+    let activate = args['activate'] !== false;
+    if (activate && widget) {
+      shell.activateById(widget.id);
+    }
+    return widget;
+  }
+
+  commands.addCommand(toggleShowAllActivity, {
+    label: args => 'Show All Kernel Activity',
+    execute: args => {
+      let current = getCurrent(args);
+      if (!current) {
+        return;
+      }
+      const handler = Private.foreignHandlerProperty.get(current.console);
+      handler.enabled = !handler.enabled;
+    },
+    isToggled: () =>
+      tracker.currentWidget !== null &&
+      Private.foreignHandlerProperty.get(tracker.currentWidget.console).enabled,
+    isEnabled: () =>
+      tracker.currentWidget !== null &&
+      tracker.currentWidget === app.shell.currentWidget
+  });
+
+  palette.addItem({
+    command: toggleShowAllActivity,
+    category,
+    args: { isPalette: true }
+  });
+
+  app.contextMenu.addItem({
+    command: toggleShowAllActivity,
+    selector: '.jp-CodeConsole'
+  });
+}
+
+/*
+ * A namespace for private data.
+ */
+namespace Private {
+  /**
+   * An attached property for a console's foreign handler.
+   */
+  export const foreignHandlerProperty = new AttachedProperty<
+    CodeConsole,
+    ForeignHandler | undefined
+  >({
+    name: 'foreignHandler',
+    create: () => undefined
+  });
+}

packages/console-extension/src/index.ts

@@ -42,6 +42,8 @@ import { ReadonlyJSONObject } from '@phosphor/coreutils';
 
 import { DockLayout, Menu } from '@phosphor/widgets';
 
+import foreign from './foreign';
+
 /**
  * The command IDs used by the console plugin.
  */
@@ -68,9 +70,6 @@ namespace CommandIDs {
 
   export const changeKernel = 'console:change-kernel';
 
-  export const toggleShowAllActivity =
-    'console:toggle-show-all-kernel-activity';
-
   export const enterToExecute = 'console:enter-to-execute';
 
   export const shiftEnterToExecute = 'console:shift-enter-to-execute';
@@ -116,7 +115,7 @@ const factory: JupyterLabPlugin<ConsolePanel.IContentFactory> = {
 /**
  * Export the plugins as the default.
  */
-const plugins: JupyterLabPlugin<any>[] = [factory, tracker];
+const plugins: JupyterLabPlugin<any>[] = [factory, tracker, foreign];
 export default plugins;
 
 /**
@@ -461,22 +460,6 @@ async function activateConsole(
     isEnabled
   });
 
-  commands.addCommand(CommandIDs.toggleShowAllActivity, {
-    label: args => 'Show All Kernel Activity',
-    execute: args => {
-      let current = getCurrent(args);
-      if (!current) {
-        return;
-      }
-      current.console.showAllActivity = !current.console.showAllActivity;
-    },
-    isToggled: () =>
-      tracker.currentWidget
-        ? tracker.currentWidget.console.showAllActivity
-        : false,
-    isEnabled
-  });
-
   // Add command palette items
   [
     CommandIDs.create,
@@ -487,8 +470,7 @@ async function activateConsole(
     CommandIDs.restart,
     CommandIDs.interrupt,
     CommandIDs.changeKernel,
-    CommandIDs.closeAndShutdown,
-    CommandIDs.toggleShowAllActivity
+    CommandIDs.closeAndShutdown
   ].forEach(command => {
     palette.addItem({ command, category, args: { isPalette: true } });
   });
@@ -619,10 +601,6 @@ async function activateConsole(
     command: CommandIDs.restart,
     selector: '.jp-CodeConsole'
   });
-  app.contextMenu.addItem({
-    command: CommandIDs.toggleShowAllActivity,
-    selector: '.jp-CodeConsole'
-  });
 
   return tracker;
 }

packages/console/src/foreign.ts

@@ -3,7 +3,7 @@
 
 import { IClientSession } from '@jupyterlab/apputils';
 
-import { Cell, CodeCell } from '@jupyterlab/cells';
+import { CodeCell } from '@jupyterlab/cells';
 
 import { nbformat } from '@jupyterlab/coreutils';
 
@@ -13,6 +13,8 @@ import { IDisposable } from '@phosphor/disposable';
 
 import { Signal } from '@phosphor/signaling';
 
+const FOREIGN_CELL_CLASS = 'jp-CodeConsole-foreignCell';
+
 /**
  * A handler for capturing API messages from other sessions that should be
  * rendered in a given parent.
@@ -27,7 +29,6 @@ export class ForeignHandler implements IDisposable {
       this.onIOPubMessage,
       this
     );
-    this._factory = options.cellFactory;
     this._parent = options.parent;
   }
 
@@ -68,7 +69,6 @@ export class ForeignHandler implements IDisposable {
       return;
     }
     this._isDisposed = true;
-    this._cells.clear();
     Signal.clearData(this);
   }
 
@@ -115,23 +115,18 @@ export class ForeignHandler implements IDisposable {
       case 'display_data':
       case 'stream':
       case 'error':
-        if (!this._cells.has(parentMsgId)) {
-          // This is an output from an input that was broadcast before our
-          // session started listening. We will ignore it.
-          console.warn('Ignoring output with no associated input cell.');
+        cell = this._parent.getCell(parentMsgId);
+        if (!cell) {
           return false;
         }
         let output = msg.content as nbformat.IOutput;
-        cell = this._cells.get(parentMsgId);
-        if (cell) {
-          output.output_type = msgType as nbformat.OutputType;
-          cell.model.outputs.add(output);
-        }
+        output.output_type = msgType as nbformat.OutputType;
+        cell.model.outputs.add(output);
         parent.update();
         return true;
       case 'clear_output':
         let wait = (msg as KernelMessage.IClearOutputMsg).content.wait;
-        cell = this._cells.get(parentMsgId);
+        cell = this._parent.getCell(parentMsgId);
         if (cell) {
           cell.model.outputs.clear(wait);
         }
@@ -145,16 +140,14 @@ export class ForeignHandler implements IDisposable {
    * Create a new code cell for an input originated from a foreign session.
    */
   private _newCell(parentMsgId: string): CodeCell {
-    let cell = this._factory();
-    this._cells.set(parentMsgId, cell);
-    this._parent.addCell(cell);
+    let cell = this.parent.createCodeCell();
+    cell.addClass(FOREIGN_CELL_CLASS);
+    this._parent.addCell(cell, parentMsgId);
     return cell;
   }
 
-  private _cells = new Map<string, CodeCell>();
   private _enabled = false;
   private _parent: ForeignHandler.IReceiver;
-  private _factory: () => CodeCell;
   private _isDisposed = false;
 }
 
@@ -175,11 +168,6 @@ export namespace ForeignHandler {
      * The parent into which the handler will inject code cells.
      */
     parent: IReceiver;
-
-    /**
-     * The cell factory for foreign handlers.
-     */
-    cellFactory: () => CodeCell;
   }
 
   /**
@@ -187,13 +175,23 @@ export namespace ForeignHandler {
    */
   export interface IReceiver {
     /**
-     * Add a newly created foreign cell.
+     * Create a cell.
      */
-    addCell(cell: Cell): void;
+    createCodeCell(): CodeCell;
+
+    /**
+     * Add a newly created cell.
+     */
+    addCell(cell: CodeCell, msgId: string): void;
 
     /**
      * Trigger a rendering update on the receiver.
      */
     update(): void;
+
+    /**
+     * Get a cell associated with a message id.
+     */
+    getCell(msgId: string): CodeCell;
   }
 }

packages/console/src/widget.ts

@@ -33,8 +33,6 @@ import { ISignal, Signal } from '@phosphor/signaling';
 
 import { Panel, PanelLayout, Widget } from '@phosphor/widgets';
 
-import { ForeignHandler } from './foreign';
-
 import { ConsoleHistory, IConsoleHistory } from './history';
 
 /**
@@ -57,11 +55,6 @@ const CONSOLE_CLASS = 'jp-CodeConsole';
  */
 const BANNER_CLASS = 'jp-CodeConsole-banner';
 
-/**
- * The class name of a cell whose input originated from a foreign session.
- */
-const FOREIGN_CELL_CLASS = 'jp-CodeConsole-foreignCell';
-
 /**
  * The class name of the active prompt cell.
  */
@@ -121,13 +114,6 @@ export class CodeConsole extends Widget {
     layout.addWidget(this._content);
     layout.addWidget(this._input);
 
-    // Set up the foreign iopub handler.
-    this._foreignHandler = new ForeignHandler({
-      session: this.session,
-      parent: this,
-      cellFactory: () => this._createCodeCell()
-    });
-
     this._history = new ConsoleHistory({
       session: this.session
     });
@@ -199,16 +185,23 @@ export class CodeConsole extends Widget {
   /**
    * Add a new cell to the content panel.
    *
-   * @param cell - The cell widget being added to the content panel.
+   * @param cell - The code cell widget being added to the content panel.
+   *
+   * @param msgId - The optional execution message id for the cell.
    *
    * #### Notes
-   * This method is meant for use by outside classes that want to inject content
-   * into a console. It is distinct from the `inject` method in that it requires
-   * rendered code cell widgets and does not execute them.
+   * This method is meant for use by outside classes that want to add cells to a
+   * console. It is distinct from the `inject` method in that it requires
+   * rendered code cell widgets and does not execute them (though it can store
+   * the execution message id).
    */
-  addCell(cell: Cell) {
+  addCell(cell: CodeCell, msgId?: string) {
     this._content.addWidget(cell);
     this._cells.push(cell);
+    if (msgId) {
+      this._msgIds.set(msgId, cell);
+      this._msgIdCells.set(cell, msgId);
+    }
     cell.disposed.connect(
       this._onCellDisposed,
       this
@@ -216,6 +209,9 @@ export class CodeConsole extends Widget {
     this.update();
   }
 
+  /**
+   * Add a banner cell.
+   */
   addBanner() {
     if (this._banner) {
       // An old banner just becomes a normal cell now.
@@ -249,6 +245,18 @@ export class CodeConsole extends Widget {
     }
   }
 
+  /**
+   * Create a new cell with the built-in factory.
+   */
+  createCodeCell(): CodeCell {
+    let factory = this.contentFactory;
+    let options = this._createCodeCellOptions();
+    let cell = factory.createCodeCell(options);
+    cell.readOnly = true;
+    cell.model.mimeType = this._mimetype;
+    return cell;
+  }
+
   /**
    * Dispose of the resources held by the widget.
    */
@@ -258,23 +266,13 @@ export class CodeConsole extends Widget {
       return;
     }
     this._cells.clear();
+    this._msgIdCells = null;
+    this._msgIds = null;
     this._history.dispose();
-    this._foreignHandler.dispose();
 
     super.dispose();
   }
 
-  /**
-   * Set whether the foreignHandler is able to inject foreign cells into a
-   * console.
-   */
-  get showAllActivity(): boolean {
-    return this._foreignHandler.enabled;
-  }
-  set showAllActivity(value: boolean) {
-    this._foreignHandler.enabled = value;
-  }
-
   /**
    * Execute the current prompt.
    *
@@ -319,6 +317,15 @@ export class CodeConsole extends Widget {
     });
   }
 
+  /**
+   * Get a cell given a message id.
+   *
+   * @param msgId - The message id.
+   */
+  getCell(msgId: string): CodeCell | undefined {
+    return this._msgIds.get(msgId);
+  }
+
   /**
    * Inject arbitrary code for the console to execute immediately.
    *
@@ -327,7 +334,7 @@ export class CodeConsole extends Widget {
    * @returns A promise that indicates when the injected cell's execution ends.
    */
   inject(code: string): Promise<void> {
-    let cell = this._createCodeCell();
+    let cell = this.createCodeCell();
     cell.model.value.text = code;
     this.addCell(cell);
     return this._execute(cell);
@@ -557,19 +564,6 @@ export class CodeConsole extends Widget {
     }
   }
 
-  /**
-   * Create a new foreign cell.
-   */
-  private _createCodeCell(): CodeCell {
-    let factory = this.contentFactory;
-    let options = this._createCodeCellOptions();
-    let cell = factory.createCodeCell(options);
-    cell.readOnly = true;
-    cell.model.mimeType = this._mimetype;
-    cell.addClass(FOREIGN_CELL_CLASS);
-    return cell;
-  }
-
   /**
    * Create the options used to initialize a code cell widget.
    */
@@ -587,6 +581,11 @@ export class CodeConsole extends Widget {
   private _onCellDisposed(sender: Cell, args: void): void {
     if (!this.isDisposed) {
       this._cells.removeValue(sender);
+      const msgId = this._msgIdCells.get(sender as CodeCell);
+      if (msgId) {
+        this._msgIdCells.delete(sender as CodeCell);
+        this._msgIds.delete(msgId);
+      }
     }
   }
 
@@ -678,11 +677,12 @@ export class CodeConsole extends Widget {
   private _cells: IObservableList<Cell>;
   private _content: Panel;
   private _executed = new Signal<this, Date>(this);
-  private _foreignHandler: ForeignHandler;
   private _history: IConsoleHistory;
   private _input: Panel;
   private _mimetype = 'text/x-ipython';
   private _mimeTypeService: IEditorMimeTypeService;
+  private _msgIds = new Map<string, CodeCell>();
+  private _msgIdCells = new Map<CodeCell, string>();
   private _promptCellCreated = new Signal<this, CodeCell>(this);
 }
 

tests/test-console/src/foreign.spec.ts

@@ -24,9 +24,25 @@ import {
 } from '@jupyterlab/testutils';
 
 class TestParent extends Panel implements ForeignHandler.IReceiver {
-  addCell(cell: CodeCell): void {
+  addCell(cell: CodeCell, msgId?: string): void {
     this.addWidget(cell);
+    if (msgId) {
+      this._cells.set(msgId, cell);
+    }
+  }
+
+  createCodeCell(): CodeCell {
+    const contentFactory = NBTestUtils.createCodeCellFactory();
+    const model = new CodeCellModel({});
+    const cell = new CodeCell({ model, rendermime, contentFactory });
+    return cell;
   }
+
+  getCell(msgId: string) {
+    return this._cells.get(msgId);
+  }
+
+  private _cells = new Map<string, CodeCell>();
 }
 
 class TestHandler extends ForeignHandler {
@@ -64,12 +80,6 @@ class TestHandler extends ForeignHandler {
 
 const rendermime = defaultRenderMime();
 
-function cellFactory(): CodeCell {
-  const contentFactory = NBTestUtils.createCodeCellFactory();
-  const model = new CodeCellModel({});
-  const cell = new CodeCell({ model, rendermime, contentFactory });
-  return cell;
-}
 const relevantTypes = [
   'execute_input',
   'execute_result',
@@ -99,7 +109,7 @@ describe('@jupyterlab/console', () => {
 
     beforeEach(() => {
       const parent = new TestParent();
-      handler = new TestHandler({ session, parent, cellFactory });
+      handler = new TestHandler({ session, parent });
     });
 
     afterEach(() => {
@@ -166,8 +176,7 @@ describe('@jupyterlab/console', () => {
         const parent = new TestParent();
         handler = new TestHandler({
           session: handler.session,
-          parent,
-          cellFactory
+          parent
         });
         expect(handler.parent).to.equal(parent);
       });

tests/test-console/src/widget.spec.ts

@@ -92,7 +92,6 @@ describe('console/widget', () => {
 
       it('should reflect the contents of the widget', async () => {
         const force = true;
-        widget.showAllActivity = true;
         Widget.attach(widget, document.body);
         await (widget.session as ClientSession).initialize();
         await widget.execute(force);