eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

Fix duplicate entries in QuickSearch dialog for nested projects #135

Closed jjohnstn closed 1 year ago

jjohnstn commented 1 year ago
jjohnstn commented 1 year ago

@mickaelistria Please review.

laeubi commented 1 year ago

This sound very promising! I wonder if this might be optimized by doing the following and only fallback to your search if the IFile is not a local files:

It might even worth it to think about a new IFile#getInnermostProject() method, then you can even work on the internal state of the object what might be more efficient (or at laest can be optimized later on) and it would be usable on other places as well.

mickaelistria commented 1 year ago

+1 with what @laeubi says, this is already implemented in multiple places with

  URI locationUri = resource.getLocationURI();
  return locationUri == null ? resource : //
     Arrays.stream(resource.getWorkspace().getRoot().findFilesForLocationURI(locationUri)) //
                .sorted(Comparator.comparingInt(aFile -> aFile.getFullPath().segments().length)) //
                .findFirst() // shortest workspace (project reletive) full path means most nested project
                .orElse(resource);

It might even worth it to think about a new IFile#getInnermostProject() method, then you can even work on the internal state of the object what might be more efficient (or at laest can be optimized later on) and it would be usable on other places as well.

The "innermost" is not related to the state of the file, but to the state of the workspace root, so such API would be IWorkspaceRoot.findInnerMostFile(IFile). It would be welcome as this code is already repeated in multiple places.

szarnekow commented 1 year ago

.sorted(Comparator.comparingInt(aFile -> aFile.getFullPath().segments().length)) //

Even though there are not many locations (on avg), it might be worthwhile to consider using min(Comparator) rather than sorted.

mickaelistria commented 1 year ago

Even though there are not many locations (on avg), it might be worthwhile to consider using min(Comparator) rather than sorted.

Indeed, and thanks for noting! That's one more reason to have a good API for it, so we optimize it in one place instead of repeating less efficient constructs in multiple places ;)

laeubi commented 1 year ago

The "innermost" is not related to the state of the file, but to the state of the workspace root, so such API would be IWorkspaceRoot.findInnerMostFile(IFile). It would be welcome as this code is already repeated in multiple places.

Each file belongs to a Workspace that has a workspace root, so I would at least want a delegate in IFile so it is easier to find/use, to be honest, as a user I would exspect that IFile#getProject aslready return exactly that value, but thats nothing one can change I assume so IFile#get<whateveritsnamed>Project() + a hint on getProject would make this more obvious and hopefully improve the overall userexperience over time.

merks commented 1 year ago

Just an FYI. People do bad things and some of those people are "us":

org.eclipse.e4.tools.emf.ui.internal.common.component.dialogs.ContributionDataFile

jabby commented 1 year ago

Thank you all for your work. I know I will enjoy this improvement a lot.