Group search results by root folders #29964

keegancsmith · 2017-06-30T18:02:21Z

This is a work in progress PR to add support for grouping of search results. I am submitting it now since I believe the rest of the work is mostly minor and I'll only getting back to this PR on Monday. So hopefully will get some early feedback.

TODOs:

Add replace actions. (For follow-up PR)
Make it works with extraFileResources
Always display some FileMatches. Currently will not auto-expand repos if many results
Render FileMatch names for relative to folder.
Render single folder searches without extra level.
Create CSS rule .foldermatch, rather than reusing .filematch.
Update textual descriptions of results to take into account folders.
~~Bug in Tree renderer for auto-expanded children~~

cc @roblourens

mention-bot · 2017-06-30T18:02:25Z

@keegancsmith, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sandy081 and @bpasero to be potential reviewers.

msftclas · 2017-06-30T18:02:25Z

@keegancsmith,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

roblourens · 2017-06-30T20:19:23Z

Thanks for the PR, but in the future, I would strongly recommend starting a discussion with us in an issue over a new feature before doing a lot of work to implement it. I plan to look into the search UI for multiroot in depth in July, and while this is one possibility, there are other UI options I want to investigate. I think one could make a good argument for not adding a new level of nodes to the results UI, but I will be experimenting more later.

keegancsmith · 2017-06-30T21:48:58Z

@roblourens apologies for not opening a discussion PR first. I've been working on multi-repo search at Sourcegraph and we found having a flat list of results frustrating. So this PR is really just the next logical step from the current UI, mainly to experiment with to see if it feels good. I understand it may not be the best UX for displaying the search results. Don't worry about it being wasted work, we want to launch a more polished multi-repo search at Sourcegraph soon so it will be used for that. I also want to try out this version to see if it feels any good. If vscode goes a different direction, I'll probably move Sourcegraph towards what vscode does.

Is the next step forward to rather close the PR and open the discussion up on an issue? Is there anything salvageable from this PR that I could maybe cherry-pick/patch into another PR?

roblourens · 2017-06-30T22:13:35Z

No worries, I just opened #29977 for discussion and I'll leave this open. It may be useful if we go this route. I'm about to go out of town for almost a week and will be thinking about it after that. I didn't realize Sourcegraph was working on this too, and I look forward to checking it out!

nicksnyder · 2017-07-01T03:50:50Z

I didn't realize Sourcegraph was working on this too, and I look forward to checking it out!

@roblourens Would you guys be interested in knowing what we are working on on an ongoing basis? If so, we could figure something out (post our monthly plan on our public issue tracker, share a google doc, send you periodic email, etc.). cc @sqs

roblourens · 2017-07-01T14:27:30Z

I would be interested in following along. Posting a monthly plan on github has worked well for us.

keegancsmith · 2017-07-03T15:33:03Z

FYI I have resolved most of the TODOs listed in the PR description. However, they require a little bit of work to be upstreamed here. So if you would like to move forward with this PR I'll cherry-pick/clean them up and push them.

roblourens · 2017-07-18T03:31:36Z

Hey @keegancsmith, this is the direction I want to go with the multiroot search UI (should have figured that out earlier but I had to investigate and convince myself). I'd love to take the PR if you are interested in porting the rest of the changes forward. Let me know how I can help.

keegancsmith · 2017-07-18T16:04:34Z

@roblourens great. I'll update the PR this week. What I have should work well, except I neglected the non "read-only" use case. So will also implement the replace action.

If you have any early UX feedback, this is what it looks like on Sourcegraph right now

roblourens · 2017-07-18T16:47:05Z

Thanks! I think that looks great. The "search groups" feature is interesting!

keegancsmith · 2017-07-19T15:00:24Z

@roblourens I've pushed some changes.

I haven't tested what happens when extraFileResources[] is non-empty. How do I get entries into that in the UI?
The informational text {0} result in {1} file is now {0} file in {1} folder. I can include file count if you have a suggestions for how to word it.
Including folderQueries as part of the search test cases was done naively. I can refactor the tests to be more readable as part of this PR if ya like.
Find Replace All works. I have only tested via clicking on the find replace button.
The action buttons are hidden for folders. Maybe could be done as a follow-up PR?
If we only have one folder in the workspace, we still render the extra level. I can change in this PR, but could also be a follow-up PR if ya like.

Some updated screenshots

roblourens · 2017-07-19T18:59:19Z

Looks awesome so far, thanks! Few comments:

I haven't tested what happens when extraFileResources[] is non-empty. How do I get entries into that in the UI?

Those are files that are open in the window but don't belong to a workspace. You can use the "open" dialog to open something outside of a root folder. I'm not sure how they should be organized in the UI. I suppose they should get their own node. "Other files" or something.

The informational text {0} result in {1} file is now {0} file in {1} folder.

Hm, I think the result count is still more relevant. I think a typical workspace will only have a few root folders. I think the result count should still be included, and the folder count maybe could be omitted, or maybe like {0} results in {1} files in {2} folders.

Including folderQueries as part of the search test cases was done naively. I can refactor the tests to be more readable as part of this PR if ya like.

That's ok. Are the folderQueries added because the result needs to belong to a workspace, otherwise it will be lost? I think this is related to the extraFileResources issue then?

The action buttons are hidden for folders. Maybe could be done as a follow-up PR?

I think the folders should have action buttons, but I don't need everything in one PR.

If we only have one folder in the workspace, we still render the extra level. I can change in this PR, but could also be a follow-up PR if ya like.

I think in general, the experience for a non-workspace window should be the same as it currently is. This would also mean showing a single level with extraFileResources mixed in to the workspace results. I think I'd like to have this before I send this change out.

keegancsmith · 2017-07-19T19:21:23Z

That all sounds great to me.

I think in general, the experience for a non-workspace window should be the same as it currently is. This would also mean showing a single level with extraFileResources mixed in to the workspace results. I think I'd like to have this before I send this change out.

Should I render as a single level when there is only one folder query, or should I use the contextService to determine that. So for the latter a workspace with one folder will get the new rendering.

roblourens · 2017-07-19T19:44:48Z

I think that we should try to match what shows up in the File Explorer. There, a workspace with one folder shows the one root folder node at the top, so let's match that.

keegancsmith · 2017-07-20T10:26:23Z

@roblourens ok I've done everything except the action bar for FolderMatches. PTAL

We want to be able to use the fields from IQueryOptions. Specifically, we want to be able to use folderResources, so we can render results within each folder in its own tree node.

We introduce a new class `FolderMatch`. It is responsible for all the life-cycle management of `FileMatch` that `SearchResult` used to own. As such most of the implementation is code factored out of `SearchResult`, with most of what `SearchResult` did now just delegating to the respective `FolderMatch`. `FolderMatch` is also rendered, so implements some functions for rendering, so is not a pure "subset" of the `SearchResult` functionality.

We expect a lot more matches in general for folder matches. So just always expand.

We never want to mutate the query of an existing FolderMatch, so rather make it part of the constructor.

This reverts commit 2bcd0d99c6997ebbbc91c41d4e0dff8ae8b3503c.

I believe the file count information is more interesting than number of line matches. And to avoid a very long line when adding folder matches, I removed the line matches.

This lead to us not autoexpanding FolderMatches, since on creation they would be empty and only later be populated.

Mimics `.filematch`, but does not hide the count badge on hover or highlight. This may change when we support actions on the folder.

We want to add a fallback for other resources. Directly using the TrieMap allows us to do that. Using Workspace is also a bit heavier than what we needed.

keegancsmith · 2017-07-20T13:56:17Z

Aah I have test failures. I ran ./scripts/test.sh -g search, but that excludes tests which just mentioned Search. I'll fix.

keegancsmith · 2017-07-20T14:01:37Z

Rebased against latest master and pushed fix

roblourens · 2017-07-20T21:21:24Z

Thanks a ton, this looks great. I've tested on Mac and Windows, and the code looks good. I'm going to wait until after today's insiders build to merge, because if any issues come up, I don't want it to be stuck in insiders for the weekend.

felixfbecker · 2017-07-21T18:04:34Z

Awesome, looking forward to this!

ssalka · 2020-03-03T00:08:08Z

Is this feature no longer supported? My team uses a monorepo, so would be great if my search results could be grouped by, for example, server and frontend (two of our top-level folders in the repo)

However, wasn't able to find any settings available for configuring this 😕

Edit: found #20224. Surprised this still isn't done nearly 3 years later

msftclas added the cla-already-signed label Jun 30, 2017

roblourens self-assigned this Jun 30, 2017

roblourens added workbench-multiroot Multi-root (multiple folders) issues search Search widget and operation issues labels Jun 30, 2017

roblourens mentioned this pull request Jun 30, 2017

Search viewlet multiroot UI #29977

Closed

keegancsmith mentioned this pull request Jul 3, 2017

Recursively refresh children on expand in Tree #30059

Closed

keegancsmith changed the title ~~WIP Group search results by root folders~~ Group search results by root folders Jul 20, 2017

keegancsmith added 7 commits July 20, 2017 15:44

Use SearchQuery rather than Pattern in SearchResult

edc2959

We want to be able to use the fields from IQueryOptions. Specifically, we want to be able to use folderResources, so we can render results within each folder in its own tree node.

Use SearchQuery.maxResults for SearchResults

b93c46b

Avoid typecasts in RemoveAction

5d00a04

Auto-expand FolderMatch

e54abe5

We expect a lot more matches in general for folder matches. So just always expand.

Move query into FolderMatch constructor

f61e164

We never want to mutate the query of an existing FolderMatch, so rather make it part of the constructor.

sort FolderMatch by order defined in input

9a9e564

keegancsmith added 16 commits July 20, 2017 15:44

Revert "Avoid typecasts in RemoveAction"

c7b7bc2

This reverts commit 2bcd0d99c6997ebbbc91c41d4e0dff8ae8b3503c.

Use FolderMatch in RemoveAction

224b977

Use original name of replacingAll

eb33af2

nit: Remove parens on single arg lambda

9ae9b7e

Render FolderMatch icon as folder

eaf76f3

Render search result path relative to repo

491771c

Result count text includes folder count

c1f922f

I believe the file count information is more interesting than number of line matches. And to avoid a very long line when adding folder matches, I removed the line matches.

Use file count for folder count badges

90011c4

Specify folderQueries in SearchModel test

51f46ed

Specify SearchResult.query in viewlet and result test

4ba7e19

Ignore empty FolderMatch in search tree model

b7f843c

This lead to us not autoexpanding FolderMatches, since on creation they would be empty and only later be populated.

Add .foldermatch CSS rule

bcf9f93

Mimics `.filematch`, but does not hide the count badge on hover or highlight. This may change when we support actions on the folder.

Include result count in informational message

4402664

Directly use TrieMap rather than Workspace

f777e92

We want to add a fallback for other resources. Directly using the TrieMap allows us to do that. Using Workspace is also a bit heavier than what we needed.

Group extraFileResources into Other files group

7bb1050

Only render folder matches if in workspace

07c3e33

Use FileKind.ROOT_FOLDER

1d51bf4

roblourens approved these changes Jul 22, 2017

View reviewed changes

roblourens merged commit 9da0fa1 into microsoft:master Jul 22, 2017

roblourens mentioned this pull request Jul 22, 2017

Action buttons for folder search result nodes #31260

Closed

keegancsmith deleted the foldermatch branch July 25, 2017 10:24

iamandrewluca mentioned this pull request Oct 18, 2018

search results in a tree view #20224

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group search results by root folders #29964

Group search results by root folders #29964

keegancsmith commented Jun 30, 2017 •

edited by roblourens

Loading

mention-bot commented Jun 30, 2017

msftclas commented Jun 30, 2017

roblourens commented Jun 30, 2017 •

edited

Loading

keegancsmith commented Jun 30, 2017

roblourens commented Jun 30, 2017 •

edited

Loading

nicksnyder commented Jul 1, 2017

roblourens commented Jul 1, 2017

keegancsmith commented Jul 3, 2017

roblourens commented Jul 18, 2017

keegancsmith commented Jul 18, 2017

roblourens commented Jul 18, 2017

keegancsmith commented Jul 19, 2017

roblourens commented Jul 19, 2017

keegancsmith commented Jul 19, 2017

roblourens commented Jul 19, 2017

keegancsmith commented Jul 20, 2017

keegancsmith commented Jul 20, 2017

keegancsmith commented Jul 20, 2017

roblourens commented Jul 20, 2017

felixfbecker commented Jul 21, 2017

ssalka commented Mar 3, 2020 •

edited

Loading

Group search results by root folders #29964

Group search results by root folders #29964

Conversation

keegancsmith commented Jun 30, 2017 • edited by roblourens Loading

mention-bot commented Jun 30, 2017

msftclas commented Jun 30, 2017

roblourens commented Jun 30, 2017 • edited Loading

keegancsmith commented Jun 30, 2017

roblourens commented Jun 30, 2017 • edited Loading

nicksnyder commented Jul 1, 2017

roblourens commented Jul 1, 2017

keegancsmith commented Jul 3, 2017

roblourens commented Jul 18, 2017

keegancsmith commented Jul 18, 2017

roblourens commented Jul 18, 2017

keegancsmith commented Jul 19, 2017

roblourens commented Jul 19, 2017

keegancsmith commented Jul 19, 2017

roblourens commented Jul 19, 2017

keegancsmith commented Jul 20, 2017

keegancsmith commented Jul 20, 2017

keegancsmith commented Jul 20, 2017

roblourens commented Jul 20, 2017

felixfbecker commented Jul 21, 2017

ssalka commented Mar 3, 2020 • edited Loading

keegancsmith commented Jun 30, 2017 •

edited by roblourens

Loading

roblourens commented Jun 30, 2017 •

edited

Loading

roblourens commented Jun 30, 2017 •

edited

Loading

ssalka commented Mar 3, 2020 •

edited

Loading