Skip to content

Commit

Permalink
Restore recursive behavior of AbstractTreeViwer#internalExpandToLevel()
Browse files Browse the repository at this point in the history 
The method AbstractTreeViewer#internalExpandToLevel() used be a
recursively called method (by implementation and specification). As a
protected method, it is part of the API.
With a recent change, the method was changed to delegate to a different
method, removing it's recursive property. Consumers that have created
subtypes of the AbstractTreeViewer and rely the recursive behavior of
that method by overwriting it face a regression by that change.

This change restores the existing, recursive behavior of that method
while still keeping the reuse of the implementation across other use
cases. A test case ensuring the contractual recursive execution is
added.
  • Loading branch information
@HeikoKlare
HeikoKlare committed Jan 16, 2025
1 parent d4aa37b commit 9bea968
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 30 deletions.
67 changes: 37 additions & 30 deletions bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.function.Function;

import org.eclipse.core.runtime.Assert;
import org.eclipse.core.runtime.IStatus;
Expand Down Expand Up @@ -1833,23 +1832,22 @@ protected Widget internalGetWidgetToSelect(Object elementOrTreePath) {
}

/**
* Recursively, conditionally expands the subtree rooted at the given widget to
* the given level. Takes the {@code shouldChildrenExpand} predicate that
* defines for a given widget if it shall be expanded.
* Recursively expands the subtree rooted at the given widget to the given level
* based on the {@code childExpansionFunction} function being executed for a
* child to be (potentially conditionally) expanded.
* <p>
* Note that the default implementation of this method does not call
* {@code setRedraw}.
* </p>
*
* @param widget the widget
* @param level non-negative level, or {@code ALL_LEVELS} to
* expand all levels of the tree
* @param shouldChildrenExpand predicate that defines for a given widget if it
* should be expanded.
* @since 3.32
* @param widget the widget
* @param level non-negative level, or {@code ALL_LEVELS} to
* expand all levels of the tree
* @param childrenExpansionFunction function to be called on a child to be
* expanded
*/
private void internalConditionalExpandToLevel(Widget widget, int level,
Function<Widget, Boolean> shouldChildrenExpand) {
private void internalCustomizedExpandToLevel(Widget widget, int level,
CustomChildrenExpansionFunction childrenExpansionFunction) {
if (level == ALL_LEVELS || level > 0) {
Object data = widget.getData();
if (widget instanceof Item it && data != null && !isExpandable(it, null, data)) {
Expand All @@ -1861,19 +1859,18 @@ private void internalConditionalExpandToLevel(Widget widget, int level,
setExpanded(it, true);
}
if (level == ALL_LEVELS || level > 1) {
Item[] children = getChildren(widget);
if (children != null && shouldChildrenExpand.apply(widget).booleanValue()) {
int newLevel = (level == ALL_LEVELS ? ALL_LEVELS
: level - 1);
for (Item element : children) {
internalConditionalExpandToLevel(element, newLevel, shouldChildrenExpand);
}
}
int newLevel = (level == ALL_LEVELS ? ALL_LEVELS : level - 1);
childrenExpansionFunction.expandChildren(widget, newLevel);
}
// XXX expanding here fails on linux
}
}

@FunctionalInterface
private interface CustomChildrenExpansionFunction {
void expandChildren(Widget parent, int previousLevel);
}

/**
* Recursively expands the subtree rooted at the given widget to the given
* level.
Expand All @@ -1887,7 +1884,14 @@ private void internalConditionalExpandToLevel(Widget widget, int level,
* levels of the tree
*/
protected void internalExpandToLevel(Widget widget, int level) {
internalConditionalExpandToLevel(widget, level, w -> Boolean.TRUE);
internalCustomizedExpandToLevel(widget, level, (parent, newLevel) -> {
Item[] children = getChildren(parent);
if (children != null) {
for (Item child : children) {
internalExpandToLevel(child, newLevel);
}
}
});
}

/**
Expand Down Expand Up @@ -2532,14 +2536,22 @@ public void treeCollapsed(TreeExpansionEvent event) {
@Override
public void treeExpanded(TreeExpansionEvent e) {
Widget item = doFindItem(e.getElement());

internalConditionalExpandToLevel(item, autoExpandOnSingleChildLevels,
w -> Boolean.valueOf(doesWidgetHaveExactlyOneChild(w)));
internalCustomizedExpandToLevel(item, autoExpandOnSingleChildLevels, singleChildExpansionFunction);
}
};
addTreeListener(autoExpandOnSingleChildListener);
}

private CustomChildrenExpansionFunction singleChildExpansionFunction = (widget, newLevel) -> {
Item[] children = getChildren(widget);
boolean hasExactlyOneChild = children != null && children.length == 1;
if (hasExactlyOneChild) {
for (Item child : children) {
internalCustomizedExpandToLevel(child, newLevel, this.singleChildExpansionFunction);
}
}
};

private void removeAutoExpandOnSingleChildListener() {
if (autoExpandOnSingleChildListener != null) {
removeTreeListener(autoExpandOnSingleChildListener);
Expand Down Expand Up @@ -2702,8 +2714,7 @@ public void setExpandedStateWithAutoExpandOnSingleChild(Object elementOrTreePath
Widget item = internalGetWidgetToSelect(elementOrTreePath);

if (autoExpandOnSingleChildLevels != NO_EXPAND && expanded) {
internalConditionalExpandToLevel(item, autoExpandOnSingleChildLevels,
w -> Boolean.valueOf(doesWidgetHaveExactlyOneChild(w)));
internalCustomizedExpandToLevel(item, autoExpandOnSingleChildLevels, singleChildExpansionFunction);
}
}

Expand Down Expand Up @@ -3535,8 +3546,4 @@ ISelection getUpdatedSelection(ISelection selection) {
return selection;
}

private boolean doesWidgetHaveExactlyOneChild(Widget w) {
return getChildren(w).length == 1;
}

}
37 changes: 37 additions & 0 deletions tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/TreeViewerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,21 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;

import org.eclipse.jface.viewers.AbstractTreeViewer;
import org.eclipse.jface.viewers.StructuredViewer;
import org.eclipse.jface.viewers.TreeViewer;
import org.eclipse.swt.SWT;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Tree;
import org.eclipse.swt.widgets.TreeItem;
import org.eclipse.swt.widgets.Widget;
import org.junit.Test;

public class TreeViewerTest extends AbstractTreeViewerTest {
Expand Down Expand Up @@ -77,4 +85,33 @@ public void testAutoExpandOnSingleChildThroughEvent() {
fTreeViewer.getExpandedState(trivialPathRoot.getFirstChild().getFirstChild()));
}

@Test
public void testInternalExpandToLevelRecursive() {
// internalExpandToLevel is recursive by contract, so we track all processed
// elements in a subtype to validate contractual recursive execution
List<Object> recursiveExpandedElements = new ArrayList<>();
TreeViewer viewer = new TreeViewer(fShell) {
@Override
protected void internalExpandToLevel(Widget widget, int level) {
if (widget != this.getTree()) {
recursiveExpandedElements.add(widget.getData());
}
super.internalExpandToLevel(widget, level);
}
};
TestElement rootElement = TestElement.createModel(2, 5);
viewer.setContentProvider(new TestModelContentProvider());
viewer.setInput(rootElement);

viewer.expandToLevel(AbstractTreeViewer.ALL_LEVELS);

Queue<TestElement> elements = new ConcurrentLinkedQueue<>(Arrays.asList(rootElement.getChildren()));
while (!elements.isEmpty()) {
TestElement currentElement = elements.poll();
assertTrue("expansion for child was not processed: " + currentElement,
recursiveExpandedElements.contains(currentElement));
elements.addAll(Arrays.asList(currentElement.getChildren()));
}
}

}

0 comments on commit 9bea968

Please sign in to comment.