Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overload method AbstractTreeViewer::internalConditionalExpandToLevel #2692

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Overload method AbstractTreeViewer::internalConditionalExpandToLevel #2692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.function.Predicate;

import org.eclipse.core.runtime.Assert;
import org.eclipse.core.runtime.IStatus;
Expand Down Expand Up @@ -139,6 +140,8 @@ public abstract class AbstractTreeViewer extends ColumnViewer {
*/
private boolean isTreePathContentProvider = false;

private Predicate<Item> shouldItemExpand;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is to be a field that has a meaning across method calls, it should, e.g., be filled during intialization. If this value is only relevant during a call of expandToLevel (or other) method (and thus reinitialized in every call of that method), it should not be a field, as it is specific to the execution time of that method.
You see the issues with this in the currently failing tests: there are other callers of internalCustomizedExpandToLevel, which accesses this field without it being initialized.

So I see two options:

  • This predicate needs to be created within the method it is valid in and passed through the methods called from there, if necessary
  • This precidate may be initialized at object initialization with a proper invariant holding for it during the object lifecycle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initialized it in the constructor too to avoid NPEs.

The tricky part is passing it all the way down through the method calls until it reaches internalCustomizedExpandToLevel without adding an extra parameter (or overloading) internalExpandToLevel(Widget widget, int level). Which is why I was only able to support the use cases where the call comes through expandToLevel(...).

Do you see any better way to do it? I couldn't come up with any.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still means you are abusing a field of class to actually pass a method parameter. Fields of classes need to have a proper (or at least a reasonable) invariant they fulfill, but this one actually has not reasonable invariant (apart from "state x regarding last execution of y") as it is actually a method parameter.

I understand that changing the method signature is not easy/possible, but in my opinion solving this by abusing a field as a method parameter is an error prone design flaw. If you need to reinitialize the shouldItemExpand in some of the methods calling internalCustomizedExpandToLevel, why not in the others? Is there potentially some state in the predicate delivered by createShouldItemExpand? Then that one will usually be invalid while executing internalCustomizedExpandToLevel without having it properly initialized.

Wouldn't it be possible to achieve the same without any extension to AbstractTreeViewer by simply implementing the according subtype with an overwrittten expandToLevel, adding the predicate creation at the beginning of the method, and then consuming it in an overwritten setExpanded?

Copy link
Contributor Author

@fedejeanne fedejeanne Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consuming it in setExpanded won't work because the execution needs to be stopped in its consumer (internalCustomizedExpandToLevel) which can only be achieved by changing its return value to boolean (API change, since it's a protected method) and doing an early return.

I've been looking at other subclasses of AbstractTreeViewer and I noticed that they end up hacking its way into the expansion mechanism by overriding methods. I think this may be a consequence of the little flexibility that the class offers. How about adding hook 2 methods that will be called only once right before and after the expansion runs? This would give the consumer/subclasses the chance to cleanly preserve some state outside AbstractTreeViewer and hopefully avoid future hacks.


/**
* Safe runnable used to update an item.
*/
Expand Down Expand Up @@ -166,6 +169,7 @@ public void run() {
*/
protected AbstractTreeViewer() {
// do nothing
this.shouldItemExpand = createShouldItemExpand();
}

/**
Expand Down Expand Up @@ -1162,6 +1166,7 @@ public void expandToLevel(Object elementOrTreePath, int level, boolean disableRe
control.setRedraw(false);
}
Widget w = internalExpand(elementOrTreePath, true);
shouldItemExpand = createShouldItemExpand();
if (w != null) {
internalExpandToLevel(w, level);
}
Expand All @@ -1172,6 +1177,17 @@ public void expandToLevel(Object elementOrTreePath, int level, boolean disableRe
}
}

/**
* @return the predicate. It should return {@code true} if the received
* {@code Item} should be expanded when expanding the whole tree and
* {@code false} otherwise.
*
* @since 3.36
*/
protected Predicate<Item> createShouldItemExpand() {
return w -> true;
}

/**
* Fires a tree collapsed event. Only listeners registered at the time this
* method is called are notified.
Expand Down Expand Up @@ -1856,6 +1872,9 @@ private void internalCustomizedExpandToLevel(Widget widget, int level,
createChildren(widget, false);
// XXX for performance widget should be expanded after expanding children:
if (widget instanceof Item it) {
if (!shouldItemExpand.test(it)) {
return;
}
setExpanded(it, true);
}
if (level == ALL_LEVELS || level > 1) {
Expand Down
46 changes: 46 additions & 0 deletions tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/TreeViewerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*******************************************************************************/
package org.eclipse.jface.tests.viewers;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

Expand All @@ -21,13 +22,15 @@
import java.util.List;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.function.Predicate;

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.Item;
import org.eclipse.swt.widgets.Tree;
import org.eclipse.swt.widgets.TreeItem;
import org.eclipse.swt.widgets.Widget;
Expand Down Expand Up @@ -114,4 +117,47 @@ protected void internalExpandToLevel(Widget widget, int level) {
}
}

@Test
public void testExpandOnlyArbitraryChild() {

TestElement modelRoot = TestElement.createModel(3, 5);

// select an arbitrary child to be expanded
TestElement childToBeExpanded = modelRoot.getChildAt(modelRoot.getChildCount() - 1);

TreeViewer viewer = new TreeViewer(fShell) {
@Override
protected Predicate<Item> createShouldItemExpand() {
// Expand only "childToBeExpanded"
return it -> {
if (!(it.getData() instanceof TestElement)) {
throw new RuntimeException("Data: " + it.getData());
}

return it.getData() instanceof TestElement d && d.equals(childToBeExpanded);
};
}
};

viewer.setContentProvider(new TestModelContentProvider());
viewer.setInput(modelRoot);

// Even when expanding all elements, the provided predicate will take precedence
viewer.expandToLevel(AbstractTreeViewer.ALL_LEVELS);

Queue<TestElement> elements = new ConcurrentLinkedQueue<>(Arrays.asList(modelRoot.getChildren()));
while (!elements.isEmpty()) {
TestElement currentElement = elements.poll();

boolean shouldExpand = currentElement.equals(childToBeExpanded);

assertEquals("The expanded state of the element " + currentElement + " is wrong", shouldExpand,
viewer.getExpandedState(currentElement));
elements.addAll(Arrays.asList(currentElement.getChildren()));
}

// Double check that the desired element is there and that it was expanded
assertTrue("The element " + childToBeExpanded + " should have been expanded",
viewer.getExpandedState(childToBeExpanded));
}
}
Loading