Remove smart-auto-complete from codebase. Add unit tests for QuickSearchField.

Remove logging code and old TODOs. Round out documentation.

Fix bug where pressing enter in Goto Line mode did nothing if a previous
search mode had been used earlier (currentPlugin was stale).

Author: peterflynn

src/brackets.js

@@ -43,7 +43,6 @@ define(function (require, exports, module) {
     require("widgets/bootstrap-modal");
     require("widgets/bootstrap-twipsy-mod");
     require("thirdparty/path-utils/path-utils.min");
-    require("thirdparty/smart-auto-complete-local/jquery.smart_autocomplete");
 
     // Load CodeMirror add-ons--these attach themselves to the CodeMirror module    
     require("thirdparty/CodeMirror2/addon/fold/xml-fold");

src/search/QuickOpen.js

@@ -26,13 +26,8 @@
 /*unittests: QuickOpen*/
 
 /*
- * Displays an auto suggest pop-up list of files to allow the user to quickly navigate to a file and lines
- * within a file.
- * 
- * TODO (issue 333) - currently jquery smart auto complete is used for the pop-up list. While it mostly works
- * it has several issues, so it should be replace with an alternative. Issues:
- * - the pop-up position logic has flaws that require CSS workarounds
- * - the pop-up properties cannot be modified once the object is constructed
+ * Displays a search bar where the user can quickly navigate to a different file by searching file names in
+ * the project, and/or jump to a line number. Providers can plug in to offer additional search modes.
  */
 
 
@@ -335,7 +330,6 @@ define(function (require, exports, module) {
      * Make sure ModalBar doesn't touch the scroll pos in cases where we're doing our own restoring instead.
      */
     QuickNavigateDialog.prototype._handleBeforeClose = function (reason) {
-        console.log("QuickOpen._handleBeforeClose()", this.isOpen, this.closePromise, reason);
         if (reason === ModalBar.CLOSE_ESCAPE) {
             // Don't let ModalBar adjust scroll pos: we're going to revert it to its original pos. (In _handleCloseBar(),
             // when the editor has been resized back to its original height, matching state when we saved the pos)
@@ -344,16 +338,12 @@ define(function (require, exports, module) {
     };
 
     /**
-     * Closes the search dialog and notifies all quick open plugins that
-     * searching is done.
+     * Closes the search bar; if search bar is already closing, returns the Promise that is tracking the
+     * existing close activity.
      * @return {$.Promise} Resolved when the search bar is entirely closed.
-     * 
-     * FIXME: replace this by just saving off the Promise in _handleCloseBar() and checking it elsewhere?
      */
     QuickNavigateDialog.prototype.close = function () {
-        console.log("QuickOpen.close()", this.isOpen, this.closePromise);
         if (!this.isOpen) {
-            console.log("  (already closing)");
             return this.closePromise;
         }
 
@@ -363,7 +353,6 @@ define(function (require, exports, module) {
     };
 
     QuickNavigateDialog.prototype._handleCloseBar = function (event, reason, modalBarClosePromise) {
-        console.log("QuickOpen._handleCloseBar()", this.isOpen, this.closePromise, reason, modalBarClosePromise);
         console.assert(!this.closePromise);
         this.closePromise = modalBarClosePromise;
         this.isOpen = false;
@@ -381,7 +370,6 @@ define(function (require, exports, module) {
 
         // Restore original selection / scroll pos if closed via Escape
         if (reason === ModalBar.CLOSE_ESCAPE) {
-            console.log("QO restoring pos");
             // We can reset the scroll position synchronously on the ModalBar "close" event (before the animation
             // completes) since the editor has already been resized at this point.
             var editor = EditorManager.getCurrentFullEditor();
@@ -448,6 +436,9 @@ define(function (require, exports, module) {
      * @return {Array} The filtered list of results.
      */
     QuickNavigateDialog.prototype._filterCallback = function (query) {
+        // Re-evaluate which plugin is active each time query string changes
+        currentPlugin = null;
+        
         // "Go to line" mode is special-cased
         var cursorPos = extractCursorPos(query);
         if (cursorPos && cursorPos.local) {
@@ -497,7 +488,6 @@ define(function (require, exports, module) {
         this._updateDialogLabel(null, query);
 
         // No matching plugin: use default file search mode
-        currentPlugin = null;
         return searchFileList(query, this._filenameMatcher);
     };
 
@@ -676,7 +666,8 @@ define(function (require, exports, module) {
         });
 
         // Start prefetching the file list, which will be needed the first time the user enters an un-prefixed query. If file index
-        // caches are out of date, this list might take some time to asynchronously build, forcing searchFileList() to wait.
+        // caches are out of date, this list might take some time to asynchronously build, forcing searchFileList() to wait. In the
+        // meantime we show our old, stale fileList (unless the user has switched projects).
         fileListPromise = ProjectManager.getAllFiles(true)
             .done(function (files) {
                 fileList = files;

src/search/QuickSearchField.js

@@ -56,7 +56,8 @@ define(function (require, exports, module) {
      *          the provider yields a null error string, input is not decorated.
      * 
      * @param {!function(*, string):string} options.formatter
-     *          Converts one result object to a string of HTML text. Passed the item and the current query.
+     *          Converts one result object to a string of HTML text. Passed the item and the current query. The
+     *          outermost element must be <li>. The ".highlight" class can be ignored as it is applied automatically.
      * @param {!function(?*, string):void} options.onCommit
      *          Called when an item is selected by clicking or pressing Enter. Passed the item and the current
      *          query. If the current result list is not up to date with the query text at the time Enter is
@@ -66,7 +67,6 @@ define(function (require, exports, module) {
      *          Called when an item is highlighted via the arrow keys. Passed the item and the current query.
      *          Always called once with the top item in the result list, each time the list is updated (because
      *          the top item is always initially highlighted).
-     *          TODO: only if query not blank - fix IN THE HANDLER by no-oping if query.length===1 !!!
      * @param {?number} options.maxResults
      *          Maximum number of items from resultProvider() to display in the popup.
      * @param {?number} options.verticalAdjust
@@ -78,6 +78,8 @@ define(function (require, exports, module) {
         this.$input = $input;
         this.options = options;
 
+        options.maxResults = options.maxResults || 10;
+        
         this._handleInput   = this._handleInput.bind(this);
         this._handleKeyDown = this._handleKeyDown.bind(this);
 
@@ -90,45 +92,49 @@ define(function (require, exports, module) {
     /** @type {!Object} */
     QuickSearchField.prototype.options = null;
 
-    /** @type {?$.Promise} */
+    /** @type {?$.Promise} Promise corresponding to latest resultProvider call. Any earlier promises ignored */
     QuickSearchField.prototype._pending = null;
 
-    /** @type {boolean} */
+    /** @type {boolean} True if Enter already pressed & just waiting for results to arrive before committing */
     QuickSearchField.prototype._commitPending = false;
 
-    /** @type {?string} */
+    /** @type {?string} Value of $input corresponding to the _displayedResults list */
     QuickSearchField.prototype._displayedQuery = null;
 
-    /** @type {?Array.<*>} */
+    /** @type {?Array.<*>}  Latest resultProvider result */
     QuickSearchField.prototype._displayedResults = null;
 
     /** @type {?number} */
     QuickSearchField.prototype._highlightIndex = null;
 
-    /** @type {?jQueryObject} */
+    /** @type {?jQueryObject} Dropdown's <ol>, while open; null while closed */
     QuickSearchField.prototype._$dropdown = null;
 
     /** @type {!jQueryObject} */
-    QuickSearchField.prototype.$input = null;  // TODO: _ prefix?
+    QuickSearchField.prototype.$input = null;
 
-    /**
-     */
+    
+    /** When text field changes, update results list */
     QuickSearchField.prototype._handleInput = function () {
         this._pending = null;  // immediately invalidate any previous Promise
 
         var valueAtEvent = this.$input.val();
         var self = this;
+        // The timeout lets us skip over a backlog of multiple keyboard events when the provider is responding
+        // so slowly that JS execution can't keep up. All the remaining input events are serviced before the
+        // first timeout runs; then all the queued-up timeouts run in a row. All except the last one can no-op.
         setTimeout(function () {
             if (self.$input.val() === valueAtEvent) {
                 self.updateResults();
             }
         }, 0);
     };
 
-    /**
-     */
+    /** Handle special keys: Enter, Up/Down */
     QuickSearchField.prototype._handleKeyDown = function (event) {
         if (event.keyCode === KeyEvent.DOM_VK_RETURN) {
+            // Enter should always act on the latest results. If input has changed and we're still waiting for
+            // new results, just flag the 'commit' for later
             if (this._displayedQuery === this.$input.val()) {
                 event.preventDefault();  // prevents keyup from going to someone else after we close
                 this._doCommit();
@@ -138,7 +144,7 @@ define(function (require, exports, module) {
             }
         } else if (event.keyCode === KeyEvent.DOM_VK_DOWN) {
             // Highlight changes are always done synchronously on the currently shown result list. If the list
-            // later changes, the highlight is reset
+            // later changes, the highlight is reset to the top
             if (this._displayedResults && this._displayedResults.length) {
                 if (this._highlightIndex === null || this._highlightIndex === this._displayedResults.length - 1) {
                     this._highlightIndex = 0;
@@ -161,19 +167,24 @@ define(function (require, exports, module) {
             event.preventDefault(); // treated as End key otherwise
         }
     };
-    QuickSearchField.prototype._doCommit = function (clickedIndex) {
+    
+    /** Call onCommit() immediately */
+    QuickSearchField.prototype._doCommit = function (index) {
         var item;
         if (this._displayedResults && this._displayedResults.length) {
-            var committedIndex = clickedIndex !== undefined ? clickedIndex : (this._highlightIndex || 0);
+            var committedIndex = index !== undefined ? index : (this._highlightIndex || 0);
             item = this._displayedResults[committedIndex];
         }
         this.options.onCommit(item, this._displayedQuery);
     };
+    
+    /** Update display to reflect value of _highlightIndex, & call onHighlight() */
     QuickSearchField.prototype._updateHighlight = function () {
         var $items = this._$dropdown.find("li");
         $items.removeClass("highlight");
         if (this._highlightIndex !== null) {
             $items.eq(this._highlightIndex).addClass("highlight");
+            
             this.options.onHighlight(this._displayedResults[this._highlightIndex], this.$input.val());
         }
     };
@@ -188,6 +199,8 @@ define(function (require, exports, module) {
         var query = this.$input.val();
         var results = this.options.resultProvider(query);
         if (results.done && results.fail) {
+            // Provider returned an async result - mark it as the latest Promise and if it's still latest when
+            // it resolves, render the results then
             this._pending = results;
             var self = this;
             this._pending.done(function (realResults) {
@@ -203,17 +216,24 @@ define(function (require, exports, module) {
                 }
             });
         } else {
+            // Synchronous result - render immediately
             this._render(results, query);
         }
     };
 
-    QuickSearchField.prototype._$dropdown = null;
+    
+    /** Close dropdown result list if visible */
     QuickSearchField.prototype._closeDropdown = function () {
         if (this._$dropdown) {
             this._$dropdown.remove();
             this._$dropdown = null;
         }
     };
+    
+    /**
+     * Open dropdown result list & populate with the given content
+     * @param {!string} htmlContent
+     */
     QuickSearchField.prototype._openDropdown = function (htmlContent) {
         if (!this._$dropdown) {
             var self = this;
@@ -235,18 +255,23 @@ define(function (require, exports, module) {
         this._$dropdown.html(htmlContent);
     };
 
+    /**
+     * Given finished provider result, format it into HTML and show in dropdown, and update "no-results" style.
+     * If an Enter key commit was pending from earlier, process it now.
+     * @param {!Array.<*>} results
+     * @param {!string} query
+     */
     QuickSearchField.prototype._render = function (results, query) {
         this._displayedQuery = query;
         this._displayedResults = results;
         this._highlightIndex = 0;
         // TODO: fixup to match prev value's item if possible?
-        // (Sublime moves arrowed highlight to stay on same item as list is filters, as long as it's
-        // still visible; then jumps back to top when not. Do we need a equals(*, *):boolean to track this?)
 
         if (results.error || results.length === 0) {
             this._closeDropdown();
             this.$input.addClass("no-results");
         } else if (results.hasOwnProperty("error")) {
+            // Error present but falsy - no results to show, but don't decorate with error style
             this._closeDropdown();
             this.$input.removeClass("no-results");
         } else {
@@ -261,33 +286,26 @@ define(function (require, exports, module) {
             this._openDropdown(html);
 
             // Highlight top item and trigger highlight callback
-            // TODO: if we had equals(), could avoid running callback when topmost item is same as before
-            // (though master doesn't do this either)
             this._updateHighlight();
         }
 
+        // If Enter key was pressed earlier, handle it now that we've gotten results back
         if (this._commitPending) {
             this._commitPending = false;
             this._doCommit();
         }
     };
 
+    
     /**
      * Programmatically changes the search text and updates the results.
+     * @param {!string} value
      */
     QuickSearchField.prototype.setText = function (value) {
         this.$input.val(value);
         this.updateResults();  // programmatic changes don't trigger "input" event
     };
 
-    /**
-     * Returns the currently highlighted item. Returns null if there is no result popup open (i.e. no text has
-     * been entered yet; the provider returned zero results; or one of those was previously true and we are
-     * still waiting for the provider to return newer results).
-     */
-    QuickSearchField.prototype.getSelectedItem = function () {
-    };
-    
     /**
      * Closes the dropdown, and discards any pending Promises.
      */