Merge pull request #1480 from afshin/instance-tracker-refactor

Instance tracker refactor

src/codeeditor/editor.ts

@@ -408,7 +408,7 @@ namespace CodeEditor {
   }
 
   /**
-   * The signals for the `CodeMirrorModel` class.
+   * The signals for the `Model` class.
    */
   defineSignal(Model.prototype, 'mimeTypeChanged');
 

src/common/instancetracker.ts

@@ -1,6 +1,10 @@
 // Copyright (c) Jupyter Development Team.
 // Distributed under the terms of the Modified BSD License.
 
+import {
+  each
+} from 'phosphor/lib/algorithm/iteration';
+
 import {
   IDisposable
 } from 'phosphor/lib/core/disposable';
@@ -17,6 +21,10 @@ import {
   CommandRegistry
 } from 'phosphor/lib/ui/commandregistry';
 
+import {
+  FocusTracker
+} from 'phosphor/lib/ui/focustracker';
+
 import {
   Widget
 } from 'phosphor/lib/ui/widget';
@@ -35,11 +43,6 @@ import {
  */
 export
 interface IInstanceTracker<T extends Widget> {
-  /**
-   * A signal emitted when a widget is added or removed from the tracker.
-   */
-  readonly changed: ISignal<this, 'add' | 'remove'>;
-
   /**
    * A signal emitted when the current widget changes.
    *
@@ -88,9 +91,6 @@ interface IInstanceTracker<T extends Widget> {
  * A class that keeps track of widget instances.
  *
  * #### Notes
- * This is meant to be used in conjunction with a `FocusTracker` and will
- * typically be kept in sync with focus tracking events.
- *
  * The API surface area of this concrete implementation is substantially larger
  * than the instance tracker interface it implements. The interface is intended
  * for export by JupyterLab plugins that create widgets and have clients who may
@@ -106,6 +106,10 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
    */
   constructor(options: InstanceTracker.IOptions) {
     this.namespace = options.namespace;
+    this._tracker.currentChanged.connect((sender, args) => {
+      this.onCurrentChanged();
+      this.currentChanged.emit(this.currentWidget);
+    }, this);
   }
 
   /**
@@ -113,16 +117,8 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
    */
   readonly namespace: string;
 
-  /**
-   * A signal emitted when a widget is added or removed from the tracker.
-   */
-  readonly changed: ISignal<this, 'add' | 'remove'>;
-
   /**
    * A signal emitted when the current widget changes.
-   *
-   * #### Notes
-   * If the last widget being tracked is disposed, `null` will be emitted.
    */
   readonly currentChanged: ISignal<this, T>;
 
@@ -130,21 +126,21 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
    * The current widget is the most recently focused widget.
    */
   get currentWidget(): T {
-    return this._currentWidget;
+    return this._tracker.currentWidget;
   }
 
   /**
    * Test whether the tracker is disposed.
    */
   get isDisposed(): boolean {
-    return this._widgets === null;
+    return this._isDisposed;
   }
 
   /**
    * The number of widgets held by the tracker.
    */
   get size(): number {
-    return this._widgets.size;
+    return this._tracker.widgets.length;
   }
 
   /**
@@ -153,12 +149,12 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
    * @param widget - The widget being added.
    */
   add(widget: T): Promise<void> {
-    if (this._widgets.has(widget)) {
+    if (this._tracker.has(widget)) {
       let warning = `${widget.id} already exists in the tracker.`;
       console.warn(warning);
       return Promise.reject(warning);
     }
-    this._widgets.add(widget);
+    this._tracker.add(widget);
 
     let injected = Private.injectedProperty.get(widget);
     let promise: Promise<void>;
@@ -183,7 +179,6 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
 
     // Handle widget disposal.
     widget.disposed.connect(() => {
-      this._widgets.delete(widget);
       // If restore data was saved, delete it from the database.
       if (!injected && this._restore) {
         let { state } = this._restore;
@@ -193,19 +188,8 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
           state.remove(name);
         }
       }
-      // Emit a changed signal.
-      this.changed.emit('remove');
-      // If this was the last widget, emit null for current widget signal.
-      if (!this._widgets.size) {
-        this._currentWidget = null;
-        this.onCurrentChanged();
-        this.currentChanged.emit(null);
-      }
     });
 
-    // Emit a changed signal.
-    this.changed.emit('add');
-
     return promise || Promise.resolve(void 0);
   }
 
@@ -216,10 +200,8 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
     if (this.isDisposed) {
       return;
     }
+    this._isDisposed = true;
     clearSignalData(this);
-    this._currentWidget = null;
-    this._widgets.clear();
-    this._widgets = null;
   }
 
   /**
@@ -229,7 +211,7 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
    */
   find(fn: (widget: T) => boolean): T {
     let result: T = null;
-    this._widgets.forEach(widget => {
+    each(this._tracker.widgets, widget => {
       // If a result has already been found, short circuit.
       if (result) {
         return;
@@ -247,7 +229,7 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
    * @param fn - The function to call on each widget.
    */
   forEach(fn: (widget: T) => void): void {
-    this._widgets.forEach(widget => { fn(widget); });
+    each(this._tracker.widgets, widget => { fn(widget); });
   }
 
   /**
@@ -276,7 +258,7 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
    * @param widget - The widget whose existence is being checked.
    */
   has(widget: Widget): boolean {
-    return this._widgets.has(widget as any);
+    return this._tracker.has(widget as any);
   }
 
   /**
@@ -342,35 +324,6 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
     }
   }
 
-  /**
-   * Syncs the state of the tracker with a widget known to have focus.
-   *
-   * @param current The currently focused widget.
-   *
-   * @returns The current widget or `null` if there is none.
-   *
-   * #### Notes
-   * Syncing acts as a gate returning a widget only if it is the current widget.
-   */
-  sync(current: Widget): T {
-    if (this.isDisposed) {
-      return;
-    }
-
-    if (current && this._widgets.has(current as any)) {
-      // If no state change needs to occur, just bail.
-      if (this._currentWidget === current) {
-        return this._currentWidget;
-      }
-      this._currentWidget = current as T;
-      this.onCurrentChanged();
-      this.currentChanged.emit(this._currentWidget);
-      return this._currentWidget;
-    }
-
-    return null;
-  }
-
   /**
    * Handle the current change event.
    *
@@ -382,14 +335,14 @@ class InstanceTracker<T extends Widget> implements IInstanceTracker<T>, IDisposa
     /* This is a no-op. */
   }
 
-  private _currentWidget: T = null;
+  private _isDisposed = false;
   private _restore: InstanceTracker.IRestoreOptions<T> = null;
+  private _tracker = new FocusTracker<T>();
   private _widgets = new Set<T>();
 }
 
 
 // Define the signals for the `InstanceTracker` class.
-defineSignal(InstanceTracker.prototype, 'changed');
 defineSignal(InstanceTracker.prototype, 'currentChanged');
 
 

src/console/plugin.ts

@@ -159,9 +159,8 @@ function activateConsole(app: JupyterLab, services: IServiceManager, rendermime:
     when: manager.ready
   });
 
-  // Sync tracker and set the source of the code inspector.
-  app.shell.currentChanged.connect((sender, args) => {
-    let widget = tracker.sync(args.newValue);
+  // Set the source of the code inspector.
+  tracker.currentChanged.connect((sender, widget) => {
     if (widget) {
       inspector.source = widget.content.inspectionHandler;
     }

src/editorwidget/plugin.ts

@@ -95,11 +95,6 @@ function activate(app: JupyterLab, registry: IDocumentRegistry, restorer: IInsta
     name: widget => widget.context.path
   });
 
-  // Sync tracker with currently focused widget.
-  app.shell.currentChanged.connect((sender, args) => {
-    tracker.sync(args.newValue);
-  });
-
   factory.widgetCreated.connect((sender, widget) => {
     widget.title.icon = `${PORTRAIT_ICON_CLASS} ${EDITOR_ICON_CLASS}`;
     // Notify the instance tracker if restore data needs to update.

src/imagewidget/plugin.ts

@@ -74,11 +74,6 @@ function activate(app: JupyterLab, registry: IDocumentRegistry, palette: IComman
     name: widget => widget.context.path
   });
 
-  // Sync tracker with currently focused widget.
-  app.shell.currentChanged.connect((sender, args) => {
-    tracker.sync(args.newValue);
-  });
-
   registry.addWidgetFactory(factory);
 
   factory.widgetCreated.connect((sender, widget) => {

src/markdownwidget/plugin.ts

@@ -56,6 +56,7 @@ const plugin: JupyterLabPlugin<void> = {
     });
     const namespace = 'rendered-markdown';
     const tracker = new InstanceTracker<MarkdownWidget>({ namespace });
+    let icon = `${PORTRAIT_ICON_CLASS} ${TEXTEDITOR_ICON_CLASS}`;
 
     // Handle state restoration.
     restorer.restore(tracker, {
@@ -64,13 +65,6 @@ const plugin: JupyterLabPlugin<void> = {
       name: widget => widget.context.path
     });
 
-    let icon = `${PORTRAIT_ICON_CLASS} ${TEXTEDITOR_ICON_CLASS}`;
-
-    // Sync tracker with currently focused widget.
-    app.shell.currentChanged.connect((sender, args) => {
-      tracker.sync(args.newValue);
-    });
-
     factory.widgetCreated.connect((sender, widget) => {
       widget.title.icon = icon;
       // Notify the instance tracker if restore data needs to update.

src/notebook/plugin.ts

@@ -189,9 +189,8 @@ function activateNotebookHandler(app: JupyterLab, registry: IDocumentRegistry, s
     when: services.ready
   });
 
-  // Sync tracker and set the source of the code inspector.
-  app.shell.currentChanged.connect((sender, args) => {
-    let widget = tracker.sync(args.newValue);
+  // Set the source of the code inspector.
+  tracker.currentChanged.connect((sender, widget) => {
     if (widget) {
       inspector.source = widget.content.inspectionHandler;
     }

src/notebook/tracker.ts

@@ -141,4 +141,4 @@ class NotebookTracker extends InstanceTracker<NotebookPanel> implements INoteboo
 }
 
 // Define the signals for the `NotebookTracker` class.
-defineSignal(InstanceTracker.prototype, 'activeCellChanged');
+defineSignal(NotebookTracker.prototype, 'activeCellChanged');

src/terminal/plugin.ts

@@ -101,11 +101,6 @@ function activate(app: JupyterLab, services: IServiceManager, mainMenu: IMainMen
     name: widget => widget.session && widget.session.name
   });
 
-  // Sync tracker with currently focused widget.
-  app.shell.currentChanged.connect((sender, args) => {
-    tracker.sync(args.newValue);
-  });
-
   // Add terminal commands.
   commands.addCommand(newTerminalId, {
     label: 'New Terminal',

test/src/common/instancetracker.spec.ts

@@ -7,6 +7,10 @@ import {
   Widget
 } from 'phosphor/lib/ui/widget';
 
+import {
+  simulate
+} from 'simulate-event';
+
 import {
   InstanceTracker
 } from '../../../lib/common/instancetracker';
@@ -42,13 +46,15 @@ describe('common/instancetracker', () => {
 
       it('should emit when the current widget has been updated', () => {
         let tracker = new InstanceTracker<Widget>({ namespace: NAMESPACE });
-        let widget = new Widget();
+        let widget = new Widget({ node: document.createElement('input') });
         let called = false;
         tracker.currentChanged.connect(() => { called = true; });
         tracker.add(widget);
+        Widget.attach(widget, document.body);
         expect(called).to.be(false);
-        tracker.sync(widget);
+        simulate(widget.node, 'focus');
         expect(called).to.be(true);
+        Widget.detach(widget);
       });
 
     });
@@ -60,13 +66,15 @@ describe('common/instancetracker', () => {
         expect(tracker.currentWidget).to.be(null);
       });
 
-      it('should be updated by sync if the tracker has the widget', () => {
+      it('should be updated if when the widget is focused', () => {
         let tracker = new InstanceTracker<Widget>({ namespace: NAMESPACE });
-        let widget = new Widget();
+        let widget = new Widget({ node: document.createElement('input') });
         tracker.add(widget);
+        Widget.attach(widget, document.body);
         expect(tracker.currentWidget).to.be(null);
-        tracker.sync(widget);
+        simulate(widget.node, 'focus');
         expect(tracker.currentWidget).to.be(widget);
+        Widget.detach(widget);
       });
 
     });
@@ -186,29 +194,17 @@ describe('common/instancetracker', () => {
 
     });
 
-    describe('#sync()', () => {
-
-      it('should emit a signal when the current widget is updated', () => {
-        let tracker = new InstanceTracker<Widget>({ namespace: NAMESPACE });
-        let widget = new Widget();
-        let called = false;
-        tracker.currentChanged.connect(() => { called = true; });
-        tracker.add(widget);
-        expect(called).to.be(false);
-        tracker.sync(widget);
-        expect(called).to.be(true);
-      });
-
-    });
-
     describe('#onCurrentChanged()', () => {
 
       it('should be called when the current widget is changed', () => {
         let tracker = new TestTracker<Widget>({ namespace: NAMESPACE });
         let widget = new Widget();
         tracker.add(widget);
-        tracker.sync(widget);
+        expect(tracker.methods).to.not.contain('onCurrentChanged');
+        Widget.attach(widget, document.body);
+        simulate(widget.node, 'focus');
         expect(tracker.methods).to.contain('onCurrentChanged');
+        Widget.detach(widget);
       });
 
     });

test/src/notebook/tracker.spec.ts

@@ -7,6 +7,14 @@ import {
   MimeData
 } from 'phosphor/lib/core/mimedata';
 
+import {
+  Widget
+} from 'phosphor/lib/ui/widget';
+
+import {
+  simulate
+} from 'simulate-event';
+
 import {
   BaseCellWidget
 } from '../../../lib/notebook/cells';
@@ -73,7 +81,6 @@ describe('notebook/tracker', () => {
         let tracker = new NotebookTracker({ namespace: NAMESPACE });
         let panel = new NotebookPanel({ rendermime, clipboard, renderer});
         tracker.add(panel);
-        tracker.sync(panel);
         expect(tracker.activeCell).to.be(null);
       });
 
@@ -81,9 +88,11 @@ describe('notebook/tracker', () => {
         let tracker = new NotebookTracker({ namespace: NAMESPACE });
         let panel = new NotebookPanel({ rendermime, clipboard, renderer});
         tracker.add(panel);
-        tracker.sync(panel);
         panel.context = createNotebookContext();
         panel.content.model.fromJSON(DEFAULT_CONTENT);
+        expect(tracker.activeCell).to.be(null);
+        Widget.attach(panel, document.body);
+        simulate(panel.node, 'focus');
         expect(tracker.activeCell).to.be.a(BaseCellWidget);
         panel.dispose();
       });
@@ -99,9 +108,10 @@ describe('notebook/tracker', () => {
         tracker.activeCellChanged.connect(() => { count++; });
         panel.context = createNotebookContext();
         panel.content.model.fromJSON(DEFAULT_CONTENT);
-        expect(count).to.be(0);
         tracker.add(panel);
-        tracker.sync(panel);
+        expect(count).to.be(0);
+        Widget.attach(panel, document.body);
+        simulate(panel.node, 'focus');
         expect(count).to.be(1);
         panel.content.activeCellIndex = 1;
         expect(count).to.be(2);
@@ -116,9 +126,11 @@ describe('notebook/tracker', () => {
         let tracker = new TestTracker({ namespace: NAMESPACE });
         let panel = new NotebookPanel({ rendermime, clipboard, renderer});
         tracker.add(panel);
-        tracker.sync(panel);
         panel.context = createNotebookContext();
         panel.content.model.fromJSON(DEFAULT_CONTENT);
+        expect(tracker.methods).to.not.contain('onCurrentChanged');
+        Widget.attach(panel, document.body);
+        simulate(panel.node, 'focus');
         expect(tracker.methods).to.contain('onCurrentChanged');
         panel.dispose();
       });