eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

Fix duplicate entries in Text Search results for nested projects #139

Closed jjohnstn closed 1 year ago

jjohnstn commented 1 year ago
jjohnstn commented 1 year ago

@mickaelistria Please review this as it fixes the other part of #31

mickaelistria commented 1 year ago

I'm not in favor of such change: the Search view cannot show nested projects in a nested view. As a result, if some results are hidden here or there, they simply feel missing and the results incomplete. It's not clear that the complete result require user to look into other projects. So I think before changing the output and adding more filtering, we first need to have the Search view capable of showing projects hierarchically.

jjohnstn commented 1 year ago

Hi @mickaelistria. I don't think I understand your reply. What missing or hidden matches are you referring to? The current File search logic will show a tree expanding from the outer project and from the inner project a file really belongs to. The outer project match cannot be used to open the file properly and IMO there is no reason to show it as it is extraneous. This is the same problem that occurred for the Quick search dialog. With this change, you see the matches in the innermost project. Any matches that exist only in the outer project are shown properly as belonging to the outer project (e.g. a match in its .project file). This matches the new behavior of the Quick search where only the innermost match is shown and clicking on the match opens the file in the correct context.

merks commented 1 year ago

I can see both points of view having run into this kind of issue repeatedly, e.g.,

image

Technically this search result is accurate and correct but also kind of inconvenient. I wonder if this "problem" should really be "fixed" in the view that presents the results rather than by changing the search results themselves. E.g., if the search view itself had a filter/option to show only the mostly shallowly nested occurrences for cases where two different IFiles share the same underlying location URI, I don't think anyone would object to such helpful functionality...

Just a thought...

mickaelistria commented 1 year ago

The outer project match cannot be used to open the file properly and IMO there is no reason to show it as it is extraneous

I don't think it's true since #57 .

With this change, you see the matches in the innermost project. Any matches that exist only in the outer project are shown properly as belonging to the outer project (e.g. a match in its .project file).

I would personally feel it as a regression: because a file is nested doesn't mean it shouldn't be visible as a search result hit in another tree where it actually is. FWIW, I'm a huge user of nested projects and hierarchical view and so on, but I really like to browse the "outer"/parent tree in search views because usually, when I'm looking for something, I don't have an idea of which particular small sub-module I should look at and I like to see the results in the outer tree more than in sub-tree.

This matches the new behavior of the Quick search where only the innermost match is shown and clicking on the match opens the file in the correct context.

Quick search doesn't present a tree and doesn't show the resource in the model but just allows to jump to a hit; it doesn't face the same issue of rendering nested stuff or not.

Moreoever I strongly agree with @merks that it seems tricky to change the search engin itself. I didn't check but this is possibly an API contract somewhere, or at least an expectation, that all resources will be returned. Nested resources are still different resources from their parent project as per the workspace model. I agree this issue is a more a presentation issue of the Search result then, and that a safer place where to fix it would be in the Search view more than the SearchEngine; or -much deeper- in the workspace model to avoid the need to have nested projects (ie allow settings and natures at folder level).

jjohnstn commented 1 year ago

The outer project match cannot be used to open the file properly and IMO there is no reason to show it as it is extraneous

I don't think it's true since #57 .

With this change, you see the matches in the innermost project. Any matches that exist only in the outer project are shown properly as belonging to the outer project (e.g. a match in its .project file).

I would personally feel it as a regression: because a file is nested doesn't mean it shouldn't be visible as a search result hit in another tree where it actually is. FWIW, I'm a huge user of nested projects and hierarchical view and so on, but I really like to browse the "outer"/parent tree in search views because usually, when I'm looking for something, I don't have an idea of which particular small sub-module I should look at and I like to see the results in the outer tree more than in sub-tree.

This matches the new behavior of the Quick search where only the innermost match is shown and clicking on the match opens the file in the correct context.

Quick search doesn't present a tree and doesn't show the resource in the model but just allows to jump to a hit; it doesn't face the same issue of rendering nested stuff or not.

Moreoever I strongly agree with @merks that it seems tricky to change the search engin itself. I didn't check but this is possibly an API contract somewhere, or at least an expectation, that all resources will be returned. Nested resources are still different resources from their parent project as per the workspace model. I agree this issue is a more a presentation issue of the Search result then, and that a safer place where to fix it would be in the Search view more than the SearchEngine; or -much deeper- in the workspace model to avoid the need to have nested projects (ie allow settings and natures at folder level).

Ok, I will scrap this and look at a filter for the duplicates. There is still a problem in that opening the file in the outer project opens a file such as a Java file in the wrong editor context. Would you be ok if I fixed that issue so the file will open as it would in the innermost project?

mickaelistria commented 1 year ago

There is still a problem in that opening the file in the outer project opens a file such as a Java file in the wrong editor context

Can you please provide detailed steps to reproduce this in some other ticket? I'm pretty certain that #57 has fixed it in the vast majority of cases.

jjohnstn commented 1 year ago

There is still a problem in that opening the file in the outer project opens a file such as a Java file in the wrong editor context

Can you please provide detailed steps to reproduce this in some other ticket? I'm pretty certain that #57 has fixed it in the vast majority of cases.

Ok. It requires going back to the generic project's match. I will open and reference you. I will close this PR.