-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Issue #11261: keep the search bar open #11996
Changes from 14 commits
8c668bc
a120c12
53f0427
c2cdc65
ea69c5a
a57debb
e44b07e
20ad291
4637646
283ceb8
923240b
9651160
2b57f0b
949bd10
ab1a454
8981389
1fa7c2e
277d241
a9c8296
dee88bf
33fbd59
76c822d
d85e888
26ba957
dc8890f
4df9aab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -398,6 +398,7 @@ define({ | |
"CMD_WORKINGSET_SORT_BY_TYPE" : "Sort by Type", | ||
"CMD_WORKING_SORT_TOGGLE_AUTO" : "Automatic Sort", | ||
"CMD_THEMES" : "Themes\u2026", | ||
"CMD_TOGGLE_SEARCH_AUTOHIDE" : "Automatically close search", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should most likely be consistent with #L763 ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opted to change the description, since the action appears as a 'close' to the user (the search has an 'x' on it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
// Navigate menu commands | ||
"NAVIGATE_MENU" : "Navigate", | ||
|
@@ -759,6 +760,7 @@ define({ | |
"DESCRIPTION_USE_TAB_CHAR" : "true to use tabs instead of spaces", | ||
"DESCRIPTION_UPPERCASE_COLORS" : "true to generate uppercase hex colors in Inline Color Editor", | ||
"DESCRIPTION_WORD_WRAP" : "Wrap lines that exceed the viewport width", | ||
"DESCRIPTION_SEARCH_AUTOHIDE" : "Hide the search bar as soon as the editor is focused", | ||
"DESCRIPTION_DETECTED_EXCLUSIONS" : "A list of files that have been detected to cause Tern to run out of control", | ||
"DESCRIPTION_INFERENCE_TIMEOUT" : "The amount of time after which Tern will time out when trying to understand files", | ||
"DESCRIPTION_SHOW_ERRORS_IN_STATUS_BAR" : "true to show errors in status bar", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -247,11 +247,28 @@ 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); | ||
|
||
this._modalBar = new ModalBar( | ||
Mustache.render(_searchBarTemplate, templateVars), | ||
!!PreferencesManager.get('autoHideSearch') | ||
); // 2nd arg = auto-close on Esc/blur | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update comment and move one line up |
||
|
||
this._modalBar = new ModalBar(Mustache.render(_searchBarTemplate, templateVars), true); // 2nd arg = auto-close on Esc/blur | ||
// Done this way because ModalBar.js seems to react underiably when | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unreliably spelt in an unreliable way. |
||
// modifying it to handle the escape key - the findbar wasn't getting | ||
// closed as it should, instead persisting in the background | ||
function _handleKeydown(e) { | ||
if (e.keyCode === KeyEvent.DOM_VK_ESCAPE) { | ||
e.stopPropagation(); | ||
e.preventDefault(); | ||
self.close(); | ||
} | ||
} | ||
window.document.body.addEventListener("keydown", _handleKeydown, true); | ||
|
||
// When the ModalBar closes, clean ourselves up. | ||
this._modalBar.on("close", function (event) { | ||
window.document.body.removeEventListener("keydown", _handleKeydown, true); | ||
|
||
// Hide error popup, since it hangs down low enough to make the slide-out look awkward | ||
self.showError(null); | ||
self._modalBar = null; | ||
|
@@ -293,8 +310,11 @@ define(function (require, exports, module) { | |
return; | ||
} | ||
currentTime = new Date().getTime(); | ||
if (lastTypedTime && (currentTime - lastTypedTime >= 100) && self.getQueryInfo().query !== lastQueriedText && | ||
|
||
if (lastTypedTime && (currentTime - lastTypedTime >= 100) && | ||
self.getQueryInfo().query !== lastQueriedText && | ||
!FindUtils.isNodeSearchInProgress()) { | ||
|
||
// init Search | ||
if (self._options.multifile) { | ||
if ($(e.target).is("#find-what")) { | ||
|
@@ -334,6 +354,9 @@ define(function (require, exports, module) { | |
self.trigger("doFind", e.shiftKey); | ||
} | ||
} | ||
}) | ||
.on("click", ".close", function () { | ||
self.close(); | ||
}); | ||
|
||
if (!this._options.multifile) { | ||
|
@@ -608,6 +631,10 @@ define(function (require, exports, module) { | |
} | ||
} | ||
|
||
if (editor) { | ||
query = getInitialQueryFromSelection(editor); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems pointless and buggy |
||
} | ||
|
||
return {query: query, replaceText: replaceText}; | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -591,6 +591,8 @@ define(function (require, exports, module) { | |
// Blank or invalid query: just jump back to initial pos | ||
editor._codeMirror.setCursor(state.searchStartPos); | ||
} | ||
|
||
editor.lastParsedQuery = state.parsedQuery; | ||
} | ||
|
||
|
||
|
@@ -612,6 +614,10 @@ define(function (require, exports, module) { | |
// Prepopulate the search field | ||
var initialQuery = FindBar.getInitialQuery(findBar, editor); | ||
|
||
if (initialQuery.query === "" && editor.lastParsedQuery !== "") { | ||
initialQuery.query = editor.lastParsedQuery; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not working well with regular expression. Further manual testing and a new test case are needed |
||
} | ||
|
||
// Close our previous find bar, if any. (The open() of the new findBar will | ||
// take care of closing any other find bar instances.) | ||
if (findBar) { | ||
|
@@ -636,6 +642,7 @@ define(function (require, exports, module) { | |
findNext(editor, searchBackwards); | ||
}) | ||
.on("close.FindReplace", function (e) { | ||
editor.lastParsedQuery = state.parsedQuery; | ||
// Clear highlights but leave search state in place so Find Next/Previous work after closing | ||
clearHighlights(cm, state); | ||
|
||
|
@@ -655,12 +662,12 @@ define(function (require, exports, module) { | |
*/ | ||
function doSearch(editor, searchBackwards) { | ||
var state = getSearchState(editor._codeMirror); | ||
|
||
if (state.parsedQuery) { | ||
findNext(editor, searchBackwards); | ||
return; | ||
} else { | ||
openSearchBar(editor, false); | ||
} | ||
|
||
openSearchBar(editor, false); | ||
} | ||
|
||
|
||
|
@@ -695,6 +702,8 @@ define(function (require, exports, module) { | |
findBar.close(); | ||
} | ||
} | ||
|
||
editor.lastEditorSearchPosStr = ""; // Positions have shifted, so we abort the auto-next on findbar launch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer relevant - remove. |
||
} | ||
|
||
function replace(editor) { | ||
|
@@ -717,10 +726,43 @@ define(function (require, exports, module) { | |
|
||
function _launchFind() { | ||
var editor = EditorManager.getActiveEditor(); | ||
|
||
if (editor) { | ||
// Make note of the original query details | ||
var state = getSearchState(editor._codeMirror), | ||
qry = (findBar && state && state.parsedQuery) || "", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nitpick: this variable could be just |
||
idx = state && state.matchIndex; | ||
|
||
// Create a new instance of the search bar UI | ||
clearSearch(editor._codeMirror); | ||
doSearch(editor, false); | ||
_findNextIfSameSearch(qry, idx, getSearchState(editor._codeMirror)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be better as a method that simply returns a boolean, and then the findNext should be executed within _launchFind. This will make the private function more reusable and make the code easier to follow. |
||
} | ||
} | ||
|
||
function _findNextIfSameSearch(originalQuery, originalIdx, state) { | ||
// Process the new query and prep the original and new for comparing | ||
var newIdx, newQuery; | ||
|
||
if (findBar && state && state.queryInfo && state.searchStartPos) { | ||
newQuery = state.parsedQuery || ""; | ||
newIdx = state.matchIndex; | ||
|
||
if (!state.queryInfo.isCaseSensitive) { | ||
originalQuery = typeof originalQuery === "string" | ||
? originalQuery.toLowerCase() | ||
: originalQuery; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Over complicated if's |
||
|
||
newQuery = typeof newQuery === "string" | ||
? newQuery.toLowerCase() | ||
: newQuery; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
} | ||
} | ||
|
||
// If the original and new query are the same, go to the next | ||
if (originalQuery && newQuery && originalQuery === newQuery && | ||
originalIdx === newIdx) { | ||
_findNext(); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1686,6 +1686,15 @@ a, img { | |
margin-left: 0px; | ||
border-radius: 0; | ||
} | ||
|
||
.close { | ||
position: absolute; | ||
right: 10px; | ||
|
||
// vertically centers close button | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not helpful |
||
top: 50%; | ||
margin-top: -8px; | ||
} | ||
} | ||
|
||
// File exclusion filter (used only in Find in Files search bar, for now) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -264,4 +264,4 @@ define(function (require, exports, module) { | |
}; | ||
|
||
exports.ModalBar = ModalBar; | ||
}); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure there is a new line at the end of the file |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -745,7 +745,7 @@ define(function (require, exports, module) { | |
}); | ||
}); | ||
|
||
it("shouldn't Find Next after search bar reopened", function () { | ||
it("should Find Next after search bar reopened", function () { | ||
runs(function () { | ||
myEditor.setCursorPos(0, 0); | ||
|
||
|
@@ -763,10 +763,10 @@ define(function (require, exports, module) { | |
twCommandManager.execute(Commands.CMD_FIND); | ||
|
||
expectSearchBarOpen(); | ||
expect(myEditor).toHaveCursorPosition(0, 0); | ||
expect(myEditor).toHaveCursorPosition(8, 8, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tests fails for me with |
||
|
||
twCommandManager.execute(Commands.CMD_FIND_NEXT); | ||
expect(myEditor).toHaveCursorPosition(0, 0); | ||
expect(myEditor).toHaveCursorPosition(8, 8, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar error with this one: |
||
}); | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the
// TODO
comment refer to?