Skip to content
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

[SIW] Add support to search in outside workspace content #8646

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

DucNgn
Copy link
Contributor

@DucNgn DucNgn commented Oct 16, 2020

What it does

Fixes #6867

How to test

  • Open a file that is not in the workspace root directory.
  • Search for content that should be found in that file.
  • Expected behavior: The search result comes from that file is displayed under Other files node in Search view.
  • Other files node is only displayed when there's multi-root in the search result tree (including the Other files node)

Review checklist

Reminder for reviewers

Signed-off-by: DukeNgn [email protected]

@DucNgn DucNgn force-pushed the dn/searchNonWScontent branch 2 times, most recently from e369c32 to 6419715 Compare October 19, 2020 19:15
@vince-fugnitto vince-fugnitto added the search in workspace issues related to the search-in-workspace label Oct 28, 2020
@DucNgn DucNgn force-pushed the dn/searchNonWScontent branch 2 times, most recently from 08b478f to 750b9ef Compare November 2, 2020 20:48
@DucNgn DucNgn force-pushed the dn/searchNonWScontent branch 3 times, most recently from bfdcaa5 to fd67d0c Compare November 3, 2020 20:42
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Code LGTM with few nitpicks, feature wise I noticed that it shows the Other files root node even when there is no matches for workspaces. The expected behavior in this case is to not show this Other files node, and only display it if there's others roots to display.

@DucNgn DucNgn force-pushed the dn/searchNonWScontent branch 2 times, most recently from 39d5c48 to 430dc7d Compare November 6, 2020 15:13
@DucNgn
Copy link
Contributor Author

DucNgn commented Nov 6, 2020

@marechal-p Thanks for the review. I tried to address the use case you mentioned but unfortunately, I don't see an approach that is available with our current implementation.

One approach is to get the number of root folder node in resultTree and set visible based on it.

https://github.com/DukeNgn/theia/blob/430dc7d2b8c010a878406796259dea55aa76d23f/packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.tsx#L526

However, because visible is readonly, it needs to be determined before any call of appendToResultTree() to make all the root folder node appear in search view (when other files presents). But the resultTree is not updated until we create root folder node in the search tree, hence we have no way to find out the visibility prior to creating the tree.

With my current approach in this PR, I believe the main functionality is not being affected. The only thing that is a bit off is the UX for a specific use-case (Other files is displayed when no matches in the workspace).

@DucNgn DucNgn force-pushed the dn/searchNonWScontent branch from 430dc7d to a97cb3a Compare November 11, 2020 14:11
@DucNgn
Copy link
Contributor Author

DucNgn commented Nov 11, 2020

I just rebased against master. Other than that, nothing has changed since the previous review. The PR is ready to be merged in my opinion.

About the mentioned use-case (no ws results are found, other files is still being displayed), I and @vince-fugnitto discussed offline. And we think that:

  • Our current implementation in siw does not allow such behavior.
  • The behavior in this PR is maybe even better since it makes a clear distinction that the results are from outside workspace files.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, works well.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

There is currently a bug which can be reproduced as follows:

  1. start the application with a single-root workspace
  2. open a file outside the workspace
  3. perform a search which does not yield results in the external file
  4. the search tree is incorrectly rendered with a root node of the workspace

@DucNgn DucNgn force-pushed the dn/searchNonWScontent branch 3 times, most recently from 5777162 to 20aa6e8 Compare November 11, 2020 16:44
+ Search results from outside workspace editor are displayed and grouped under `Other
files ` node in search view.

Signed-off-by: Duc Nguyen <[email protected]>
@DucNgn DucNgn force-pushed the dn/searchNonWScontent branch from 20aa6e8 to 093e3be Compare November 11, 2020 19:15
@DucNgn
Copy link
Contributor Author

DucNgn commented Nov 12, 2020

I fixed the bug and tested again 👍

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The latest changes work well for me 👍

@paul-marechal paul-marechal merged commit 16fec09 into eclipse-theia:master Nov 12, 2020
@DucNgn DucNgn deleted the dn/searchNonWScontent branch November 12, 2020 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
search in workspace issues related to the search-in-workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[search-in-workspace] opening files outside the workspace root are not included in the search results
3 participants