Only use first line of primary selection when populating FindInFiles bar #8470

Merged
merged 5 commits into from Aug 8, 2014

Projects

None yet

4 participants

@MarcelGerber
Member

For #8211

@JeffryBooher JeffryBooher self-assigned this Aug 5, 2014
@JeffryBooher
Contributor

I'm getting an RTE:

Uncaught TypeError: Object #<Editor> has no method 'getSelectionText'
@JeffryBooher
Contributor

Did you forget to add changes to editor.js maybe?

@JeffryBooher
Contributor

This should have some unit tests : some unit tests for Editor.getSelectionText() and some integration tests for Find to ensure only 1 line shows in the modal bar.

@MarcelGerber
Member

@JeffryBooher The function was actually called getSelectedText().
I'll add unit tests soon.

@MarcelGerber
Member

@JeffryBooher Unit tests added.

@JeffryBooher
Contributor

The Selection in the current editor isn't updated to reflect the new initial string like it is when searching in a single file. In FindReplace.js we make a call to handleQueryChange(editor, state, true); that we also need to do when searching across files due to the changing of the initial string in FindInFiilesUI.js

@peterflynn peterflynn and 1 other commented on an outdated diff Aug 6, 2014
src/search/FindInFilesUI.js
@@ -136,6 +136,12 @@ define(function (require, exports, module) {
// The modalBar was already up. When creating the new modalBar, copy the
// current query instead of using the passed-in selected text.
initialString = _findBar.getQueryInfo().query;
+ } else if (initialString) {
+ // Eliminate newlines since we don't generally support searching across line boundaries (#2960)
+ var newline = initialString.indexOf("\n");
+ if (newline !== -1) {
+ initialString = initialString.substr(0, newline);
+ }
@peterflynn
peterflynn Aug 6, 2014 Member

Could you hoist this into FindUtils so it's not duplicated between FindInFilesUI & FindReplace? Maybe a utility like FindUtils.getInitialQueryFromSelection()?

@peterflynn
peterflynn Aug 6, 2014 Member

Also, the flow here could probably be cleaned up to something more like this:

var initialString = "";
if (_findBar && !_findBar.isClosed()) {
    initialString = ...;
} else if (currentEditor) {
    initialString = FindUtils.getQueryFromSelection(currentEditor);
}
@MarcelGerber
MarcelGerber Aug 6, 2014 Member

@peterflynn I tend to introduce a more generic function, like StringUtils.getFirstLine(). Is that ok with you?

@peterflynn
peterflynn Aug 6, 2014 Member

I'm not sure there's that much of a wider use for it (do you see other copies of this pattern elsewhere in core yet?). But also, putting something in FindUitls means you can encapsulate the getSelection() call too, and there's only one place to update when we later change to support multi-line selections.

@peterflynn peterflynn commented on the diff Aug 6, 2014
test/spec/FindInFiles-test.js
+ waitsForDone(CommandManager.execute(Commands.FILE_ADD_TO_WORKING_SET, { fullPath: testPath + "/foo.html" }), "open file");
+ });
+ runs(function () {
+ doc = DocumentManager.getOpenDocumentForPath(testPath + "/foo.html");
+ expect(doc).toBeTruthy();
+ DocumentManager.setCurrentDocument(doc);
+ editor = doc._masterEditor;
+ expect(editor).toBeTruthy();
+ editor.setSelection({line: 4, ch: 7}, {line: 4, ch: 10});
+ });
+
+ openSearchBar(null);
+ runs(function () {
+ expect($("#find-what").val()).toBe("Foo");
+ });
+ waitsForDone(CommandManager.execute(Commands.FILE_CLOSE_ALL), "closing all files");
@peterflynn
peterflynn Aug 6, 2014 Member

Is this line necessary? The other tests below don't need it...

@peterflynn
peterflynn Aug 6, 2014 Member

Oh nm, I see they don't initially open any files while these two tests do.

@redmunds
Contributor
redmunds commented Aug 6, 2014

@MarcelGerber I haven't looked at the code, but the current selection should be used (not the first). Note that the current selection is the last selection added chronologically (nothing to do with order in file).
cc @JeffryBooher

@JeffryBooher
Contributor

I think it's fine to make "FindInFiles" work the way that "Find" works -- which is how this is written. @peterflynn suggested that we combine the two implementations into a single utility function (which I agree with) so we can change it there at a later date if it makes sense. Personally I think it's fine that it uses the first line in the selection that's it's the first thing the user will see so it will make sense to the user.

@peterflynn
Member

@redmunds That should be how this works already: it uses Editor.getSelectedText() / getSelection(), which do return the current selection. The "first" in the title of the PR is referring to how the text gets trimmed down within that single current selection -- if the current selection spans multiple lines, we truncate it to just the first line.

@redmunds
Contributor
redmunds commented Aug 6, 2014

@peterflynn That sounds right. I was confused by wording of title.

@JeffryBooher
Contributor

@peterflynn the way I understood @redmunds comment was that if I select starting on line 21 and select upwards through line 15 then whatever was on line 21 would be the line that was chosen. Currently we choose whatever is on line 15 and disregard the order in which the selections were added to the result.

@MarcelGerber MarcelGerber changed the title from Only use first selection line when populating FindInFiles bar to Only use first line of primary selection when populating FindInFiles bar Aug 6, 2014
@MarcelGerber
Member

@JeffryBooher Re this comment: I don't think we should change the selection. In the single-file Find, we update the results on every query change, which is the only reason why the selection changes, but of course that isn't possible with mutliple files.

@peterflynn
Member

@JeffryBooher, re:

the way I understood @redmunds comment was that if I select starting on line 21 and select upwards through line 15 then whatever was on line 21 would be the line that was chosen

I'm pretty sure he was talking about multiple, separate selections (the "multiple cursors" feature). And this code already behaves correctly in that regard.

Re:

The Selection in the current editor isn't updated to reflect the new initial string like it is when searching in a single file.

I agree with @MarcelGerber -- Find in Files is not an incremental search, so we don't update the selection in the current editor when the query is edited. Tweaking that behavior seems outside the scope of this pull request, in any event...

@MarcelGerber
Member

@peterflynn Changes pushed.

@redmunds
Contributor
redmunds commented Aug 6, 2014

@JeffryBooher Yes, I was referring to multiple selections, not a multiline selection. For a multiline selection I would expect it to behave the same for both selection directions (in a LTR language like English, anyway).

@peterflynn
Member

Looks good to me! Thanks @MarcelGerber

@peterflynn peterflynn merged commit 3051977 into adobe:master Aug 8, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@MarcelGerber MarcelGerber deleted the MarcelGerber:find-in-files-selection-newline branch Aug 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment