-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Group search results by root folders #29964
Conversation
@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. |
@keegancsmith, |
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. |
@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? |
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! |
@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 |
I would be interested in following along. Posting a monthly plan on github has worked well for us. |
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. |
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. |
@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 |
Thanks! I think that looks great. The "search groups" feature is interesting! |
@roblourens I've pushed some changes.
Some updated screenshots |
Looks awesome so far, thanks! Few comments:
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.
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
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?
I think the folders should have action buttons, but I don't need everything in one PR.
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 |
That all sounds great to me.
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. |
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. |
@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.
Aah I have test failures. I ran |
Rebased against latest master and pushed fix |
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. |
Awesome, looking forward to this! |
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, 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 |
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:
.foldermatch
, rather than reusing.filematch
.Bug in Tree renderer for auto-expanded childrencc @roblourens