Merge remote-tracking branch 'origin/GT-3323-dragonmacher-gtree-restore-state'

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTree.java

@@ -33,6 +33,8 @@ import javax.swing.Timer;
 import javax.swing.event.*;
 import javax.swing.tree.*;
 
+import org.apache.commons.lang3.StringUtils;
+
 import docking.DockingWindowManager;
 import docking.widgets.JTreeMouseListenerDelegate;
 import docking.widgets.filter.FilterTextField;
@@ -88,7 +90,6 @@ public class GTree extends JPanel implements BusyListener {
 	private JTreeMouseListenerDelegate mouseListenerDelegate;
 	private GTreeDragNDropHandler dragNDropHandler;
 	private boolean isFilteringEnabled = true;
-	private boolean hasFilterText = false;
 
 	private AtomicLong modificationID = new AtomicLong();
 	private ThreadLocal<TaskMonitor> threadLocalMonitor = new ThreadLocal<>();
@@ -100,7 +101,7 @@ public class GTree extends JPanel implements BusyListener {
 
 	private JPanel mainPanel;
 
-	private GTreeState restoreTreeState;
+	private GTreeState filterRestoreTreeState;
 	private GTreeFilterTask lastFilterTask;
 	private String uniquePreferenceKey;
 
@@ -225,7 +226,6 @@ public class GTree extends JPanel implements BusyListener {
 				// don't care
 			}
 		});
-		model.addTreeModelListener(new FilteredExpansionListener());
 
 		tree = new AutoScrollTree(model);
 		tree.setRowHeight(-1);// variable size rows
@@ -249,7 +249,7 @@ public class GTree extends JPanel implements BusyListener {
 		addGTreeSelectionListener(e -> {
 			if (e.getEventOrigin() == GTreeSelectionEvent.EventOrigin.USER_GENERATED ||
 				e.getEventOrigin() == GTreeSelectionEvent.EventOrigin.API_GENERATED) {
-				restoreTreeState = getTreeState();
+				filterRestoreTreeState = getTreeState();
 			}
 		});
 
@@ -310,15 +310,10 @@ public class GTree extends JPanel implements BusyListener {
 		}
 	}
 
-// TODO: doc on how to override to extend listener stuff
 	protected JTreeMouseListenerDelegate createMouseListenerDelegate() {
 		return new GTreeMouseListenerDelegate(tree, this);
 	}
 
-	public GTreeState getRestoreTreeState() {
-		return restoreTreeState;
-	}
-
 	/**
 	 * Returns a state object that allows this tree to later restore its expanded and selected
 	 * state.
@@ -351,6 +346,25 @@ public class GTree extends JPanel implements BusyListener {
 		runTask(new GTreeRestoreTreeStateTask(this, state));
 	}
 
+	/**
+	 * Signal to the tree that it should record its expanded and selected state when a 
+	 * new filter is applied
+	 */
+	void saveFilterRestoreState() {
+		// this may be called by sub-filter tasks and we wish to save only the first one
+		if (filterRestoreTreeState == null) {
+			filterRestoreTreeState = new GTreeState(this);
+		}
+	}
+
+	GTreeState getFilterRestoreState() {
+		return filterRestoreTreeState;
+	}
+
+	void clearFilterRestoreState() {
+		filterRestoreTreeState = null;
+	}
+
 	/**
 	 * A method that subclasses can use to be notified when tree state has been restored.  This
 	 * method is called after a major structural tree change has happened <b>and</b> the paths
@@ -646,10 +660,6 @@ public class GTree extends JPanel implements BusyListener {
 		updateModelFilter();
 	}
 
-	public String getFilterText() {
-		return filterProvider.getFilterText();
-	}
-
 	/**
 	 * Disabled the filter text field, but allows the tree to still filter.  This is useful if
 	 * you want to allow programmatic filtering, but to not allow the user to filter.
@@ -1121,8 +1131,16 @@ public class GTree extends JPanel implements BusyListener {
 		return filter;
 	}
 
+	public boolean isFiltered() {
+		return filter != null;
+	}
+
 	public boolean hasFilterText() {
-		return hasFilterText;
+		return !StringUtils.isBlank(filterProvider.getFilterText());
+	}
+
+	public String getFilterText() {
+		return filterProvider.getFilterText();
 	}
 
 	public void clearFilter() {
@@ -1170,10 +1188,6 @@ public class GTree extends JPanel implements BusyListener {
 		// for now only subclasses of GTree will set a node editable.
 	}
 
-	public boolean isFiltered() {
-		return filter != null;
-	}
-
 	@Override
 	public String toString() {
 		GTreeNode rootNode = getModelRoot();
@@ -1340,74 +1354,6 @@ public class GTree extends JPanel implements BusyListener {
 		}
 	}
 
-	/**
-	 * Listens for changes to nodes in the tree to refilter and expand nodes as they are changed.
-	 * We do this work here in the GTree, as opposed to doing it inside the GTreeNode, since the
-	 * GTree can buffer requests and trigger the work to happen in tasks.  If the work was done
-	 * in the nodes, then long running operations could block the Swing thread.
-	 */
-	private class FilteredExpansionListener implements TreeModelListener {
-
-		/**
-		 * We need this method to handle opening newly added tree nodes.  The GTreeNode will
-		 * properly handle filtering for us in this case, but it does not expand nodes, as this
-		 * is usually done in a separate task after the normal filtering process.
-		 */
-		@Override
-		public void treeNodesInserted(TreeModelEvent e) {
-			if (!hasFilterText) {
-				return;
-			}
-
-			Object[] children = e.getChildren();
-			for (Object child : children) {
-				GTreeNode node = (GTreeNode) child;
-				expandTree(node);
-			}
-		}
-
-		/**
-		 * We need this method to handle major tree changes that bypass the add and remove system
-		 * of the GTreeNode.
-		 */
-		@Override
-		public void treeStructureChanged(TreeModelEvent e) {
-			if (!hasFilterText) {
-				return;
-			}
-
-			Object lastPathComponent = e.getTreePath().getLastPathComponent();
-			GTreeNode node = (GTreeNode) lastPathComponent;
-			maybeTriggerUpdateForNode(node);
-		}
-
-		private void maybeTriggerUpdateForNode(GTreeNode node) {
-			if ((node instanceof InProgressGTreeNode) ||
-				(node instanceof InProgressGTreeRootNode)) {
-				return;
-			}
-
-			// root structure changes imply that we are being rebuilt/refiltered and those tasks
-			// are the result of an update, so there is nothing to do in that case
-			if (node == model.getModelRoot()) {
-				return;
-			}
-
-			updateModelFilter();
-		}
-
-		@Override
-		public void treeNodesChanged(TreeModelEvent e) {
-			// this is handled by the GTreeNode internally via adds and removes
-		}
-
-		@Override
-		public void treeNodesRemoved(TreeModelEvent e) {
-			// currently, the GTreeNode handles adds and removes on its own and updates the
-			// model accordingly
-		}
-	}
-
 	private class GTreeMouseListenerDelegate extends JTreeMouseListenerDelegate {
 		private final GTree gTree;
 

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeFilterTask.java

@@ -16,7 +16,8 @@
 package docking.widgets.tree;
 
 import docking.widgets.tree.support.GTreeFilter;
-import docking.widgets.tree.tasks.*;
+import docking.widgets.tree.tasks.GTreeClearTreeFilterTask;
+import docking.widgets.tree.tasks.GTreeExpandAllTask;
 import ghidra.util.Msg;
 import ghidra.util.exception.CancelledException;
 import ghidra.util.task.TaskMonitor;
@@ -24,15 +25,13 @@ import ghidra.util.task.TaskMonitor;
 public class GTreeFilterTask extends GTreeTask {
 
 	private final GTreeFilter filter;
-	private final GTreeState defaultRestoreState;
 	private boolean cancelledProgramatically;
 
 	public GTreeFilterTask(GTree tree, GTreeFilter filter) {
 		super(tree);
 		this.filter = filter;
 
-		// save this now, before we modify the tree
-		defaultRestoreState = tree.getTreeState();
+		tree.saveFilterRestoreState();
 	}
 
 	@Override
@@ -81,8 +80,7 @@ public class GTreeFilterTask extends GTreeTask {
 
 	private void restoreInSameTask(TaskMonitor monitor) {
 
-		GTreeState existingState = tree.getRestoreTreeState();
-		GTreeState state = (existingState == null) ? defaultRestoreState : existingState;
+		GTreeState state = tree.getFilterRestoreState();
 		GTreeRestoreTreeStateTask restoreTask = new GTreeRestoreTreeStateTask(tree, state);
 		restoreTask.run(monitor);
 	}

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeRestoreTreeStateTask.java

@@ -13,15 +13,16 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package docking.widgets.tree.tasks;
+package docking.widgets.tree;
 
 import java.util.List;
 
 import javax.swing.tree.TreePath;
 
-import docking.widgets.tree.*;
 import docking.widgets.tree.support.GTreeSelectionEvent.EventOrigin;
-import ghidra.util.SystemUtilities;
+import docking.widgets.tree.tasks.GTreeExpandPathsTask;
+import docking.widgets.tree.tasks.GTreeSelectPathsTask;
+import ghidra.util.Swing;
 import ghidra.util.task.TaskMonitor;
 
 public class GTreeRestoreTreeStateTask extends GTreeTask {
@@ -57,6 +58,7 @@ public class GTreeRestoreTreeStateTask extends GTreeTask {
 
 			// this allows some tress to perform cleanup
 			tree.expandedStateRestored(monitor);
+			tree.clearFilterRestoreState();
 		}
 	}
 
@@ -90,7 +92,7 @@ public class GTreeRestoreTreeStateTask extends GTreeTask {
 		for (TreePath path : viewPaths) {
 			TreePath currentPath = translatePath(path, monitor);
 			if (currentPath != null) {
-				SystemUtilities.runSwingLater(() -> tree.scrollPathToVisible(currentPath));
+				Swing.runLater(() -> tree.scrollPathToVisible(currentPath));
 				break;
 			}
 		}

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/tasks/GTreeExpandPathsTask.java

@@ -17,7 +17,6 @@ package docking.widgets.tree.tasks;
 
 import java.util.List;
 
-import javax.swing.JTree;
 import javax.swing.tree.TreePath;
 
 import docking.widgets.tree.*;

Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/tasks/GTreeSelectPathsTask.java

@@ -48,7 +48,7 @@ public class GTreeSelectPathsTask extends GTreeTask {
 	 * 
 	 * @param disabled true to disable
 	 */
-	void setExpandingDisabled(boolean disabled) {
+	public void setExpandingDisabled(boolean disabled) {
 		this.expandingDisabled = disabled;
 	}
 

Ghidra/Framework/Docking/src/test.slow/java/docking/widgets/tree/GTreeTest.java

@@ -25,6 +25,7 @@ import java.util.List;
 import javax.swing.*;
 import javax.swing.tree.TreePath;
 
+import org.apache.commons.lang3.StringUtils;
 import org.junit.*;
 
 import docking.test.AbstractDockingTest;
@@ -482,7 +483,7 @@ public class GTreeTest extends AbstractDockingTest {
 	}
 
 	@Test
-	public void testGetAndRestoreTreeState() {
+	public void testRestoreTreeState() {
 		//
 		// Test that we can setup the tree, record its state, change the tree and then restore
 		// the saved state
@@ -528,7 +529,7 @@ public class GTreeTest extends AbstractDockingTest {
 	}
 
 	@Test
-	public void testGetAndRestoreTreeState_ExpandedStateOnly() {
+	public void testRestoreTreeState_ExpandedStateOnly() {
 		//
 		// Test that we can setup the tree, record its expanded state, change the tree 
 		// and then restore the saved state
@@ -560,6 +561,40 @@ public class GTreeTest extends AbstractDockingTest {
 		assertEquals(originalNode, expandedPaths.get(0).getLastPathComponent());
 	}
 
+	@Test
+	public void testRestoreTreeState_NoSelectedNodes_BeforeOrAfterFilter() {
+
+		//
+		// Tests the base use case for the tree's 'restore state', which is to put the tree
+		// back to the expanded state before a filter *if the user does NOT click the tree*.
+		//
+
+		installLargeTreeModel_WithManyExpandablePaths();
+
+		GTreeNode originalNode = findNodeInTree("Leaf Child - Single B0").getParent();
+		assertNotNull("Did not find existing child node in non filtered tree", originalNode);
+
+		gTree.expandPath(originalNode);
+		waitForTree();
+
+		TreePath expected = getLastVisiblePath();
+
+		// Set a filter that opens more paths than we had initially expanded.  This ensures
+		// that the tree's state changes after the filter is applied.
+		setFilterText("Leaf");
+
+		clearFilterText();
+
+		TreePath selectionPath = gTree.getSelectionPath();
+		assertNull("No node should be selected", selectionPath);
+
+		List<TreePath> expandedPaths = gTree.getExpandedPaths();
+		assertExpaned(expandedPaths, originalNode);
+
+		TreePath newFirstVisiblePath = getLastVisiblePath();
+		assertCloseEnough(expected, newFirstVisiblePath);
+	}
+
 	@Test
 	public void testFilterPathsRestoredWithFurtherFiltering_NoSelection() throws Exception {
 
@@ -735,7 +770,8 @@ public class GTreeTest extends AbstractDockingTest {
 			}
 		}
 
-		fail("Node not expaded: " + expected + "; expanded paths: " + expandedPaths);
+		String pretty = StringUtils.join(expandedPaths, "\n\t");
+		fail("\n\tNode not expanded: " + expected + ";\n\texpanded paths:\n\t" + pretty);
 	}
 
 	/** Verifies the actual is within one of the expected */
@@ -905,8 +941,8 @@ public class GTreeTest extends AbstractDockingTest {
 	private void setFilterOptions(final TextFilterStrategy filterStrategy, final boolean inverted) {
 		runSwing(() -> {
 			FilterOptions filterOptions = new FilterOptions(filterStrategy, false, false, inverted);
-			((DefaultGTreeFilterProvider) gTree.getFilterProvider())
-					.setFilterOptions(filterOptions);
+			((DefaultGTreeFilterProvider) gTree.getFilterProvider()).setFilterOptions(
+				filterOptions);
 		});
 		waitForTree();