Skip to content

add 'search.collapseAllResults' setting#56438

Merged
roblourens merged 1 commit intomicrosoft:masterfrom
purefun:collapse-all-search-results-config
Aug 17, 2018
Merged

add 'search.collapseAllResults' setting#56438
roblourens merged 1 commit intomicrosoft:masterfrom
purefun:collapse-all-search-results-config

Conversation

@purefun
Copy link
Contributor

@purefun purefun commented Aug 15, 2018

@purefun purefun force-pushed the collapse-all-search-results-config branch from 256d16f to 85ca2a1 Compare August 16, 2018 08:42
@purefun
Copy link
Contributor Author

purefun commented Aug 16, 2018

@roblourens Hi there. Sorry I made a mistake about file match count. I have updated the code.

@roblourens
Copy link
Member

roblourens commented Aug 16, 2018

Instead of a boolean, could you make it a string enum setting? Can have "alwaysCollapse" and "auto" (the default).

That leaves us open to adding more "modes" in the future, like "alwaysExpand".

But, don't implement "alwaysExpand" right now. Perf will be an issue with that option.

roblourens
roblourens reviewed Aug 16, 2018
View reviewed changes
src/vs/workbench/parts/search/browser/searchResultsView.ts Outdated
Copy link
Member

@roblourens roblourens Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not depend on the DOM.

Copy link
Contributor Author

@purefun purefun Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it will expand automatically if just 1 file matched. I'll just remove it.
Thanks.
https://github.com/Microsoft/vscode/blob/d2148923f578ed65cec6c8a10f396933b3d1d054/src/vs/workbench/parts/search/browser/searchView.ts#L1166-L1178

@purefun purefun force-pushed the collapse-all-search-results-config branch from 85ca2a1 to e64436b Compare August 17, 2018 02:23
@purefun
Copy link
Contributor Author

purefun commented Aug 17, 2018

Your idea is great. There are a lot of string enum settings. It makes the setting more extensible.
I had updated search.collapseAllResults to string enum.

Thanks for your review.

@purefun purefun force-pushed the collapse-all-search-results-config branch from e64436b to 095758e Compare August 17, 2018 03:17
@roblourens
Copy link
Member

roblourens commented Aug 17, 2018

Thanks for the PR!

@roblourens roblourens added this to the August 2018 milestone Aug 17, 2018
@roblourens roblourens merged commit f926c3a into microsoft:master Aug 17, 2018
@rightaway
Copy link

rightaway commented Aug 17, 2018

In #48641 it was discussed to have an option to expand all the results by default. Would you please include this option? For example call the setting search.resultDisplay and have the options be auto, collapse, expand?

@purefun
Copy link
Contributor Author

purefun commented Aug 17, 2018

like @roblourens said above, It will cause a perf issue if all results are always expended. Collapse All button acts as a toggle is a good idea for now.

@roblourens
Copy link
Member

roblourens commented Aug 17, 2018

I tried it, it's not as bad as I thought it might be, I'll add that option 👍

@roblourens roblourens mentioned this pull request Sep 13, 2018
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@roblourens roblourens roblourens approved these changes

Assignees

@roblourens roblourens

Labels

None yet

Projects

None yet

Milestone

August 2018

Development

Successfully merging this pull request may close these issues.

3 participants

@purefun @roblourens @rightaway