File exclusions usability improvements (bug #7149) #7400
Conversation
* 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.
| @@ -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 <a href='{0}' title='{0}'>globs</a>. Enter each string on a new line.", | |||
| "FILE_FILTER_INSTRUCTIONS" : "Exclude files and folders matching any of the following strings / substrings or <a href='{0}' title='{0}'>wildcards</a>. Enter each string on a new line.", | |||
There was a problem hiding this comment.
We changed from "globs" to "wildcards" in last sprint's update notification & the wiki documentation, but didn't update the UI to match. This syncs it up with the preferred terminology.
|
What about adding a percentage? |
|
@SAplayer I think the absolute number of files is actually more meaningful, in terms of how long the search will take to complete, etc. It's easy enough to guesstimate the percentage in your head looking at the numbers we show, anyway :-) I'd rather not have too much 'numbers overload' in the display at bottom, personally. |
|
Ok, sounds good. |
| /** | ||
| * 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() |
There was a problem hiding this comment.
nit: if {!string} means that it can never be null, why do we if (!compiledFilter) in the first line?
There was a problem hiding this comment.
Good catch, will fix the docs
|
I think we should right align the input field with the |
| context.promise.done(function (files) { | ||
| var filter = getValue(); | ||
| if (filter.length) { | ||
| var filtered = filterFileList(compile(getValue()), files); |
There was a problem hiding this comment.
instead of calling getValue() filter could be used.
|
@peterflynn I'm done with initial review. Some minor nits, but nothing big. Works fine for me. I think we should add some more unit tests, but we can do this later if we don't have time today. |
cleaner. Fix some stale docs and other nits.
|
@ingorichter Changes pushed. For unit tests, since we have zero tests for Find in Files so far I was thinking all tests for that area would be rolled up in https://trello.com/c/lBKh0WXZ/1077-unit-tests-for-existing-find-replace-find-in-files-features |
|
@peterflynn Yes, that sounds reasonable to me. Perhaps we should mention this new behavior on the card to get it covered. Otherwise it looks good to me and I'm going to merge your changes. Thanks. |
File exclusions usability improvements (bug #7149)
|
@ingorichter @peterflynn I keep forgetting about the parallel cards in the Brackets board. Maybe they should be explicitly linked? The associated card in the Kanban Board (https://trello.com/c/qvHL6r4i/45-unit-tests-for-existing-find-replace-find-in-files-features) is already in progress and has been tasked and edited, so it's quite different from the one in Brackets board, so that's the one that needs to be updated. Also, I copied the Task List to the pull request (#7328) to make it easier to follow, so that also should be updated. |
|
@redmunds that trello stuff is private only? i'm getting |
|
Well, the two cards are supposed to be explicitly linked when the Kanban copy is created... but we seem like a forgetful bunch :-) I've added tasks to the Kanban card checklist to write unit tests for the filtering stuff. @redmunds can you sync that up with your other copy of the checklist in the PR? (Or maybe it would be best to just link the PR to the card so we don't have to keep doing that...) |
|
@zaggino Yeah, it had been kind of this internal team workflow thing so we didn't bother making it public while we were getting the hang of the new workflow, but due to the syncing overhead and the reduced visibility for the community, we're working now on merging it back into the main Trello board. |
Fix several issues raised in #7149:
Also cleans up FindInFiles a little bit to centralize the filtering code more, and simplify
_doSearchInOneFile()& its call sites.