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

expose doSearch for extensions #7445

Closed
wants to merge 1 commit into from
Closed

expose doSearch for extensions #7445

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 18 additions & 3 deletions src/search/FindInFiles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,16 +838,28 @@ define(function (require, exports, module) {

return result.promise();
}


/**
* @public
* Executes the Find in Files search using passed RegExp
* @param {!RegExp} query RegExp to search with
*/
function doSearch(query) {
if (!(query instanceof RegExp)) {
throw new Error("RegExp query is required");
}
return _doSearch(query.toString(), getCandidateFiles(), query);
}
Copy link
Member

Choose a reason for hiding this comment

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

A bunch of the search options are globals in the module, so the behavior in terms of what gets used will be awfuly inconsistent:

  • Case-sensitive & regexp settings will be ignored (since this API always passes in its own regexp)
  • It will use whatever exclusion filter was in place the last time a search was run, since it's initialized in the Enter-key handler
  • It will use whatever subtree scope was in place the last time the search bar was open (even if it was canceled, unlike previous bullet), since that's initialized in _doFindInFiles()

Actually, looking at what else _doFindInFiles() does, it appears that this function would break pretty badly if an existing set of search results was still visible in the bottom panel.

Overall it seems like to expose this in a reliable way we'd need to rework how state is stored in this module to be more compartmentalized. Doable, and a worthwhile cleanup anyway, but it requires much further-reaching code changes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll work on this little later and ping you for review @peterflynn

Do you think it'd be better to pass some kind of state object between functions or is there a cleaner solution when passing around too many variables?

Copy link
Member

Choose a reason for hiding this comment

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

Putting the methods directly on some sort of state object would be another option potentially...

Copy link
Contributor

Choose a reason for hiding this comment

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

A search class might be the best solution. We could export it and you can
even extend it to change what you don't want.

I actually did something similar but just for the results to be able to
share code between the find in files results and the replace all. But then
I figured that an easier solution would be to place a replace input and
button in the find in files results and make the replace all command
execute a find in file. This would also just work for replace in files.


/**
* @private
* Executes the Find in Files search inside the 'currentScope'
* @param {string} query String to be searched
* @param {!$.Promise} candidateFilesPromise Promise from getCandidateFiles(), which was called earlier
*/
function _doSearch(query, candidateFilesPromise) {
function _doSearch(query, candidateFilesPromise, implicitRegExp) {
currentQuery = query;
currentQueryExpr = _getQueryRegExp(query);
currentQueryExpr = implicitRegExp || _getQueryRegExp(query);

if (!currentQueryExpr) {
StatusBar.hideBusyIndicator();
Expand Down Expand Up @@ -1240,6 +1252,9 @@ define(function (require, exports, module) {
CommandManager.register(Strings.CMD_FIND_IN_FILES, Commands.EDIT_FIND_IN_FILES, _doFindInFiles);
CommandManager.register(Strings.CMD_FIND_IN_SUBTREE, Commands.EDIT_FIND_IN_SUBTREE, _doFindInSubtree);

// Public API
exports.doSearch = doSearch;

// For unit testing - updated in _doSearch() when search complete
exports._searchResults = null;
});