switch to start/endQuery and endSearch, made search lifetime tweaks, fixed small bugs

packages/documentsearch-extension/src/index.ts

@@ -60,10 +60,20 @@ export interface ISearchProvider {
    *
    * @returns A promise that resolves with a list of all matches
    */
-  startSearch(query: RegExp, searchTarget: any): Promise<ISearchMatch[]>;
+  startQuery(query: RegExp, searchTarget: any): Promise<ISearchMatch[]>;
 
   /**
-   * Resets UI state, removes all matches.
+   * Clears state of a search provider to prepare for startQuery to be called
+   * in order to start a new query or refresh an existing one.
+   *
+   * @returns A promise that resolves when the search provider is ready to
+   * begin a new search.
+   */
+  endQuery(): Promise<void>;
+
+  /**
+   * Resets UI state as it was before the search process began.  Cleans up and
+   * disposes of all internal state.
    *
    * @returns A promise that resolves when all state has been cleaned up.
    */
@@ -84,7 +94,7 @@ export interface ISearchProvider {
   highlightPrevious(): Promise<ISearchMatch | undefined>;
 
   /**
-   * The same list of matches provided by the startSearch promise resoluton
+   * The same list of matches provided by the startQuery promise resoluton
    */
   readonly matches: ISearchMatch[];
 
@@ -96,7 +106,7 @@ export interface ISearchProvider {
   /**
    * The current index of the selected match.
    */
-  readonly currentMatchIndex: number;
+  readonly currentMatchIndex: number | null;
 }
 
 export interface IDisplayState {

packages/documentsearch-extension/src/providers/codemirrorsearchprovider.ts

@@ -51,15 +51,15 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
    *
    * @returns A promise that resolves with a list of all matches
    */
-  async startSearch(query: RegExp, domain: any): Promise<ISearchMatch[]> {
+  async startQuery(query: RegExp, domain: any): Promise<ISearchMatch[]> {
     if (domain instanceof CodeMirrorEditor) {
       this._cm = domain;
     } else if (domain) {
       this._cm = domain.content.editor;
     }
+    await this.endQuery();
 
     this._query = query;
-    this._clearSearch();
 
     CodeMirror.on(this._cm.doc, 'change', this._onDocChanged.bind(this));
     this._refreshOverlay();
@@ -69,8 +69,7 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
     if (matches.length === 0) {
       return [];
     }
-    if (this.shouldLoop) {
-      // TODO: refactor out to the SearchInstance
+    if (!this.isSubProvider) {
       const cursorMatch = this._findNext(false);
       const match = this._matchState[cursorMatch.from.line][
         cursorMatch.from.ch
@@ -80,19 +79,30 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
     return matches;
   }
 
+  /**
+   * Clears state of a search provider to prepare for startQuery to be called
+   * in order to start a new query or refresh an existing one.
+   *
+   * @returns A promise that resolves when the search provider is ready to
+   * begin a new search.
+   */
+  async endQuery(): Promise<void> {
+    this._matchState = {};
+    this._matchIndex = null;
+    this._cm.removeOverlay(this._overlay);
+    CodeMirror.off(this._cm.doc, 'change', this._onDocChanged.bind(this));
+  }
+
   /**
    * Resets UI state, removes all matches.
    *
    * @returns A promise that resolves when all state has been cleaned up.
    */
   async endSearch(): Promise<void> {
-    this._cm.focus();
-    this._matchState = {};
-    this._matchIndex = 0;
-    if (this._cm) {
-      this._clearSearch();
+    if (!this.isSubProvider) {
+      this._cm.focus();
     }
-    CodeMirror.off(this._cm.doc, 'change', this._onDocChanged.bind(this));
+    this.endQuery();
   }
 
   /**
@@ -133,7 +143,7 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
   }
 
   /**
-   * The same list of matches provided by the startSearch promise resoluton
+   * The same list of matches provided by the startQuery promise resoluton
    */
   get matches(): ISearchMatch[] {
     return this._parseMatchesFromState();
@@ -161,13 +171,14 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
    * Set whether or not the CodemirrorSearchProvider will wrap to the beginning
    * or end of the document on invocations of highlightNext or highlightPrevious, respectively
    */
-  shouldLoop = true;
+  isSubProvider = false;
 
   private _onDocChanged(_: any, changeObj: CodeMirror.EditorChange) {
     // If we get newlines added/removed, the line numbers across the
     // match state are all shifted, so here we need to recalculate it
     if (changeObj.text.length > 1 || changeObj.removed.length > 1) {
       this._setInitialMatches(this._query);
+      this._changed.emit(undefined);
     }
   }
 
@@ -181,10 +192,6 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
     });
   }
 
-  private _clearSearch() {
-    this._cm.removeOverlay(this._overlay);
-  }
-
   /**
    * Do a full search on the entire document.
    *
@@ -271,6 +278,12 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
           this._matchState[line][currentPos] = matchObj;
           // move the stream along and return searching style for the token
           stream.pos += matchLength || 1;
+
+          // if the last thing on the line was a match, make sure we still
+          // emit the changed signal so the display can pick up the updates
+          if (stream.eol) {
+            this._changed.emit(undefined);
+          }
           return 'searching';
         } else if (match) {
           // there's a match in the stream, advance the stream to its position
@@ -297,8 +310,9 @@ export class CodeMirrorSearchProvider implements ISearchProvider {
       );
       if (!cursor.find(reverse)) {
         // if we don't want to loop, no more matches found, reset the cursor and exit
-        if (!this.shouldLoop) {
+        if (this.isSubProvider) {
           this._cm.setCursorPosition(position);
+          this._matchIndex = null;
           return null;
         }
 

packages/documentsearch-extension/src/providers/notebooksearchprovider.ts

@@ -26,19 +26,19 @@ export class NotebookSearchProvider implements ISearchProvider {
    *
    * @returns A promise that resolves with a list of all matches
    */
-  async startSearch(
+  async startQuery(
     query: RegExp,
     searchTarget: NotebookPanel
   ): Promise<ISearchMatch[]> {
     this._searchTarget = searchTarget;
     const cells = this._searchTarget.content.widgets;
-    console.log('startSearch: ', query, this._searchTarget);
 
+    this._query = query;
     // Listen for cell model change to redo the search in case of
     // new/pasted/deleted cells
     const cellList = this._searchTarget.model.cells;
     cellList.changed.connect(
-      this._restartSearch.bind(this, query, this._searchTarget),
+      this._restartQuery.bind(this),
       this
     );
 
@@ -49,7 +49,7 @@ export class NotebookSearchProvider implements ISearchProvider {
     for (let cell of cells) {
       const cmEditor = cell.editor as CodeMirrorEditor;
       const cmSearchProvider = new CodeMirrorSearchProvider();
-      cmSearchProvider.shouldLoop = false;
+      cmSearchProvider.isSubProvider = true;
 
       // If a rendered MarkdownCell contains a match, unrender it so that
       // CodeMirror can show the match(es).  If the MarkdownCell is not
@@ -68,7 +68,7 @@ export class NotebookSearchProvider implements ISearchProvider {
         cell.inputHidden = false;
       }
       // chain promises to ensure indexing is sequential
-      const matchesFromCell = await cmSearchProvider.startSearch(
+      const matchesFromCell = await cmSearchProvider.startQuery(
         query,
         cmEditor
       );
@@ -106,6 +106,27 @@ export class NotebookSearchProvider implements ISearchProvider {
     return allMatches;
   }
 
+  /**
+   * Clears state of a search provider to prepare for startQuery to be called
+   * in order to start a new query or refresh an existing one.
+   *
+   * @returns A promise that resolves when the search provider is ready to
+   * begin a new search.
+   */
+  async endQuery(): Promise<void> {
+    this._cmSearchProviders.forEach(({ provider }) => {
+      provider.endQuery();
+      provider.changed.disconnect(this._onCmSearchProviderChanged, this);
+    });
+    Signal.disconnectBetween(this._searchTarget.model.cells, this);
+
+    this._cmSearchProviders = [];
+    this._unRenderedMarkdownCells.forEach((cell: MarkdownCell) => {
+      cell.rendered = true;
+    });
+    this._unRenderedMarkdownCells = [];
+  }
+
   /**
    * Resets UI state, removes all matches.
    *
@@ -119,13 +140,15 @@ export class NotebookSearchProvider implements ISearchProvider {
       provider.endSearch();
       provider.changed.disconnect(this._onCmSearchProviderChanged, this);
     });
-    this._searchTarget.content.activeCellIndex = index;
-    this._searchTarget.content.mode = 'edit';
+
     this._cmSearchProviders = [];
     this._unRenderedMarkdownCells.forEach((cell: MarkdownCell) => {
       cell.rendered = true;
     });
     this._unRenderedMarkdownCells = [];
+
+    this._searchTarget.content.activeCellIndex = index;
+    this._searchTarget.content.mode = 'edit';
     this._searchTarget = null;
     this._currentMatch = null;
   }
@@ -160,7 +183,7 @@ export class NotebookSearchProvider implements ISearchProvider {
   }
 
   /**
-   * The same list of matches provided by the startSearch promise resoluton
+   * The same list of matches provided by the startQuery promise resoluton
    */
   get matches(): ISearchMatch[] {
     return [].concat(...this._getMatchesFromCells());
@@ -194,7 +217,7 @@ export class NotebookSearchProvider implements ISearchProvider {
     const { provider } = this._cmSearchProviders[cellIndex];
 
     // highlightNext/Previous will not be able to search rendered MarkdownCells or
-    // hidden code cells, but that is okay here because in startSearch, we unrendered
+    // hidden code cells, but that is okay here because in startQuery, we unrendered
     // all cells with matches and unhid all cells
     const match = reverse
       ? await provider.highlightPrevious()
@@ -226,9 +249,9 @@ export class NotebookSearchProvider implements ISearchProvider {
     return match;
   }
 
-  private async _restartSearch(query: RegExp) {
-    await this.endSearch();
-    await this.startSearch(query, this._searchTarget);
+  private async _restartQuery() {
+    await this.endQuery();
+    await this.startQuery(this._query, this._searchTarget);
     this._changed.emit(undefined);
   }
 
@@ -251,6 +274,7 @@ export class NotebookSearchProvider implements ISearchProvider {
   }
 
   private _searchTarget: NotebookPanel;
+  private _query: RegExp;
   private _cmSearchProviders: ICellSearchPair[] = [];
   private _currentMatch: ISearchMatch;
   private _unRenderedMarkdownCells: MarkdownCell[] = [];

packages/documentsearch-extension/src/searchinstance.ts

@@ -25,7 +25,7 @@ export class SearchInstance implements IDisposable {
       this._onRegexToggled.bind(this),
       this._highlightNext.bind(this),
       this._highlightPrevious.bind(this),
-      this._startSearch.bind(this),
+      this._startQuery.bind(this),
       this.dispose.bind(this)
     );
 
@@ -79,13 +79,13 @@ export class SearchInstance implements IDisposable {
     this._displayUpdateSignal.emit(this._displayState);
   }
 
-  private async _startSearch(query: RegExp) {
+  private async _startQuery(query: RegExp) {
     // save the last query (or set it to the current query if this is the first)
     if (this._activeProvider && this._displayState.query) {
-      await this._activeProvider.endSearch();
+      await this._activeProvider.endQuery();
     }
     this._displayState.query = query;
-    await this._activeProvider.startSearch(query, this._widget);
+    await this._activeProvider.startQuery(query, this._widget);
     this.updateIndices();
 
     // this signal should get injected when the widget is
@@ -105,7 +105,11 @@ export class SearchInstance implements IDisposable {
     }
     this._isDisposed = true;
 
-    this._activeProvider.endSearch();
+    // If a query hasn't been executed yet, no need to call endSearch
+    if (this._displayState.query) {
+      this._activeProvider.endSearch();
+    }
+
     this._searchWidget.dispose();
     Signal.clearData(this);
   }

packages/documentsearch-extension/src/searchoverlay.tsx

@@ -132,7 +132,7 @@ interface ISearchOverlayProps {
   onRegexToggled: Function;
   onHightlightNext: Function;
   onHighlightPrevious: Function;
-  onStartSearch: Function;
+  onStartQuery: Function;
   onEndSearch: Function;
 }
 
@@ -186,7 +186,7 @@ class SearchOverlay extends React.Component<
       return;
     }
 
-    this.props.onStartSearch(query);
+    this.props.onStartQuery(query);
   }
 
   private onClose() {
@@ -240,7 +240,7 @@ export function createSearchOverlay(
   onRegexToggled: Function,
   onHightlightNext: Function,
   onHighlightPrevious: Function,
-  onStartSearch: Function,
+  onStartQuery: Function,
   onEndSearch: Function
 ): Widget {
   const widget = ReactWidget.create(
@@ -252,7 +252,7 @@ export function createSearchOverlay(
             onRegexToggled={onRegexToggled}
             onHightlightNext={onHightlightNext}
             onHighlightPrevious={onHighlightPrevious}
-            onStartSearch={onStartSearch}
+            onStartQuery={onStartQuery}
             onEndSearch={onEndSearch}
             overlayState={args}
           />