- Fix bug: pressing Enter to close Quick Open or Goto Line inserted newline

in editor (new, cmv4-only issue)
- Fix bug: clicking Ctrl+T item without typing any text didn't work
- Fix bug: pressing Enter to finish Goto Line didn't set cursor pos
- Clean up docs, remove some TODOs
- Remove unused keyup listener code (jQuery was silently no-oping when
asked to attach a null event handler)
- Simplify "@" detection in the three core Ctrl+T providers

Author: peterflynn

src/extensions/default/QuickOpenCSS/main.js

@@ -84,10 +84,7 @@ define(function (require, exports, module) {
      * @param {boolean} returns true if this plugin wants to provide results for this query
      */
     function match(query) {
-        // TODO: match any location of @ when QuickOpen._handleItemFocus() is modified to
-        // dynamic open files
-        //if (query.indexOf("@") !== -1) {
-        if (query.indexOf("@") === 0) {
+        if (query[0] === "@") {
             return true;
         }
     }
@@ -97,8 +94,8 @@ define(function (require, exports, module) {
      * in which case the topmost list item is irrelevant)
      * @param {?SearchResult} selectedItem
      */
-    function itemFocus(selectedItem, query) {
-        if (!selectedItem || query.length < 2) {
+    function itemFocus(selectedItem, query, force) {
+        if (!selectedItem || (query.length < 2 && !force)) {
             return;
         }
         var selectorInfo = selectedItem.selectorInfo;
@@ -109,7 +106,7 @@ define(function (require, exports, module) {
     }
 
     function itemSelect(selectedItem, query) {
-        itemFocus(selectedItem, query);
+        itemFocus(selectedItem, query, true);
     }
 
 

src/extensions/default/QuickOpenHTML/main.js

@@ -118,10 +118,7 @@ define(function (require, exports, module) {
      * @param {boolean} returns true if this plug-in wants to provide results for this query
      */
     function match(query) {
-        // TODO: match any location of @ when QuickOpen._handleItemFocus() is modified to
-        // dynamic open files
-        //if (query.indexOf("@") !== -1) {
-        if (query.indexOf("@") === 0) {
+        if (query[0] === "@") {
             return true;
         }
     }
@@ -132,8 +129,8 @@ define(function (require, exports, module) {
      * in which case the topmost list item is irrelevant)
      * @param {?SearchResult} selectedItem
      */
-    function itemFocus(selectedItem, query) {
-        if (!selectedItem || query.length < 2) {
+    function itemFocus(selectedItem, query, force) {
+        if (!selectedItem || (query.length < 2 && !force)) {
             return;
         }
         var fileLocation = selectedItem.fileLocation;
@@ -144,7 +141,7 @@ define(function (require, exports, module) {
     }
 
     function itemSelect(selectedItem, query) {
-        itemFocus(selectedItem, query);
+        itemFocus(selectedItem, query, true);
     }
 
 

src/extensions/default/QuickOpenJavaScript/main.js

@@ -112,10 +112,7 @@ define(function (require, exports, module) {
      */
     function match(query) {
         // only match @ at beginning of query for now
-        // TODO: match any location of @ when QuickOpen._handleItemFocus() is modified to
-        // dynamic open files
-        //if (query.indexOf("@") !== -1) {
-        if (query.indexOf("@") === 0) {
+        if (query[0] === "@") {
             return true;
         }
     }
@@ -125,8 +122,8 @@ define(function (require, exports, module) {
      * in which case the topmost list item is irrelevant)
      * @param {?SearchResult} selectedItem
      */
-    function itemFocus(selectedItem, query) {
-        if (!selectedItem || query.length < 2) {
+    function itemFocus(selectedItem, query, force) {
+        if (!selectedItem || (query.length < 2 && !force)) {
             return;
         }
         var fileLocation = selectedItem.fileLocation;
@@ -137,7 +134,7 @@ define(function (require, exports, module) {
     }
 
     function itemSelect(selectedItem, query) {
-        itemFocus(selectedItem, query);
+        itemFocus(selectedItem, query, true);
     }
 
 

src/search/QuickOpen.js

@@ -292,18 +292,16 @@ define(function (require, exports, module) {
         if (currentPlugin) {
             currentPlugin.itemSelect(selectedItem, query);
         } else {
-            // extract line number, if any
+            // Extract line/col number, if any
             var cursorPos = extractCursorPos(query);
 
             // Navigate to file and line number
             var fullPath = selectedItem && selectedItem.fullPath;
             if (fullPath) {
-                // This case is tricky. We want to switch editors, so we need to deal with
-                // resizing/rescrolling the current editor first. But we don't actually want
-                // to start the animation of the ModalBar until afterward (otherwise it glitches
-                // because it gets starved of cycles during the creation of the new editor). 
-                // So we call `prepareClose()` first, and finish the close later.
-                // FIXME: what about this case??
+                // We need to fix up the current editor's scroll pos before switching to the next one. But if
+                // we run the full close() now, ModalBar's animate-out won't be smooth (gets starved of cycles
+                // during creation of the new editor). So we call prepareClose() to do *only* the scroll pos
+                // fixup, and let the full close() be triggered later when the new editor takes focus.
                 doClose = false;
                 this.modalBar.prepareClose();
                 CommandManager.execute(Commands.CMD_ADD_TO_WORKINGSET_AND_OPEN, {fullPath: fullPath})
@@ -312,9 +310,6 @@ define(function (require, exports, module) {
                             var editor = EditorManager.getCurrentFullEditor();
                             editor.setCursorPos(cursorPos.line, cursorPos.ch, true);
                         }
-                    })
-                    .always(function () {
-                        self.close();
                     });
             } else if (cursorPos) {
                 EditorManager.getCurrentFullEditor().setCursorPos(cursorPos.line, cursorPos.ch, true);
@@ -338,13 +333,13 @@ define(function (require, exports, module) {
     };
 
     /**
-     * Make sure ModalBar doesn't restore the scroll pos in cases where we're doing our own restoring instead.
+     * 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 actually restore scroll pos yet though: wait for _handleCloseBar() when the editor has
-            // been resized back to its original height, matching the state it was in when we saved the pos.
+            // 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)
             return { restoreScrollPos: false };
         }
     };
@@ -378,13 +373,13 @@ define(function (require, exports, module) {
             }
         }
 
-        // Close popup & ensure we avoid any still-pending result promises
+        // Close popup & ensure we ignore any still-pending result promises
         this.searchField.destroy();
 
         // Restore original selection / scroll pos if closed via Escape
         if (reason === ModalBar.CLOSE_ESCAPE) {
             console.log("QO restoring pos");
-            // We reset the scroll position synchronously on the ModalBar "close" event (before the animation
+            // 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();
             if (this._origSelection) {
@@ -617,7 +612,7 @@ define(function (require, exports, module) {
         initialString = initialString || "";
         initialString = prefix + initialString;
 
-        this.searchField.setText(initialString, true);
+        this.searchField.setText(initialString);
 
         // Select just the text after the prefix
         this.$searchField[0].setSelectionRange(prefix.length, initialString.length);
@@ -680,11 +675,10 @@ define(function (require, exports, module) {
         $(this.modalBar).on("close", this._handleCloseBar);
 
         this.$searchField = $("input#quickOpenSearch");
-        this.$searchField.keyup(this._handleKeyUp);
 
         this.searchField = new QuickSearchField(this.$searchField, {
             maxResults: 20,
-            verticalAdjust: this.modalBar.getRoot().outerHeight(),  // FIXME: hide popup until animation done! (setExtraDropdownCSS()?)
+            verticalAdjust: this.modalBar.getRoot().outerHeight(),
             resultProvider: this._filterCallback,
             formatter: this._resultsFormatterCallback,
             onCommit: this._handleItemSelect,

src/search/QuickSearchField.js

@@ -37,9 +37,6 @@
  * And the text field is:
  *      input
  *      input.no-results
- * 
- * BUGS to verify:
- * #1855, #1384
  */
 define(function (require, exports, module) {
     "use strict";
@@ -60,10 +57,11 @@ define(function (require, exports, module) {
      * 
      * @param {!function(*, string):string} options.formatter
      *          Converts one result object to a string of HTML text. Passed the item and the current query.
-     * @param {!function(*, string):void} options.onCommit
+     * @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
-     *          pressed, waits until it is before running this callback. The popup remains open after this event.
+     *          pressed, waits until it is before running this callback. If Enter pressed with no results, passed
+     *          null. The popup remains open after this event.
      * @param {!function(*, string):void} options.onHighlight
      *          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
@@ -132,6 +130,7 @@ define(function (require, exports, module) {
     QuickSearchField.prototype._handleKeyDown = function (event) {
         if (event.keyCode === KeyEvent.DOM_VK_RETURN) {
             if (this._displayedQuery === this.$input.val()) {
+                event.preventDefault();  // prevents keyup from going to someone else after we close
                 this._doCommit();
             } else {
                 // Once the current wait resolves, _render() will run the commit
@@ -163,10 +162,12 @@ define(function (require, exports, module) {
         }
     };
     QuickSearchField.prototype._doCommit = function (clickedIndex) {
+        var item;
         if (this._displayedResults && this._displayedResults.length) {
             var committedIndex = clickedIndex !== undefined ? clickedIndex : (this._highlightIndex || 0);
-            this.options.onCommit(this._displayedResults[committedIndex], this._displayedQuery);
+            item = this._displayedResults[committedIndex];
         }
+        this.options.onCommit(item, this._displayedQuery);
     };
     QuickSearchField.prototype._updateHighlight = function () {
         var $items = this._$dropdown.find("li");
@@ -233,8 +234,6 @@ define(function (require, exports, module) {
         }
         this._$dropdown.html(htmlContent);
     };
-//    QuickSearchField.prototype._setExtraDropdownCSS = function (cssProps) {
-//    };
 
     QuickSearchField.prototype._render = function (results, query) {
         this._displayedQuery = query;
@@ -275,11 +274,10 @@ define(function (require, exports, module) {
 
     /**
      * Programmatically changes the search text and updates the results.
-     * FIXME: is this needed if we listen for "input" events?
      */
     QuickSearchField.prototype.setText = function (value) {
         this.$input.val(value);
-        this.updateResults();
+        this.updateResults();  // programmatic changes don't trigger "input" event
     };
 
     /**