[replace-across-files] Code review fixes

src/document/Document.js

@@ -275,6 +275,17 @@ define(function (require, exports, module) {
         // _handleEditorChange() triggers "change" event
     };
 
+    /**
+     * @private
+     * Triggers the appropriate events when a change occurs: "change" on the Document instance
+     * and "documentChange" on the Document module.
+     * @param {Object} changeList Changelist in CodeMirror format
+     */
+    Document.prototype._notifyDocumentChange = function (changeList) {
+        $(this).triggerHandler("change", [this, changeList]);
+        $(exports).triggerHandler("documentChange", [this, changeList]);
+    };
+
     /**
      * Sets the contents of the document. Treated as reloading the document from disk: the document
      * will be marked clean with a new timestamp, the undo/redo history is cleared, and we re-check
@@ -303,9 +314,7 @@ define(function (require, exports, module) {
                 // TODO: Dumb to split it here just to join it again in the change handler, but this is
                 // the CodeMirror change format. Should we document our change format to allow this to
                 // either be an array of lines or a single string?
-                var fakeChangeList = [{text: text.split(/\r?\n/)}];
-                $(this).triggerHandler("change", [this, fakeChangeList]);
-                $(exports).triggerHandler("documentChange", [this, fakeChangeList]);
+                this._notifyDocumentChange([{text: text.split(/\r?\n/)}]);
             }
         }
         this._updateTimestamp(newTimestamp);
@@ -398,6 +407,9 @@ define(function (require, exports, module) {
      * @private
      */
     Document.prototype._handleEditorChange = function (event, editor, changeList) {
+        // TODO: This needs to be kept in sync with SpecRunnerUtils.createMockActiveDocument(). In the
+        // future, we should fix things so that we either don't need mock documents or that this
+        // is factored so it will just run in both.
         if (!this._refreshInProgress) {
             // Sync isDirty from CodeMirror state
             var wasDirty = this.isDirty;
@@ -410,11 +422,7 @@ define(function (require, exports, module) {
         }
 
         // Notify that Document's text has changed
-        // TODO: This needs to be kept in sync with SpecRunnerUtils.createMockDocument(). In the
-        // future, we should fix things so that we either don't need mock documents or that this
-        // is factored so it will just run in both.
-        $(this).triggerHandler("change", [this, changeList]);
-        $(exports).triggerHandler("documentChange", [this, changeList]);
+        this._notifyDocumentChange(changeList);
     };
 
     /**

src/document/DocumentCommandHandlers.js

@@ -1174,7 +1174,7 @@ define(function (require, exports, module) {
 
         } else {
             // Multiple unsaved files: show a single bulk prompt listing all files
-            var message = Strings.SAVE_CLOSE_MULTI_MESSAGE + StringUtils.makeDialogFileList(_.map(unsavedDocs, _shortTitleForDocument));
+            var message = Strings.SAVE_CLOSE_MULTI_MESSAGE + FileUtils.makeDialogFileList(_.map(unsavedDocs, _shortTitleForDocument));
 
             Dialogs.showModalDialog(
                 DefaultDialogs.DIALOG_ID_SAVE_CLOSE,
@@ -1232,9 +1232,12 @@ define(function (require, exports, module) {
     /**
      * Closes all open documents; equivalent to calling handleFileClose() for each document, except
      * that unsaved changes are confirmed once, in bulk.
-     * @param {?{promptOnly: boolean}}  If true, only displays the relevant confirmation UI and does NOT
+     * @param {?{promptOnly: boolean, _forceClose: boolean}}
+     *          If promptOnly is true, only displays the relevant confirmation UI and does NOT
      *          actually close any documents. This is useful when chaining close-all together with
      *          other user prompts that may be cancelable.
+     *          If _forceClose is true, forces the files to close with no confirmation even if dirty. 
+     *          Should only be used for unit test cleanup.
      * @return {$.Promise} a promise that is resolved when all files are closed
      */
     function handleFileCloseAll(commandData) {

src/file/FileUtils.js

@@ -183,6 +183,21 @@ define(function (require, exports, module) {
         );
     }
 
+    /**
+     * Creates an HTML string for a list of files to be reported on, suitable for use in a dialog.
+     * @param {Array.<string>} Array of filenames or paths to display.
+     */
+    function makeDialogFileList(paths) {
+        var result = "<ul class='dialog-list'>";
+        paths.forEach(function (path) {
+            result += "<li><span class='dialog-filename'>";
+            result += StringUtils.breakableUrl(path);
+            result += "</span></li>";
+        });
+        result += "</ul>";
+        return result;
+    }
+
     /**
      * Convert a URI path to a native path.
      * On both platforms, this unescapes the URI
@@ -446,11 +461,12 @@ define(function (require, exports, module) {
     }
 
     /**
-     * Compares two paths. Useful for sorting.
-     * @param {string} filename1
-     * @param {string} filename2
-     * @param {boolean} extFirst If true it compares the extensions first and then the file names.
-     * @return {number} The result of the local compare function
+     * Compares two paths segment-by-segment, used for sorting. Sorts folders before files,
+     * and sorts files based on `compareFilenames()`.
+     * @param {string} path1
+     * @param {string} path2
+     * @return {number} -1, 0, or 1 depending on whether path1 is less than, equal to, or greater than
+     *     path2 according to this ordering.
      */
     function comparePaths(path1, path2) {
         var entryName1, entryName2,
@@ -486,6 +502,7 @@ define(function (require, exports, module) {
     exports.translateLineEndings           = translateLineEndings;
     exports.showFileOpenError              = showFileOpenError;
     exports.getFileErrorString             = getFileErrorString;
+    exports.makeDialogFileList             = makeDialogFileList;
     exports.readAsText                     = readAsText;
     exports.writeText                      = writeText;
     exports.convertToNativePath            = convertToNativePath;

src/htmlContent/findreplace-bar.html

@@ -3,29 +3,30 @@
         --><div class="search-input-container"><input type="text" id="find-what" placeholder="{{queryPlaceholder}}" value="{{initialQuery}}" /><div class="error"></div><span id="find-counter"></span></div><!--
         --><button id="find-case-sensitive" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_CASESENSITIVE_HINT}}"><div class="button-icon"></div></button><!--
         --><button id="find-regexp" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_REGEXP_HINT}}"><div class="button-icon"></div></button><!--
-        {{#navigator}}
+        {{^multifile}}
         --><div class="navigator"><!--
             --><button id="find-prev" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_PREV_HINT}}">{{Strings.BUTTON_PREV}}</button><!--
             --><button id="find-next" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_NEXT_HINT}}">{{Strings.BUTTON_NEXT}}</button><!--
         --></div><!--
-        {{/navigator}}
+        {{/multifile}}
     --></div>
 
     {{#replace}}
     <div id="replace-group" {{#scope}}class="has-scope"{{/scope}}><!--
         --><input type="text" id="replace-with" placeholder="{{Strings.REPLACE_PLACEHOLDER}}"/><!--
-        {{^replaceAllOnly}}
+        {{^multifile}}
         --><button id="replace-yes" class="btn no-focus" tabindex="-1">{{Strings.BUTTON_REPLACE}}</button><!--
-        {{/replaceAllOnly}}
-        --><button id="replace-all" class="btn no-focus {{#replaceAllOnly}}solo{{/replaceAllOnly}}" tabindex="-1">{{replaceAllLabel}}</button></div>
+        {{/multifile}}
+        --><button id="replace-all" class="btn no-focus {{#multifile}}solo{{/multifile}}" tabindex="-1">{{replaceAllLabel}}</button></div>
     {{/replace}}
 </div>
 
-{{#scope}}
+{{#multifile}}
 <div class="scope-group">
     <div class="no-results-message">{{Strings.FIND_NO_RESULTS}}</div>
     <div class="message">
         <span id="searchlabel">{{{scopeLabel}}}</span>
     </div>
+    <!-- For Find in Files, the filter picker will be added here programmatically. -->
 </div>
-{{/scope}}
+{{/multifile}}

src/htmlContent/search-results.html

@@ -1,14 +1,14 @@
 <table class="bottom-panel-table table table-striped table-condensed row-highlight">
     <tbody>
         {{#searchList}}
-        <tr class="file-section" data-file="{{file}}">
+        <tr class="file-section" data-file-index="{{fileIndex}}">
             <td colspan="{{#replace}}3{{/replace}}{{^replace}}2{{/replace}}">
                 <span class="disclosure-triangle expanded" title="{{Strings.FIND_IN_FILES_EXPAND_COLLAPSE}}"></span>
                 {{{filename}}}
             </td>
         </tr>
         {{#items}}
-        <tr data-file="{{file}}" data-item="{{item}}" data-index="{{index}}">
+        <tr data-file-index="{{fileIndex}}" data-item-index="{{itemIndex}}" data-match-index="{{matchIndex}}">
             {{#replace}}<td class="checkbox-column"><input type="checkbox" class="check-one" {{#isChecked}}checked="true"{{/isChecked}} /></td>{{/replace}}
             <td class="line-number">{{line}}</td>
             <td class="line-text">{{pre}}<span class="highlight">{{highlight}}</span>{{post}}</td>

src/htmlContent/search-summary.html

@@ -13,7 +13,15 @@
 <div class="contracting-col">{{{scope}}}</div>
 {{/scope}}
 <div class="fixed-col">{{{summary}}}</div>
-{{>paging}}
+{{#hasPages}}
+<div class="pagination-col">
+    <span class="first-page {{^hasPrev}}disabled{{/hasPrev}}"></span>
+    <span class="prev-page {{^hasPrev}}disabled{{/hasPrev}}"></span>
+    {{{results}}}
+    <span class="next-page {{^hasNext}}disabled{{/hasNext}}"></span>
+    <span class="last-page {{^hasNext}}disabled{{/hasNext}}"></span>
+</div>
+{{/hasPages}}
 {{#replace}}
 <div class="replace-col">
     <button class="btn small replace-checked">{{Strings.BUTTON_REPLACE}}</button>

src/search/FindBar.js

@@ -24,6 +24,9 @@
 /*jslint vars: true, plusplus: true, devel: true, nomen: true, regexp: true, indent: 4, maxerr: 50 */
 /*global define, $, window, Mustache */
 
+/*
+ * UI for the Find/Replace and Find in Files modal bar.
+ */
 define(function (require, exports, module) {
     "use strict";
 
@@ -51,30 +54,26 @@ define(function (require, exports, module) {
      *
      * Dispatches these events:
      *
-     * queryChange - when the user types in the input field or sets a query option. Use getQuery()
+     * - queryChange - when the user types in the input field or sets a query option. Use getQueryInfo()
      *      to get the current query state.
-     * doFind - when the user hits enter/shift-enter in an input field or clicks the Find Previous or Find Next button.
+     * - doFind - when the user chooses to do a Find Previous or Find Next.
      *      Parameters are:
-     *          shiftKey - boolean, false for Find Next, true for Find Previous.
-     *          replace - boolean, true if they hit enter in the Replace field
-     * doReplace - when the user clicks on the Replace or Replace All button. Parameter is a boolean,
-     *      false for single Replace, true for Replace All. Use getReplaceText() to get the current
-     *      replacement text.
-     * close - when the find bar is closed
+     *          shiftKey - boolean, false for Find Next, true for Find Previous
+     * - doReplace - when the user chooses to do a single replace. Use getReplaceText() to get the current replacement text.
+     * - doReplaceAll - when the user chooses to initiate a Replace All. Use getReplaceText() to get the current replacement text.
+     *-  close - when the find bar is closed
      *
-     * @param {{navigator: boolean, replace: boolean, queryPlaceholder: string, initialQuery: string}} options
-     *      Options for the Find bar. 
-     *      navigator - true to show the Find Previous/Find Next buttons - default false
-     *      replace - true to show the Replace controls - default false
-     *      replaceAllOnly - true to show only a Replace All button (no Replace button) - default false
-     *      scope - true to show the scope filter controls - default false
-     *      queryPlaceholder - label to show in the Find field - default empty string
-     *      initialQuery - query to populate in the Find field on open - default empty string
-     *      scopeLabel - label to show for the scope of the search - default empty string
+     * @param {boolean=} options.multifile - true if this is a Find/Replace in Files (changes the behavior of Enter in
+     *      the fields, hides the navigator controls, shows the scope/filter controls, and if in replace mode, hides the
+     *      Replace button (so there's only Replace All)
+     * @param {boolean=} options.replace - true to show the Replace controls - default false
+     * @param {string=}  options.queryPlaceholder - label to show in the Find field - default empty string
+     * @param {string=}  options.initialQuery - query to populate in the Find field on open - default empty string
+     * @param {string=}  scopeLabel - HTML label to show for the scope of the search, expected to be already escaped - default empty string
      */
     function FindBar(options) {
         var defaults = {
-            navigator: false,
+            multifile: false,
             replace: false,
             queryPlaceholder: "",
             initialQuery: "",
@@ -97,6 +96,7 @@ define(function (require, exports, module) {
      * @private
      * Register a find bar so we can close it later if another one tries to open.
      * Note that this is a global function, not an instance function.
+     * @param {!FindBar} findBar The find bar to register.
      */
     FindBar._addFindBar = function (findBar) {
         FindBar._bars = FindBar._bars || [];
@@ -125,7 +125,7 @@ define(function (require, exports, module) {
         var bars = FindBar._bars;
         if (bars) {
             bars.forEach(function (bar) {
-                bar.close(true);
+                bar.close(true, false);
             });
             bars = [];
         }
@@ -138,7 +138,7 @@ define(function (require, exports, module) {
     /**
      * @private
      * Options passed into the FindBar.
-     * @type {?{navigator: boolean, replace: boolean, queryPlaceholder: string, initialQuery: string}}
+     * @type {!{multifile: boolean, replace: boolean, queryPlaceholder: string, initialQuery: string, scopeLabel: string}}
      */
     FindBar.prototype._options = null;
 
@@ -184,8 +184,10 @@ define(function (require, exports, module) {
      * Set the state of the toggles in the Find bar to the saved prefs state.
      */
     FindBar.prototype._updateSearchBarFromPrefs = function () {
-        this.$("#find-case-sensitive").toggleClass("active", PreferencesManager.getViewState("caseSensitive"));
-        this.$("#find-regexp").toggleClass("active", PreferencesManager.getViewState("regexp"));
+        // Have to make sure we explicitly cast the second parameter to a boolean, because
+        // toggleClass expects literal true/false.
+        this.$("#find-case-sensitive").toggleClass("active", !!PreferencesManager.getViewState("caseSensitive"));
+        this.$("#find-regexp").toggleClass("active", !!PreferencesManager.getViewState("regexp"));
     };
 
     /**
@@ -227,7 +229,7 @@ define(function (require, exports, module) {
 
         var templateVars = _.clone(this._options);
         templateVars.Strings = Strings;
-        templateVars.replaceAllLabel = (templateVars.replaceAllOnly ? Strings.BUTTON_REPLACE_ALL_IN_FILES : Strings.BUTTON_REPLACE_ALL);
+        templateVars.replaceAllLabel = (templateVars.multifile ? Strings.BUTTON_REPLACE_ALL_IN_FILES : Strings.BUTTON_REPLACE_ALL);
 
         this._modalBar = new ModalBar(Mustache.render(_searchBarTemplate, templateVars), true);  // 2nd arg = auto-close on Esc/blur
 
@@ -258,11 +260,27 @@ define(function (require, exports, module) {
                 if (e.keyCode === KeyEvent.DOM_VK_RETURN) {
                     e.preventDefault();
                     e.stopPropagation();
-                    $(self).triggerHandler("doFind", [e.shiftKey, (e.target.id === "replace-with")]);
+                    if (self._options.multifile) {
+                        if ($(e.target).is("#find-what")) {
+                            if (self._options.replace) {
+                                // Just set focus to the Replace field.
+                                self.focusReplace();
+                            } else {
+                                // Trigger a Find (which really means "Find All" in this context).
+                                $(self).triggerHandler("doFind");
+                            }
+                        } else {
+                            $(self).triggerHandler("doReplaceAll");
+                        }
+                    } else {
+                        // In the single file case, we just want to trigger a Find Next (or Find Previous
+                        // if Shift is held down).
+                        $(self).triggerHandler("doFind", [e.shiftKey]);
+                    }
                 }
             });
 
-        if (this._options.navigator) {
+        if (!this._options.multifile) {
             this._addShortcutToTooltip($("#find-next"), Commands.CMD_FIND_NEXT);
             this._addShortcutToTooltip($("#find-prev"), Commands.CMD_FIND_PREVIOUS);
             $root
@@ -278,10 +296,10 @@ define(function (require, exports, module) {
             this._addShortcutToTooltip($("#replace-yes"), Commands.CMD_REPLACE);
             $root
                 .on("click", "#replace-yes", function (e) {
-                    $(self).triggerHandler("doReplace", false);
+                    $(self).triggerHandler("doReplace");
                 })
                 .on("click", "#replace-all", function (e) {
-                    $(self).triggerHandler("doReplace", true);
+                    $(self).triggerHandler("doReplaceAll");
                 })
                 // One-off hack to make Find/Replace fields a self-contained tab cycle
                 // TODO: remove once https://trello.com/c/lTSJgOS2 implemented
@@ -400,9 +418,7 @@ define(function (require, exports, module) {
      */
     FindBar.prototype.enable = function (enable) {
         var self = this;
-        ["#find-what", "#replace-with", "#find-prev", "#find-next", "#find-case-sensitive", "#find-regexp"].forEach(function (selector) {
-            self.$(selector).prop("disabled", !enable);
-        });
+        this.$("#find-what, #replace-with, #find-prev, #find-next, #find-case-sensitive, #find-regexp").prop("disabled", !enable);
         this._enabled = enable;
     };
 
@@ -413,6 +429,13 @@ define(function (require, exports, module) {
         return this._enabled;
     };
 
+    /**
+     * @return {boolean} true if the Replace button is enabled.
+     */
+    FindBar.prototype.isReplaceEnabled = function () {
+        return this.$("#replace-yes").is(":enabled");
+    };
+
     /**
      * Enable or disable the navigation controls if present. Note that if the Find bar is currently disabled
      * (i.e. isEnabled() returns false), this will have no effect.