Add explorer.autorevealExclude setting

[jzyrobert]

This PR fixes #87956

Loosely based on the original PR by @joelday and the FilesFilter class, using the suggestion of @JacksonKearl to be in the ExplorerService file.

• Inherits from files.exclude
• Defaults to same values as search.exclude: **/node_modules, **/bower_components, **/*.code-search
• Calls to refresh, selectResource and calls from ExplorerView are all routed to ExplorerService.select(), where the reveal check occurs
• Recursively checks from the file up to the first root child, if any of them match a pattern then the file is not revealed.

Example of glob matching nested folder:

Example of using when clause to hide .js files:

Some things to consider:

• Tests: I wasn't sure if there is support for testing whether the reveal correctly shows or not. files.exclude only has tests for the find files function, and not for FilesFilter. I suppose you could create a RevealFilter manually and check the boolean function, but I didn't see other tests having support for mocking contextService etc
  • Edit: Could be added to integration tests?
• Behaviour if folder is already open:
  • Should the file still be revealed?
  • Should it be ignored? (<- current behaviour)
• Behaviour if an open folder is added to the setting:
  • Should the tree close the folder?
  • Should it be left as is? (<- current behaviour)
• Performance:
  • Should the check run on each selection? How much does iterating through the parent tree of an item each time matter (No initial cost, each call only iterates up it's own parent(s))
  • Or should it mirror the files.exclude setting, which runs top-down each time files are updated and sets a cached boolean for each folder/file. (Bigger initial cost as has to check all folder/files, no cost after)

[JacksonKearl]

It's not clear do me what the FilesFilter class does here, it seems to duplicate a lot of the existing functionality of the glob module but with more code to maintain, I'd like to see what the smallest possible diff to implement the desired functionality would look like.

[jzyrobert]

@JacksonKearl I have implemented it using ResourceGlobMatcher instead as you have requested, please take another look.

I'm not sure if its possible the required recursive matching into the matcher, as you cannot import from workbench/contrib

[JacksonKearl]

Couple notes:

• the name of this isn't very discriptive shouldAutoRevealItem might be better?
• walking up the directly tree for every item isn't great, the glob matcher should be able to know that node_modules/** matches node_modules/abc/d without stepping through every directory?

Edit: Oh, I see it's for the sibiling clauses... we don't typically support those for directories as far as I'm aware, that seems like a failrly expensive operation.

[jzyrobert]

I will change the name as you suggested.

The glob matcher can match files if specified, e.g. **/node_modules/* would work fine without recursive checking.
The reason for this implementation is that the settings are inherited from the files.exclude setting, which allows specifying both folder and file patterns for showing files in the explorer. That setting is passed recursively and only needs to be called once for every tree item.
However, I also don't think the performance impact is too serious, as:

1. Focusing on a file is not a super frequent operation
2. File structures do not tend to be super deep, in the case of **/node_modules it would be 4-5 match operations for a source file inside

In that case, there are two options I would suggest here:

1. Keep the matching as is, but only check for sibling clauses with the initial item, and do not pass as sibling clause for the parent matching.
2. Remove the recursive matching, and edit the default setting and setting description to say that you must explicitly specify file patterns (**/node_modules/*) and not folder patterns (**/node_modules)

[lramos15]

@jzyrobert Did we settle on anything here I think we only support file patterns for files.exclude so this should mirror that.

[Livan-pro]

It looks very helpful. @JacksonKearl, what about this?

[JacksonKearl]

I wonder how this should interact with recently added file nesting.

[AmitPr]

Hi, any update on this?

[lramos15]

@jzyrobert Jackson has since left the company so I have taken over reviewing this PR. Please resolve merge conflicts and get CI to pass and then ping me for a review. Hopefully we can get this in for not this release but the next one. Thank you for your contribution.

[lramos15]

Thanks for the super detailed response, will review and test this for the November release. Currently busy finishing up October's release, but this is definitely on our radar and I will get back to you shortly 👍

[lramos15]

I'm a bit hesitant in inheriting from files.exclude I quite like that if I do open those files they appear in the explorer so that they can be edited and acted upon. I don't mind sharing the same defaults, but I don't know if I'm comfortable in them always being merged.

[isidorn]

Thank you for this PR. I have left some minor comments. It would be great if @bpasero also gives it a glance for exclude and globs.

[isidorn]

What is code-search and why is it in the default?

[isidorn]

I do not understand how is the inheriting done and how can the user change that?
How can the user discover that if they miss this desciprtion? We do not have inheritence structure in our settings and I suggest that we do not introduce it now (unless I am missing some case)

[bpasero]

I do not fully understand the need for glob patterns that check on siblings for this purpose here: imho extending from files.exclude does not really make a lot of sense because the explorer will not show anything that is file.exclude (at least to my knowledge) and as such will not auto-reveal anyway. And for the setting I would believe simple glob patterns are sufficient and not the ones with sibling support?

[jzyrobert]

I do not fully understand the need for glob patterns that check on siblings for this purpose here: imho extending from files.exclude does not really make a lot of sense because the explorer will not show anything that is file.exclude (at least to my knowledge) and as such will not auto-reveal anyway. And for the setting I would believe simple glob patterns are sufficient and not the ones with sibling support?
@bpasero

The explorer will show files in file.exclude, for example:

The tree shows nothing because I excluded hiddenFolder

Then I access a file in there by some way (Ctrl+ P or if it was imported etc):

It shows up in the explorer. What it doesn't show is anything else in that folder.

@isidorn makes a point that it wouldn't be obvious to users unless they find the setting description so that can be removed and users can add back what they want.

I can remove the sibling support if required, but wouldn't it be useful to have? It doesn't interfere with the normal glob functionality and can be useful such as wanting to check a compiled JS file but having it revealed if a TS file exists with the same name in that folder.

[lramos15]

This looks good to me from the explorer side of things and I'm hoping to take this in either today or tomorrow to get ample testing in insiders.

@bpasero I'll defer to you if we should keep sibling support or not. It does seem like it only is wiring through an existing check via matches so unsure if that still feels like tainting the interface. We can probably gauge via telemetry how often when patterns are used and decide whether or not we should implement it in the future or remove it in the future deciding which way we want to go.

[bpasero]

Yeah its fine for me, doesn't need to block this PR from merging.

[lramos15]

This is an amazing contribution, thanks for working with us as we understand how long it takes to get something like this merged in!

Decided to keep the sibling check for this iteration

[lramos15]

This will be in tomorrow's insiders.