Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[CLOSED] Exclude files from Find in Files operations #6351

Open
core-ai-bot opened this issue Aug 30, 2021 · 29 comments
Open

[CLOSED] Exclude files from Find in Files operations #6351

core-ai-bot opened this issue Aug 30, 2021 · 29 comments

Comments

@core-ai-bot
Copy link
Member

Issue by peterflynn
Thursday Feb 27, 2014 at 00:34 GMT
Originally opened as adobe/brackets#7015


Implements Find in Files exclusions user story, with basic unit tests.

  • New FileFilters module manages the UI (picker widget & editor dialog), glob-matching APIs, and last-used filter preference.
  • New DropdownButton widget hoisted out from CSSInlineEditor -- this will later be reused as a filter picker UI once we support saving multiple. separate filters. Originally this story also called for a dropdown widget, so now that it's already implemented it seemed simpler to leave this change in the diff. (It also removes about 100 lines of code from the CSSInlineEditor module).
  • Switches search bar to use the standard ModalBar autoClose behavior, rather than its own one-off focus tracking logic.
  • Enhances ModalBar with two new capabilities:
    • Can specify an isLockedOpen() function that keeps the bar open regardless of focus changes. Needed for existing Find in Files functionality - bar stays open while search in progress.
    • Popups launched from widgets inside the bar can take focus without closing it (even when parented to <body>) if they specify an "attached-to" attribute pointing back at the widget inside the bar. Needed for newly-added picker dropdown.

peterflynn included the following code: https://github.com/adobe/brackets/pull/7015/commits

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Thursday Feb 27, 2014 at 00:34 GMT


@RaymondLim You might want to take a quick look at 6cec6e9, which is the main place where I reconciled your earlier code with my UI code.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Feb 27, 2014 at 02:39 GMT


This is awesome :)

But I think we can improve the filters management UI a bit more since I didn't found it that intuitive.

I think that when you select Edit Filter... it should take you to a UI that would let you add/edit/delete any filter. That UI could be just a select with all the filters and Add New Filter and the textarea. After selecting a filter or Add New Filter, the textarea fills with the required content and the buttons change accordingly: Edit and Delete for editing and Add for adding. After clicking any button it could confirm the changes and stay in the dialog or close it, if the dialog isn't automatically closed, a Close button would be required.

An alternative could be a 2 steps UI, The first dialog opens where you select a filter and after clicking Edit a new dialog opens to edit it, or from the first dialog you can click on Delete to delete it. There would be an Add button that opens the second dialog to add a new filter.

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 27, 2014 at 02:45 GMT


I think the intent was to keep this simple, so you don't "manage a list of filters" - we just have a history of the filters you've used. But I think what's confusing is that when you choose "Edit filter", it feels like you should be editing the current filter, but instead it adds a new filter.

I wonder if it would be better to make it so "Edit filter" really does edit the last selected filter, and there's a separate "New filter" for creating a new one.

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 27, 2014 at 02:48 GMT


A couple other thoughts on the UI:

  • The first time the user sees this UI, it would make sense for it to just be a "Filter..." button (not a dropdown), so you can get right into the filter UI. Once there's at least one filter in the history, it could change to a dropdown.
  • The file glob docs say the user should use forward slashes on Windows. Could we just translate backslashes on Windows automatically? It seems unfriendly to require forward slashes for Windows users.

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 27, 2014 at 02:49 GMT


Actually, another thought...what if when you hover over a filter in the list, it gets icons next to it - "x" to delete, pencil to edit? This would be similar to what we do in the Recent Project dropdown. Then we would just need a "New filter" entry at the bottom to create a new one.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Feb 27, 2014 at 02:53 GMT


I see. I thought I was editing the filter I selected, but it just creates a new one.

I like@njx idea about the x and pencil. That would keep it simple enough. If I created a wrong filter, I would like to remove it from the list, or edit it.

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 27, 2014 at 02:57 GMT


/cc@larz0 relevant to your interests

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Thursday Feb 27, 2014 at 03:11 GMT


@TomMalbran@njx I think the idea was that most of the filter management stuff -- delete, name/rename, save explicitly -- was sliced out into a separate story so we could ship the core functionality sooner. I'd rather save things like an "x" and pencil icon for that story since it seems like we'd still prefer to wrap this sprint up sooner.

The idea of splitting Edit Filter from New Filter seems reasonable... although it does mean that the list isn't really a history list anymore since it might no longer contain the filter you just recently used (if you've edited it since then). It seems like this creates a pitfall -- it could lead to the list only ever having 1 thing in it (which the user repeatedly edits as needed), losing the benefit of having a list in the first place. Maybe one way around that would be to keep the current UI (Edit only), but don't add repeated edits as separate items to the history list -- only add something when you actually run a search using it...

Re backslashes: my only concern with that is that it'd be inconsistent with the rest of Brackets. We display paths with forward slashes everywhere in the UI, and other places that do matching (e.g. Quick Open, the preferences system) don't do this translation.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Thursday Feb 27, 2014 at 03:14 GMT


In the long run (i.e. user story linked above) I think we should do the larger UI Tom mentioned, since it has the benefit that you can name common filters. Right now, it's very hard to represent the arbitrary filter strings in a compact way, so allowing "friendly names" feels like a big plus. Especially since the list of filters is global across all projects...

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 27, 2014 at 03:20 GMT


Makes sense to keep the other functionality separate.

I don't think the change you're suggesting to Edit fixes the fundamental problem, which is that "Edit" sounds like it should modify something rather than creating a new thing. I agree that if we split it into New/Edit it won't feel as much like history, but I think it actually makes it easier for the user to map what they want onto the functionality - tweaking a filter vs. creating a new one. It does make it harder to base a new filter on an existing one, though.

You're right about backslashes - I had forgotten that we don't really deal with them anywhere in the UI. I guess no one's complained so far :)

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Feb 27, 2014 at 03:35 GMT


If the edit will be in a different story, that is good.

I do think that Edit filter should be renamed to Add new filter since you are not editing any filter, just adding a new one. We could just complete the textarea with the last filter, but it should be clear that you are adding a filter based on that one.

On the complete UI, we could duplicate filters, and even make filters to be used only in the current project.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Thursday Feb 27, 2014 at 04:06 GMT


Hmm, I think we have a conceptual model mismatch going on. The original proposal for the first-pass story was: you have one filter, which you can edit, and that's it. Then we extended it by saying there's also a history of previously-used filters; but what you're editing is always the one, current filter. You're not 'making a new filter' when you modify that filter, but the history list does grow since the once-current value is now a previous value.

If that model isn't clear in this UI -- and it sounds like it isn't -- we could try to make it less ambiguous, e.g. by moving Edit to the top, adding a "History" heading to the list below it, and only adding to the history list when you execute the search... But I wonder if we should actually go back to the original plan at this point -- one filter, no list at all. That might be better than putting more effort into this temporary history/MRU list that's actually going to get replaced with a full filter-management UI in the near future.

Or a third option would be to say we shouldn't ship this at all until the full UI can be implemented...

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 27, 2014 at 23:21 GMT


@peterflynn and I chatted today and we decided to just simplify it for now as he suggested above (one filter, no history).

@core-ai-bot
Copy link
Member Author

Comment by larz0
Thursday Feb 27, 2014 at 23:37 GMT


Yes I think we should simplify it for now. We could rename "Edit Filter" to "Edit Filter List" to make it clearer.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Feb 28, 2014 at 19:20 GMT


@JeffryBooher Ready for review, except for the little bit of unit tests at the end of the diff. Will finish those up shortly.

@RaymondLim Since the new, simpler UI doesn't require as much test coverage, I can wrap up the tests on my end -- no need to worry about that.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Feb 28, 2014 at 22:56 GMT


@JeffryBooher Unit tests are ready to go now too

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Saturday Mar 01, 2014 at 01:07 GMT


Updated description

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Saturday Mar 01, 2014 at 02:21 GMT


Hmm, one thing I've noticed now that I'm using the feature more is that the actual globmatch filtering is fairly slow -- the getAllFiles() phase of the search takes 5-6 seconds now, vs. just 35 ms without any filters. The more filters you add, the slower; I have about 10 in my test.

I'm going to look into whether that can be improved...

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Sunday Mar 02, 2014 at 23:12 GMT


This is becoming problematic... I'm able to get about 10x better performance (which seems good enough) by preprocessing the glob expression into a regexp with Minimatch.makeRe()... but unfortunately the implementation seems buggy and doesn't match the same strings. I.e. if mm is a Minimatch instance, path.match(mm.makeRe()) != mm.match(path) in some cases.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Sunday Mar 02, 2014 at 23:26 GMT


If we roll our own regexp conversion using a simpler syntax, we can get a 100x speedup, so at least there's that path forward if we need it. I'll continue to investigate the Minimatch issues though...

@core-ai-bot
Copy link
Member Author

Comment by dangoor
Monday Mar 03, 2014 at 14:31 GMT


@peterflynn wow that performance difference is crazy. I guess we've hit on a use case that is not common among the node tools that use minimatch.

Personally, I don't think it's important that we support everything minimatch does. If we just support the kinds of common cases that you've outlined, I think we'd be fine.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Monday Mar 03, 2014 at 19:34 GMT


Example of minimatch makeRe() bug: isaacs/minimatch#29.

We agreed at the standup today to eschew minimatch/globmatch and just do a simpler syntax of our own, supporting only /*/?. This will be easier to document and much faster (as noted above). I'll file a spinoff bug on changing the prefs system to use this new setup too, for consistency.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Monday Mar 03, 2014 at 21:42 GMT


@JeffryBooher The performance fix is pushed up, with a big chunk of new unit tests, and I've updated the syntax docs on the wiki as well. Should be 100% ready for full review now.

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Tuesday Mar 04, 2014 at 00:15 GMT


@peterflynn done with initial review. I just want to play around with the wild card stuff a bit and I think we're ready to go.

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Tuesday Mar 04, 2014 at 23:01 GMT


@peterflynn done with testing, can you merge master into this branch so I can do the merge?

@core-ai-bot
Copy link
Member Author

Comment by RaymondLim
Wednesday Mar 05, 2014 at 00:18 GMT


Done with my initial review.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Mar 05, 2014 at 00:57 GMT


@JeffryBooher@RaymondLim@TomMalbran Changes pushed. Merged up with master as well.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Mar 05, 2014 at 22:29 GMT


@JeffryBooher I pushed a commit to address@TomMalbran's last few bits of feedback. I think this is ready to merge if you are.

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Thursday Mar 06, 2014 at 05:25 GMT


Looks good. Merging.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant