Merge pull request #46 from telamonian/iconRenderer-for-commands

IRenderer cleanup; normalize icon fields across all interfaces
jupyterlab · Feb 10, 2020 · bf35b5f · bf35b5f
2 parents 1beb8b3 + c8284df
commit bf35b5f
Show file tree

Hide file tree

Showing 15 changed files with 367 additions and 73 deletions.
diff --git a/.gitignore b/.gitignore
@@ -7,10 +7,12 @@ build
 review/api/temp
 *.tsbuildinfo
 
-# jetbrains IDE stuff
+# jetbrains ide stuff
 *.iml
 .idea/
 
-# ms IDE stuff
+# ms ide stuff
+*.code-workspace
+.history
 .vscode
 
diff --git a/packages/commands/package.json b/packages/commands/package.json
@@ -34,10 +34,20 @@
     "clean": "rimraf lib && rimraf *.tsbuildinfo",
     "clean:test": "rimraf tests/build",
     "docs": "typedoc --options tdoptions.json src",
+
     "test": "npm run test:firefox",
-    "test:chrome": "cd tests && karma start --browsers=Chrome",
-    "test:firefox": "cd tests && karma start --browsers=Firefox",
-    "test:ie": "cd tests && karma start --browsers=IE",
+    "test:chrome": "npm run test:nobrowser -- --browsers=Chrome",
+    "test:chrome-headless": "npm run test:nobrowser -- --browsers=ChromeHeadless",
+    "test:ie": "npm run test:nobrowser -- --browsers=IE",
+    "test:firefox": "npm run test:nobrowser -- --browsers=Firefox",
+    "test:nobrowser": "cd tests && karma start",
+
+    "test:debug": "npm run test:debug:firefox",
+    "test:debug:chrome-headless": "npm run test:debug:nobrowser -- --browsers=ChromeHeadless",
+    "test:debug:chrome": "npm run test:debug:nobrowser -- --browsers=Chrome",
+    "test:debug:firefox": "npm run test:debug:nobrowser -- --browsers=Firefox",
+    "test:debug:nobrowser": "cd tests && karma start --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
+
     "watch": "tsc --build --watch"
   },
   "dependencies": {
@@ -46,7 +56,8 @@
     "@lumino/disposable": "^1.3.4",
     "@lumino/domutils": "^1.1.7",
     "@lumino/keyboard": "^1.1.6",
-    "@lumino/signaling": "^1.3.4"
+    "@lumino/signaling": "^1.3.4",
+    "@lumino/virtualdom": "^1.5.0"
   },
   "devDependencies": {
     "@microsoft/api-extractor": "^7.6.0",

diff --git a/packages/commands/src/index.ts b/packages/commands/src/index.ts
@@ -31,6 +31,9 @@ import {
   ISignal, Signal
 } from '@lumino/signaling';
 
+import {
+  VirtualElement
+} from '@lumino/virtualdom';
 
 /**
  * An object which manages a collection of commands.
@@ -189,10 +192,25 @@ class CommandRegistry {
   }
 
   /**
-   * @deprecated Use `iconClass()` instead.
+   * Get the icon renderer for a specific command.
+   *
+   * DEPRECATED: if set to a string value, the .icon field will
+   * function as an alias for the .iconClass field, for backwards
+   * compatibility. In the future when this is removed, the default
+   * return type will become undefined.
+   *
+   * @param id - The id of the command of interest.
+   *
+   * @param args - The arguments for the command.
+   *
+   * @returns The icon renderer for the command, or
+   *   an empty string if the command is not registered.
    */
-  icon(id: string, args: ReadonlyPartialJSONObject = JSONExt.emptyObject): string {
-    return this.iconClass(id, args);
+  icon(id: string, args: ReadonlyPartialJSONObject = JSONExt.emptyObject): VirtualElement.IRenderer | undefined
+  /* <DEPRECATED> */ | string /* </DEPRECATED> */
+  {
+    let cmd = this._commands[id];
+    return cmd ? cmd.icon.call(undefined, args) : /* <DEPRECATED> */ '' /* </DEPRECATED> */ /* <FUTURE> undefined </FUTURE> */;
   }
 
   /**
@@ -647,10 +665,24 @@ namespace CommandRegistry {
     mnemonic?: number | CommandFunc<number>;
 
     /**
-     * @deprecated Use `iconClass` instead.
+     * The icon renderer for the command.
+     *
+     * #### Notes
+     * This can be an IRenderer object, or a function which returns the
+     * renderer based on the provided command arguments.
+     *
+     * The default value is undefined.
+     * 
+     * DEPRECATED: if set to a string value, the .icon field will function as
+     * an alias for the .iconClass field, for backwards compatibility
      */
-    icon?: string | CommandFunc<string>;
-
+    icon?: VirtualElement.IRenderer | undefined
+    /* <DEPRECATED> */ | string /* </DEPRECATED> */
+    | CommandFunc<
+      VirtualElement.IRenderer | undefined
+      /* <DEPRECATED> */ | string /* </DEPRECATED> */
+    >;
+
     /**
      * The icon class for the command.
      *
@@ -1165,6 +1197,12 @@ namespace Private {
     readonly execute: CommandFunc<any>;
     readonly label: CommandFunc<string>;
     readonly mnemonic: CommandFunc<number>;
+
+    readonly icon: CommandFunc<
+      VirtualElement.IRenderer | undefined
+      /* <DEPRECATED> */ | string /* </DEPRECATED> */
+    >;
+
     readonly iconClass: CommandFunc<string>;
     readonly iconLabel: CommandFunc<string>;
     readonly caption: CommandFunc<string>;
@@ -1181,11 +1219,30 @@ namespace Private {
    */
   export
   function createCommand(options: CommandRegistry.ICommandOptions): ICommand {
+    let icon;
+    let iconClass;
+
+    /* <DEPRECATED> */
+    if (!(options.icon) || typeof options.icon === 'string') {
+      // alias icon to iconClass
+      iconClass = asFunc(options.iconClass || options.icon, emptyStringFunc);
+      icon = iconClass;
+    } else {
+    /* /<DEPRECATED> */
+
+    iconClass = asFunc(options.iconClass, emptyStringFunc);
+    icon = asFunc(options.icon, undefinedFunc);
+
+    /* <DEPRECATED> */
+    }
+    /* </DEPRECATED> */
+
     return {
       execute: options.execute,
       label: asFunc(options.label, emptyStringFunc),
       mnemonic: asFunc(options.mnemonic, negativeOneFunc),
-      iconClass: asFunc(options.iconClass || options.icon, emptyStringFunc),
+      icon,
+      iconClass,
       iconLabel: asFunc(options.iconLabel, emptyStringFunc),
       caption: asFunc(options.caption, emptyStringFunc),
       usage: asFunc(options.usage, emptyStringFunc),
@@ -1325,6 +1382,11 @@ namespace Private {
    */
   const emptyDatasetFunc = () => ({});
 
+  /**
+   * A singleton undefined function
+   */
+  const undefinedFunc = () => undefined;
+
   /**
    * Cast a value or command func to a command func.
    */

diff --git a/packages/commands/tests/src/index.spec.ts b/packages/commands/tests/src/index.spec.ts
@@ -275,13 +275,21 @@ describe('@lumino/commands', () => {
 
     describe('#icon()', () => {
 
+      const iconRenderer = {
+        render: (host: HTMLElement, options?: any) => {
+          const renderNode = document.createElement('div');
+          renderNode.className = 'p-render';
+          host.appendChild(renderNode);
+        }
+      };
+
       it('should get the icon for a specific command', () => {
         let cmd = {
           execute: (args: JSONObject) => { return args; },
-          icon: 'foo'
+          icon: iconRenderer
         };
         registry.addCommand('test', cmd);
-        expect(registry.icon('test')).to.equal('foo');
+        expect(registry.icon('test')).to.equal(iconRenderer);
       });
 
       it('should give the appropriate icon given arguments', () => {
@@ -302,6 +310,25 @@ describe('@lumino/commands', () => {
         expect(registry.icon('test')).to.equal('');
       });
 
+      /* <DEPRECATED> */
+      it('should be able to return a string value', () => {
+        let cmd = {
+          execute: (args: JSONObject) => { return args; },
+          icon: 'foo'
+        };
+        registry.addCommand('test', cmd);
+        expect(registry.icon('test')).to.equal('foo');
+      });
+
+      it('should alias .iconClass() if cmd.icon is unset', () => {
+        let cmd = {
+          execute: (args: JSONObject) => { return args; },
+          iconClass: 'foo'
+        };
+        registry.addCommand('test', cmd);
+        expect(registry.icon('test')).to.equal('foo');
+      });
+      /* </DEPRECATED> */
     });
 
     describe('#caption()', () => {

diff --git a/packages/commands/tsconfig.json b/packages/commands/tsconfig.json
@@ -40,6 +40,9 @@
     },
     {
       "path": "../signaling"
+    },
+    {
+      "path": "../virtualdom"
     }
   ]
 }
diff --git a/packages/virtualdom/package.json b/packages/virtualdom/package.json
@@ -34,13 +34,20 @@
     "clean": "rimraf lib && rimraf *.tsbuildinfo",
     "clean:test": "rimraf tests/build",
     "docs": "typedoc --options tdoptions.json src",
+
     "test": "npm run test:firefox",
-    "test:chrome": "cd tests && karma start --browsers=Chrome",
-    "test:chrome-headless": "cd tests && karma start --browsers=ChromeHeadless",
-    "test:debug": "cd tests && karma start --browsers=Chrome --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
-    "test:debug:chrome-headless": "cd tests && karma start  --browsers=ChromeHeadless --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
-    "test:firefox": "cd tests && karma start --browsers=Firefox",
-    "test:ie": "cd tests && karma start --browsers=IE",
+    "test:chrome": "npm run test:nobrowser -- --browsers=Chrome",
+    "test:chrome-headless": "npm run test:nobrowser -- --browsers=ChromeHeadless",
+    "test:ie": "npm run test:nobrowser -- --browsers=IE",
+    "test:firefox": "npm run test:nobrowser -- --browsers=Firefox",
+    "test:nobrowser": "cd tests && karma start",
+
+    "test:debug": "npm run test:debug:firefox",
+    "test:debug:chrome-headless": "npm run test:debug:nobrowser -- --browsers=ChromeHeadless",
+    "test:debug:chrome": "npm run test:debug:nobrowser -- --browsers=Chrome",
+    "test:debug:firefox": "npm run test:debug:nobrowser -- --browsers=Firefox",
+    "test:debug:nobrowser": "cd tests && karma start --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
+
     "watch": "tsc --build --watch"
   },
   "dependencies": {

diff --git a/packages/virtualdom/src/index.ts b/packages/virtualdom/src/index.ts
@@ -801,11 +801,15 @@ namespace VirtualElement {
      *
      * For example, if render calls `ReactDOM.render(..., host)`, then
      * there has to also be a corresponding implementation of unrender that
-     * calls `ReactDOM.unmountComponentAtNode(host)`.
+     * calls `ReactDOM.unmountComponentAtNode(host)` in order to prevent
+     * a memory leak.
      *
      * @param host - the DOM element to be removed.
+     *
+     * @param options - Will be populated with the .attrs and .children fields
+     * set on the VirtualElement being unrendered.
      */
-    unrender?: (host: HTMLElement) => void;
+    unrender?: (host: HTMLElement, options?: {attrs?: ElementAttrs, children?: ReadonlyArray<VirtualNode>}) => void;
   };
 }
 
@@ -1312,7 +1316,7 @@ namespace Private {
 
       // Update the element content.
       if (newVNode.renderer) {
-        newVNode.renderer.render(currElem as HTMLElement);
+        newVNode.renderer.render(currElem as HTMLElement, {attrs: newVNode.attrs, children: newVNode.children});
       } else {
         updateContent(currElem as HTMLElement, oldVNode.children, newVNode.children);
       }
@@ -1341,7 +1345,7 @@ namespace Private {
 
       // recursively clean up host children
       if (oldNode.type === 'text') {} else if (oldNode.renderer && oldNode.renderer.unrender) {
-        oldNode.renderer.unrender(child!);
+        oldNode.renderer.unrender(child!, {attrs: oldNode.attrs, children: oldNode.children});
       } else {
         removeContent(child!, oldNode.children, 0, false);
       }

diff --git a/packages/widgets/package.json b/packages/widgets/package.json
@@ -36,15 +36,20 @@
     "clean": "rimraf lib && rimraf *.tsbuildinfo",
     "clean:test": "rimraf tests/build",
     "docs": "typedoc --options tdoptions.json src",
+
     "test": "npm run test:firefox",
-    "test:chrome": "cd tests && karma start --browsers=Chrome",
-    "test:chrome-headless": "cd tests && karma start --browsers=ChromeHeadless",
+    "test:chrome": "npm run test:nobrowser -- --browsers=Chrome",
+    "test:chrome-headless": "npm run test:nobrowser -- --browsers=ChromeHeadless",
+    "test:ie": "npm run test:nobrowser -- --browsers=IE",
+    "test:firefox": "npm run test:nobrowser -- --browsers=Firefox",
+    "test:nobrowser": "cd tests && karma start",
+
     "test:debug": "npm run test:debug:firefox",
-    "test:debug:chrome": "cd tests && karma start --browsers=Chrome --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
-    "test:debug:chrome-headless": "cd tests && karma start  --browsers=ChromeHeadless --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
-    "test:debug:firefox": "cd tests && karma start --browsers=Firefox --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
-    "test:firefox": "cd tests && karma start --browsers=Firefox",
-    "test:ie": "cd tests && karma start --browsers=IE",
+    "test:debug:chrome-headless": "npm run test:debug:nobrowser -- --browsers=ChromeHeadless",
+    "test:debug:chrome": "npm run test:debug:nobrowser -- --browsers=Chrome",
+    "test:debug:firefox": "npm run test:debug:nobrowser -- --browsers=Firefox",
+    "test:debug:nobrowser": "cd tests && karma start --singleRun=false --debug=true --browserNoActivityTimeout=10000000 --browserDisconnectTimeout=10000000",
+
     "watch": "tsc --build --watch"
   },
   "dependencies": {

diff --git a/packages/widgets/src/commandpalette.ts b/packages/widgets/src/commandpalette.ts
@@ -587,6 +587,12 @@ namespace CommandPalette {
      */
     readonly caption: string;
 
+    /**
+     * The icon renderer for the command item.
+     */
+    readonly icon: VirtualElement.IRenderer | undefined
+    /* <DEPRECATED> */ | string /* </DEPRECATED> */;
+
     /**
      * The icon class for the command item.
      */
@@ -779,7 +785,15 @@ namespace CommandPalette {
      */
     renderItemIcon(data: IItemRenderData): VirtualElement {
       let className = this.createIconClass(data);
-      return h.div({ className }, data.item.iconLabel);
+
+      /* <DEPRECATED> */
+      if (typeof data.item.icon === 'string') {
+        return h.div({className}, data.item.iconLabel);
+      }
+      /* </DEPRECATED> */
+
+      // if data.item.icon is undefined, it will be ignored
+      return h.div({className}, data.item.icon!, data.item.iconLabel);
     }
 
     /**
@@ -1446,6 +1460,15 @@ namespace Private {
       return this._commands.label(this.command, this.args);
     }
 
+    /**
+     * The icon renderer for the command item.
+     */
+    get icon(): VirtualElement.IRenderer | undefined
+    /* <DEPRECATED> */ | string /* </DEPRECATED> */
+    {
+      return this._commands.icon(this.command, this.args);
+    }
+
     /**
      * The icon class for the command item.
      */