Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

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

Merged
peterflynn merged 5 commits into from
Aug 8, 2014
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0664b33
Only use first selection line when populating FindInFiles bar
Jul 19, 2014
cd2d52e
Fix typo
Aug 5, 2014
8b595dc
Merge remote-tracking branch 'upstream/master' into find-in-files-sel…
Aug 5, 2014
d62e08e
Add unit tests
Aug 5, 2014
ae4881a
Introduce FindUtils.getInitialQueryFromSelection
Aug 6, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/search/FindInFilesUI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Copy link
Copy Markdown
Member

@peterflynn peterflynn Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@peterflynn peterflynn Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Copy Markdown
Contributor Author

@marcelgerber marcelgerber Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@peterflynn peterflynn Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

FindInFiles.clearSearch();
Expand Down
3 changes: 1 addition & 2 deletions src/search/FindReplace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,7 @@ define(function (require, exports, module) {
initialQuery = findBar.getQueryInfo().query;
} else {
// Prepopulate with the current primary selection, if any
var sel = editor.getSelection();
initialQuery = cm.getRange(sel.start, sel.end);
initialQuery = editor.getSelectedText();

// Eliminate newlines since we don't generally support searching across line boundaries (#2960)
var newline = initialQuery.indexOf("\n");
Expand Down
46 changes: 46 additions & 0 deletions test/spec/FindInFiles-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1699,6 +1699,52 @@ define(function (require, exports, module) {
});

describe("Full workflow", function () {
it("should prepopulate the find bar with selected text", function () {
var doc, editor;

openTestProjectCopy(defaultSourcePath);
runs(function () {
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");
Copy link
Copy Markdown
Member

@peterflynn peterflynn Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@peterflynn peterflynn Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

it("should prepopulate the find bar with only first line of selected text", function () {
var doc, editor;

openTestProjectCopy(defaultSourcePath);
runs(function () {
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: 6, ch: 10});
});

openSearchBar(null);
runs(function () {
expect($("#find-what").val()).toBe("Foo</title>");
});
waitsForDone(CommandManager.execute(Commands.FILE_CLOSE_ALL), "closing all files");
});

it("should show results from the search with all checkboxes checked", function () {
showSearchResults("foo", "bar");
runs(function () {
Expand Down
⚡