Merge pull request #1508 from blink1073/audit-signals

Audit of signal connections

src/application/shell.ts

@@ -146,16 +146,7 @@ class ApplicationShell extends Widget {
 
     this.layout = rootLayout;
 
-    this._dockPanel.currentChanged.connect((sender, args) => {
-      if (args.newValue) {
-        args.newValue.title.className += ` ${CURRENT_CLASS}`;
-      }
-      if (args.oldValue) {
-        let title = args.oldValue.title;
-        title.className = title.className.replace(CURRENT_CLASS, '');
-      }
-      this.currentChanged.emit(args);
-    });
+    this._dockPanel.currentChanged.connect(this._onCurrentChanged, this);
   }
 
   /**
@@ -345,9 +336,9 @@ class ApplicationShell extends Widget {
       return this._save().then(() => { this._restored.resolve(saved); });
     });
     // Catch current changed events on the side handlers.
-    this._dockPanel.currentChanged.connect(() => { this._save(); });
-    this._leftHandler.sideBar.currentChanged.connect(() => { this._save(); });
-    this._rightHandler.sideBar.currentChanged.connect(() => { this._save(); });
+    this._dockPanel.currentChanged.connect(this._save, this);
+    this._leftHandler.sideBar.currentChanged.connect(this._save, this);
+    this._rightHandler.sideBar.currentChanged.connect(this._save, this);
   }
 
   /**
@@ -366,6 +357,20 @@ class ApplicationShell extends Widget {
     return this._database.save(data);
   }
 
+  /**
+   * Handle a change to the dock area current widget.
+   */
+  private _onCurrentChanged(sender: DockPanel, args: DockPanel.ICurrentChangedArgs): void {
+    if (args.newValue) {
+      args.newValue.title.className += ` ${CURRENT_CLASS}`;
+    }
+    if (args.oldValue) {
+      let title = args.oldValue.title;
+      title.className = title.className.replace(CURRENT_CLASS, '');
+    }
+    this.currentChanged.emit(args);
+  }
+
   private _database: IInstanceRestorer.ILayoutDB = null;
   private _dockPanel: DockPanel;
   private _isRestored = false;

src/codemirror/editor.ts

@@ -121,7 +121,7 @@ class CodeMirrorEditor implements CodeEditor.IEditor {
 
     // Connect to changes.
     model.value.changed.connect(this._onValueChanged, this);
-    model.mimeTypeChanged.connect(() => this._onMimeTypeChanged(), this);
+    model.mimeTypeChanged.connect(this._onMimeTypeChanged, this);
     model.selections.changed.connect(this._onSelectionsChanged, this);
 
     CodeMirror.on(editor, 'keydown', (editor, event) => {

src/csvwidget/widget.ts

@@ -64,28 +64,16 @@ class CSVWidget extends Widget {
     this._model = new CSVModel({ content: context.model.toString() });
     this._table = new CSVTable();
     this._table.model = this._model;
-    this._model.maxExceeded.connect((sender, overflow) => {
-      let { available, maximum } = overflow;
-      let message = `Table is too long to render,
-        rendering ${maximum} of ${available} rows`;
-      this._warning.node.textContent = message;
-    }, this);
+    this._model.maxExceeded.connect(this._onMaxExceeded, this);
 
     this._toolbar = new CSVToolbar();
-    this._toolbar.delimiterChanged.connect((sender, delimiter) => {
-      this._table.model.delimiter = delimiter;
-    }, this);
-
+    this._toolbar.delimiterChanged.connect(this._onDelimiterChanged, this);
     layout.addWidget(this._toolbar);
     layout.addWidget(this._table);
     layout.addWidget(this._warning);
 
-    context.pathChanged.connect((c, path) => {
-      this.title.label = path.split('/').pop();
-    }, this);
-    context.model.contentChanged.connect(() => {
-      this._table.model.content = context.model.toString();
-    }, this);
+    context.pathChanged.connect(this._onPathChanged, this);
+    context.model.contentChanged.connect(this._onContentChanged, this);
   }
 
   /**
@@ -134,6 +122,37 @@ class CSVWidget extends Widget {
     this.node.focus();
   }
 
+  /**
+   * Handle a max exceeded in a csv widget.
+   */
+  private _onMaxExceeded(sender: CSVModel, overflow: CSVModel.IOverflow): void {
+    let { available, maximum } = overflow;
+    let message = `Table is too long to render,
+      rendering ${maximum} of ${available} rows`;
+    this._warning.node.textContent = message;
+  }
+
+  /**
+   * Handle a change in delimiter.
+   */
+  private _onDelimiterChanged(sender: CSVToolbar, delimiter: string): void {
+    this._table.model.delimiter = delimiter;
+  }
+
+  /**
+   * Handle a change in content.
+   */
+  private _onContentChanged(): void {
+    this._table.model.content = this._context.model.toString();
+  }
+
+  /**
+   * Handle a change in path.
+   */
+  private _onPathChanged(): void {
+    this.title.label = this._context.path.split('/').pop();
+  }
+
   private _context: DocumentRegistry.Context = null;
   private _model: CSVModel = null;
   private _table: CSVTable = null;

src/docmanager/manager.ts

@@ -299,13 +299,18 @@ class DocumentManager implements IDisposable {
     context.ready.then(() => {
       handler.start();
     });
-    context.disposed.connect(() => {
-      this._contexts.remove(context);
-    });
+    context.disposed.connect(this._onContextDisposed, this);
     this._contexts.pushBack(context);
     return context;
   }
 
+  /**
+   * Handle a context disposal.
+   */
+  private _onContextDisposed(context: Private.IContext): void {
+    this._contexts.remove(context);
+  }
+
   /**
    * Get the model factory for a given widget name.
    */

src/docmanager/widgetmanager.ts

@@ -106,13 +106,11 @@ class DocumentWidgetManager implements IDisposable {
     each(this._registry.widgetExtensions(name), extender => {
       disposables.add(extender.createNew(widget, context));
     });
-    widget.disposed.connect(() => {
-      disposables.dispose();
-    });
+    Private.disposablesProperty.set(widget, disposables);
+    widget.disposed.connect(this._onWidgetDisposed, this);
+
     this.adoptWidget(context, widget);
-    context.fileChanged.connect(() => {
-      this.setCaption(widget);
-    });
+    context.fileChanged.connect(this._onFileChanged, this);
     context.ready.then(() => {
       this.setCaption(widget);
     });
@@ -135,14 +133,7 @@ class DocumentWidgetManager implements IDisposable {
     });
     widget.addClass(DOCUMENT_CLASS);
     widget.title.closable = true;
-    widget.disposed.connect(() => {
-      // Remove the widget.
-      widgets.remove(widget);
-      // Dispose of the context if this is the last widget using it.
-      if (!widgets.length) {
-        context.dispose();
-      }
-    });
+    widget.disposed.connect(this._widgetDisposed, this);
     Private.contextProperty.set(widget, context);
   }
 
@@ -310,6 +301,36 @@ class DocumentWidgetManager implements IDisposable {
     });
   }
 
+  /**
+   * Handle the disposal of a widget.
+   */
+  private _widgetDisposed(widget: Widget): void {
+    let context = Private.contextProperty.get(widget);
+    let widgets = Private.widgetsProperty.get(context);
+     // Remove the widget.
+    widgets.remove(widget);
+    // Dispose of the context if this is the last widget using it.
+    if (!widgets.length) {
+      context.dispose();
+    }
+  }
+
+  /**
+   * Handle the disposal of a widget.
+   */
+  private _onWidgetDisposed(widget: Widget): void {
+    let disposables = Private.disposablesProperty.get(widget);
+    disposables.dispose();
+  }
+
+  /**
+   * Handle a file changed signal for a context.
+   */
+  private _onFileChanged(context: DocumentRegistry.Context): void {
+    let widgets = Private.widgetsProperty.get(context);
+    each(widgets, widget => { this.setCaption(widget); });
+  }
+
   private _closeGuard = false;
   private _registry: DocumentRegistry = null;
 }
@@ -363,4 +384,12 @@ namespace Private {
       return new Vector<Widget>();
     }
   });
+
+  /**
+   * A private attached property for a widget's disposables.
+   */
+  export
+  const disposablesProperty = new AttachedProperty<Widget, DisposableSet>({
+    name: 'disposables'
+  });
 }

src/docregistry/context.ts

@@ -421,15 +421,8 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
       }
       this._session = session;
       this.kernelChanged.emit(session.kernel);
-      session.pathChanged.connect((s, path) => {
-        if (path !== this._path) {
-          this._path = path;
-          this.pathChanged.emit(path);
-        }
-      });
-      session.kernelChanged.connect((s, kernel) => {
-        this.kernelChanged.emit(kernel);
-      });
+      session.pathChanged.connect(this._onSessionPathChanged, this);
+      session.kernelChanged.connect(this._onKernelChanged, this);
       return session.kernel;
     }).catch(err => {
       let response = JSON.parse(err.xhr.response);
@@ -444,6 +437,24 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
     });
   }
 
+  /**
+   * Handle a change to a session path.
+   */
+  private _onSessionPathChanged(sender: Session.ISession) {
+    let path = sender.path;
+    if (path !== this._path) {
+      this._path = path;
+      this.pathChanged.emit(path);
+    }
+  }
+
+  /**
+   * Handle a change to the kernel.
+   */
+  private _onKernelChanged(sender: Session.ISession): void {
+    this.kernelChanged.emit(sender.kernel);
+  }
+
   /**
    * Update our contents model, without the content.
    */

src/filebrowser/buttons.ts

@@ -133,10 +133,10 @@ class FileButtons extends Widget {
     this.addClass(FILE_BUTTONS_CLASS);
     this._model = options.model;
 
-    this._buttons.create.onmousedown = this._onCreateButtonPressed;
-    this._buttons.upload.onclick = this._onUploadButtonClicked;
-    this._buttons.refresh.onclick = this._onRefreshButtonClicked;
-    this._input.onchange = this._onInputChanged;
+    this._buttons.create.onmousedown = this._onCreateButtonPressed.bind(this);
+    this._buttons.upload.onclick = this._onUploadButtonClicked.bind(this);
+    this._buttons.refresh.onclick = this._onRefreshButtonClicked.bind(this);
+    this._input.onchange = this._onInputChanged.bind(this);
 
     let node = this.node;
     node.appendChild(this._buttons.create);
@@ -241,7 +241,7 @@ class FileButtons extends Widget {
   /**
    * The 'mousedown' handler for the create button.
    */
-  private _onCreateButtonPressed = (event: MouseEvent) => {
+  private _onCreateButtonPressed(event: MouseEvent) {
     // Do nothing if nothing if it's not a left press.
     if (event.button !== 0) {
       return;
@@ -265,49 +265,58 @@ class FileButtons extends Widget {
     // Setup the `aboutToClose` signal handler. The menu is disposed on an
     // animation frame to allow a mouse press event which closed the
     // menu to run its course. This keeps the button from re-opening.
-    dropdown.aboutToClose.connect(() => {
-      requestAnimationFrame(() => { dropdown.dispose(); });
-    });
+    dropdown.aboutToClose.connect(this._onDropDownAboutToClose, this);
 
     // Setup the `disposed` signal handler. This restores the button
     // to the non-active state and allows a new menu to be opened.
-    dropdown.disposed.connect(() => {
-      button.classList.remove(ACTIVE_CLASS);
-    });
+    dropdown.disposed.connect(this._onDropDownDisposed, this);
 
     // Popup the menu aligned with the bottom of the create button.
     dropdown.open(rect.left, rect.bottom, { forceX: false, forceY: false });
   };
 
+  /**
+   * Handle a dropdwon about to close.
+   */
+  private _onDropDownAboutToClose(sender: Menu): void {
+    requestAnimationFrame(() => { sender.dispose(); });
+  }
+
+  /**
+   * Handle a dropdown disposal.
+   */
+  private _onDropDownDisposed(sender: Menu): void {
+    this._buttons.create.classList.remove(ACTIVE_CLASS);
+  }
 
   /**
    * The 'click' handler for the upload button.
    */
-  private _onUploadButtonClicked = (event: MouseEvent) => {
+  private _onUploadButtonClicked(event: MouseEvent) {
     if (event.button !== 0) {
       return;
     }
     this._input.click();
-  };
+  }
 
   /**
    * The 'click' handler for the refresh button.
    */
-  private _onRefreshButtonClicked = (event: MouseEvent) => {
+  private _onRefreshButtonClicked(event: MouseEvent) {
     if (event.button !== 0) {
       return;
     }
     // Force a refresh of the current directory.
     this._model.cd('.');
-  };
+  }
 
   /**
    * The 'change' handler for the input field.
    */
-  private _onInputChanged = () => {
+  private _onInputChanged(): void {
     let files = Array.prototype.slice.call(this._input.files);
     Private.uploadFiles(this, files as File[]);
-  };
+  }
 
   private _buttons = Private.createButtons();
   private _commands: CommandRegistry = null;

src/instancerestorer/instancerestorer.ts

@@ -262,7 +262,7 @@ class InstanceRestorer implements IInstanceRestorer {
   add(widget: Widget, name: string): void {
     Private.nameProperty.set(widget, name);
     this._widgets.set(name, widget);
-    widget.disposed.connect(() => { this._widgets.delete(name); });
+    widget.disposed.connect(this._onWidgetDisposed, this);
   }
 
   /**
@@ -415,6 +415,14 @@ class InstanceRestorer implements IInstanceRestorer {
     return { collapsed, currentWidget, widgets };
   }
 
+  /**
+   * Handle a widget disposal.
+   */
+  private _onWidgetDisposed(widget: Widget): void {
+    let name = Private.nameProperty.get(widget);
+    this._widgets.delete(name);
+  }
+
   private _first: Promise<any> = null;
   private _promises: Promise<any>[] = [];
   private _restored = new utils.PromiseDelegate<void>();

src/mainmenu/index.ts

@@ -2,7 +2,7 @@
 // Distributed under the terms of the Modified BSD License.
 
 import {
-  find, indexOf, upperBound
+  findIndex, indexOf, upperBound
 } from 'phosphor/lib/algorithm/searching';
 
 import {
@@ -73,15 +73,23 @@ class MainMenu extends MenuBar implements IMainMenu {
     let index = upperBound(this._items, rankItem, Private.itemCmp);
 
     // Upon disposal, remove the menu and its rank reference.
-    menu.disposed.connect(() => {
-      this.removeMenu(menu);
-      this._items.remove(rankItem);
-    });
+    menu.disposed.connect(this._onMenuDisposed, this);
 
     this._items.insert(index, rankItem);
     this.insertMenu(index, menu);
   }
 
+  /**
+   * Handle the disposal of a menu.
+   */
+  private _onMenuDisposed(menu: Menu): void {
+    this.removeMenu(menu);
+    let index = findIndex(this._items, item => item.menu === menu);
+    if (index !== -1) {
+      this._items.removeAt(index);
+    }
+  }
+
   private _items = new Vector<Private.IRankItem>();
 }
 

src/markdownwidget/widget.ts

@@ -54,16 +54,14 @@ class MarkdownWidget extends Widget {
     rendermime.resolver = context;
     this._context = context;
 
-    context.pathChanged.connect((c, path) => {
-      this.title.label = path.split('/').pop();
-    });
+    context.pathChanged.connect(this._onPathChanged, this);
 
     // Throttle the rendering rate of the widget.
     this._monitor = new ActivityMonitor({
       signal: context.model.contentChanged,
       timeout: RENDER_TIMEOUT
     });
-    this._monitor.activityStopped.connect(() => { this.update(); });
+    this._monitor.activityStopped.connect(this.update, this);
   }
 
   /**
@@ -111,6 +109,13 @@ class MarkdownWidget extends Widget {
     layout.addWidget(widget);
   }
 
+  /**
+   * Handle a path change.
+   */
+  private _onPathChanged(): void {
+    this.title.label = this._context.path.split('/').pop();
+  }
+
   private _context: DocumentRegistry.Context = null;
   private _monitor: ActivityMonitor<any, any> = null;
   private _rendermime: RenderMime = null;

src/notebook/cells/model.ts

@@ -332,9 +332,7 @@ class CodeCellModel extends CellModel implements ICodeCellModel {
         this._outputs.add(output);
       }
     }
-    this._outputs.changed.connect(() => {
-      this.contentChanged.emit(void 0);
-    });
+    this._outputs.changed.connect(this._onOutputsChanged, this);
   }
 
   /**
@@ -396,6 +394,13 @@ class CodeCellModel extends CellModel implements ICodeCellModel {
     return cell;
   }
 
+  /**
+   * Handle the outputs changing.
+   */
+  private _onOutputsChanged(): void {
+    this.contentChanged.emit(void 0);
+  }
+
   private _outputs: OutputAreaModel = null;
   private _executionCount: nbformat.ExecutionCount = null;
 }

src/notebook/notebook/default-toolbar.ts

@@ -213,7 +213,7 @@ class CellTypeSwitcher extends Widget {
     super({ node: createCellTypeSwitcherNode() });
     this.addClass(TOOLBAR_CELLTYPE_CLASS);
 
-    let select = this.node.firstChild as HTMLSelectElement;
+    let select = this._select = this.node.firstChild as HTMLSelectElement;
     this._wildCard = document.createElement('option');
     this._wildCard.value = '-';
     this._wildCard.textContent = '-';
@@ -229,26 +229,26 @@ class CellTypeSwitcher extends Widget {
       }
     });
 
+    this._notebook = widget;
+
     // Set the initial value.
     if (widget.model) {
-      this._updateValue(widget, select);
+      this._updateValue();
     }
 
     // Follow the type of the active cell.
-    widget.activeCellChanged.connect((sender, cell) => {
-      this._updateValue(widget, select);
-    });
+    widget.activeCellChanged.connect(this._updateValue, this);
 
     // Follow a change in the selection.
-    widget.selectionChanged.connect(() => {
-      this._updateValue(widget, select);
-    });
+    widget.selectionChanged.connect(this._updateValue, this);
   }
 
   /**
    * Update the value of the dropdown from the widget state.
    */
-  private _updateValue(widget: Notebook, select: HTMLSelectElement): void {
+  private _updateValue(): void {
+    let widget = this._notebook;
+    let select = this._select;
     if (!widget.activeCell) {
       return;
     }
@@ -273,6 +273,8 @@ class CellTypeSwitcher extends Widget {
 
   private _changeGuard = false;
   private _wildCard: HTMLOptionElement = null;
+  private _select: HTMLSelectElement = null;
+  private _notebook: Notebook = null;
 }
 
 

src/toolbar/kernel.ts

@@ -105,29 +105,38 @@ function createRestartButton(kernelOwner: IKernelOwner, host?: HTMLElement): Too
  */
 export
 function createKernelNameItem(kernelOwner: IKernelOwner): Widget {
-  let widget = new Widget();
-  widget.addClass(TOOLBAR_KERNEL_CLASS);
-  updateKernelNameItem(widget, kernelOwner.kernel);
-  kernelOwner.kernelChanged.connect(() => {
-    updateKernelNameItem(widget, kernelOwner.kernel);
-  });
-  return widget;
+  return new KernelName(kernelOwner);
 }
 
 
 /**
- * Update the text of the kernel name item.
+ * A kernel name widget.
  */
-function updateKernelNameItem(widget: Widget, kernel: Kernel.IKernel): void {
-  widget.node.textContent = 'No Kernel!';
-  if (!kernel) {
-    return;
+class KernelName extends Widget {
+  /**
+   * Construct a new kernel name widget.
+   */
+  constructor(kernelOwner: IKernelOwner) {
+    super();
+    this.addClass(TOOLBAR_KERNEL_CLASS);
+    this._onKernelChanged(kernelOwner, kernelOwner.kernel);
+    kernelOwner.kernelChanged.connect(this._onKernelChanged, this);
   }
-  kernel.getSpec().then(spec => {
-    if (!widget.isDisposed) {
-      widget.node.textContent = spec.display_name;
+
+  /**
+   * Update the text of the kernel name item.
+   */
+  _onKernelChanged(sender: IKernelOwner, kernel: Kernel.IKernel): void {
+    this.node.textContent = 'No Kernel!';
+    if (!kernel) {
+      return;
     }
-  });
+    kernel.getSpec().then(spec => {
+      if (!this.isDisposed) {
+        this.node.textContent = spec.display_name;
+      }
+    });
+  }
 }
 
 
@@ -156,22 +165,25 @@ class KernelIndicator extends Widget {
   constructor(kernelOwner: IKernelOwner) {
     super();
     this.addClass(TOOLBAR_INDICATOR_CLASS);
-    if (kernelOwner.kernel) {
-      this._handleStatus(kernelOwner.kernel, kernelOwner.kernel.status);
-      kernelOwner.kernel.statusChanged.connect(this._handleStatus, this);
+    this._onKernelChanged(kernelOwner, kernelOwner.kernel);
+    kernelOwner.kernelChanged.connect(this._onKernelChanged, this);
+  }
+
+  /**
+   * Handle a change in kernel.
+   */
+  private _onKernelChanged(sender: IKernelOwner, kernel: Kernel.IKernel): void {
+    if (this._kernel) {
+      this._kernel.statusChanged.disconnect(this._handleStatus, this);
+    }
+    this._kernel = kernel;
+    if (kernel) {
+      this._handleStatus(kernel, kernel.status);
+      kernel.statusChanged.connect(this._handleStatus, this);
     } else {
-      this.addClass(TOOLBAR_BUSY_CLASS);
       this.node.title = 'No Kernel!';
+      this.addClass(TOOLBAR_BUSY_CLASS);
     }
-    kernelOwner.kernelChanged.connect((c, kernel) => {
-      if (kernel) {
-        this._handleStatus(kernel, kernel.status);
-        kernel.statusChanged.connect(this._handleStatus, this);
-      } else {
-        this.node.title = 'No Kernel!';
-        this.addClass(TOOLBAR_BUSY_CLASS);
-      }
-    });
   }
 
   /**
@@ -185,4 +197,6 @@ class KernelIndicator extends Widget {
     let title = 'Kernel ' + status[0].toUpperCase() + status.slice(1);
     this.node.title = title;
   }
+
+  private _kernel: Kernel.IKernel = null;
 }

tutorial/patterns.md

@@ -75,7 +75,11 @@ the sender as the first argument, and contain a single second argument
 with the payload.  Signals should generally not be used to trigger the 
 "default" behavior for an action, but to allow others to trigger additional
 behavior.  If a "default" behavior is intended to be provided by another
-object, then a callback should be provided by that object.
+object, then a callback should be provided by that object.  Wherever possible
+as signal connection should be made with the pattern 
+`.connect(this._onFoo, this)`.  Providing the `this` context allows the
+connection to be properly cleared by `clearSignalData(this)`.  Using a
+private method avoids allocating a closure for each connection.
 
 
 ## Models