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

Find in Files Improvements (Part 4: Sorting) #6585

Merged
merged 3 commits into from
Mar 4, 2014
Merged

Find in Files Improvements (Part 4: Sorting) #6585

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f4ac4b
Sorts the Find in Files results
TomMalbran Jan 19, 2014
a650d05
Changes after first Review
TomMalbran Feb 27, 2014
d597f59
Merge remote-tracking branch 'origin/master' into tom/find-in-files-p4
TomMalbran Feb 28, 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
80 changes: 67 additions & 13 deletions src/search/FindInFiles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ define(function (require, exports, module) {
*/
var searchResults = {};

/** @type {Array.<string>} Keeps a copy of the searched files sorted by name and with the selected file first */
var searchFiles = [];

/** @type {Panel} Bottom panel holding the search results. Initialized in htmlReady() */
var searchResultsPanel;

Expand Down Expand Up @@ -158,6 +161,7 @@ define(function (require, exports, module) {
* @private
* Returns label text to indicate the search scope. Already HTML-escaped.
* @param {?Entry} scope
* @return {string}
*/
function _labelForScope(scope) {
var projName = ProjectManager.getProjectRoot().name;
Expand Down Expand Up @@ -236,7 +240,7 @@ define(function (require, exports, module) {
* @param {string} fullPath
* @param {string} contents
* @param {RegExp} queryExpr
* @return {boolean} True iff matches were added to the search results
* @return {boolean} True if the matches were added to the search results
*/
function _addSearchMatches(fullPath, contents, queryExpr) {
var matches = _getSearchMatches(contents, queryExpr);
Expand All @@ -253,7 +257,51 @@ define(function (require, exports, module) {

/**
* @private
* Count the total number of matches and files
* Sorts the file keys to show the results from the selected file first and the rest sorted by path
*/
function _sortResultFiles() {
var selectedEntry = ProjectManager.getSelectedItem().fullPath;
searchFiles = Object.keys(searchResults);

searchFiles.sort(function (key1, key2) {
if (selectedEntry === key1) {
return -1;
} else if (selectedEntry === key2) {
return 1;
}

var entryName1, entryName2,
pathParts1 = key1.split("/"),
pathParts2 = key2.split("/"),
length = Math.min(pathParts1.length, pathParts2.length),
folders1 = pathParts1.length - 1,
folders2 = pathParts2.length - 1,
index = 0;

while (index < length) {
entryName1 = pathParts1[index];
entryName2 = pathParts2[index];

if (entryName1 !== entryName2) {
if (index < folders1 && index < folders2) {
return entryName1.toLocaleLowerCase().localeCompare(entryName2.toLocaleLowerCase());
} else if (index >= folders1 && index < folders2) {
return 1;
} else if (index < folders1 && index >= folders2) {
return -1;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need that else.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, execution will never enter this branch

Copy link
Contributor

Choose a reason for hiding this comment

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

you can probably simplify this with return (index >= folders1 && index < folder2) ? 1 : -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets this branch when index >= folders1 && index >= folders2. What we can do is:

if (index < folders1 && index < folders2) {
    return entryName1.toLocaleLowerCase().localeCompare(entryName2.toLocaleLowerCase());
} else if (index >= folders1 && index >= folders2) {
    return FileUtils.compareFilenames(entryName1, entryName2);
}
return (index >= folders1 && index < folder2) ? 1 : -1;

return FileUtils.compareFilenames(entryName1, entryName2);
}
}
index++;
}
return 0;
});
}

/**
* @private
* Counts the total number of matches and files
* @return {{files: number, matches: number}}
*/
function _countFilesMatches() {
Expand Down Expand Up @@ -316,13 +364,14 @@ define(function (require, exports, module) {
}));

// Create the results template search list
var searchItems, match, i,
var searchItems, match, i, item,
searchList = [],
matchesCounter = 0,
showMatches = false;

_.some(searchResults, function (item, fullPath) {
searchFiles.some(function (fullPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here? I think I was stumped on this the first time around that we're basically using .some to exit the loop with return true below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It is doing the same as before. The only difference is that it is now iterating over the file names and then graving the results for each file on the results object, while before it was iterating directly over the results.

showMatches = true;
item = searchResults[fullPath];

// Since the amount of matches on this item plus the amount of matches we skipped until
// now is still smaller than the first match that we want to display, skip these.
Expand Down Expand Up @@ -484,14 +533,15 @@ define(function (require, exports, module) {
}

FileSystem.on("change", _fileSystemChangeHandler);

} else {

_hideSearchResults();

if (dialog) {
dialog.getDialogTextField().addClass("no-results")
.removeAttr("disabled")
.get(0).select();
dialog.getDialogTextField()
.addClass("no-results")
.removeAttr("disabled")
.get(0).select();
$(".modal-bar .no-results-message").show();
}
}
Expand Down Expand Up @@ -647,6 +697,7 @@ define(function (require, exports, module) {
}
if (updateResults) {
timeoutID = window.setTimeout(function () {
_sortResultFiles();
_restoreSearchResults();
timeoutID = null;
}, UPDATE_TIMEOUT);
Expand Down Expand Up @@ -674,9 +725,9 @@ define(function (require, exports, module) {
}

/**
* @private
* Used to filter out image files when building a list of file in which to
* search. Ideally this would filter out ALL binary files.
* @private
* @param {FileSystemEntry} entry The entry to test
* @return {boolean} Whether or not the entry's contents should be searched
*/
Expand Down Expand Up @@ -710,6 +761,7 @@ define(function (require, exports, module) {
})
.done(function () {
// Done searching all files: show results
_sortResultFiles();
_showSearchResults();
StatusBar.hideBusyIndicator();
PerfUtils.addMeasurement(perfTimer);
Expand Down Expand Up @@ -909,6 +961,7 @@ define(function (require, exports, module) {

// Restore the results if needed
if (resultsChanged) {
_sortResultFiles();
_restoreSearchResults();
}
}
Expand All @@ -927,7 +980,7 @@ define(function (require, exports, module) {

/*
* Remove existing search results that match the given entry's path
* @param {File|Directory}
* @param {(File|Directory)} entry
*/
function _removeSearchResultsForEntry(entry) {
Object.keys(searchResults).forEach(function (fullPath) {
Expand All @@ -940,8 +993,8 @@ define(function (require, exports, module) {

/*
* Add new search results for this entry and all of its children
* @param {File|Directory}
* @param {jQuery.Promise} Resolves when the results have been added
* @param {(File|Directory)} entry
* @return {jQuery.Promise} Resolves when the results have been added
*/
function _addSearchResultsForEntry(entry) {
var addedFiles = [],
Expand Down Expand Up @@ -1006,12 +1059,13 @@ define(function (require, exports, module) {
addPromise.always(function () {
// Restore the results if needed
if (resultsChanged) {
_sortResultFiles();
_restoreSearchResults();
}
});

};


// Initialize items dependent on HTML DOM
AppInit.htmlReady(function () {
var panelHtml = Mustache.render(searchPanelTemplate, Strings);
Expand Down