Add quick-filter to dependency tree/table #1737

ptziegler · 2025-04-23T19:11:22Z

This adds a search field to the tree/table viewer used in the dependency view to allow user to check whether a specific artifact appears in the dependency chain.

github-actions · 2025-04-23T19:40:13Z

Test Results

288 files +3 288 suites +3 50m 15s ⏱️ - 3m 5s
3 613 tests +2 3 537 ✅ +2 76 💤 ±0 0 ❌ ±0
11 031 runs +6 10 800 ✅ +6 231 💤 ±0 0 ❌ ±0

Results for commit 16533a3. ± Comparison against base commit 1121eb8.

♻️ This comment has been updated with latest results.

HannesWell

Thank you for this PR, I really appreciate it and I think that can be very helpful!
Unfortunately there is currently an issue with cyclic-dependencies in TPs.

HannesWell · 2025-04-25T06:14:28Z

...ipse.pde.ui/src/org/eclipse/pde/internal/ui/views/dependencies/DependenciesViewTreePage.java

+		fFilteredTreeViewer = new FilteredTree(parent, SWT.MULTI | SWT.V_SCROLL | SWT.H_SCROLL, new PatternFilter(),
+				true, true);


Unfortunately the bundles in a target-platform can have cycles and when such cycle is encountered one gets a StackOverflowException.
I have tried to create a guard for that situation, but I'm not yet sure this is sufficient for all cases. In general we could also think about adding that to PatternFilter directly (maybe optionally).

Suggested change

fFilteredTreeViewer = new FilteredTree(parent, SWT.MULTI | SWT.V_SCROLL | SWT.H_SCROLL, new PatternFilter(),

true, true);

fFilteredTreeViewer = new FilteredTree(parent, SWT.MULTI | SWT.V_SCROLL | SWT.H_SCROLL, new PatternFilter() {

private final Map<Object, Boolean> visited = new HashMap<>();

int depth = 0;

@Override

public boolean isElementVisible(Viewer viewer, Object element) {

depth++;

Boolean isMatch = visited.get(element);

if (isMatch == null) {

visited.put(element, false);

// Put in surrogate to prevent stack-overflows

isMatch = super.isElementVisible(viewer, element);

visited.put(element, isMatch);

}

depth--;

if (depth == 0) {

visited.clear();

}

return isMatch;

}

}, true, true);

Do you have an example for such a cycle so that I can also test this on my side?

The TP that is active in that case is the following wild mixture (I haven't stripped it down yet, but I think it's also not that important for this issue):

<target includeMode="feature" name="featureBased"> <locations> <location includeAllPlatforms = "false" includeConfigurePhase = "true" includeMode="planner" includeSource="true" type="InstallableUnit"> <repository location="https://download.eclipse.org/releases/2023-12/"/> <unit id="org.eclipse.pde.feature.group" /> <unit id="org.eclipse.m2e.pde.feature.feature.group"/> <unit id="jakarta.annotation-api" version="0.0.0"/> <unit id="org.eclipse.ecf.filetransfer.ssl.feature.feature.group" version="[1.0.0,2.0.0)"/> <unit id="org.eclipse.ecf.filetransfer.httpclient5.feature.feature.group" version="[1.0.0,2.0.0)"/> <unit id="org.eclipse.rcp.feature.group"/> <unit id="org.eclipse.ecf.core.feature.feature.group" /> <unit id="org.eclipse.ecf.filetransfer.ssl.feature.source.feature.group" version="0.0.0"/> <unit id="org.eclipse.ecf.core.feature.source.feature.group"/> <unit id="org.eclipse.acceleo.feature.group" version="3.7.14.202311201319"/> <unit id="org.eclipse.acceleo.source.feature.group" version="3.7.14.202311201319"/> <unit id="com.google.guava"/> <unit id="org.eclipse.rap.jface"/> <unit id="org.eclipse.sdk.feature.group"/> <unit id="org.eclipse.m2e.sdk.feature.feature.group"/>  </location> </locations> </target>

For the sake of documentation: The StackOverflowError can be reproduced using the following example configuration.

HannesWell · 2025-05-13T21:02:55Z

@ptziegler could we complete this soon?

ptziegler · 2025-05-14T04:13:15Z

@HannesWell I can try to have a look at the weekend, but things are really busy right now...

HannesWell · 2025-05-14T21:14:01Z

@HannesWell I can try to have a look at the weekend, but things are really busy right now...

Sure, no problem. It would be great if we could have this in the upcoming release (which has M3 on Thursday) and RC1 freeze on Monday/Tuesday.
I have now created eclipse-platform/eclipse.platform.ui#2982, in order to implement the cycle-detection in the PatternFilter class directly.

ptziegler · 2025-05-15T05:58:15Z

I've updated the PR to also take cycles into consideration.

HannesWell

Thanks for the update.

ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/project/PluginDependencyTests.java

HannesWell · 2025-05-15T22:08:29Z

...ipse.pde.ui/src/org/eclipse/pde/internal/ui/views/dependencies/DependenciesViewTreePage.java

+		return fFilteredTreeViewer;
+	}
+
+	private static class DependencyFilteredTree extends FilteredTree {


Could you please explain, why it's now necessary to subclass FilteredTree with a lot of non-trivial logic, that to some extend seem to be similar to the parent class (but I fail to understand the difference)?

If that's required to make the cycle-prevention work it would be an additional argument for me to move it up the hiearachy, i.e. into

Add cycle detection for e4.ui.dialogs.filteredtree.PatternFilter eclipse-platform/eclipse.platform.ui#2982

When you open the dependency view, the tree is automatically expanded to show the direct dependencies. But when you remove the filter, the entire tree is collapsed. This is an inconsistent behavior caused by the following part of the base class:

if (!narrowingDown) { // collapse all TreeItem[] is = treeViewer.getTree().getItems(); for (TreeItem item : is) { if (item.getExpanded()) { treeViewer.setExpandedState(item.getData(), false); } } }

If you disable the narrowingDown, the tree isn't collapsed at all. If you hook it to an IJobChangeListener and expand the tree after the job has finished, you end up with flickering because both the collapse and the expansion are rendered. So the alternative is to use a custom job.

I've thought about this problem and I'm also not happy about effectively duplicating the code from the FilteredTree, just to change the way the tree is collapsed.

Once the platform is open again, I'll try to make this mechanism configurable with either "collapse all" or "collapse to auto-level".

Once the platform is open again, I'll try to make this mechanism configurable with either "collapse all" or "collapse to auto-level".

Thanks, I was about to suggest the same.

HannesWell · 2025-05-15T22:22:15Z

...ipse.pde.ui/src/org/eclipse/pde/internal/ui/views/dependencies/DependenciesViewTreePage.java

+		@Override
+		public Object[] filter(Viewer viewer, Object parent, Object[] elements) {
+			try {
+				return super.filter(viewer, parent, elements);
+			} finally {
+				visited.clear();
+			}
+		}
+
+		@Override
+		protected boolean isParentMatch(Viewer viewer, Object element) {
+			// Cycle detection
+			if (!visited.add(element)) {
+				return false;
+			}
+			return super.isParentMatch(viewer, element);
+		}


Comparing this to my proposal I was first concerned that this implementation might lead to false results, because the same parent might be reached through multiple paths, in this case if e.g. the root plugin requires multiple plugins, which require the same plugin, which in turn requires the plugin we searched for.
I tried it with the TP I posted above and the results looked actually good as far as I can tell, but my concerns have not yet fulled vanished.

Can you tell with certainty what is right and what's wrong here? And can you please explain if/why, one approach is better then the other?

because the same parent might be reached through multiple paths

That doesn't matter. The filter is applied on each element in the tree, so it only matters if the item has at least one child that matches the filter. Lets take the following example:

When the filter is modified, the entire tree is refreshed. This refresh is done in a top-down fashion, so in the example above, the first level is filtered, then the second and then the third.

For each tree item, the pattern filter then goes through its sub-tree, to see if there is any element matching the filter. This is where the cycle check is needed, in order to handle the case where the sub-tree has infinite elements.

Or to rephrase:

Plugin-A is shown because it contains Plugin-C in its sub-tree.

Plugin-B is shown because it contains Plugin-C in its sub-tree

Plugin-C is shown because it is Plugin-C.

Can you tell with certainty what is right and what's wrong here?

Both approaches effectively solve the same problem and I don't think any on those are wrong.

This adds a search field to the tree/table viewer used in the dependency view to allow user to check whether a specific artifact appears in the dependency chain.

ptziegler mentioned this pull request Apr 23, 2025

Wrap viewer in CheckboxTablePart within a FilteredTable #1666

Merged

HannesWell force-pushed the add-filter-to-dependency-viewer branch from d94e714 to 64845c3 Compare April 25, 2025 04:51

HannesWell requested changes Apr 25, 2025

View reviewed changes

HannesWell force-pushed the add-filter-to-dependency-viewer branch from 64845c3 to d1c6d0a Compare May 13, 2025 20:58

HannesWell mentioned this pull request May 14, 2025

Add cycle detection for e4.ui.dialogs.filteredtree.PatternFilter eclipse-platform/eclipse.platform.ui#2982

Open

ptziegler force-pushed the add-filter-to-dependency-viewer branch 2 times, most recently from e23488f to b1beffc Compare May 15, 2025 05:57

HannesWell reviewed May 15, 2025

View reviewed changes

Add quick-filter to dependency tree/table

16533a3

This adds a search field to the tree/table viewer used in the dependency view to allow user to check whether a specific artifact appears in the dependency chain.

ptziegler force-pushed the add-filter-to-dependency-viewer branch from b1beffc to 16533a3 Compare May 16, 2025 05:15

		fFilteredTreeViewer = new FilteredTree(parent, SWT.MULTI \| SWT.V_SCROLL \| SWT.H_SCROLL, new PatternFilter(),
		true, true);

-		fFilteredTreeViewer = new FilteredTree(parent, SWT.MULTI | SWT.V_SCROLL | SWT.H_SCROLL, new PatternFilter(),
-				true, true);
+		fFilteredTreeViewer = new FilteredTree(parent, SWT.MULTI | SWT.V_SCROLL | SWT.H_SCROLL, new PatternFilter() {
+			private final Map<Object, Boolean> visited = new HashMap<>();
+			int depth = 0;
+			@Override
+			public boolean isElementVisible(Viewer viewer, Object element) {
+				depth++;
+				Boolean isMatch = visited.get(element);
+				if (isMatch == null) {
+					visited.put(element, false);
+					// Put in surrogate to prevent stack-overflows
+					isMatch = super.isElementVisible(viewer, element);
+					visited.put(element, isMatch);
+				}
+				depth--;
+				if (depth == 0) {
+					visited.clear();
+				}
+				return isMatch;
+			}
+		}, true, true);

Add quick-filter to dependency tree/table #1737

Are you sure you want to change the base?

Add quick-filter to dependency tree/table #1737

Uh oh!

Conversation

ptziegler commented Apr 23, 2025

Uh oh!

github-actions bot commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

HannesWell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HannesWell commented May 13, 2025

Uh oh!

ptziegler commented May 14, 2025

Uh oh!

HannesWell commented May 14, 2025

Uh oh!

ptziegler commented May 15, 2025

Uh oh!

HannesWell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HannesWell May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 23, 2025 •

edited

Loading

HannesWell May 15, 2025 •

edited

Loading