Take care of a few more lifecycle and enabling issues; add commands to menu.

Also, I refactored the code a bit, which ended up moving logic into the actual command execute functions.

Now find next and previous are only enabled when there is an active search open.

packages/documentsearch-extension/src/index.ts

@@ -12,6 +12,8 @@ import {
 
 import { ICommandPalette } from '@jupyterlab/apputils';
 
+import { IMainMenu } from '@jupyterlab/mainmenu';
+
 import { ISignal } from '@phosphor/signaling';
 import { Widget } from '@phosphor/widgets';
 
@@ -162,118 +164,124 @@ const extension: JupyterFrontEndPlugin<void> = {
   id: '@jupyterlab/documentsearch:plugin',
   autoStart: true,
   requires: [ICommandPalette],
-  activate: (app: JupyterFrontEnd, palette: ICommandPalette) => {
+  optional: [IMainMenu],
+  activate: (
+    app: JupyterFrontEnd,
+    palette: ICommandPalette,
+    mainMenu: IMainMenu | null
+  ) => {
     // Create registry, retrieve all default providers
     const registry: SearchProviderRegistry = new SearchProviderRegistry();
-    const activeSearches: Private.ActiveSearchMap = {};
+    const activeSearches: {
+      [key: string]: SearchInstance;
+    } = {};
 
     const startCommand: string = 'documentsearch:start';
     const nextCommand: string = 'documentsearch:highlightNext';
     const prevCommand: string = 'documentsearch:highlightPrevious';
     app.commands.addCommand(startCommand, {
-      label: 'Search the open document',
+      label: 'Find…',
+      isEnabled: () => {
+        const currentWidget = app.shell.currentWidget;
+        if (!currentWidget) {
+          return;
+        }
+        return registry.getProviderForWidget(currentWidget) !== undefined;
+      },
       execute: () => {
-        let currentWidget = app.shell.currentWidget;
+        const currentWidget = app.shell.currentWidget;
         if (!currentWidget) {
           return;
         }
-        Private.onStartCommand(currentWidget, registry, activeSearches);
+        const widgetId = currentWidget.id;
+        let searchInstance = activeSearches[widgetId];
+        if (!searchInstance) {
+          const searchProvider = registry.getProviderForWidget(currentWidget);
+          if (!searchProvider) {
+            return;
+          }
+          searchInstance = new SearchInstance(currentWidget, searchProvider);
+          Widget.attach(searchInstance.searchWidget, currentWidget.node);
+
+          activeSearches[widgetId] = searchInstance;
+          // find next and previous are now enabled
+          app.commands.notifyCommandChanged();
+
+          searchInstance.disposed.connect(() => {
+            delete activeSearches[widgetId];
+            // find next and previous are now not enabled
+            app.commands.notifyCommandChanged();
+          });
+        }
+        searchInstance.focusInput();
       }
     });
 
     app.commands.addCommand(nextCommand, {
-      label: 'Next match in open document',
-      execute: () => {
-        let currentWidget = app.shell.currentWidget;
+      label: 'Find Next',
+      isEnabled: () => {
+        const currentWidget = app.shell.currentWidget;
+        if (!currentWidget) {
+          return;
+        }
+        return !!activeSearches[currentWidget.id];
+      },
+      execute: async () => {
+        const currentWidget = app.shell.currentWidget;
         if (!currentWidget) {
           return;
         }
-        Private.openBoxOrExecute(
-          currentWidget,
-          registry,
-          activeSearches,
-          Private.onNextCommand
-        );
+        const instance = activeSearches[currentWidget.id];
+        if (!instance) {
+          return;
+        }
+
+        await instance.provider.highlightNext();
+        instance.updateIndices();
       }
     });
 
     app.commands.addCommand(prevCommand, {
-      label: 'Previous match in open document',
-      execute: () => {
-        let currentWidget = app.shell.currentWidget;
+      label: 'Find Previous',
+      isEnabled: () => {
+        const currentWidget = app.shell.currentWidget;
         if (!currentWidget) {
           return;
         }
-        Private.openBoxOrExecute(
-          currentWidget,
-          registry,
-          activeSearches,
-          Private.onPrevCommand
-        );
+        return !!activeSearches[currentWidget.id];
+      },
+      execute: async () => {
+        const currentWidget = app.shell.currentWidget;
+        if (!currentWidget) {
+          return;
+        }
+        const instance = activeSearches[currentWidget.id];
+        if (!instance) {
+          return;
+        }
+
+        await instance.provider.highlightPrevious();
+        instance.updateIndices();
       }
     });
 
     // Add the command to the palette.
     palette.addItem({ command: startCommand, category: 'Main Area' });
-  }
-};
-
-namespace Private {
-  export type ActiveSearchMap = {
-    [key: string]: SearchInstance;
-  };
-
-  export function openBoxOrExecute(
-    currentWidget: Widget,
-    registry: SearchProviderRegistry,
-    activeSearches: ActiveSearchMap,
-    command: (instance: SearchInstance) => void
-  ): void {
-    const instance = activeSearches[currentWidget.id];
-    if (instance) {
-      command(instance);
-    } else {
-      onStartCommand(currentWidget, registry, activeSearches);
+    palette.addItem({ command: nextCommand, category: 'Main Area' });
+    palette.addItem({ command: prevCommand, category: 'Main Area' });
+
+    // Add main menu notebook menu.
+    if (mainMenu) {
+      mainMenu.editMenu.addGroup(
+        [
+          { command: startCommand },
+          { command: nextCommand },
+          { command: prevCommand }
+        ],
+        10
+      );
     }
   }
-
-  export function onStartCommand(
-    currentWidget: Widget,
-    registry: SearchProviderRegistry,
-    activeSearches: ActiveSearchMap
-  ): void {
-    const widgetId = currentWidget.id;
-    if (activeSearches[widgetId]) {
-      activeSearches[widgetId].focusInput();
-      return;
-    }
-    const searchProvider = registry.getProviderForWidget(currentWidget);
-    if (!searchProvider) {
-      // TODO: Is there a way to pass the invocation of ctrl+f through to the browser?
-      return;
-    }
-    const searchInstance = new SearchInstance(currentWidget, searchProvider);
-    activeSearches[widgetId] = searchInstance;
-
-    searchInstance.searchWidget.disposed.connect(() => {
-      delete activeSearches[widgetId];
-    });
-    Widget.attach(searchInstance.searchWidget, currentWidget.node);
-    // Focusing after attach even though we're focusing on componentDidMount
-    // because the notebook steals focus when switching to command mode on blur.
-    // This is a bit of a kludge to be addressed later.
-    searchInstance.focusInput();
-  }
-
-  export async function onNextCommand(instance: SearchInstance) {
-    await instance.provider.highlightNext();
-    instance.updateIndices();
-  }
-
-  export async function onPrevCommand(instance: SearchInstance) {
-    await instance.provider.highlightPrevious();
-    instance.updateIndices();
-  }
-}
+};
 
 export default extension;

packages/documentsearch-extension/src/searchinstance.ts

@@ -7,7 +7,7 @@ import { createSearchOverlay } from './searchoverlay';
 import { MainAreaWidget } from '@jupyterlab/apputils';
 
 import { Widget } from '@phosphor/widgets';
-import { Signal } from '@phosphor/signaling';
+import { ISignal, Signal } from '@phosphor/signaling';
 import { IDisposable } from '@phosphor/disposable';
 
 /**
@@ -29,6 +29,13 @@ export class SearchInstance implements IDisposable {
       this.dispose.bind(this)
     );
 
+    this._widget.disposed.connect(() => {
+      this.dispose();
+    });
+    this._searchWidget.disposed.connect(() => {
+      this.dispose();
+    });
+
     // TODO: this does not update if the toolbar changes height.
     if (this._widget instanceof MainAreaWidget) {
       // Offset the position of the search widget to not cover the toolbar.
@@ -111,6 +118,7 @@ export class SearchInstance implements IDisposable {
     }
 
     this._searchWidget.dispose();
+    this._disposed.emit(undefined);
     Signal.clearData(this);
   }
 
@@ -121,6 +129,13 @@ export class SearchInstance implements IDisposable {
     return this._isDisposed;
   }
 
+  /**
+   * A signal emitted when the context is disposed.
+   */
+  get disposed(): ISignal<this, void> {
+    return this._disposed;
+  }
+
   private async _highlightNext() {
     if (!this._displayState.query) {
       return;
@@ -162,4 +177,5 @@ export class SearchInstance implements IDisposable {
   private _activeProvider: ISearchProvider;
   private _searchWidget: Widget;
   private _isDisposed = false;
+  private _disposed = new Signal<this, void>(this);
 }

packages/documentsearch-extension/src/searchoverlay.tsx

@@ -43,10 +43,6 @@ class SearchEntry extends React.Component<ISearchEntryProps> {
     super(props);
   }
 
-  componentDidMount() {
-    this.focusInput();
-  }
-
   /**
    * Focus the input.
    */