Add cycle detection for e4.ui.dialogs.filteredtree.PatternFilter

[HannesWell]

Enabling it, prevents stack-overflows when handling trees/tables with recurring elements (which are not trees from a graph-theory point of view as these graphs have cycles).

This is useful for example in

• Add quick-filter to dependency tree/table eclipse-pde/eclipse.pde#1737

As far as I can tell, there are no testes for this class yet, I only found PatternFilterTest which tests the equivalent PatternFilter from the package org.eclipse.ui.dialogs. I'll check if the latter can be used as a foundation for this case.

[github-actions]

Test Results

 1 824 files  ±0   1 824 suites  ±0   1h 37m 44s ⏱️ -14s
 7 925 tests ±0   7 696 ✅  - 1  228 💤 ±0  1 ❌ +1 
23 862 runs  ±0  23 113 ✅  - 1  748 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 8d4263d. ± Comparison against base commit 13d5c4f.

[ptziegler]

Does it really make sense to bake this check directly into the PatternFilter? From my understanding, cycles in a tree are the exception, rather than the norm.

[HannesWell]

Does it really make sense to bake this check directly into the PatternFilter? From my understanding, cycles in a tree are the exception, rather than the norm.

I expect the same, but for those cases where it happens I think it's better to have it available for all. But since if should be the exception I intend it as optional/opt-in feature with low overhead if not used.

[ptziegler]

I expect the same, but for those cases where it happens I think it's better to have it available for all. But since if should be the exception I intend it as optional/opt-in feature with low overhead if not used.

A tree is by definition an acyclic, directed graph. So running into a cycle indicates a flaw in the implementation (in this case the dependency viewer) and my concern is that this is now something encouraged by the Platform, rather than discouraged.

You can then also enable this cycle check for a PatternFilter that is used in combination with a TableViewer (i.e. where cycles are impossible), which I think is weird...

[laeubi]

A tree is by definition an acyclic, directed graph. So running into a cycle indicates a flaw in the implementation (in this case the dependency viewer) and my concern is that this is now something encouraged by the Platform, rather than discouraged.

You really can't apply graph theory to an UI element, a Tree Control can easily contain "cycles" or represent a Graph instead of a formal Tree. Also by design a TreeViewer does not have a single root! It has a ContentProvider that provides a list of "roots" by the given input, but the input is not the root of the tree!

In the case of the Dependency viewer many bundles can depend on the same package. So this package needs to appear more than once in a given tree as it is obviously a child of the bundle importing it. Now the provider of this package might be displayed then also multiple times and be a part of the tree as well.

There might be other examples where you display a file system tree and if the file system allows symbolic links you also might show the "same" as a child when following this symbolic links.

[ptziegler]

Also by design a TreeViewer does not have a single root! It has a ContentProvider that provides a list of "roots" by the given input, but the input is not the root of the tree!

Which then makes this a disjoint union of trees.

In the case of the Dependency viewer many bundles can depend on the same package. So this package needs to appear more than once in a given tree as it is obviously a child of the bundle importing it. Now the provider of this package might be displayed then also multiple times and be a part of the tree as well.

But this is not what this issue is about.

This is perfectly valid:
Bundle A

• Required-Bundle: Bundle B
  • Required-Bundle: Bundle C
• Required-Bundle: Bundle C

This is problematic:
Bundle A

• Required-Bundle: Bundle B
  • Required-Bundle: Bundle A
    • Required-Bundle: Bundle B
      • Required-Bundle: Bundle A
        • ...

Latter is also bad from a UX perspective, because for a non-trivial example, it's not obvious whether a bundle has just a really deep dependency chain or whether you have a cycle.

But if you still think that this is a good idea, I can only encourage you to run expandAll() on your cyclic tree...

[laeubi]

If you have a real cycle A->B -> A PDE will complain as this is invalid in OSGi. Anyways whatever we do here it should not lead to endless recursion so it is e.g valid to simply break out and ignore further items.

[ptziegler]

If you have a real cycle A->B -> A PDE will complain as this is invalid in OSGi.

Whether this is supported by OSGi has nothing to do with this. The dependency viewer will still generate an infinite tree:

Anyways whatever we do here it should not lead to endless recursion so it is e.g valid to simply break out and ignore further items.

I agree. But this is something that should happen at the content provider, not the pattern filter.