From 04b87180ee40fe45b3a44c22240d7b6a58f62cb7 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Thu, 3 Apr 2014 12:41:56 -0700 Subject: [PATCH 1/2] Fix several issues raised in #7149: * When search scope is a single file, skip exclusion filter entirely * If exclusions filter results in 0 files to search, show a message to that effect and leave search bar open so user can adjust filter * When editing a filter, show how many files are still included out of the total number of files in the current search scope Also cleans up FindInFiles to centralize the filtering code more, and simplify _doSearchInOneFile() & its call sites. --- src/nls/root/strings.js | 6 +- src/search/FileFilters.js | 184 ++++++++++++++++++++++---------------- src/search/FindInFiles.js | 127 +++++++++++++++++--------- src/styles/brackets.less | 7 ++ 4 files changed, 206 insertions(+), 118 deletions(-) diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 8e76da3f0ac..154d00a854b 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -160,6 +160,7 @@ define({ "FIND_IN_FILES_TITLE_PART3" : "— {0} {1} {2} in {3} {4}", "FIND_IN_FILES_SCOPED" : "in {0}", "FIND_IN_FILES_NO_SCOPE" : "in project", + "FIND_IN_FILES_ZERO_FILES" : "Filter excludes all files {0}", "FIND_IN_FILES_FILE" : "file", "FIND_IN_FILES_FILES" : "files", "FIND_IN_FILES_MATCH" : "match", @@ -175,9 +176,12 @@ define({ "NO_FILE_FILTER" : "Exclude files\u2026", "EDIT_FILE_FILTER" : "Edit\u2026", "FILE_FILTER_DIALOG" : "Edit Filter", - "FILE_FILTER_INSTRUCTIONS" : "Exclude files and folders matching any of the following strings / substrings or globs. Enter each string on a new line.", + "FILE_FILTER_INSTRUCTIONS" : "Exclude files and folders matching any of the following strings / substrings or wildcards. Enter each string on a new line.", "FILE_FILTER_LIST_PREFIX" : "except", "FILE_FILTER_CLIPPED_SUFFIX" : "and {0} more", + "FILTER_COUNTING_FILES" : "Counting files\u2026", + "FILTER_FILE_COUNT" : "Allows {0} of {1} files {2}", + "FILTER_FILE_COUNT_ALL" : "Allows all {0} files {1}", // Quick Edit "ERROR_QUICK_EDIT_PROVIDER_NOT_FOUND" : "No Quick Edit provider found for current cursor position", diff --git a/src/search/FileFilters.js b/src/search/FileFilters.js index 0d3ddc46033..6debf6f6aac 100644 --- a/src/search/FileFilters.js +++ b/src/search/FileFilters.js @@ -32,6 +32,7 @@ define(function (require, exports, module) { "use strict"; var _ = require("thirdparty/lodash"), + ProjectManager = require("project/ProjectManager"), DefaultDialogs = require("widgets/DefaultDialogs"), Dialogs = require("widgets/Dialogs"), DropdownButton = require("widgets/DropdownButton").DropdownButton, @@ -59,44 +60,6 @@ define(function (require, exports, module) { } - /** - * Opens a dialog box to edit the given filter. When editing is finished, the value of getLastFilter() changes to - * reflect the edits. If the dialog was canceled, the preference is left unchanged. - * @param {!Array.} filter - * @return {!$.Promise} Dialog box promise - */ - function editFilter(filter) { - var lastFocus = window.document.activeElement; - - var html = StringUtils.format(Strings.FILE_FILTER_INSTRUCTIONS, brackets.config.glob_help_url) + - ""; - var buttons = [ - { className : Dialogs.DIALOG_BTN_CLASS_NORMAL, id: Dialogs.DIALOG_BTN_CANCEL, text: Strings.CANCEL }, - { className : Dialogs.DIALOG_BTN_CLASS_PRIMARY, id: Dialogs.DIALOG_BTN_OK, text: Strings.OK } - ]; - var dialog = Dialogs.showModalDialog(DefaultDialogs.DIALOG_ID_INFO, Strings.FILE_FILTER_DIALOG, html, buttons); - - dialog.getElement().find(".exclusions-editor").val(filter.join("\n")).focus(); - - dialog.done(function (buttonId) { - if (buttonId === Dialogs.DIALOG_BTN_OK) { - var newFilter = dialog.getElement().find(".exclusions-editor").val().split("\n"); - - // Remove blank lines - newFilter = newFilter.filter(function (glob) { - return glob.trim().length; - }); - - // Update saved filter preference - setLastFilter(newFilter); - } - lastFocus.focus(); // restore focus to old pos - }); - - return dialog.getPromise(); - } - - /** * Converts a user-specified filter object (as chosen in picker or retrieved from getFilters()) to a 'compiled' form * that can be used with filterPath()/filterFileList(). @@ -153,6 +116,109 @@ define(function (require, exports, module) { } + /** + * Returns false if the given path matches any of the exclusion globs in the given filter. Returns true + * if the path does not match any of the globs. If filtering many paths at once, use filterFileList() + * for much better performance. + * + * @param {!string} compiledFilter 'Compiled' filter object as returned by compile() + * @param {!string} fullPath + * @return {boolean} + */ + function filterPath(compiledFilter, fullPath) { + if (!compiledFilter) { + return true; + } + + var re = new RegExp(compiledFilter); + return !fullPath.match(re); + } + + /** + * Returns a copy of 'files' filtered to just those that don't match any of the exclusion globs in the filter. + * + * @param {!string} compiledFilter 'Compiled' filter object as returned by compile() + * @param {!Array.} files + * @return {!Array.} + */ + function filterFileList(compiledFilter, files) { + if (!compiledFilter) { + return files; + } + + var re = new RegExp(compiledFilter); + return files.filter(function (f) { + return !f.fullPath.match(re); + }); + } + + + /** + * Opens a dialog box to edit the given filter. When editing is finished, the value of getLastFilter() changes to + * reflect the edits. If the dialog was canceled, the preference is left unchanged. + * @param {!Array.} filter + * @param {?{label:string, promise:$.Promise}} context Info on which files the filter will be applied to. If specified, + * editing UI will indicate how many files are excluded by the filter. Label should be of the form "in ..." + * @return {!$.Promise} Dialog box promise + */ + function editFilter(filter, context) { + var lastFocus = window.document.activeElement; + + var html = StringUtils.format(Strings.FILE_FILTER_INSTRUCTIONS, brackets.config.glob_help_url) + + "
" + Strings.FILTER_COUNTING_FILES + "
"; + var buttons = [ + { className : Dialogs.DIALOG_BTN_CLASS_NORMAL, id: Dialogs.DIALOG_BTN_CANCEL, text: Strings.CANCEL }, + { className : Dialogs.DIALOG_BTN_CLASS_PRIMARY, id: Dialogs.DIALOG_BTN_OK, text: Strings.OK } + ]; + var dialog = Dialogs.showModalDialog(DefaultDialogs.DIALOG_ID_INFO, Strings.FILE_FILTER_DIALOG, html, buttons); + + var $editField = dialog.getElement().find(".exclusions-editor"); + $editField.val(filter.join("\n")).focus(); + + function getValue() { + var newFilter = $editField.val().split("\n"); + + // Remove blank lines + return newFilter.filter(function (glob) { + return glob.trim().length; + }); + } + + dialog.done(function (buttonId) { + if (buttonId === Dialogs.DIALOG_BTN_OK) { + // Update saved filter preference + setLastFilter(getValue()); + } + lastFocus.focus(); // restore focus to old pos + }); + + + // Code to update the file count readout at bottom of dialog (if context provided) + var $fileCount = dialog.getElement().find(".exclusions-filecount"); + + function updateFileCount() { + context.promise.done(function (files) { + var filter = getValue(); + if (filter.length) { + var filtered = filterFileList(compile(getValue()), files); + $fileCount.html(StringUtils.format(Strings.FILTER_FILE_COUNT, filtered.length, files.length, context.label)); + } else { + $fileCount.html(StringUtils.format(Strings.FILTER_FILE_COUNT_ALL, files.length, context.label)); + } + }); + } + + if (context) { + $editField.on("input", _.debounce(updateFileCount, 400)); + updateFileCount(); + } else { + $fileCount.hide(); + } + + return dialog.getPromise(); + } + + /** * Marks the filter picker's currently selected item as most-recently used, and returns the corresponding * 'compiled' filter object ready for use with filterPath(). @@ -170,9 +236,10 @@ define(function (require, exports, module) { * client should call commitDropdown() when the UI containing the filter picker is confirmed (which updates the MRU * order) and then use the returned filter object as needed. * + * @param {?{label:string, promise:$.Promise}} context Info on files filter will apply to - see editFilter() * @return {!jQueryObject} Picker UI. To retrieve the selected value, use commitPicker(). */ - function createFilterPicker() { + function createFilterPicker(contextPromise) { var $picker = $("
"), $button = $picker.find("button"); @@ -208,7 +275,7 @@ define(function (require, exports, module) { updatePicker(); $button.click(function () { - editFilter(getLastFilter()) + editFilter(getLastFilter(), contextPromise) .done(function (buttonId) { if (buttonId === Dialogs.DIALOG_BTN_OK) { updatePicker(); @@ -220,43 +287,6 @@ define(function (require, exports, module) { } - /** - * Returns false if the given path matches any of the exclusion globs in the given filter. Returns true - * if the path does not match any of the globs. If filtering many paths at once, use filterFileList() - * for much better performance. - * - * @param {!string} compiledFilter 'Compiled' filter object as returned by compile() - * @param {!string} fullPath - * @return {boolean} - */ - function filterPath(compiledFilter, fullPath) { - if (!compiledFilter) { - return true; - } - - var re = new RegExp(compiledFilter); - return !fullPath.match(re); - } - - /** - * Returns a copy of 'files' filtered to just those that don't match any of the exclusion globs in the filter. - * - * @param {!string} compiledFilter 'Compiled' filter object as returned by compile() - * @param {!Array.} files - * @return {!Array.} - */ - function filterFileList(compiledFilter, files) { - if (!compiledFilter) { - return files; - } - - var re = new RegExp(compiledFilter); - return files.filter(function (f) { - return !f.fullPath.match(re); - }); - } - - exports.createFilterPicker = createFilterPicker; exports.commitPicker = commitPicker; exports.getLastFilter = getLastFilter; diff --git a/src/search/FindInFiles.js b/src/search/FindInFiles.js index 376917419d3..ef2df0c12af 100644 --- a/src/search/FindInFiles.js +++ b/src/search/FindInFiles.js @@ -79,6 +79,9 @@ define(function (require, exports, module) { FIND_IN_FILE_MAX = 300, UPDATE_TIMEOUT = 400; + /** @const @type {!Object} Token used to indicate a specific reason for zero search results */ + var ZERO_FILES_TO_SEARCH = {}; + /** * Map of all the last search results * @type {Object., collapsed: boolean}>} @@ -346,8 +349,9 @@ define(function (require, exports, module) { /** * @private * Shows the results in a table and adds the necessary event listeners + * @param {?Object} zeroFiles The 'ZERO_FILES_TO_SEARCH' token, if no results found for this reason */ - function _showSearchResults() { + function _showSearchResults(zeroFilesToken) { if (!$.isEmptyObject(searchResults)) { var count = _countFilesMatches(); @@ -580,7 +584,13 @@ define(function (require, exports, module) { .addClass("no-results") .removeAttr("disabled") .get(0).select(); - $(".modal-bar .no-results-message").show(); + if (zeroFilesToken === ZERO_FILES_TO_SEARCH) { + $(".modal-bar .error") + .show() + .html(StringUtils.format(Strings.FIND_IN_FILES_ZERO_FILES, _labelForScope(currentScope))); + } else { + $(".modal-bar .no-results-message").show(); + } } } } @@ -731,22 +741,35 @@ define(function (require, exports, module) { return !language.isBinary(); } + /** + * Finds all candidate files to search in currentScope's subtree that are not binary content. Does NOT apply + * currentFilter yet. + */ + function getCandidateFiles() { + function filter(file) { + return _subtreeFilter(file, currentScope) && _isReadableText(file); + } + + return ProjectManager.getAllFiles(filter, true); + } + /** * Checks that the file is eligible for inclusion in the search (matches the user's subtree scope and * file exclusion filters, and isn't binary). Used when updating results incrementally - during the - * initial search, the filter and part of the scope are taken care of in bulk instead, so _subtreeFilter() - * is just called directly. + * initial search, these checks are done in bulk via getCandidateFiles() and the filterFileList() call + * after it. * @param {!File} file * @return {boolean} */ function _inSearchScope(file) { + // Replicate the checks getCandidateFiles() does if (currentScope) { if (!_subtreeFilter(file, currentScope)) { return false; } } else { // Still need to make sure it's within project or working set - // In the initial search, this is covered by getAllFiles() + // In getCandidateFiles(), this is covered by the baseline getAllFiles() itself if (file.fullPath.indexOf(ProjectManager.getProjectRoot().fullPath) !== 0) { var inWorkingSet = DocumentManager.getWorkingSet().some(function (wsFile) { return wsFile.fullPath === file.fullPath; @@ -756,11 +779,11 @@ define(function (require, exports, module) { } } } - // In the initial search, this is passed as a getAllFiles() filter if (!_isReadableText(file)) { return false; } - // In the initial search, this is covered by the filterFileList() pass + + // Replicate the filtering filterFileList() does return FileFilters.filterPath(currentFilter, file.fullPath); } @@ -791,31 +814,38 @@ define(function (require, exports, module) { } }; - function _doSearchInOneFile(addMatches, file) { + /** + * Finds search results in the given file and adds them to 'searchResults.' Resolves with + * true if any matches found, false if none found. Errors reading the file are treated the + * same as if no results found. + * + * Does not perform any filtering - assumes caller has already vetted this file as a search + * candidate. + */ + function _doSearchInOneFile(file) { var result = new $.Deferred(); - - if (!_subtreeFilter(file, currentScope)) { - result.resolve(); - } else { - DocumentManager.getDocumentText(file) - .done(function (text) { - addMatches(file.fullPath, text, currentQueryExpr); - }) - .always(function () { - // Always resolve. If there is an error, this file - // is skipped and we move on to the next file. - result.resolve(); - }); - } + + DocumentManager.getDocumentText(file) + .done(function (text) { + var foundMatches = _addSearchMatches(file.fullPath, text, currentQueryExpr); + result.resolve(foundMatches); + }) + .fail(function () { + // Always resolve. If there is an error, this file + // is skipped and we move on to the next file. + result.resolve(); + }); + return result.promise(); } - + /** * @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) { + function _doSearch(query, candidateFilesPromise) { currentQuery = query; currentQueryExpr = _getQueryRegExp(query); @@ -828,18 +858,21 @@ define(function (require, exports, module) { var scopeName = currentScope ? currentScope.fullPath : ProjectManager.getProjectRoot().fullPath, perfTimer = PerfUtils.markStart("FindIn: " + scopeName + " - " + query); - ProjectManager.getAllFiles(_isReadableText, true) + candidateFilesPromise .then(function (fileListResult) { // Filter out files/folders that match user's current exclusion filter fileListResult = FileFilters.filterFileList(currentFilter, fileListResult); - var doSearch = _doSearchInOneFile.bind(undefined, _addSearchMatches); - return Async.doInParallel(fileListResult, doSearch); + if (fileListResult.length) { + return Async.doInParallel(fileListResult, _doSearchInOneFile); + } else { + return ZERO_FILES_TO_SEARCH; + } }) - .done(function () { + .done(function (zeroFilesToken) { // Done searching all files: show results _sortResultFiles(); - _showSearchResults(); + _showSearchResults(zeroFilesToken); StatusBar.hideBusyIndicator(); PerfUtils.addMeasurement(perfTimer); @@ -929,6 +962,7 @@ define(function (require, exports, module) { }; var $searchField = $("input#find-what"), + candidateFilesPromise = getCandidateFiles(), // used for eventual search, and in exclusions editor UI filterPicker; function handleQueryChange() { @@ -956,8 +990,14 @@ define(function (require, exports, module) { } else if (event.keyCode === KeyEvent.DOM_VK_RETURN) { StatusBar.showBusyIndicator(true); that.getDialogTextField().attr("disabled", "disabled"); - currentFilter = FileFilters.commitPicker(filterPicker); - _doSearch(query); + + if (filterPicker) { + currentFilter = FileFilters.commitPicker(filterPicker); + } else { + // Single-file scope: don't use any file filters + currentFilter = null; + } + _doSearch(query, candidateFilesPromise); } } }) @@ -971,8 +1011,16 @@ define(function (require, exports, module) { handleQueryChange(); // re-validate regexp if needed }); - filterPicker = FileFilters.createFilterPicker(); - this.modalBar.getRoot().find("#find-group").append(filterPicker); + // Show file-exclusion UI *unless* search scope is just a single file + if (!scope || scope.isDirectory) { + var exclusionsContext = { + label: _labelForScope(scope), + promise: candidateFilesPromise + }; + + filterPicker = FileFilters.createFilterPicker(exclusionsContext); + this.modalBar.getRoot().find("#find-group").append(filterPicker); + } // Initial UI state (including prepopulated initialString passed into template) FindReplace._updateSearchBarFromPrefs(); @@ -1099,12 +1147,6 @@ define(function (require, exports, module) { var addedFiles = [], deferred = new $.Deferred(); - var doSearch = _doSearchInOneFile.bind(undefined, function () { - if (_addSearchMatches.apply(undefined, arguments)) { - resultsChanged = true; - } - }); - // gather up added files var visitor = function (child) { // Replicate filtering that getAllFiles() does @@ -1127,7 +1169,12 @@ define(function (require, exports, module) { } // find additional matches in all added files - Async.doInParallel(addedFiles, doSearch).always(deferred.resolve); + Async.doInParallel(addedFiles, function (file) { + return _doSearchInOneFile(file) + .done(function (foundMatches) { + resultsChanged = resultsChanged || foundMatches; + }); + }).always(deferred.resolve); }); return deferred.promise(); diff --git a/src/styles/brackets.less b/src/styles/brackets.less index b6beb0d4a3a..c0cd848d2a0 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -1216,6 +1216,13 @@ textarea.exclusions-editor { margin-bottom: 0; .code-font(); } +.exclusions-filecount { + margin: 12px 0 -20px 0; + padding: 4px 6px; + background-color: @tc-warning-background; + border: 1px solid @bc-yellow; + border-radius: @tc-control-border-radius; +} // Search result highlighting - CodeMirror highlighting is pretty constrained. Highlights are From 611878608d781e4eead77ae3a6a6395c866b8df7 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Fri, 4 Apr 2014 16:26:16 -0700 Subject: [PATCH 2/2] Code review: restyle filter editor file count and make UI element widths cleaner. Fix some stale docs and other nits. --- src/search/FileFilters.js | 10 +++++----- src/search/FindInFiles.js | 2 +- src/styles/brackets.less | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/search/FileFilters.js b/src/search/FileFilters.js index 6debf6f6aac..b22de7441e9 100644 --- a/src/search/FileFilters.js +++ b/src/search/FileFilters.js @@ -121,7 +121,7 @@ define(function (require, exports, module) { * if the path does not match any of the globs. If filtering many paths at once, use filterFileList() * for much better performance. * - * @param {!string} compiledFilter 'Compiled' filter object as returned by compile() + * @param {?string} compiledFilter 'Compiled' filter object as returned by compile(), or null to no-op * @param {!string} fullPath * @return {boolean} */ @@ -137,7 +137,7 @@ define(function (require, exports, module) { /** * Returns a copy of 'files' filtered to just those that don't match any of the exclusion globs in the filter. * - * @param {!string} compiledFilter 'Compiled' filter object as returned by compile() + * @param {?string} compiledFilter 'Compiled' filter object as returned by compile(), or null to no-op * @param {!Array.} files * @return {!Array.} */ @@ -200,7 +200,7 @@ define(function (require, exports, module) { context.promise.done(function (files) { var filter = getValue(); if (filter.length) { - var filtered = filterFileList(compile(getValue()), files); + var filtered = filterFileList(compile(filter), files); $fileCount.html(StringUtils.format(Strings.FILTER_FILE_COUNT, filtered.length, files.length, context.label)); } else { $fileCount.html(StringUtils.format(Strings.FILTER_FILE_COUNT_ALL, files.length, context.label)); @@ -239,7 +239,7 @@ define(function (require, exports, module) { * @param {?{label:string, promise:$.Promise}} context Info on files filter will apply to - see editFilter() * @return {!jQueryObject} Picker UI. To retrieve the selected value, use commitPicker(). */ - function createFilterPicker(contextPromise) { + function createFilterPicker(context) { var $picker = $("
"), $button = $picker.find("button"); @@ -275,7 +275,7 @@ define(function (require, exports, module) { updatePicker(); $button.click(function () { - editFilter(getLastFilter(), contextPromise) + editFilter(getLastFilter(), context) .done(function (buttonId) { if (buttonId === Dialogs.DIALOG_BTN_OK) { updatePicker(); diff --git a/src/search/FindInFiles.js b/src/search/FindInFiles.js index ef2df0c12af..a203ea792df 100644 --- a/src/search/FindInFiles.js +++ b/src/search/FindInFiles.js @@ -349,7 +349,7 @@ define(function (require, exports, module) { /** * @private * Shows the results in a table and adds the necessary event listeners - * @param {?Object} zeroFiles The 'ZERO_FILES_TO_SEARCH' token, if no results found for this reason + * @param {?Object} zeroFilesToken The 'ZERO_FILES_TO_SEARCH' token, if no results found for this reason */ function _showSearchResults(zeroFilesToken) { if (!$.isEmptyObject(searchResults)) { diff --git a/src/styles/brackets.less b/src/styles/brackets.less index c0cd848d2a0..bfaf0ef64ea 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -1210,8 +1210,9 @@ a, img { // File exclusion filter editor dialog textarea.exclusions-editor { display: block; - width: 547px; + width: 100%; height: 160px; + box-sizing: border-box; // needed for width: 100% since it has padding margin-top: 12px; margin-bottom: 0; .code-font(); @@ -1219,8 +1220,7 @@ textarea.exclusions-editor { .exclusions-filecount { margin: 12px 0 -20px 0; padding: 4px 6px; - background-color: @tc-warning-background; - border: 1px solid @bc-yellow; + background-color: @tc-highlight; border-radius: @tc-control-border-radius; }