fixed memory leak in browser icons; updates for latest version of hpass

jupyterlab · Oct 14, 2019 · b2fffcd · b2fffcd
1 parent 92396a0
commit b2fffcd
Show file tree

Hide file tree

Showing 11 changed files with 57 additions and 39 deletions.
diff --git a/packages/apputils/src/mainareawidget.ts b/packages/apputils/src/mainareawidget.ts
@@ -174,7 +174,7 @@ export class MainAreaWidget<T extends Widget = Widget> extends Widget
     this.title.mnemonic = content.title.mnemonic;
     this.title.iconClass = content.title.iconClass;
     this.title.iconLabel = content.title.iconLabel;
-    this.title.iconPass = content.title.iconPass;
+    this.title.iconRender = content.title.iconRender;
     this.title.caption = content.title.caption;
     this.title.className = content.title.className;
     this.title.dataset = content.title.dataset;
@@ -194,7 +194,7 @@ export class MainAreaWidget<T extends Widget = Widget> extends Widget
     content.title.mnemonic = this.title.mnemonic;
     content.title.iconClass = this.title.iconClass;
     content.title.iconLabel = this.title.iconLabel;
-    content.title.iconPass = this.title.iconPass;
+    content.title.iconRender = this.title.iconRender;
     content.title.caption = this.title.caption;
     content.title.className = this.title.className;
     content.title.dataset = this.title.dataset;

diff --git a/packages/csvviewer-extension/src/index.ts b/packages/csvviewer-extension/src/index.ts
@@ -130,7 +130,7 @@ function activateCsv(
     if (ft) {
       widget.title.iconClass = ft.iconClass!;
       widget.title.iconLabel = ft.iconLabel!;
-      widget.title.iconPass = ft.iconPass!;
+      widget.title.iconRender = ft.iconRender!;
     }
     // Set the theme for the new widget.
     widget.content.style = style;
@@ -210,7 +210,7 @@ function activateTsv(
     if (ft) {
       widget.title.iconClass = ft.iconClass!;
       widget.title.iconLabel = ft.iconLabel!;
-      widget.title.iconPass = ft.iconPass!;
+      widget.title.iconRender = ft.iconRender!;
     }
     // Set the theme for the new widget.
     widget.content.style = style;

diff --git a/packages/docregistry/src/mimedocument.ts b/packages/docregistry/src/mimedocument.ts
@@ -287,7 +287,7 @@ export class MimeDocumentFactory extends ABCWidgetFactory<MimeDocument> {
 
     content.title.iconClass = ft.iconClass;
     content.title.iconLabel = ft.iconLabel;
-    content.title.iconPass = ft.iconPass;
+    content.title.iconRender = ft.iconRender;
 
     const widget = new MimeDocument({ content, context });
 

diff --git a/packages/docregistry/src/registry.ts b/packages/docregistry/src/registry.ts
@@ -1180,7 +1180,7 @@ export namespace DocumentRegistry {
      */
     readonly iconLabel?: string;
 
-    readonly iconPass?: JLIcon.IPhosphor;
+    readonly iconRender?: JLIcon.IPhosphor;
 
     /**
      * The content type of the new file.
@@ -1201,7 +1201,6 @@ export namespace DocumentRegistry {
     extensions: [],
     mimeTypes: [],
     iconClass: '',
-    iconPass: fileIcon.phosphor({ kind: 'mainAreaTab', center: true }),
     iconLabel: '',
     contentType: 'file',
     fileFormat: 'text'
@@ -1238,7 +1237,8 @@ export namespace DocumentRegistry {
     ...fileTypeDefaults,
     name: 'text',
     mimeTypes: ['text/plain'],
-    extensions: ['.txt']
+    extensions: ['.txt'],
+    iconRender: fileIcon.phosphor({ kind: 'mainAreaTab', center: true })
   };
 
   /**
@@ -1252,7 +1252,7 @@ export namespace DocumentRegistry {
     extensions: ['.ipynb'],
     contentType: 'notebook',
     fileFormat: 'json',
-    iconPass: notebookIcon.phosphor({ kind: 'mainAreaTab', center: true })
+    iconRender: notebookIcon.phosphor({ kind: 'mainAreaTab', center: true })
   };
 
   /**
@@ -1264,7 +1264,7 @@ export namespace DocumentRegistry {
     extensions: [],
     mimeTypes: ['text/directory'],
     contentType: 'directory',
-    iconPass: folderIcon.phosphor({ kind: 'mainAreaTab', center: true })
+    iconRender: folderIcon.phosphor({ kind: 'mainAreaTab', center: true })
   };
 
   /**
@@ -1279,56 +1279,62 @@ export namespace DocumentRegistry {
       displayName: 'Markdown File',
       extensions: ['.md'],
       mimeTypes: ['text/markdown'],
-      iconPass: markdownIcon.phosphor({ kind: 'mainAreaTab', center: true })
+      iconRender: markdownIcon.phosphor({ kind: 'mainAreaTab', center: true })
     },
     {
       name: 'python',
       displayName: 'Python File',
       extensions: ['.py'],
       mimeTypes: ['text/x-python'],
-      iconPass: pythonIcon.phosphor({ kind: 'mainAreaTab', center: true })
+      iconRender: pythonIcon.phosphor({ kind: 'mainAreaTab', center: true })
     },
     {
       name: 'json',
       displayName: 'JSON File',
       extensions: ['.json'],
       mimeTypes: ['application/json'],
-      iconPass: jsonIcon.phosphor({ kind: 'mainAreaTab', center: true })
+      iconRender: jsonIcon.phosphor({ kind: 'mainAreaTab', center: true })
     },
     {
       name: 'csv',
       displayName: 'CSV File',
       extensions: ['.csv'],
       mimeTypes: ['text/csv'],
-      iconPass: spreadsheetIcon.phosphor({ kind: 'mainAreaTab', center: true })
+      iconRender: spreadsheetIcon.phosphor({
+        kind: 'mainAreaTab',
+        center: true
+      })
     },
     {
       name: 'tsv',
       displayName: 'TSV File',
       extensions: ['.tsv'],
       mimeTypes: ['text/csv'],
-      iconPass: spreadsheetIcon.phosphor({ kind: 'mainAreaTab', center: true })
+      iconRender: spreadsheetIcon.phosphor({
+        kind: 'mainAreaTab',
+        center: true
+      })
     },
     {
       name: 'r',
       displayName: 'R File',
       mimeTypes: ['text/x-rsrc'],
       extensions: ['.r'],
-      iconPass: rKernelIcon.phosphor({ kind: 'mainAreaTab', center: true })
+      iconRender: rKernelIcon.phosphor({ kind: 'mainAreaTab', center: true })
     },
     {
       name: 'yaml',
       displayName: 'YAML File',
       mimeTypes: ['text/x-yaml', 'text/yaml'],
       extensions: ['.yaml', '.yml'],
-      iconPass: yamlIcon.phosphor({ kind: 'mainAreaTab', center: true })
+      iconRender: yamlIcon.phosphor({ kind: 'mainAreaTab', center: true })
     },
     {
       name: 'svg',
       displayName: 'Image',
       mimeTypes: ['image/svg+xml'],
       extensions: ['.svg'],
-      iconPass: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
+      iconRender: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
       fileFormat: 'base64'
     },
     {
@@ -1337,39 +1343,39 @@ export namespace DocumentRegistry {
       mimeTypes: ['image/tiff'],
       extensions: ['.tif', '.tiff'],
       iconClass: 'jp-MaterialIcon jp-ImageIcon',
-      iconPass: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
+      iconRender: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
       fileFormat: 'base64'
     },
     {
       name: 'jpeg',
       displayName: 'Image',
       mimeTypes: ['image/jpeg'],
       extensions: ['.jpg', '.jpeg'],
-      iconPass: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
+      iconRender: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
       fileFormat: 'base64'
     },
     {
       name: 'gif',
       displayName: 'Image',
       mimeTypes: ['image/gif'],
       extensions: ['.gif'],
-      iconPass: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
+      iconRender: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
       fileFormat: 'base64'
     },
     {
       name: 'png',
       displayName: 'Image',
       mimeTypes: ['image/png'],
       extensions: ['.png'],
-      iconPass: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
+      iconRender: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
       fileFormat: 'base64'
     },
     {
       name: 'bmp',
       displayName: 'Image',
       mimeTypes: ['image/bmp'],
       extensions: ['.bmp'],
-      iconPass: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
+      iconRender: imageIcon.phosphor({ kind: 'mainAreaTab', center: true }),
       fileFormat: 'base64'
     }
   ];

diff --git a/packages/filebrowser/package.json b/packages/filebrowser/package.json
@@ -50,7 +50,8 @@
     "@phosphor/messaging": "^1.3.0",
     "@phosphor/signaling": "^1.3.0",
     "@phosphor/widgets": "^1.9.0",
-    "react": "~16.8.4"
+    "react": "~16.8.4",
+    "react-dom": "~16.8.4"
   },
   "devDependencies": {
     "rimraf": "~2.6.2",

diff --git a/packages/filebrowser/src/listing.ts b/packages/filebrowser/src/listing.ts
@@ -45,6 +45,8 @@ import { ISignal, Signal } from '@phosphor/signaling';
 
 import { Widget } from '@phosphor/widgets';
 
+import ReactDOM from 'react-dom';
+
 import { FileBrowserModel } from './model';
 
 /**
@@ -734,7 +736,10 @@ export class DirListing extends Widget {
 
     // Remove any excess item nodes.
     while (nodes.length > items.length) {
-      content.removeChild(nodes.pop());
+      let node = nodes.pop();
+      let icon = DOMUtils.findElement(node, ITEM_ICON_CLASS);
+      ReactDOM.unmountComponentAtNode(icon);
+      content.removeChild(node);
     }
 
     // Add any missing item nodes.
@@ -1810,20 +1815,27 @@ export namespace DirListing {
       if (fileType) {
         // TODO: remove workaround if...else/code in else clause in v2.0.0
         // workaround for 1.0.x versions of Jlab pulling in 1.1.x versions of filebrowser
-        if (fileType.iconPass) {
+        if (fileType.iconRender) {
           // add icon as svg node. Can be styled using CSS
-          fileType.iconPass.render(icon, {
+          fileType.iconRender(icon, {
             className: ITEM_ICON_CLASS,
+            container: icon,
             title: fileType.iconLabel,
             center: true,
             kind: 'listing'
           });
         } else {
+          // cleanup after react
+          ReactDOM.unmountComponentAtNode(icon);
+
           // add icon as CSS background image. Can't be styled using CSS
           icon.className = `${ITEM_ICON_CLASS} ${fileType.iconClass || ''}`;
           icon.textContent = fileType.iconLabel || '';
         }
       } else {
+        // cleanup after react
+        ReactDOM.unmountComponentAtNode(icon);
+
         // use default icon as CSS background image
         icon.className = ITEM_ICON_CLASS;
         icon.textContent = '';

diff --git a/packages/fileeditor/src/widget.ts b/packages/fileeditor/src/widget.ts
@@ -326,7 +326,7 @@ export class FileEditorFactory extends ABCWidgetFactory<
       mimeTypeService: this._services.mimeTypeService
     });
 
-    content.title.iconPass = textEditorIcon.phosphor({
+    content.title.iconRender = textEditorIcon.phosphor({
       kind: 'mainAreaTab',
       center: true
     });

diff --git a/packages/imageviewer-extension/src/index.ts b/packages/imageviewer-extension/src/index.ts
@@ -107,7 +107,7 @@ function activate(
     if (types.length > 0) {
       widget.title.iconClass = types[0].iconClass;
       widget.title.iconLabel = types[0].iconLabel;
-      widget.title.iconPass = types[0].iconPass;
+      widget.title.iconRender = types[0].iconRender;
     }
   });
 

diff --git a/packages/markdownviewer/src/widget.ts b/packages/markdownviewer/src/widget.ts
@@ -309,7 +309,7 @@ export class MarkdownViewerFactory extends ABCWidgetFactory<MarkdownDocument> {
     const content = new MarkdownViewer({ context, renderer });
     content.title.iconClass = this._fileType.iconClass;
     content.title.iconLabel = this._fileType.iconLabel;
-    content.title.iconPass = this._fileType.iconPass;
+    content.title.iconRender = this._fileType.iconRender;
     const widget = new MarkdownDocument({ content, context });
 
     return widget;

diff --git a/packages/notebook-extension/src/index.ts b/packages/notebook-extension/src/index.ts
@@ -589,7 +589,7 @@ function activateNotebookHandler(
     widget.id = widget.id || `notebook-${++id}`;
     widget.title.iconClass = ft.iconClass;
     widget.title.iconLabel = ft.iconLabel;
-    widget.title.iconPass = ft.iconPass;
+    widget.title.iconRender = ft.iconRender;
     // Notify the widget tracker if restore data needs to update.
     widget.context.pathChanged.connect(() => {
       void tracker.save(widget);

diff --git a/packages/ui-components/src/icon/jlicon.tsx b/packages/ui-components/src/icon/jlicon.tsx
@@ -76,11 +76,9 @@ export class JLIcon {
   }
 
   phosphor(props: JLIcon.IProps = {}): JLIcon.IPhosphor {
-    return {
-      render: (host: HTMLElement, innerProps: JLIcon.IProps = {}) => {
-        const comb = { ...props, ...innerProps };
-        return ReactDOM.render(<this.react {...comb} />, host);
-      }
+    return (host: HTMLElement, innerProps: JLIcon.IProps = {}) => {
+      const comb = { ...props, ...innerProps };
+      return ReactDOM.render(<this.react {...comb} container={host} />, host);
     };
   }
 
@@ -163,9 +161,10 @@ export namespace JLIcon {
     title?: string;
   }
 
-  export interface IPhosphor {
-    render: (host: HTMLElement, innerProps?: JLIcon.IProps) => void;
-  }
+  export type IPhosphor = (
+    host: HTMLElement,
+    innerProps?: JLIcon.IProps
+  ) => void;
 }
 
 namespace Private {