From 824fe4d9c85bcd970fda74ca4cac07bd265ffde5 Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 19 Dec 2024 12:58:41 +0530 Subject: [PATCH 1/5] fix: find in files and search ux issues --- src/extensions/default/DarkTheme/main.less | 2 +- src/extensions/default/LightTheme/main.less | 2 +- src/search/FindBar.js | 42 +++++++++++++++--- src/search/FindReplace.js | 46 ++++++++++++++++++-- src/styles/brackets_codemirror_override.less | 2 +- 5 files changed, 81 insertions(+), 13 deletions(-) diff --git a/src/extensions/default/DarkTheme/main.less b/src/extensions/default/DarkTheme/main.less index f8c2a8fe38..5f705a5a10 100644 --- a/src/extensions/default/DarkTheme/main.less +++ b/src/extensions/default/DarkTheme/main.less @@ -93,7 +93,7 @@ color: @foreground !important; } -.CodeMirror-matchingtag { +.CodeMirror-matchingtag:not(.CodeMirror-searching) { /* Ensure visibility against gray inline editor background */ background-color: @matching-tags; --border-height: calc((var(--editor-line-height) *1em - 1em) / 2); diff --git a/src/extensions/default/LightTheme/main.less b/src/extensions/default/LightTheme/main.less index 04a970563f..19ce7d7c66 100644 --- a/src/extensions/default/LightTheme/main.less +++ b/src/extensions/default/LightTheme/main.less @@ -24,7 +24,7 @@ @matching-bracket: #cfead6; -.CodeMirror-matchingtag { +.CodeMirror-matchingtag:not(.CodeMirror-searching) { background: @matching-bracket; --border-height: calc((var(--editor-line-height) *1em - 1em) / 2); border-top: var(--border-height) solid @matching-bracket; diff --git a/src/search/FindBar.js b/src/search/FindBar.js index b0502c3013..2c7a362670 100644 --- a/src/search/FindBar.js +++ b/src/search/FindBar.js @@ -53,7 +53,8 @@ define(function (require, exports, module) { let intervalId = 0, lastQueriedText = "", lastTypedText = "", - lastTypedTextWasRegexp = false; + lastTypedTextWasRegexp = false, + lastClosedQuery = null; const MAX_HISTORY_RESULTS = 50; const PREF_MAX_HISTORY = "maxSearchHistory"; @@ -631,6 +632,12 @@ define(function (require, exports, module) { FindBar.prototype.close = function (suppressAnimation) { lastQueriedText = ""; if (this._modalBar) { + lastClosedQuery = { + query: this.$("#find-what").val() || "", + replaceText: this.getReplaceText(), + isCaseSensitive: this.$("#find-case-sensitive").is(".active"), + isRegexp: this.$("#find-regexp").is(".active") + }; this._addElementToSearchHistory($("#find-what").val(), $("#fif-filter-input").val()); // 1st arg = restore scroll pos; 2nd arg = no animation, since getting replaced immediately this._modalBar.close(true, !suppressAnimation); @@ -656,7 +663,9 @@ define(function (require, exports, module) { * @return {{query: string, caseSensitive: boolean, isRegexp: boolean}} */ FindBar.prototype.getQueryInfo = function (usePlatformLineEndings = true) { - let query = this.$("#find-what").val() || ""; + const $findWhat = this.$("#find-what"); + const findTextArea = $findWhat[0]; + let query = $findWhat.val() || ""; const lineEndings = FileUtils.sniffLineEndings(query); if (usePlatformLineEndings && lineEndings === FileUtils.LINE_ENDINGS_LF && brackets.platform === "win") { query = query.replace(/\n/g, "\r\n"); @@ -664,7 +673,8 @@ define(function (require, exports, module) { return { query: query, isCaseSensitive: this.$("#find-case-sensitive").is(".active"), - isRegexp: this.$("#find-regexp").is(".active") + isRegexp: this.$("#find-regexp").is(".active"), + isQueryTextSelected: findTextArea.selectionStart !== findTextArea.selectionEnd }; }; @@ -851,7 +861,7 @@ define(function (require, exports, module) { * @private * @static * @param {?FindBar} currentFindBar - The currently open Find Bar, if any. - * @param {?Editor} activeEditor - The active editor, if any. + * @param {?Editor} editor - The active editor, if any. * @return {{query: string, replaceText: string}} An object containing the query and replacement text * to prepopulate the Find Bar. */ @@ -872,11 +882,31 @@ define(function (require, exports, module) { return !bar.isClosed(); } ); - - if (openedFindBar) { + // this happens when the find in files bar is opened and we are trying to open single file search or + // vice versa. we need to detect the other findbar and determine what is the search term to use + + //debugger + const currentQueryInfo = openedFindBar && openedFindBar.getQueryInfo(); + if(!openedFindBar && selection){ + // when no findbar is open, the selected text always takes precedence in both single and multi file + query = selection; + } else if(openedFindBar && selection && currentQueryInfo && !currentQueryInfo.isRegexp && currentQueryInfo.isQueryTextSelected) { + // we are switching between single<>multi file search without the user editing the search text in between + // while there is an active selection, the selection takes precedence. + query = selection; + replaceText = openedFindBar.getReplaceText(); + } else if (openedFindBar) { + // there is no selection and we are switching between single<>multi file search, copy the + // current query from the open findbar as is query = openedFindBar.getQueryInfo().query; replaceText = openedFindBar.getReplaceText(); + } else if (lastClosedQuery) { + // these is no open find bar currently and there is no selection, but there is a last saved query, so + // load the last query. this happenes on all freash search cases apart from the very first time + query = lastClosedQuery.query; + replaceText = lastClosedQuery.replaceText; } else if (editor) { + // the very first query after app start, nothing to restore. query = (!lastTypedTextWasRegexp && selection) || lastQueriedText || lastTypedText; } } diff --git a/src/search/FindReplace.js b/src/search/FindReplace.js index c10d52fe7c..29ad766cfc 100644 --- a/src/search/FindReplace.js +++ b/src/search/FindReplace.js @@ -400,6 +400,45 @@ define(function (require, exports, module) { } } + function _markCurrentPos(editor) { + var cm = editor._codeMirror; + const pos = editor.getCursorPos(false, "start"); + cm.operation(function () { + var state = getSearchState(cm); + clearCurrentMatchHighlight(cm, state); + + let curIndex = editor.indexFromPos(pos); + curIndex = curIndex >= 1 ? curIndex-- : 0; + let thisMatch = _getNextMatch(editor, false, editor.posFromIndex(curIndex)); + if (thisMatch) { + let previousMatch = _getNextMatch(editor, true, thisMatch.start); + if(previousMatch){ + const start = editor.indexFromPos(previousMatch.start), end = editor.indexFromPos(previousMatch.end); + if(curIndex >= start && curIndex <= end) { + thisMatch = previousMatch; + } + } + // Update match index indicators - only possible if we have resultSet saved (missing if FIND_MAX_FILE_SIZE threshold hit) + if (state.resultSet.length) { + _updateFindBarWithMatchInfo(state, + {from: thisMatch.start, to: thisMatch.end}, false); + // Update current-tickmark indicator - only if highlighting enabled (disabled if FIND_HIGHLIGHT_MAX threshold hit) + if (state.marked.length) { + ScrollTrackMarkers.markCurrent(state.matchIndex); // _updateFindBarWithMatchInfo() has updated this index + } + } + + // Only mark text with "current match" highlight if search bar still open + if (findBar && !findBar.isClosed()) { + // If highlighting disabled, apply both match AND current-match styles for correct appearance + var curentMatchClassName = state.marked.length ? "searching-current-match" : "CodeMirror-searching searching-current-match"; + state.markedCurrent = cm.markText(thisMatch.start, thisMatch.end, + { className: curentMatchClassName, startStyle: "searching-first", endStyle: "searching-last" }); + } + } + }); + } + /** * Selects the next match (or prev match, if searchBackwards==true) starting from either the current position * (if pos unspecified) or the given position (if pos specified explicitly). The starting position @@ -574,13 +613,12 @@ define(function (require, exports, module) { setQueryInfo(state, findBar.getQueryInfo(false)); updateResultSet(editor); - if (state.parsedQuery) { + if(initial){ + _markCurrentPos(editor); + } else if (state.parsedQuery && !initial) { // 3rd arg: prefer to avoid scrolling if result is anywhere within view, since in this case user // is in the middle of typing, not navigating explicitly; viewport jumping would be distracting. findNext(editor, false, true, state.searchStartPos); - } else if (!initial) { - // Blank or invalid query: just jump back to initial pos - editor._codeMirror.setCursor(state.searchStartPos); } editor.lastParsedQuery = state.parsedQuery; diff --git a/src/styles/brackets_codemirror_override.less b/src/styles/brackets_codemirror_override.less index 4c6c84680b..915923ce78 100644 --- a/src/styles/brackets_codemirror_override.less +++ b/src/styles/brackets_codemirror_override.less @@ -271,7 +271,7 @@ div.CodeMirror-cursors { } } - .CodeMirror-matchingtag { + .CodeMirror-matchingtag:not(.CodeMirror-searching) { background: @matching-bracket; } } From 0ae595605c74b6da26f3050fc3661c7bcbe1c279 Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 19 Dec 2024 15:01:43 +0530 Subject: [PATCH 2/5] fix: find replace integ tests --- src/search/FindReplace.js | 3 +++ test/spec/FindReplace-integ-test.js | 32 +++++++++++++++++------------ 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/search/FindReplace.js b/src/search/FindReplace.js index 29ad766cfc..cc79b14b0e 100644 --- a/src/search/FindReplace.js +++ b/src/search/FindReplace.js @@ -619,6 +619,9 @@ define(function (require, exports, module) { // 3rd arg: prefer to avoid scrolling if result is anywhere within view, since in this case user // is in the middle of typing, not navigating explicitly; viewport jumping would be distracting. findNext(editor, false, true, state.searchStartPos); + } else { + // Blank or invalid query: just jump back to initial pos + editor._codeMirror.setCursor(state.searchStartPos); } editor.lastParsedQuery = state.parsedQuery; diff --git a/test/spec/FindReplace-integ-test.js b/test/spec/FindReplace-integ-test.js index 01e5d079f0..9a4bb81808 100644 --- a/test/spec/FindReplace-integ-test.js +++ b/test/spec/FindReplace-integ-test.js @@ -305,18 +305,18 @@ define(function (require, exports, module) { twCommandManager.execute(Commands.CMD_FIND); // The previous search term "b" was pre-filled, so the editor was centered there already - expect(myEditor.centerOnCursor.calls.count()).toEql(1); + expect(myEditor.centerOnCursor.calls.count()).toEql(0); enterSearchText("foo"); expectHighlightedMatches(fooExpectedMatches); expectSelection(fooExpectedMatches[0]); expectMatchIndex(0, 4); - expect(myEditor.centerOnCursor.calls.count()).toEql(2); + expect(myEditor.centerOnCursor.calls.count()).toEql(1); twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(fooExpectedMatches[1]); expectMatchIndex(1, 4); - expect(myEditor.centerOnCursor.calls.count()).toEql(3); + expect(myEditor.centerOnCursor.calls.count()).toEql(2); twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(fooExpectedMatches[2]); expectMatchIndex(2, 4); @@ -329,7 +329,7 @@ define(function (require, exports, module) { twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(fooExpectedMatches[0]); expectMatchIndex(0, 4); - expect(myEditor.centerOnCursor.calls.count()).toEql(6); + expect(myEditor.centerOnCursor.calls.count()).toEql(5); }); it("should find all case-insensitive matches with mixed-case text", async function () { @@ -337,18 +337,18 @@ define(function (require, exports, module) { twCommandManager.execute(Commands.CMD_FIND); // The previous search term "foo" was pre-filled, so the editor was centered there already - expect(myEditor.centerOnCursor.calls.count()).toEql(1); + expect(myEditor.centerOnCursor.calls.count()).toEql(0); enterSearchText("Foo"); expectHighlightedMatches(fooExpectedMatches); expectSelection(fooExpectedMatches[0]); expectMatchIndex(0, 4); - expect(myEditor.centerOnCursor.calls.count()).toEql(2); + expect(myEditor.centerOnCursor.calls.count()).toEql(1); twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(fooExpectedMatches[1]); expectMatchIndex(1, 4); - expect(myEditor.centerOnCursor.calls.count()).toEql(3); + expect(myEditor.centerOnCursor.calls.count()).toEql(2); twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(fooExpectedMatches[2]); expectMatchIndex(2, 4); @@ -361,7 +361,7 @@ define(function (require, exports, module) { twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(fooExpectedMatches[0]); expectMatchIndex(0, 4); - expect(myEditor.centerOnCursor.calls.count()).toEql(6); + expect(myEditor.centerOnCursor.calls.count()).toEql(5); }); it("should find all case-sensitive matches with mixed-case text", async function () { @@ -454,7 +454,7 @@ define(function (require, exports, module) { twCommandManager.execute(Commands.CMD_FIND); // The previous search term "Foo" was pre-filled, so the editor was centered there already - expect(myEditor.centerOnCursor.calls.count()).toEql(1); + expect(myEditor.centerOnCursor.calls.count()).toEql(0); enterSearchText("foo"); pressEscape(); @@ -463,12 +463,12 @@ define(function (require, exports, module) { await waitsForSearchBarClose(); expectSelection({start: {line: LINE_FIRST_REQUIRE, ch: 8}, end: {line: LINE_FIRST_REQUIRE, ch: 11}}); - expect(myEditor.centerOnCursor.calls.count()).toEql(2); + expect(myEditor.centerOnCursor.calls.count()).toEql(1); // Simple linear Find Next twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection({start: {line: LINE_FIRST_REQUIRE, ch: 31}, end: {line: LINE_FIRST_REQUIRE, ch: 34}}); - expect(myEditor.centerOnCursor.calls.count()).toEql(3); + expect(myEditor.centerOnCursor.calls.count()).toEql(2); twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection({start: {line: 6, ch: 17}, end: {line: 6, ch: 20}}); twCommandManager.execute(Commands.CMD_FIND_NEXT); @@ -565,14 +565,16 @@ define(function (require, exports, module) { expectSearchBarOpen(); expect(getSearchField().val()).toEql("Foo"); expectHighlightedMatches(capitalFooSelections); + // not select anything by default at start + twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(capitalFooSelections[0]); expectMatchIndex(0, 3); - expect(myEditor.centerOnCursor.calls.count()).toEql(3); + expect(myEditor.centerOnCursor.calls.count()).toEql(2); twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(capitalFooSelections[1]); expectMatchIndex(1, 3); - }); + }, 100000); it("should open search bar on Find Next with no previous search", async function () { // Make sure we have no previous query @@ -806,6 +808,10 @@ define(function (require, exports, module) { myEditor.setCursorPos(LINE_FIRST_REQUIRE, 0); twCommandManager.execute(Commands.CMD_FIND); + // just executing find will load the search string from history(last search string) and it will not set + // selection so that the user can press escape and carry on from his cursor. the selection is set only + // on interacting with the find workflow + twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection({start: {line: LINE_FIRST_REQUIRE, ch: CH_REQUIRE_START}, end: {line: LINE_FIRST_REQUIRE, ch: CH_REQUIRE_PAREN}}); twCommandManager.execute(Commands.CMD_FIND_NEXT); From b62fb4608341f6c4e264ca66fd63b8b33ed704da Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 19 Dec 2024 16:00:56 +0530 Subject: [PATCH 3/5] fix: find in files integ tests --- test/spec/FindInFiles-integ-test.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test/spec/FindInFiles-integ-test.js b/test/spec/FindInFiles-integ-test.js index 2228139b0b..577db31cd0 100644 --- a/test/spec/FindInFiles-integ-test.js +++ b/test/spec/FindInFiles-integ-test.js @@ -136,6 +136,12 @@ define(function (require, exports, module) { }, "indexing complete", 20000); var $searchField = $("#find-what"); FindInFiles._searchDone = false; + // if the search str is same as previous string, fif will not do any search as its already loaded + $searchField.val("another_str_to_trigger_fif").trigger("input"); + await awaitsFor(function () { + return FindInFiles._searchDone; + }, "Find in Files done"); + FindInFiles._searchDone = false; $searchField.val(searchString).trigger("input"); await awaitsFor(function () { return FindInFiles._searchDone; @@ -549,11 +555,17 @@ define(function (require, exports, module) { expect(Object.keys(FindInFiles.searchModel.results).length).not.toBe(0); await closeSearchBar(); - await openSearchBar(fileEntry); - // Search model shouldn't be cleared from merely reopening search bar + // Search model shouldn't be cleared after search bar closed expect(Object.keys(FindInFiles.searchModel.results).length).not.toBe(0); + await openSearchBar(fileEntry); + // opening searchbar will also trigger a search with last searched string + + await awaitsFor(function () { + return Object.keys(FindInFiles.searchModel.results).length; + }, "waiting for search results to be there"); + await closeSearchBar(); // Search model shouldn't be cleared after search bar closed without running a search From 549dc01d342d8a4489546f92094fd20da33cba99 Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 19 Dec 2024 17:16:19 +0530 Subject: [PATCH 4/5] test: find in files integ tests --- test/spec/FindInFiles-integ-test.js | 32 ++++++++++++++++++++++++++++- test/spec/FindReplace-integ-test.js | 31 ++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/test/spec/FindInFiles-integ-test.js b/test/spec/FindInFiles-integ-test.js index 577db31cd0..7e7547dfbe 100644 --- a/test/spec/FindInFiles-integ-test.js +++ b/test/spec/FindInFiles-integ-test.js @@ -49,7 +49,8 @@ define(function (require, exports, module) { ProjectManager, SearchResultsView, testWindow, - $; + $, + __PR; beforeAll(async function () { await SpecRunnerUtils.createTempDirectory(); @@ -71,6 +72,7 @@ define(function (require, exports, module) { SearchResultsView = testWindow.brackets.test.SearchResultsView; $ = testWindow.$; PreferencesManager = testWindow.brackets.test.PreferencesManager; + __PR = testWindow.__PR; PreferencesManager.set("findInFiles.nodeSearch", false); PreferencesManager.set("findInFiles.instantSearch", false); PreferencesManager.set("maxSearchHistory", 5); @@ -89,6 +91,8 @@ define(function (require, exports, module) { $ = null; testWindow = null; PreferencesManager = null; + __PR = null; + await SpecRunnerUtils.closeTestWindow(); await SpecRunnerUtils.removeTempDirectory(); }, 30000); @@ -125,6 +129,10 @@ define(function (require, exports, module) { }); } + function getSearchField() { + return $("#find-what"); + } + async function closeSearchBar() { FindInFilesUI._closeFindBar(); await waitForSearchBarClose(); @@ -571,6 +579,28 @@ define(function (require, exports, module) { // Search model shouldn't be cleared after search bar closed without running a search expect(Object.keys(FindInFiles.searchModel.results).length).not.toBe(0); }); + + it("should switch between find replace and find in files with selection work as expected", async function () { + await closeSearchBar(); + const filePath = testPath + "/foo.js"; + await __PR.openFile(filePath); + __PR.setCursors(["7:5-7:13"]); // select text: function + CommandManager.execute(Commands.CMD_FIND); + + expect(getSearchField().val()).toEql("function"); + + CommandManager.execute(Commands.CMD_FIND_IN_FILES); + await awaitsFor(()=>{ + return $("#find-in-files-results .contracting-col").text() === "function"; + }, ()=>{ + return `Find in files search to be function but got${$("#find-in-files-results .contracting-col").text()}`; + }); + + __PR.setCursors(["5:15-5:22"]); // select text: require + CommandManager.execute(Commands.CMD_FIND); + expect(getSearchField().val()).toEql("require"); + await openSearchBar(); + }); }); }); }); diff --git a/test/spec/FindReplace-integ-test.js b/test/spec/FindReplace-integ-test.js index 9a4bb81808..d853ce0004 100644 --- a/test/spec/FindReplace-integ-test.js +++ b/test/spec/FindReplace-integ-test.js @@ -90,7 +90,7 @@ define(function (require, exports, module) { ]; - var testWindow, twCommandManager, twEditorManager, twFindInFiles, tw$; + var testWindow, twCommandManager, twEditorManager, twFindInFiles, tw$, __PR; var myDocument, myEditor; // Helper functions for testing cursor position / selection range @@ -574,7 +574,34 @@ define(function (require, exports, module) { twCommandManager.execute(Commands.CMD_FIND_NEXT); expectSelection(capitalFooSelections[1]); expectMatchIndex(1, 3); - }, 100000); + }); + + it("should highlight the current position match properly", async function () { + myEditor.setCursorPos(0, 0); + + twCommandManager.execute(Commands.CMD_FIND); + + enterSearchText("Foo"); + pressEscape(); + + await waitsForSearchBarClose(); + + // Open search bar a second time + myEditor.setCursorPos(2, 8); // |Foo + twCommandManager.execute(Commands.CMD_FIND); + + expectSearchBarOpen(); + expect(getSearchField().val()).toEql("Foo"); + expectMatchIndex(0, 4); + + myEditor.setCursorPos(2, 9); // F|oo + twCommandManager.execute(Commands.CMD_FIND); + expectMatchIndex(0, 4); + + myEditor.setCursorPos(2, 11); // Foo| + twCommandManager.execute(Commands.CMD_FIND); + expectMatchIndex(0, 4); + }); it("should open search bar on Find Next with no previous search", async function () { // Make sure we have no previous query From dc0607dd70c2987cabb744f5d90c1ea138fb3fdf Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 19 Dec 2024 17:32:13 +0530 Subject: [PATCH 5/5] chore: reduce default search history to 25 only --- src/search/FindBar.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/search/FindBar.js b/src/search/FindBar.js index 2c7a362670..f7fac40009 100644 --- a/src/search/FindBar.js +++ b/src/search/FindBar.js @@ -55,7 +55,7 @@ define(function (require, exports, module) { lastTypedText = "", lastTypedTextWasRegexp = false, lastClosedQuery = null; - const MAX_HISTORY_RESULTS = 50; + const MAX_HISTORY_RESULTS = 25; const PREF_MAX_HISTORY = "maxSearchHistory"; const INSTANT_SEARCH_INTERVAL_MS = 50;