Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Make files to include/exclude configurable on whether to use 1 or 2 inputs #46315

Closed
Crazy-P opened this issue Mar 22, 2018 · 46 comments

Comments

@Crazy-P
Copy link

commented Mar 22, 2018

With issue #45702 and commit 7fdf1e3 the search function: files to include and exclude got merged together into 1 input.

Generally usage this is okay (maybe). However, I often use both input fields with a long list to exclude or include.

The new approach is a rather poor UI/ UX. (Some might consider it as a good UI/ UX)

But e.g.:
Search: FunctionA
Include/exclude: !folderA/*, !folderB/*, folderC/*, !folderD/*, folderE/*, fileA.ext, !fileA.ext

It quickly gets tough to deal with. It gets hard to know where are the included and excluded parts.

In the previous design this was separated into 2 fields. Hence much easier to control the search.

I see no issue with using another 30 px or so for an extra field. However, if the merging of these input fields are so desired, I do believe it will be nice with a config to either let it stay as a merged field or separated into 2 fields.

@roblourens

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

Are you using search in the sidebar, or panel (via the search.location setting)?

@Crazy-P

This comment has been minimized.

Copy link
Author

commented Mar 22, 2018

I am referring to the search sidebar (I guess?)

image

@roblourens

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

Would it make sense to put those exclude patterns in your search.exclude workspace setting? Or are you constantly changing them for different searches?

@Crazy-P

This comment has been minimized.

Copy link
Author

commented Mar 23, 2018

I already use search.exclude for folders/files I permanent want to exclude.
The one in search panel I use quite dynamically. Not static enough to be put into the workspace setting.

I was thinking of whether it is possible to add that arrow option like the Search input has to expand it to use search/replace. For include/exclude it could be expanding the field to use 2 input fields.

@roblourens

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

Maybe. I would wait for more feedback before making a change here.

@Yalhu

This comment has been minimized.

Copy link

commented Mar 26, 2018

I want to include search/user.js and exclude db/user.js . Does it work when I set todohighlight.exclude:["**/db/user.js"] or todohighlight.exclude:["./db/user.js"]?

@dpsthree

This comment has been minimized.

Copy link

commented Mar 29, 2018

I have another use case in support of @Crazy-P 's suggestion

I often open a root folder containing many sub-projects beneath them. For example, client, server, tools, common, etc... I often want to perform searches within just one of these top level folders at a time. With the new UI I need to explicitly exclude each top level folder in order to search within the client folder.

The previous version of this UI made it much easier to work in this way. Having the option to choose between the two would be nice.

@roblourens

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

With the new UI I need to explicitly exclude each top level folder in order to search within the client folder.

There shouldn't be any real behavior difference between the old and new versions. If you want to include one folder, you can do that the same way that you used to. client or ./client. Can you explain some more?

@dpsthree

This comment has been minimized.

Copy link

commented Mar 29, 2018

@roblourens This appears to be working as you describe. I was using incorrectly using /client. Thanks!

@ssandler

This comment has been minimized.

Copy link

commented Apr 9, 2018

I preferred the old UI for this. I constantly changed my include/exclude files and having them separate was just a little easier to manage.

@nazarkryp

This comment has been minimized.

Copy link

commented Apr 10, 2018

I do not like new feature as well. It is really annoying to go through all input to check everything.

@viktoriatelyatynska

This comment has been minimized.

Copy link

commented Apr 10, 2018

go back to two inputs please. New UX is awful

@dcormier

This comment has been minimized.

Copy link

commented Apr 10, 2018

I use inclusions/exclusions quite a bit and was surprised when suddenly there was only one box. I had an exclusion, and it seemed to work fine, but when I went to also add an inclusion it didn't work as I expected. I had to resort to actually reading the docs (something I didn't need with the old way).

Seeing as the old way was intuitive (at least for me), and the new way required that I read the docs, it seems to be a step backwards for the UX.

@scottsb

This comment has been minimized.

Copy link

commented Apr 10, 2018

Also prefer the old separate boxes. For me the big issue is that include and exclude rules can now be interspersed with each other, making it hard to quickly evaluate what's getting searched if there are multiple patterns.

I could move some of those entries to the search.* settings, but I often want to add or remove them on the fly depending on what I'm looking for (e.g., excluding test files or not, including vendor files or not, etc.), so that's not a good solution.

For what it's worth, the separate include/exclude boxes right in the search pane were one of the things I've specifically preferred about VSCode over the JetBrains IDEs.

@yoni-tock

This comment has been minimized.

Copy link

commented Apr 11, 2018

I also preferred the separation, basically just for the explicitness / clarity. I find it harder to mentally parse and look for the !s in the one field.

@MarcosEllys

This comment has been minimized.

Copy link

commented Apr 11, 2018

Go back to two input, New UX is awful

@fongandrew

This comment has been minimized.

Copy link

commented Apr 12, 2018

Or are you constantly changing them for different searches?

FWIW, I personally switch these up quite a bit. To give some context, I work a lot in repos where I'm searching for *.js and *.ts files but excluding things like *.test.js and *.d.ts. Whether I want to exclude the latter is highly depend on context and not something I want to stick in a settings file.

@Crazy-P

This comment has been minimized.

Copy link
Author

commented Apr 12, 2018

Another reason to keep them separated is to easily remove all exclusions/inclusions with just

  • ctrl + a -> ctrl + x //Removes filter
  • Do a search
  • ctrl + z or ctrl + v //Adds filter back
@ucirello

This comment has been minimized.

Copy link

commented Apr 12, 2018

The point is that separated fields for inclusion/exclusion provided the perfect middle-way between exclusions needed once (thus combining inclusions and exclusions ad-hoc is fine), and exclusions that you know you will never care about (like .git and friends).

Perhaps, the use case to understand what is lost is the interaction between your code and your vendor code.

You know often you don't want to search for content in the vendor/ directory - but vendor code is not perfect, and sometimes you do need to be able to search there. Often enough that it doesn't justify the long path of going to global or workspace configurations and remove the exclusion rule only to add it back a couple of minutes later.

@roblourens

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

The previous two comments are the motivation behind the "Use Exclude Settings and Ignore Files" button - it provides an easy way to do a quick search in folders that are normally excluded. Does anyone use that?

@scottsb

This comment has been minimized.

Copy link

commented Apr 12, 2018

I never use that button because there are some files I do always want to ignore. Those are the ones that in my settings. The items in the dedicated exclude box are the ones that I may pull in and out--and not in a simple binary all-on or all-off way.

@ucirello

This comment has been minimized.

Copy link

commented Apr 12, 2018

Does anyone use that?

I never did. Never met someone who did. It doesn't solve my problem because each part of the code base needs a different set of changes to the complete exclude set to do what I want.

@ucirello

This comment has been minimized.

Copy link

commented Apr 13, 2018

This doesn't solve every possible case but I think it helps a lot.

This seems a weird trade-off. Trading a feature that was simpler and did more to a more complex feature that only achieves part of the functionality of its counterpart.

@roblourens

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

I'm hearing feedback in a few categories

  • Reading/skimming the include/exclude patterns is less convenient, when they're in a single box
  • Temporarily removing the excludes was more convenient when they were in a separate box
  • General confusion/the perception that functionality has been lost (besides the above)

Some reasons it was originally changed

  • Simpler to edit - keep your cursor in one box
  • Keeping include/exclude patterns together in a single history list makes more sense, since they are usually associated
  • Saves vertical space (especially in search panel layout)
  • Matches other editors

@roblourens roblourens added this to the April 2018 milestone Apr 16, 2018

@Crazy-P

This comment has been minimized.

Copy link
Author

commented Apr 16, 2018

  • Simpler to edit - keep your cursor in one box
  • Keeping include/exclude patterns together in a single history list makes more sense, since they are usually associated

Very debatable and is a pure preference as both ways are rather logical.

  • Matches other editors

Please don't match the other editors. :(
Sure some other editors got some good design approaches, however VS Code is epic because it is not totally like all the others and is so flexible when it comes to customizations - fitting one's own or a specific project's needs while being extremely light.

@scottsb

This comment has been minimized.

Copy link

commented Apr 16, 2018

Right, "matching other editors" is not necessarily a good reason to change. As I had mentioned, the separate boxes was one of the things that I thought made VS Code better than other editors, so this is just reversion to the mediocre mean.

@G1itcher

This comment has been minimized.

Copy link

commented Apr 18, 2018

Have to agree with the zeitgeist on this one. One box doesn't feel like good UX; requires more typing to do the same thing, is slower to parse mentally, and feels like a needless combining of concerns. I don't think the reasons for changing it out weigh the reasons for leaving it as it was.

roblourens added a commit that referenced this issue Apr 19, 2018

Restore split include/exclude boxes
Revert "Fix #47157 - Focus include/exclude box when toggling open"

This reverts commit cf87ba3.

See #46315

Revert "Fix #46628 - wrong "no results found" message"

This reverts commit 0bd949c.

See #46315

Revert "Fix default search exclusions string"

This reverts commit 234d100.

See #46315

Revert "Improve migrating from split include/exclude inputs to the new merged one"

This reverts commit 22f49f8.

See #46315

Revert "Fix #45702 - merge include/exclude inputs"

This reverts commit 7fdf1e3.

See #46315

roblourens added a commit that referenced this issue Apr 19, 2018

@roblourens

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

I reverted back to two boxes... please try it in tomorrow's Insiders build.

@szyhf

This comment has been minimized.

Copy link

commented Apr 19, 2018

Hi,

Using multi-line box with auto-height control, as both include box and exclude box (two box) is my dream solve.

That can make a balance of people has long pattern who want clear and people has short pattern who want focus.

@roblourens roblourens added bug and removed under-discussion labels Apr 22, 2018

@roblourens

This comment has been minimized.

Copy link
Member

commented Apr 22, 2018

Verifier: We are back to two boxes and persisted patterns/history should be transitioned correctly.

@DavidBabel

This comment has been minimized.

Copy link

commented May 3, 2018

This is huge decision !

Cause the one boxe brokes totally when it's used with "search in this folder"

image

Since it erase everything in your include/exclude field.

@ADTC

This comment has been minimized.

Copy link

commented May 4, 2018

Uh, an option would be nice. Default to two box, but a simple setting could merge them to one box if I'd like.

@balta2ar

This comment has been minimized.

Copy link

commented May 4, 2018

@roblourens I don't like having two separate boxes because I have nearly all my excludes in .gitignore. So in my case that's just wasted precious vertical space that I could use to see more results. Could you please add a button to collapse these boxes into one and bring back the support of the negation search term !? I think that would satisfy all the users.

@DavidBabel

This comment has been minimized.

Copy link

commented May 4, 2018

Because your usage is very specific.

When you have a well designed large application, you pass your time to work in a specific folder and switch frequently.
Beiing able to automate all your searches without having to change the folder time to time is a big improvement.

Consider beginners too, the "!" notation is less intuitive.

@guidobouman

This comment has been minimized.

Copy link

commented May 4, 2018

Am I the only one to dislike the revert? VS Code was finally consistent between Atom and other editors. Now it's inconsistent again...

@DavidBabel Actually, this is harder for beginners as they might have worked with editors like Atom or Brackets, where only one input for limiting your search is present. In Atom it's include + exclude, in Brackets it's just exclude.

@G1itcher

This comment has been minimized.

Copy link

commented May 4, 2018

@guidobouman it's difficult for beginners to use two boxes because of other packages? But beginners are much less likely to have experienced multiple dev software and are even less likely to have settled on a process.

Parity between unrelated software packages is a complete non issue; unless we all fancy moving to a vi style interface?

Two clearly labeled boxes for two distinct modifications to search criteria is pretty clear. Having to combine the two requirements into an odd mish mash of include/exclude is needlessly obtuse demonstrably more complex, as illustrated by the need to add ! to everything you don't want.

I'm having a tough time seeing any reason to combine the two. It's not simpler, and how other packages do things shouldn't consider unless they do it better. In which case that should mean we would have no issues in defining why it's better surely?

@ADTC

This comment has been minimized.

Copy link

commented May 5, 2018

I don't get why there's a debate which is better. Why not just provide both, default to split version and leave it to the user to decide if they want to switch to combined version. It's just a matter of adding combineSearchIncludeExclude: true to the user preferences. That way, both camps are happy.

@roblourens

This comment has been minimized.

Copy link
Member

commented May 5, 2018

Because I don't think it's worth making the code more complex and supporting both forever.

@guidobouman

This comment has been minimized.

Copy link

commented May 6, 2018

@G1itcher Hmmm... Great points. I think I've been disappointed that at one moment I got a synced feature between my two favorite editors (VS Code & Atom), and lost it in a next update.

Your last sentence is the strongest point. I have a hard time coming up with solid arguments that I can't also use the other way around. So this must be personal preference because of muscle memory. Which means I'll get used to this. If this is easier for new developers to get started, then fine by me.

@acls

This comment has been minimized.

Copy link

commented May 8, 2018

Thank you for the revert. With them combined it was easier to scroll through the extra files that I wanted excluded than it was to clutter the includes text box with !'s.

Downgrading VS Code to match other editors sounds like a bad idea. If I wanted to use Atom, I wouldn't be using VS Code.

@GammaGames

This comment has been minimized.

Copy link

commented May 8, 2018

If I remember correctly the change was included to use space more wisely with the horizontal search panel. I think having the include and exclude boxes move to one line in response to the panel width would be a nice middle ground, since the two boxes take up a lot of room and I prefer the panel view.

@alejosky

This comment has been minimized.

Copy link

commented May 16, 2018

Consider beginners too, the "!" notation is less intuitive.

!true

@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
You can’t perform that action at this time.