티스토리 수익 글 보기
Add explorer.autorevealExclude setting – #136905
Conversation
7414f00 to
3c42745
Compare
3c42745 to
d9e12ce
Compare
d9e12ce to
4e781ac
Compare
|
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. |
4e781ac to
b2cd4ae
Compare
|
Jackson Kearl (@JacksonKearl) I have implemented it using I’m not sure if its possible the required recursive matching into the matcher, as you cannot import from workbench/contrib |
| } | ||
|
|
||
| // Reveal excludes | ||
| private matchesUpToRoot(item: ExplorerItem | undefined): boolean { |
There was a problem hiding this comment.
Couple notes:
- the name of this isn’t very discriptive
shouldAutoRevealItemmight be better? - walking up the directly tree for every item isn’t great, the glob matcher should be able to know that
node_modules/**matchesnode_modules/abc/dwithout 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.
There was a problem hiding this comment.
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:
- Focusing on a file is not a super frequent operation
- File structures do not tend to be super deep, in the case of
**/node_modulesit would be 4-5 match operations for a source file inside
In that case, there are two options I would suggest here:
- 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.
- 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)
There was a problem hiding this comment.
Robert Jin (@jzyrobert) Did we settle on anything here I think we only support file patterns for files.exclude so this should mirror that.
b2cd4ae to
1496945
Compare
|
It looks very helpful. Jackson Kearl (@JacksonKearl), what about this? |
|
I wonder how this should interact with recently added file nesting. |
1496945 to
a357daf
Compare
Jackson Kearl (JacksonKearl)
assigned
Logan Ramos (lramos15) and unassigned
Jackson Kearl (JacksonKearl)
|
Hi, any update on this? |
|
Robert Jin (@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. |
18caae5 to
a6d4981
Compare
|
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 👍 |
|
I’m a bit hesitant in inheriting from |
Isidor Nikolic (isidorn)
left a comment
Isidor Nikolic (isidorn)
left a comment
There was a problem hiding this comment.
Thank you for this PR. I have left some minor comments. It would be great if Benjamin Pasero (@bpasero) also gives it a glance for exclude and globs.
| 'explorer.autoRevealExclude': { | ||
| 'type': 'object', | ||
| 'markdownDescription': nls.localize('autoRevealExclude', "Configure glob patterns for excluding files and folders from being revealed and selected in the explorer when they are opened. Inherits all glob patterns from the `#files.exclude#` setting. Read more about glob patterns [here](https://code.visualstudio.com/docs/editor/codebasics#_advanced-search-options)."), | ||
| 'default': { '**/node_modules': true, '**/bower_components': true, '**/*.code-search': true }, |
There was a problem hiding this comment.
What is code-search and why is it in the default?
| }, | ||
| 'explorer.autoRevealExclude': { | ||
| 'type': 'object', | ||
| 'markdownDescription': nls.localize('autoRevealExclude', "Configure glob patterns for excluding files and folders from being revealed and selected in the explorer when they are opened. Inherits all glob patterns from the `#files.exclude#` setting. Read more about glob patterns [here](https://code.visualstudio.com/docs/editor/codebasics#_advanced-search-options)."), |
There was a problem hiding this comment.
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)
Benjamin Pasero (bpasero)
left a comment
Benjamin Pasero (bpasero)
left a comment
There was a problem hiding this comment.
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?
The explorer will show files in Then I access a file in there by some way (Ctrl+ P or if it was imported etc): Isidor Nikolic (@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. |
1767a5f to
25ea489
Compare
|
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. Benjamin Pasero (@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 |
|
Yeah its fine for me, doesn’t need to block this PR from merging. |
Logan Ramos (lramos15)
left a comment
Logan Ramos (lramos15)
left a comment
There was a problem hiding this comment.
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
|
This will be in tomorrow’s insiders. |
12 participants
This PR fixes #87956
Loosely based on the original PR by Joel Day (@joelday) and the
FilesFilterclass, using the suggestion of Jackson Kearl (@JacksonKearl) to be in theExplorerServicefile.files.excludesearch.exclude:**/node_modules,**/bower_components,**/*.code-searchrefresh,selectResourceand calls fromExplorerVieware all routed toExplorerService.select(), where the reveal check occursExample of glob matching nested folder:
Example of using when clause to hide .js files:
Some things to consider:
files.excludeonly has tests for the find files function, and not forFilesFilter. I suppose you could create aRevealFiltermanually and check the boolean function, but I didn’t see other tests having support for mockingcontextServiceetcfiles.excludesetting, 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)