Skip to content

Commit

Permalink
Fix tree node removal while filter is active deleting the entire tree
Browse files Browse the repository at this point in the history
  • Loading branch information
Col-E committed Nov 2, 2024
1 parent 923a298 commit 16b3b5a
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ protected FilterableTreeItem() {
setUnderlyingChildren(filteredChildren);
}

/**
* @return Predicate property.
*/
@Nonnull
public ObjectProperty<Predicate<TreeItem<T>>> predicateProperty() {
return predicate;
}

@Override
@Deprecated(since = "Use FilterableTreeItem dedicated methods for interacting with children")
public ObservableList<TreeItem<T>> getChildren() {
Expand Down Expand Up @@ -210,11 +218,10 @@ public <I extends TreeItem<T>> void sortChildren(@Nonnull Comparator<I> comparat
}

/**
* @return Predicate property.
* @return {@code true} when this tree item is a true leaf based on the unfiltered children.
*/
@Nonnull
public ObjectProperty<Predicate<TreeItem<T>>> predicateProperty() {
return predicate;
protected boolean isUnfilteredLeaf() {
return sourceChildren.isEmpty();
}

static {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import software.coley.recaf.path.BundlePathNode;
import software.coley.recaf.path.DirectoryPathNode;
import software.coley.recaf.path.PathNode;
import software.coley.recaf.util.StringUtil;

/**
* Tree item subtype for more convenience tree building operations.
Expand Down Expand Up @@ -43,7 +44,7 @@ public synchronized boolean removeNodeByPath(@Nonnull PathNode<?> path) {
WorkspaceTreeNode parentNode = nodeByPath.getParentNode();
if (parentNode != null) {
boolean removed = parentNode.removeSourceChild(nodeByPath);
while (parentNode.isLeaf() && parentNode.getParentNode() != null) {
while (parentNode.isUnfilteredLeaf() && parentNode.getParentNode() != null) {
WorkspaceTreeNode parentOfParent = parentNode.getParentNode();
parentOfParent.removeSourceChild(parentNode);
parentNode = parentOfParent;
Expand Down Expand Up @@ -151,6 +152,26 @@ public String toString() {
return getClass().getSimpleName() + "[" + getValue().toString() + "]";
}

/**
* A debugging util to inspect the state of the tree without having to dig through
* the actual references in the debugger.
*
* @return String representation of this tree and all of its descendants.
*/
@Nonnull
public String printTree() {
StringBuilder sb = new StringBuilder(toString());
for (TreeItem<PathNode<?>> child : getSourceChildren()) {
if (child instanceof WorkspaceTreeNode childNode) {
String childTree = childNode.printTree();
for (String childTreeEntry : StringUtil.fastSplit(childTree, false, '\n')) {
sb.append("\n ").append(childTreeEntry);
}
}
}
return sb.toString();
}

/**
* Get/insert a {@link WorkspaceTreeNode} holding the given {@link PathNode} from/to the tree model.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,30 @@ void removeWhileFilteredStillUpdatesChildren() {
assertTrue(workspaceNode.removeNodeByPath(p1a), "Class could not be removed");
assertNull(workspaceNode.getNodeByPath(p1a), "Class accessible after removed");
}

@Test
void removeWhileFilteredDoesNotEliminateOtherClasses() {
// Create workspace root and insert the class, which should generate all paths between the class and the workspace node.
WorkspaceTreeNode workspaceNode = new WorkspaceTreeNode(p5);
WorkspaceTreeNode classNodeA = getOrInsertIntoTree(workspaceNode, p1a);
WorkspaceTreeNode classNodeB = getOrInsertIntoTree(workspaceNode, p1b);
WorkspaceTreeNode classNodeC = getOrInsertIntoTree(workspaceNode, p1c);
assertNotNull(classNodeA, "Class not yielded by original assert");
assertNotNull(classNodeB, "Class not yielded by original assert");
assertNotNull(classNodeC, "Class not yielded by original assert");
assertNotNull(workspaceNode.getNodeByPath(p1a), "Could not get class after setup");
assertNotNull(workspaceNode.getNodeByPath(p1b), "Could not get class after setup");
assertNotNull(workspaceNode.getNodeByPath(p1c), "Could not get class after setup");

// Apply a filter so that nothing is shown.
workspaceNode.predicateProperty().set(p -> false);

// Remove 'Class A' and verify 'Class B' and 'Class C' are still accessible
assertTrue(workspaceNode.removeNodeByPath(p1a), "Could not remove class after filtering");
assertNull(workspaceNode.getNodeByPath(p1a), "Class A accessible after removed");
assertNotNull(workspaceNode.getNodeByPath(p1b), "Class B not accessible after adjacent leaf removed");
assertNotNull(workspaceNode.getNodeByPath(p1c), "Class C not accessible after adjacent leaf removed");
}
}

@Nested
Expand Down

0 comments on commit 16b3b5a

Please sign in to comment.