Cleanup mainmenu api (#4890)

* Simplify LabMenu API for removing a group.

* Update test.

packages/mainmenu-extension/package.json

@@ -32,6 +32,7 @@
     "@jupyterlab/apputils": "^0.17.0-1",
     "@jupyterlab/mainmenu": "^0.6.0-1",
     "@phosphor/algorithm": "^1.1.2",
+    "@phosphor/disposable": "^1.1.2",
     "@phosphor/widgets": "^1.6.0"
   },
   "devDependencies": {

packages/mainmenu-extension/src/index.ts

@@ -3,6 +3,8 @@
 
 import { each } from '@phosphor/algorithm';
 
+import { IDisposable } from '@phosphor/disposable';
+
 import { Menu, Widget } from '@phosphor/widgets';
 
 import { JupyterLab, JupyterLabPlugin } from '@jupyterlab/application';
@@ -557,7 +559,10 @@ export function createTabsMenu(app: JupyterLab, menu: TabsMenu): void {
     0
   );
 
-  let tabGroup: Menu.IItemOptions[] = [];
+  // A list of the active tabs in the main area.
+  const tabGroup: Menu.IItemOptions[] = [];
+  // A disposable for getting rid of the out-of-date tabs list.
+  let disposable: IDisposable;
 
   // Utility function to create a command to activate
   // a given tab, or get it if it already exists.
@@ -591,8 +596,12 @@ export function createTabsMenu(app: JupyterLab, menu: TabsMenu): void {
     // main area, and add them to the tab group
     // of the menu.
     const populateTabs = () => {
-      menu.removeGroup(tabGroup);
+      // remove the previous tab list
+      if (disposable && !disposable.isDisposed) {
+        disposable.dispose();
+      }
       tabGroup.length = 0;
+
       let isPreviouslyUsedTabAttached = false;
       each(app.shell.widgets('main'), widget => {
         if (widget.id === previousId) {
@@ -600,7 +609,7 @@ export function createTabsMenu(app: JupyterLab, menu: TabsMenu): void {
         }
         tabGroup.push(createMenuItem(widget));
       });
-      menu.addGroup(tabGroup, 1);
+      disposable = menu.addGroup(tabGroup, 1);
       previousId = isPreviouslyUsedTabAttached ? previousId : '';
     };
     populateTabs();

packages/mainmenu/src/labmenu.ts

@@ -5,7 +5,7 @@ import { IInstanceTracker } from '@jupyterlab/apputils';
 
 import { ArrayExt } from '@phosphor/algorithm';
 
-import { IDisposable } from '@phosphor/disposable';
+import { DisposableDelegate, IDisposable } from '@phosphor/disposable';
 
 import { Menu, Widget } from '@phosphor/widgets';
 
@@ -22,7 +22,7 @@ export interface IJupyterLabMenu extends IDisposable {
    * Add a group of menu items specific to a particular
    * plugin.
    */
-  addGroup(items: Menu.IItemOptions[], rank?: number): void;
+  addGroup(items: Menu.IItemOptions[], rank?: number): IDisposable;
 }
 
 /**
@@ -73,8 +73,11 @@ export class JupyterLabMenu implements IJupyterLabMenu {
    *
    * @param rank - the rank in the menu in which to insert the group.
    */
-  addGroup(items: Menu.IItemOptions[], rank?: number): void {
-    const rankGroup = { items, rank: rank === undefined ? 100 : rank };
+  addGroup(items: Menu.IItemOptions[], rank?: number): IDisposable {
+    const rankGroup = {
+      size: items.length,
+      rank: rank === undefined ? 100 : rank
+    };
 
     // Insert the plugin group into the list of groups.
     const groupIndex = ArrayExt.upperBound(
@@ -86,72 +89,38 @@ export class JupyterLabMenu implements IJupyterLabMenu {
     // Determine the index of the menu at which to insert the group.
     let insertIndex = 0;
     for (let i = 0; i < groupIndex; ++i) {
-      if (this._groups[i].items.length > 0) {
-        insertIndex += this._groups[i].items.length;
+      if (this._groups[i].size > 0) {
+        insertIndex += this._groups[i].size;
         // Increase the insert index by two extra in order
         // to include the leading and trailing separators.
         insertIndex += this._includeSeparators ? 2 : 0;
       }
     }
 
+    // Keep an array of the menu items that have been created.
+    const added: Menu.IItem[] = [];
+
     // Insert a separator before the group.
     // Phosphor takes care of superfluous leading,
     // trailing, and duplicate separators.
     if (this._includeSeparators) {
-      this.menu.insertItem(insertIndex++, { type: 'separator' });
+      added.push(this.menu.insertItem(insertIndex++, { type: 'separator' }));
     }
     // Insert the group.
     for (let item of items) {
-      this.menu.insertItem(insertIndex++, item);
+      added.push(this.menu.insertItem(insertIndex++, item));
     }
     // Insert a separator after the group.
     if (this._includeSeparators) {
-      this.menu.insertItem(insertIndex++, { type: 'separator' });
+      added.push(this.menu.insertItem(insertIndex++, { type: 'separator' }));
     }
 
     ArrayExt.insert(this._groups, groupIndex, rankGroup);
-  }
 
-  /**
-   * Remove a group of menu items. These items should have been
-   * previously added.
-   *
-   * @param items - the previously added group to remove.
-   */
-  removeGroup(items: Menu.IItemOptions[]): void {
-    // Get the index within the current groups.
-    const index = ArrayExt.findFirstIndex(
-      this._groups,
-      rankGroup => rankGroup.items === items
-    );
-    if (index === -1) {
-      return;
-    }
-
-    // Determine the index within the menu for removal.
-    let removeIndex = 0;
-    for (let i = 0; i < index; ++i) {
-      if (this._groups[i].items.length > 0) {
-        removeIndex += this._groups[i].items.length;
-        // Increase the insert index by two extra in order
-        // to include the leading and trailing separators.
-        removeIndex += this._includeSeparators ? 2 : 0;
-      }
-    }
-
-    // Do the removal
-    if (this._includeSeparators) {
-      this.menu.removeItemAt(removeIndex); // Leading separator.
-    }
-    for (let i = 0; i < items.length; ++i) {
-      this.menu.removeItemAt(removeIndex);
-    }
-    if (this._includeSeparators) {
-      this.menu.removeItemAt(removeIndex); // Trailing separator.
-    }
-
-    // Insert a separator before the group.
-    this._groups.splice(index);
+    return new DisposableDelegate(() => {
+      added.forEach(i => this.menu.removeItem(i));
+      this._groups.splice(groupIndex, 1);
+    });
   }
 
   /**
@@ -189,9 +158,9 @@ namespace Private {
    */
   export interface IRankGroup {
     /**
-     * A menu grouping.
+     * The number of items in the menu grouping.
      */
-    items: Menu.IItemOptions[];
+    size: number;
 
     /**
      * The sort rank of the group.

tests/test-mainmenu/src/labmenu.spec.ts

@@ -100,15 +100,13 @@ describe('@jupyterlab/mainmenu', () => {
         expect(idx4 < idx1).to.be(true);
         expect(idx1 < idx2).to.be(true);
       });
-    });
 
-    describe('#removeGroup()', () => {
-      it('should remove a group from the menu', () => {
+      it('should return a disposable that can be used to remove the group', () => {
         const group1 = [{ command: 'run1' }, { command: 'run2' }];
         const group2 = [{ command: 'run3' }, { command: 'run4' }];
-        menu.addGroup(group1);
+        const disposable = menu.addGroup(group1);
         menu.addGroup(group2);
-        menu.removeGroup(group1);
+        disposable.dispose();
 
         let idx1 = ArrayExt.findFirstIndex(
           menu.menu.items,