Add "replace all" button when doing find and replace (#12988)

amrelnaggar · ficristo · commit 4be271ed622c · 2016-12-22T19:05:17.000+01:00
* Add replace all button when doing find and replace

* Update the id of batch replace button in tests

* Add test for replace all button

* Addressing CR comments
diff --git a/src/htmlContent/findreplace-bar.html b/src/htmlContent/findreplace-bar.html
@@ -17,7 +17,11 @@
         {{^multifile}}
         --><button id="replace-yes" class="btn no-focus" tabindex="-1">{{Strings.BUTTON_REPLACE}}</button><!--
         {{/multifile}}
-        --><button id="replace-all" class="btn no-focus {{#multifile}}solo{{/multifile}}" tabindex="-1">{{replaceAllLabel}}</button></div>
+        --><button id="replace-batch" class="btn no-focus {{#multifile}}solo{{/multifile}}" tabindex="-1">{{replaceBatchLabel}}</button><!--
+        {{^multifile}}
+        --><button id="replace-all" class="btn no-focus" tabindex="-1">{{replaceAllLabel}}</button><!--
+        {{/multifile}}
+        --></div>
     {{/replace}}
 </div>
 
diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js
@@ -153,7 +153,8 @@ define({
     "FIND_NO_RESULTS"                   : "No results",
     "FIND_QUERY_PLACEHOLDER"            : "Find\u2026",
     "REPLACE_PLACEHOLDER"               : "Replace with\u2026",
-    "BUTTON_REPLACE_ALL"                : "Batch\u2026",
+    "BUTTON_REPLACE_ALL"                : "Replace All",
+    "BUTTON_REPLACE_BATCH"              : "Batch\u2026",
     "BUTTON_REPLACE_ALL_IN_FILES"       : "Replace\u2026",
     "BUTTON_REPLACE"                    : "Replace",
     "BUTTON_NEXT"                       : "\u25B6",
diff --git a/src/search/FindBar.js b/src/search/FindBar.js
@@ -68,7 +68,8 @@ define(function (require, exports, module) {
      *      Parameters are:
      *          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.
+     * - doReplaceBatch - when the user chooses to initiate a Replace All. Use getReplaceText() to get the current replacement text.
+     * - doReplaceAll - when the user chooses to perform a Replace All. Use getReplaceText() to get the current replacement text.
      *-  close - when the find bar is closed
      *
      * @param {boolean=} options.multifile - true if this is a Find/Replace in Files (changes the behavior of Enter in
@@ -244,7 +245,8 @@ define(function (require, exports, module) {
 
         var templateVars = _.clone(this._options);
         templateVars.Strings = Strings;
-        templateVars.replaceAllLabel = (templateVars.multifile ? Strings.BUTTON_REPLACE_ALL_IN_FILES : Strings.BUTTON_REPLACE_ALL);
+        templateVars.replaceBatchLabel = (templateVars.multifile ? Strings.BUTTON_REPLACE_ALL_IN_FILES : Strings.BUTTON_REPLACE_BATCH);
+        templateVars.replaceAllLabel = Strings.BUTTON_REPLACE_ALL;
 
         this._modalBar = new ModalBar(Mustache.render(_searchBarTemplate, templateVars), true);  // 2nd arg = auto-close on Esc/blur
 
@@ -324,7 +326,7 @@ define(function (require, exports, module) {
                             }
                         } else {
                             HealthLogger.searchDone(HealthLogger.SEARCH_REPLACE_ALL);
-                            self.trigger("doReplaceAll");
+                            self.trigger("doReplaceBatch");
                         }
                     } else {
                         // In the single file case, we just want to trigger a Find Next (or Find Previous
@@ -352,6 +354,9 @@ define(function (require, exports, module) {
                 .on("click", "#replace-yes", function (e) {
                     self.trigger("doReplace");
                 })
+                .on("click", "#replace-batch", function (e) {
+                    self.trigger("doReplaceBatch");
+                })
                 .on("click", "#replace-all", function (e) {
                     self.trigger("doReplaceAll");
                 })
@@ -515,7 +520,7 @@ define(function (require, exports, module) {
      */
     FindBar.prototype.enableReplace = function (enable) {
         if (this.isEnabled) {
-            this.$("#replace-yes, #replace-all").prop("disabled", !enable);
+            this.$("#replace-yes, #replace-batch, #replace-all").prop("disabled", !enable);
         }
     };
 
diff --git a/src/search/FindInFilesUI.js b/src/search/FindInFilesUI.js
@@ -61,6 +61,13 @@ define(function (require, exports, module) {
     /** @type {FindBar} Find bar containing the search UI. */
     var _findBar = null;
 
+    /**
+     * @private
+     * Forward declaration for JSLint.
+     * @type {Function}
+     */
+    var _finishReplaceBatch;
+    
     /**
      * Does a search in the given scope with the given filter. Shows the result list once the search is complete.
      * @param {{query: string, caseSensitive: boolean, isRegexp: boolean}} queryInfo Query info object
@@ -106,6 +113,37 @@ define(function (require, exports, module) {
             });
     }
 
+    /**
+     * Does a search in the given scope with the given filter. Replace the result list once the search is complete.
+     * @param {{query: string, caseSensitive: boolean, isRegexp: boolean}} queryInfo Query info object
+     * @param {?Entry} scope Project file/subfolder to search within; else searches whole project.
+     * @param {?string} filter A "compiled" filter as returned by FileFilters.compile(), or null for no filter
+     * @param {?string} replaceText If this is a replacement, the text to replace matches with.
+     * @param {?$.Promise} candidateFilesPromise If specified, a promise that should resolve with the same set of files that
+     *      getCandidateFiles(scope) would return.
+     * @return {$.Promise} A promise that's resolved with the search results or rejected when the find competes.
+     */
+    function searchAndReplaceResults(queryInfo, scope, filter, replaceText, candidateFilesPromise) {
+        return FindInFiles.doSearchInScope(queryInfo, scope, filter, replaceText, candidateFilesPromise)
+            .done(function (zeroFilesToken) {
+                // Done searching all files: replace all
+                if (FindInFiles.searchModel.hasResults()) {
+                    _finishReplaceBatch(FindInFiles.searchModel);
+
+                    if (_findBar) {
+                        _findBar.enable(true);
+                        _findBar.focus();
+                    }
+
+                }
+                StatusBar.hideBusyIndicator();
+            })
+            .fail(function (err) {
+                console.log("replace all failed: ", err);
+                StatusBar.hideBusyIndicator();
+            });
+    }
+
     /**
      * @private
      * Displays a non-modal embedded dialog above the code mirror editor that allows the user to do
@@ -222,7 +260,7 @@ define(function (require, exports, module) {
         if (showReplace) {
             // We shouldn't get a "doReplace" in this case, since the Replace button
             // is hidden when we set options.multifile.
-            _findBar.on("doReplaceAll.FindInFiles", startReplace);
+            _findBar.on("doReplaceBatch.FindInFiles", startReplace);
         }
 
         var oldModalBarHeight = _findBar._modalBar.height();
@@ -261,7 +299,7 @@ define(function (require, exports, module) {
      * Finish a replace across files operation when the user clicks "Replace" on the results panel.
      * @param {SearchModel} model The model for the search associated with ths replace.
      */
-    function _finishReplaceAll(model) {
+    function _finishReplaceBatch(model) {
         var replaceText = model.replaceText;
         if (replaceText === null) {
             return;
@@ -416,8 +454,8 @@ define(function (require, exports, module) {
         var model = FindInFiles.searchModel;
         _resultsView = new SearchResultsView(model, "find-in-files-results", "find-in-files.results");
         _resultsView
-            .on("replaceAll", function () {
-                _finishReplaceAll(model);
+            .on("replaceBatch", function () {
+                _finishReplaceBatch(model);
             })
             .on("close", function () {
                 FindInFiles.clearSearch();
@@ -455,6 +493,7 @@ define(function (require, exports, module) {
 
     // Public exports
     exports.searchAndShowResults = searchAndShowResults;
+    exports.searchAndReplaceResults = searchAndReplaceResults;
 
     // For unit testing
     exports._showFindBar  = _showFindBar;
diff --git a/src/search/FindReplace.js b/src/search/FindReplace.js
@@ -672,7 +672,10 @@ define(function (require, exports, module) {
             state = getSearchState(cm),
             replaceText = findBar.getReplaceText();
 
-        if (all) {
+        if (all === null) {
+            findBar.close();
+            FindInFilesUI.searchAndReplaceResults(state.queryInfo, editor.document.file, null, replaceText);
+        } else if (all) {
             findBar.close();
             // Delegate to Replace in Files.
             FindInFilesUI.searchAndShowResults(state.queryInfo, editor.document.file, null, replaceText);
@@ -702,8 +705,11 @@ define(function (require, exports, module) {
             .on("doReplace.FindReplace", function (e) {
                 doReplace(editor, false);
             })
-            .on("doReplaceAll.FindReplace", function (e) {
+            .on("doReplaceBatch.FindReplace", function (e) {
                 doReplace(editor, true);
+            })
+            .on("doReplaceAll.FindReplace", function (e) {
+                doReplace(editor, null);
             });
     }
 
diff --git a/src/search/SearchResultsView.js b/src/search/SearchResultsView.js
@@ -66,7 +66,7 @@ define(function (require, exports, module) {
      * @constructor
      * Handles the search results panel.
      * Dispatches the following events:
-     *      replaceAll - when the "Replace" button is clicked.
+     *      replaceBatch - when the "Replace" button is clicked.
      *      close - when the panel is closed.
      *
      * @param {SearchModel} model The model that this view is showing.
@@ -328,7 +328,7 @@ define(function (require, exports, module) {
                     e.stopPropagation();
                 })
                 .on("click.searchResults", ".replace-checked", function (e) {
-                    self.trigger("replaceAll");
+                    self.trigger("replaceBatch");
                 });
         }
     };
diff --git a/src/styles/brackets.less b/src/styles/brackets.less
@@ -1670,7 +1670,7 @@ a, img {
     }
 
     // Make button pairs snug
-    #find-prev, #replace-yes, #find-case-sensitive {
+    #find-prev, #replace-yes, #replace-batch, #find-case-sensitive {
         border-right: none;
         margin-right: 0;
     }
@@ -1679,10 +1679,16 @@ a, img {
         border-bottom-left-radius: 0;
         margin-left: 0;
     }
-    #replace-all.solo {
-        border-left: none;
+    #replace-batch {
+        border-radius: 0;
         margin-left: 0;
     }
+    #replace-batch.solo {
+        border-top-right-radius: 3px;
+        border-bottom-right-radius: 3px;
+        border-left: none;
+        border-right: 1px solid #b2b5b5;
+    }
 
     // Make find field snug with options buttons
     //    & replace snug with replace commands
diff --git a/test/spec/FindInFiles-test.js b/test/spec/FindInFiles-test.js
@@ -1716,7 +1716,7 @@ define(function (require, exports, module) {
                         if (fromKeyboard) {
                             SpecRunnerUtils.simulateKeyEvent(KeyEvent.DOM_VK_RETURN, "keydown", $("#replace-with").get(0));
                         } else {
-                            $("#replace-all").click();
+                            $("#replace-batch").click();
                         }
                     });
                 }
@@ -1740,7 +1740,7 @@ define(function (require, exports, module) {
                         openSearchBar(null, true);
                         runs(function () {
                             expect($("#replace-yes").length).toBe(0);
-                            expect($("#replace-all").length).toBe(1);
+                            expect($("#replace-batch").length).toBe(1);
                         });
                     });
 
@@ -1749,7 +1749,7 @@ define(function (require, exports, module) {
                         openSearchBar(null, true);
                         runs(function () {
                             $("#find-what").val("").trigger("input");
-                            expect($("#replace-all").is(":disabled")).toBe(true);
+                            expect($("#replace-batch").is(":disabled")).toBe(true);
                         });
                     });
 
@@ -1758,7 +1758,7 @@ define(function (require, exports, module) {
                         openSearchBar(null, true);
                         runs(function () {
                             $("#find-what").val("my query").trigger("input");
-                            expect($("#replace-all").is(":disabled")).toBe(false);
+                            expect($("#replace-batch").is(":disabled")).toBe(false);
                         });
                     });
 
@@ -1768,7 +1768,7 @@ define(function (require, exports, module) {
                         runs(function () {
                             $("#find-regexp").click();
                             $("#find-what").val("[invalid").trigger("input");
-                            expect($("#replace-all").is(":disabled")).toBe(true);
+                            expect($("#replace-batch").is(":disabled")).toBe(true);
                         });
                     });
 
@@ -1778,7 +1778,7 @@ define(function (require, exports, module) {
                         runs(function () {
                             $("#find-regexp").click();
                             $("#find-what").val("[valid]").trigger("input");
-                            expect($("#replace-all").is(":disabled")).toBe(false);
+                            expect($("#replace-batch").is(":disabled")).toBe(false);
                         });
                     });
 
diff --git a/test/spec/FindReplace-test.js b/test/spec/FindReplace-test.js
