From 51ec275aa996f7dabb3e86f9ce8179d45898fab4 Mon Sep 17 00:00:00 2001 From: ghidravore Date: Fri, 1 Nov 2019 12:48:11 -0400 Subject: [PATCH] GT-3279 moved basic tree operations back to swing thread more changes to hopefully make it more understandable changes from review and fixed issue with comments in review tool fixed checking cancelled response in loading symbol tree --- .../symboltree/nodes/OrganizationNode.java | 15 +- .../symboltree/nodes/SymbolCategoryNode.java | 2 +- .../docking/widgets/tree/CoreGTreeNode.java | 142 ++++++++++++++++-- .../main/java/docking/widgets/tree/GTree.java | 119 +++++++++------ .../java/docking/widgets/tree/GTreeNode.java | 70 ++------- .../widgets/tree/GTreeSlowLoadingNode.java | 3 +- .../widgets/tree/internal/GTreeModel.java | 115 +++++++++----- .../docking/widgets/tree/GTreeNodeTest.java | 18 +-- 8 files changed, 303 insertions(+), 181 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/OrganizationNode.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/OrganizationNode.java index eef44f14fb..e705e5c580 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/OrganizationNode.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/OrganizationNode.java @@ -49,7 +49,7 @@ public class OrganizationNode extends SymbolTreeNode { private OrganizationNode(List list, int max, int parentLevel, TaskMonitor monitor) throws CancelledException { - setChildren(computeChildren(list, max, this, parentLevel, monitor)); + doSetChildren(computeChildren(list, max, this, parentLevel, monitor)); GTreeNode child = getChild(0); baseName = child.getName().substring(0, getPrefixSizeForGrouping(getChildren(), 1) + 1); @@ -128,8 +128,7 @@ public class OrganizationNode extends SymbolTreeNode { monitor.checkCanceled(); String str = list.get(i).getName(); if (stringsDiffer(prevStr, str, characterOffset)) { - addNode(children, list, start, i - 1, maxNodes, characterOffset, - monitor); + addNode(children, list, start, i - 1, maxNodes, characterOffset, monitor); start = i; } prevStr = str; @@ -144,16 +143,14 @@ public class OrganizationNode extends SymbolTreeNode { return true; } return s1.substring(0, diffLevel + 1) - .compareToIgnoreCase( - s2.substring(0, diffLevel + 1)) != 0; + .compareToIgnoreCase(s2.substring(0, diffLevel + 1)) != 0; } private static void addNode(List children, List list, int start, int end, - int max, int diffLevel, TaskMonitor monitor) - throws CancelledException { + int max, int diffLevel, TaskMonitor monitor) throws CancelledException { if (end - start > 0) { - children.add(new OrganizationNode(list.subList(start, end + 1), max, diffLevel, - monitor)); + children.add( + new OrganizationNode(list.subList(start, end + 1), max, diffLevel, monitor)); } else { GTreeNode node = list.get(start); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/SymbolCategoryNode.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/SymbolCategoryNode.java index ac0912127f..098444f696 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/SymbolCategoryNode.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/SymbolCategoryNode.java @@ -79,8 +79,8 @@ public abstract class SymbolCategoryNode extends SymbolTreeNode { while (it.hasNext()) { Symbol s = it.next(); monitor.incrementProgress(1); + monitor.checkCanceled(); if (s != null && (s.getSymbolType() == symbolType)) { - monitor.checkCanceled(); list.add(SymbolNode.createNode(s, program)); } } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/CoreGTreeNode.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/CoreGTreeNode.java index 01d50f14a6..099e92c268 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/CoreGTreeNode.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/CoreGTreeNode.java @@ -18,8 +18,11 @@ package docking.widgets.tree; import java.util.*; import java.util.concurrent.CopyOnWriteArrayList; +import javax.swing.JTree; + import docking.widgets.tree.internal.InProgressGTreeNode; import ghidra.util.Swing; +import ghidra.util.SystemUtilities; /** * This class exists to help prevent threading errors in {@link GTreeNode} and subclasses, @@ -27,24 +30,34 @@ import ghidra.util.Swing; *

* This implementation uses a {@link CopyOnWriteArrayList} to store its children. The theory is * that this will allow direct thread-safe access to the children without having to worry about - * {@link ConcurrentModificationException}s. Also, the assumption is that accessing the children - * will occur much more frequently than modifying the children. This should only be a problem if - * a direct descendent of AbstractGTreeNode creates it children by calling + * {@link ConcurrentModificationException}s while iterating the children. Also, the assumption + * is that accessing the children will occur much more frequently than modifying the children. + * This should only be a problem if a direct descendent of GTreeNode creates its children by calling * addNode many times. But in that case, the tree should be using Lazy or * SlowLoading nodes which always load into another list first and all the children will be set * on a node in a single operation. *

- * Subclasses that need access to the children - * can call the {@link #children()} method which will ensure that the children are - * loaded (not null). Since this class uses a {@link CopyOnWriteArrayList}, subclasses that call - * the {@link #children()} method can safely operate and iterate on the list they get back without - * having to worry about getting a {@link ConcurrentModificationException}. + * Subclasses that need access to the children can call the {@link #children()} method which will + * ensure that the children are loaded (not null). Since this class uses a + * {@link CopyOnWriteArrayList}, subclasses that call the {@link #children()} method can safely + * iterate the list without having to worry about getting a {@link ConcurrentModificationException}. *

* This class uses synchronization to assure that the parent/children relationship is stable across * threads. To avoid deadlocks, the sychronization strategy is that if you have the lock on - * a parent node, you can safely acquire the lock on any of its descendants, put never its + * a parent node, you can safely acquire the lock on any of its descendants, but never its * ancestors. To facilitate this strategy, the {@link #getParent()} is not synchronized, but it * is made volatile to assure the current value is always used. + *

+ * Except for the {@link #doSetChildren(List)} method, all other calls that mutate the + * children must be called on the swing thread. The idea is that bulk operations can work efficiently + * by avoiding constantly switching to the swing thread to mutate the tree. This works because + * the bulk setting of the children generates a coarse "node structure changed" event, which causes the + * underlying {@link JTree} to rebuild its internal cache of the tree. Individual add/remove operations + * have to be done very carefully such that the {@link JTree} is always updated on one change before any + * additional changes are done. This is why those operations are required to be done on the swing + * thread, which combined with the fact that all mutate operations are synchronized, keeps the JTree + * happy. + * */ abstract class CoreGTreeNode implements Cloneable { // the parent is volatile to facilitate the synchronization strategy (see comments above) @@ -118,6 +131,25 @@ abstract class CoreGTreeNode implements Cloneable { */ protected abstract List generateChildren(); + /** + * Sets the children of this node to the given list of child nodes and fires the appropriate + * tree event to kick the tree to update the display. Note: This method must be called + * from the swing thread because it will notify the underlying JTree. + * @param childList the list of child nodes to assign as children to this node + * @see #doSetChildren(List) if calling from a background thread. + */ + protected synchronized void doSetChildrenAndFireEvent(List childList) { + doSetChildren(childList); + doFireNodeStructureChanged(); + } + + /** + * Sets the children of this node to the given list of child nodes, but does not notify the + * tree. This method does not have to be called from the swing thread. It is intended to be + * used by background threads that want to populate all or part of the tree, but wait until + * the bulk operations are completed before notifying the tree. + * @param childList the list of child nodes to assign as children to this node + */ protected synchronized void doSetChildren(List childList) { List oldChildren = children; children = null; @@ -142,6 +174,57 @@ abstract class CoreGTreeNode implements Cloneable { } } + /** + * Adds a node to this node's children. Must be called from the swing thread. + * @param node the node to add as a child to this node + */ + protected synchronized void doAddNode(GTreeNode node) { + children().add(node); + node.setParent((GTreeNode) this); + doFireNodeAdded(node); + } + + /** + * Adds a node to this node's children at the given index and notifies the tree. + * Must be called from the swing thread. + * @param index the index at which to add the new node + * @param node the node to add as a child to this node + */ + protected synchronized void doAddNode(int index, GTreeNode node) { + List kids = children(); + int insertIndex = Math.min(kids.size(), index); + kids.add(insertIndex, node); + node.setParent((GTreeNode) this); + doFireNodeAdded(node); + } + + /** + * Removes the node from this node's children and notifies the tree. Must be called + * from the swing thread. + * @param node the node to remove + */ + protected synchronized void doRemoveNode(GTreeNode node) { + List kids = children(); + int index = kids.indexOf(node); + if (index >= 0) { + kids.remove(index); + node.setParent(null); + doFireNodeRemoved(node, index); + } + } + + /** + * Adds the given nodes to this node's children. Must be called from the swing thread. + * @param nodes the nodes to add to the children this node + */ + protected synchronized void doAddNodes(List nodes) { + for (GTreeNode node : nodes) { + node.setParent((GTreeNode) this); + } + children().addAll(nodes); + doFireNodeStructureChanged(); + } + /** * Creates a clone of this node. The clone should contain a shallow copy of all the node's * attributes except that the parent and children are null. @@ -157,7 +240,6 @@ abstract class CoreGTreeNode implements Cloneable { } public void dispose() { - List oldChildren; synchronized (this) { oldChildren = children; @@ -242,4 +324,44 @@ abstract class CoreGTreeNode implements Cloneable { return false; } + protected void doFireNodeAdded(GTreeNode newNode) { + assertSwing(); + GTree tree = getTree(); + if (tree != null) { + tree.getModel().fireNodeAdded((GTreeNode) this, newNode); + tree.refilterLater(newNode); + } + } + + protected void doFireNodeRemoved(GTreeNode removedNode, int index) { + assertSwing(); + GTree tree = getTree(); + if (tree != null) { + tree.getModel().fireNodeRemoved((GTreeNode) this, removedNode, index); + } + } + + protected void doFireNodeStructureChanged() { + assertSwing(); + GTree tree = getTree(); + if (tree != null) { + tree.getModel().fireNodeStructureChanged((GTreeNode) this); + tree.refilterLater(); + } + } + + protected void doFireNodeChanged() { + assertSwing(); + GTree tree = getTree(); + if (tree != null) { + tree.getModel().fireNodeDataChanged((GTreeNode) this); + tree.refilterLater(); + } + } + + private void assertSwing() { + SystemUtilities + .assertThisIsTheSwingThread("tree events must be called from the AWT thread"); + } + } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTree.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTree.java index 2f876131cd..6a64e00b09 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTree.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTree.java @@ -25,6 +25,7 @@ import java.awt.event.MouseListener; import java.io.PrintWriter; import java.util.*; import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import javax.swing.*; @@ -40,8 +41,8 @@ import docking.widgets.tree.internal.*; import docking.widgets.tree.support.*; import docking.widgets.tree.support.GTreeSelectionEvent.EventOrigin; import docking.widgets.tree.tasks.*; -import ghidra.util.FilterTransformer; -import ghidra.util.SystemUtilities; +import ghidra.util.*; +import ghidra.util.exception.AssertException; import ghidra.util.exception.CancelledException; import ghidra.util.task.*; import ghidra.util.worker.PriorityWorker; @@ -56,13 +57,20 @@ public class GTree extends JPanel implements BusyListener { private GTreeModel model; /** - * This is the root node that either is the actual current root node, or the node that will - * be the real root node, once the Worker has loaded it. Thus, it is possible that a call to - * {@link GTreeModel#getRoot()} will return an {@link InProgressGTreeRootNode}. By keeping - * this variable around, we can give this node to clients, regardless of the root node - * visible in the tree. + * This is the root node of the tree's data model. It may or may not be the root node + * that is currently being displayed by the tree. If there is currently a + * filter applied, then then the displayed root node will be a clone whose children have been + * trimmed to only those that match the filter. By keeping this variable around, we can give + * this node to clients, regardless of the root node visible in the tree. */ - private GTreeNode realRootNode; + private volatile GTreeNode realModelRootNode; + + /** + * This is the root that is currently being displayed. This node will be either exactly the + * same instance as the realModelRootNode (if no filter has been applied) or it will be the + * filtered clone of the realModelRootNode. + */ + private volatile GTreeNode realViewRootNode; /** * The rootParent is a node that is assigned as the parent to the realRootNode. It's primary purpose is @@ -108,7 +116,8 @@ public class GTree extends JPanel implements BusyListener { */ public GTree(GTreeNode root) { uniquePreferenceKey = generateFilterPreferenceKey(); - this.realRootNode = root; + this.realModelRootNode = root; + this.realViewRootNode = root; monitor = new TaskMonitorComponent(); monitor.setShowProgressValue(false);// the tree's progress is fabricated--don't paint it worker = new PriorityWorker("GTree Worker", monitor); @@ -265,7 +274,8 @@ public class GTree extends JPanel implements BusyListener { if (root != null) { root.dispose(); } - realRootNode.dispose();// just in case we were loading + realModelRootNode.dispose(); + realViewRootNode.dispose(); model.dispose(); } @@ -728,57 +738,66 @@ public class GTree extends JPanel implements BusyListener { * @param rootNode The node to set. */ public void setRootNode(GTreeNode rootNode) { - GTreeNode oldRoot = doSetRootNode(rootNode, true); - oldRoot.dispose(); - if (filter != null) { - filterUpdateManager.update(); - } - } - - private GTreeNode doSetRootNode(GTreeNode rootNode, boolean waitForJobs) { worker.clearAllJobs(); - GTreeNode root = model.getModelRoot(); - - this.realRootNode = rootNode; rootNode.setParent(rootParent); - - // - // We need to use our standard 'worker pipeline' for mutations to the tree. This means - // that requests from the Swing thread must go through the worker. However, - // non-Swing-thread requests can just block while we wait for cancelled work to finish - // and setup the new root. The assumption is that other threads (like test threads and - // client background threads) will want to block in order to get real-time data. Further, - // since they are not in the Swing thread, blocking will not lock-up the GUI. - // - if (SwingUtilities.isEventDispatchThread()) { - model.setRootNode(new InProgressGTreeRootNode()); - runTask(new SetRootNodeTask(this, rootNode, model)); - } - else { - if (waitForJobs) { - worker.waitUntilNoJobsScheduled(Integer.MAX_VALUE); + realModelRootNode = rootNode; + realViewRootNode = rootNode; + GTreeNode oldRoot; + try { + oldRoot = doSetModelRootNode(rootNode); + oldRoot.dispose(); + if (filter != null) { + filterUpdateManager.update(); } - monitor.clearCanceled(); - model.setRootNode(rootNode); } - return root; + catch (CancelledException e) { + throw new AssertException("Setting the root node should never be cancelled"); + } } void setFilteredRootNode(GTreeNode filteredRootNode) { filteredRootNode.setParent(rootParent); - GTreeNode currentRoot = (GTreeNode) model.getRoot(); - model.setRootNode(filteredRootNode); - if (currentRoot != realRootNode) { - currentRoot.disposeClones(); + realViewRootNode = filteredRootNode; + try { + GTreeNode currentRoot = doSetModelRootNode(filteredRootNode); + if (currentRoot != realModelRootNode) { + currentRoot.disposeClones(); + } + } + catch (CancelledException e) { + // the filter task was cancelled } } void restoreNonFilteredRootNode() { - GTreeNode currentRoot = (GTreeNode) model.getRoot(); - model.setRootNode(realRootNode); - if (currentRoot != realRootNode) { - currentRoot.disposeClones(); + realViewRootNode = realModelRootNode; + try { + GTreeNode currentRoot = doSetModelRootNode(realModelRootNode); + if (currentRoot != realModelRootNode) { + currentRoot.disposeClones(); + } } + catch (CancelledException e) { + // the filter task was cancelled + } + } + + private GTreeNode doSetModelRootNode(GTreeNode rootNode) throws CancelledException { + // If this method is called from a background filter task, then it may be cancelled + // by other tree operations. Not all tasks can be cancelled. + AtomicBoolean wasCancelled = new AtomicBoolean(true); + GTreeNode node = Swing.runNow(() -> { + GTreeNode old = model.getModelRoot(); + model.setRootNode(rootNode); + + wasCancelled.set(false); + return old; + }); + + if (wasCancelled.get()) { + throw new CancelledException(); + } + return node; } /** @@ -790,7 +809,7 @@ public class GTree extends JPanel implements BusyListener { * @return the root node as provided by the client. */ public GTreeNode getModelRoot() { - return realRootNode; + return realModelRootNode; } /** @@ -801,7 +820,7 @@ public class GTree extends JPanel implements BusyListener { * @return the root node currently being display by the {@link JTree} */ public GTreeNode getViewRoot() { - return (GTreeNode) model.getRoot(); + return realViewRootNode; } /** diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeNode.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeNode.java index 8944dd9875..618d22d7b9 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeNode.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeNode.java @@ -33,6 +33,9 @@ import util.CollectionUtils; * all their children in hand when initially constructed (either in their constructor or externally * using {@link #addNode(GTreeNode)} or {@link #setChildren(List)}. For large trees, subclasses * should instead extend {@link GTreeLazyNode} or {@link GTreeSlowLoadingNode} + *

+ * All methods in this class that mutate the children node must perform that operation in + * the swing thread. */ public abstract class GTreeNode extends CoreGTreeNode implements Comparable { private static AtomicLong NEXT_ID = new AtomicLong(); @@ -85,9 +88,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable doAddNode(node)); } /** @@ -95,11 +96,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable nodes) { - for (GTreeNode node : nodes) { - node.setParent(this); - } - children().addAll(nodes); - fireNodeStructureChanged(this); + Swing.runNow(() -> doAddNodes(nodes)); } /** @@ -108,9 +105,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable doAddNode(index, node)); } /** @@ -216,17 +211,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable children = children(); - if (children != null) { - for (GTreeNode child : children) { - child.dispose(); - } - children().clear(); - fireNodeStructureChanged(this); - } + Swing.runNow(() -> doSetChildrenAndFireEvent(null)); } /** @@ -234,14 +219,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable children = children(); - if (children.remove(node)) { - node.setParent(null); - fireNodeRemoved(this, node); - } + Swing.runNow(() -> doRemoveNode(node)); } /** @@ -249,8 +227,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable childList) { - doSetChildren(childList); - fireNodeStructureChanged(this); + Swing.runNow(() -> doSetChildrenAndFireEvent(childList)); } /** @@ -430,11 +407,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable tree.getModel().fireNodeStructureChanged(node)); - tree.refilterLater(); - } + Swing.runNow(() -> doFireNodeStructureChanged()); } /** @@ -443,28 +416,7 @@ public abstract class GTreeNode extends CoreGTreeNode implements Comparable tree.getModel().fireNodeDataChanged(parentNode, node)); - tree.refilterLater(); - } - } - - protected void fireNodeAdded(GTreeNode parentNode, GTreeNode newNode) { - GTree tree = getTree(); - if (tree != null) { - Swing.runNow(() -> tree.getModel().fireNodeAdded(parentNode, newNode)); - tree.refilterLater(newNode); - } - } - - protected void fireNodeRemoved(GTreeNode parentNode, GTreeNode removedNode) { - - GTree tree = getTree(); - if (tree != null) { - Swing.runNow( - () -> tree.getModel().fireNodeRemoved(parentNode, removedNode)); - } + Swing.runNow(() -> doFireNodeChanged()); } private GTreeNode[] getPathToRoot(GTreeNode node, int depth) { diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeSlowLoadingNode.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeSlowLoadingNode.java index 73dbb0accb..85683f44ba 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeSlowLoadingNode.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeSlowLoadingNode.java @@ -37,8 +37,7 @@ public abstract class GTreeSlowLoadingNode extends GTreeLazyNode { * @return the list of children for this node. * @throws CancelledException if the monitor is cancelled */ - public abstract List generateChildren(TaskMonitor monitor) - throws CancelledException; + public abstract List generateChildren(TaskMonitor monitor) throws CancelledException; @Override protected final List generateChildren() { diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/internal/GTreeModel.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/internal/GTreeModel.java index f8f8497f60..a147e89f77 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/internal/GTreeModel.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/internal/GTreeModel.java @@ -25,11 +25,12 @@ import javax.swing.tree.TreePath; import docking.widgets.tree.GTree; import docking.widgets.tree.GTreeNode; +import ghidra.util.Swing; import ghidra.util.SystemUtilities; public class GTreeModel implements TreeModel { - private GTreeNode root; + private volatile GTreeNode root; private List listeners = new ArrayList(); private boolean isFiringNodeStructureChanged; private volatile boolean eventsEnabled = true; @@ -76,8 +77,14 @@ public class GTreeModel implements TreeModel { return gTreeParent.getChild(index); } catch (IndexOutOfBoundsException e) { - // children must have be changed outside of swing thread, should get another event - // to fix things up, so just return an in-progress node + // This can happen if the client code mutates the children of this node in a background + // thread such that there are fewer child nodes on this node, and then before the tree + // is notified, the JTree attempts to access a child that is no longer present. The + // GTree design specifically allows this situation to occur as a trade off for + // better performance when performing bulk operations (such as filtering). If this + // does occur, this can be handled easily by temporarily returning a dummy node and + // scheduling a node structure changed event to reset the JTree. + Swing.runLater(() -> fireNodeStructureChanged((GTreeNode) parent)); return new InProgressGTreeNode(); } } @@ -121,9 +128,10 @@ public class GTreeModel implements TreeModel { } if (node != changedNode) { node.setChildren(null); + return; // the previous call will generate the proper event, so bail } - TreeModelEvent event = new TreeModelEvent(this, changedNode.getTreePath()); + TreeModelEvent event = new TreeModelEvent(this, node.getTreePath()); for (TreeModelListener listener : listeners) { listener.treeStructureChanged(event); } @@ -137,43 +145,23 @@ public class GTreeModel implements TreeModel { if (!eventsEnabled) { return; } - SystemUtilities.runIfSwingOrPostSwingLater(new Runnable() { - @Override - public void run() { - GTreeNode rootNode = root; - if (rootNode != null) { - fireNodeStructureChanged(root); - } + Swing.runIfSwingOrRunLater(() -> { + GTreeNode rootNode = root; + if (rootNode != null) { + fireNodeStructureChanged(rootNode); } }); } - public void fireNodeDataChanged(final GTreeNode parentNode, final GTreeNode changedNode) { + public void fireNodeDataChanged(GTreeNode changedNode) { if (!eventsEnabled) { return; } SystemUtilities.assertThisIsTheSwingThread( "GTreeModel.fireNodeDataChanged() must be " + "called from the AWT thread"); - TreeModelEvent event; - if (parentNode == null) { // special case when root node changes. - event = new TreeModelEvent(this, root.getTreePath(), null, null); - } - else { - GTreeNode node = convertToViewNode(changedNode); - if (node == null) { - return; - } + TreeModelEvent event = getChangedNodeEvent(changedNode); - int indexInParent = node.getIndexInParent(); - if (indexInParent < 0) { - return; - } - event = - new TreeModelEvent(this, node.getParent().getTreePath(), - new int[] { indexInParent }, - new Object[] { changedNode }); - } for (TreeModelListener listener : listeners) { listener.treeNodesChanged(event); } @@ -186,33 +174,49 @@ public class GTreeModel implements TreeModel { SystemUtilities.assertThisIsTheSwingThread( "GTreeModel.fireNodeAdded() must be " + "called from the AWT thread"); - GTreeNode node = convertToViewNode(parentNode); - if (node == null) { + GTreeNode parent = convertToViewNode(parentNode); + if (parent == null) { // it will be null if filtered out return; } - TreeModelEvent event = new TreeModelEvent(this, node.getTreePath()); + int index = parent.getIndexOfChild(newNode); + if (index < 0) { + // the index will be -1 if filtered out + return; + } + + TreeModelEvent event = new TreeModelEvent(this, parent.getTreePath(), new int[] { index }, + new Object[] { newNode }); for (TreeModelListener listener : listeners) { - listener.treeStructureChanged(event); + listener.treeNodesInserted(event); } } - public void fireNodeRemoved(final GTreeNode parentNode, final GTreeNode removedNode) { + public void fireNodeRemoved(GTreeNode parentNode, GTreeNode removedNode, int index) { SystemUtilities.assertThisIsTheSwingThread( "GTreeModel.fireNodeRemoved() must be " + "called from the AWT thread"); - GTreeNode node = convertToViewNode(parentNode); - if (node == null) { + GTreeNode parent = convertToViewNode(parentNode); + if (parent == null) { // will be null if filtered out return; } - if (node != parentNode) { - node.removeNode(removedNode); + + // if filtered, remove filtered node + if (parent != parentNode) { + index = removeFromFiltered(parent, removedNode); + return; // the above call will generate the event for the filtered node } - TreeModelEvent event = new TreeModelEvent(this, node.getTreePath()); + if (index < 0) { // will be -1 if filtered out + return; + } + + TreeModelEvent event = new TreeModelEvent(this, parent.getTreePath(), new int[] { index }, + new Object[] { removedNode }); + for (TreeModelListener listener : listeners) { - listener.treeStructureChanged(event); + listener.treeNodesRemoved(event); } } @@ -224,6 +228,27 @@ public class GTreeModel implements TreeModel { eventsEnabled = b; } + private TreeModelEvent getChangedNodeEvent(GTreeNode changedNode) { + GTreeNode parentNode = changedNode.getParent(); + if (parentNode == null) { // tree requires different event form when it is the root that changes + return new TreeModelEvent(this, root.getTreePath(), null, null); + } + + GTreeNode node = convertToViewNode(changedNode); + if (node == null) { + return null; + } + + int indexInParent = node.getIndexInParent(); + if (indexInParent < 0) { + return null; + } + + return new TreeModelEvent(this, node.getParent().getTreePath(), new int[] { indexInParent }, + new Object[] { changedNode }); + + } + private GTreeNode convertToViewNode(GTreeNode node) { if (node.getRoot() == root) { return node; @@ -234,4 +259,12 @@ public class GTreeModel implements TreeModel { } return null; } + + private int removeFromFiltered(GTreeNode parent, GTreeNode removedNode) { + int index = parent.getIndexOfChild(removedNode); + if (index >= 0) { + parent.removeNode(removedNode); + } + return index; + } } diff --git a/Ghidra/Framework/Docking/src/test/java/docking/widgets/tree/GTreeNodeTest.java b/Ghidra/Framework/Docking/src/test/java/docking/widgets/tree/GTreeNodeTest.java index 3db7274b2d..3f684871ff 100644 --- a/Ghidra/Framework/Docking/src/test/java/docking/widgets/tree/GTreeNodeTest.java +++ b/Ghidra/Framework/Docking/src/test/java/docking/widgets/tree/GTreeNodeTest.java @@ -285,7 +285,7 @@ public class GTreeNodeTest { assertEquals(1, events.size()); TestEvent event = events.get(0); assertEquals(EventType.NODE_REMOVED, event.type); - assertEquals(root, event.node); + assertEquals(root, event.parent); } @Test @@ -432,23 +432,23 @@ public class GTreeNodeTest { } @Override - public void fireNodeStructureChanged(GTreeNode node) { - events.add(new TestEvent(EventType.STRUCTURE_CHANGED, null, node, -1)); + public void doFireNodeStructureChanged() { + events.add(new TestEvent(EventType.STRUCTURE_CHANGED, null, this, -1)); } @Override - public void fireNodeChanged(GTreeNode parent, GTreeNode node) { - events.add(new TestEvent(EventType.NODE_CHANGED, parent, node, -1)); + public void doFireNodeChanged() { + events.add(new TestEvent(EventType.NODE_CHANGED, getParent(), this, -1)); } @Override - protected void fireNodeAdded(GTreeNode parentNode, GTreeNode newNode) { - events.add(new TestEvent(EventType.NODE_ADDED, parentNode, newNode, -1)); + protected void doFireNodeAdded(GTreeNode newNode) { + events.add(new TestEvent(EventType.NODE_ADDED, this, newNode, -1)); } @Override - protected void fireNodeRemoved(GTreeNode parentNode, GTreeNode removedNode) { - events.add(new TestEvent(EventType.NODE_REMOVED, null, parentNode, -1)); + protected void doFireNodeRemoved(GTreeNode removedNode, int index) { + events.add(new TestEvent(EventType.NODE_REMOVED, this, removedNode, -1)); } }