GP-1159 fixes #3264 where symbol tree becomes unstable when grouping duplicate symbols

Ghidra/Features/Base/src/main/help/help/topics/SymbolTreePlugin/SymbolTree.htm

@@ -82,9 +82,8 @@
       Symbol Tree will show <I>group nodes</I> that represent groups of symbols in order to reduce
       the "clutter" in the tree. In the sample image above, the <I>Labels</I> category contains a
       group node (indicated by the <IMG alt="" src="images/openFolderGroup.png"> icon). The
-      <I>group node</I> shows the names of the first and last symbols in the group. Long names are
+      <I>group node</I> shows the common prefix for all the symbols in the group. The tool tip
-      truncated and are displayed with "..." to indicate this. The tool tip for the node shows the
+      will display the total number of nodes in the group.</P>
-      complete names for the first and last symbols in the group.</P>
 
       <BLOCKQUOTE>
         <P align="left"><IMG alt="" src="../../shared/note.png"> The Symbol Tree does not show
@@ -291,10 +290,6 @@
       <P>To delete a symbol, make a selection of symbols, right mouse click and choose the
       <B>Delete</B> option.</P>
 
-      <BLOCKQUOTE>
-        <P><IMG alt="" src="../../shared/warning.png">If you select a <A href="#GroupNode">group
-        node</A> to delete, <I><B>all</B></I> the symbols in that group are deleted.</P>
-      </BLOCKQUOTE>
     </BLOCKQUOTE>
 
     <H2><A name="Make_Selection"></A>Make a Selection</H2>

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/SymbolTreeProvider.java

@@ -398,8 +398,8 @@ public class SymbolTreeProvider extends ComponentProviderAdapter {
 		tree.refilterLater();
 	}
 
-	private void symbolChanged(Symbol symbol) {
+	private void symbolChanged(Symbol symbol, String oldName) {
-		addTask(new SymbolChangedTask(tree, symbol));
+		addTask(new SymbolChangedTask(tree, symbol, oldName));
 	}
 
 	private void symbolAdded(Symbol symbol) {
@@ -577,15 +577,18 @@ public class SymbolTreeProvider extends ComponentProviderAdapter {
 
 	private class SymbolChangedTask extends AbstactSymbolUpdateTask {
 
-		SymbolChangedTask(GTree tree, Symbol symbol) {
+		private String oldName;
+
+		SymbolChangedTask(GTree tree, Symbol symbol, String oldName) {
 			super(tree, symbol);
+			this.oldName = oldName;
 		}
 
 		@Override
 		void doRun(TaskMonitor monitor) throws CancelledException {
 
 			SymbolTreeRootNode root = (SymbolTreeRootNode) tree.getModelRoot();
-			root.symbolRemoved(symbol, monitor);
+			root.symbolRemoved(symbol, oldName, monitor);
 
 			// the symbol may have been deleted while we are processing bulk changes
 			if (!symbol.isDeleted()) {
@@ -662,7 +665,7 @@ public class SymbolTreeProvider extends ComponentProviderAdapter {
 
 				if (eventType == ChangeManager.DOCR_SYMBOL_RENAMED) {
 					Symbol symbol = (Symbol) object;
-					symbolChanged(symbol);
+					symbolChanged(symbol, (String) rec.getOldValue());
 				}
 				else if (eventType == ChangeManager.DOCR_SYMBOL_DATA_CHANGED ||
 					eventType == ChangeManager.DOCR_SYMBOL_SCOPE_CHANGED ||
@@ -676,7 +679,7 @@ public class SymbolTreeProvider extends ComponentProviderAdapter {
 						symbol = ((Namespace) object).getSymbol();
 					}
 
-					symbolChanged(symbol);
+					symbolChanged(symbol, symbol.getName());
 				}
 				else if (eventType == ChangeManager.DOCR_SYMBOL_ADDED) {
 					Symbol symbol = (Symbol) rec.getNewValue();
@@ -693,7 +696,7 @@ public class SymbolTreeProvider extends ComponentProviderAdapter {
 					SymbolTable symbolTable = program.getSymbolTable();
 					Symbol[] symbols = symbolTable.getSymbols(address);
 					for (Symbol symbol : symbols) {
-						symbolChanged(symbol);
+						symbolChanged(symbol, symbol.getName());
 					}
 				}
 			}

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/actions/DeleteAction.java

@@ -1,6 +1,5 @@
 /* ###
  * IP: GHIDRA
- * REVIEWED: YES
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,20 +15,19 @@
  */
 package ghidra.app.plugin.core.symboltree.actions;
 
-import ghidra.app.plugin.core.symboltree.SymbolTreeActionContext;
-import ghidra.app.plugin.core.symboltree.SymbolTreePlugin;
-import ghidra.app.plugin.core.symboltree.nodes.SymbolNode;
-import ghidra.program.model.listing.Program;
-import ghidra.program.model.symbol.Symbol;
-
 import java.awt.event.KeyEvent;
 
 import javax.swing.Icon;
 import javax.swing.tree.TreePath;
 
-import resources.ResourceManager;
 import docking.action.KeyBindingData;
 import docking.action.MenuData;
+import ghidra.app.plugin.core.symboltree.SymbolTreeActionContext;
+import ghidra.app.plugin.core.symboltree.SymbolTreePlugin;
+import ghidra.app.plugin.core.symboltree.nodes.SymbolNode;
+import ghidra.program.model.listing.Program;
+import ghidra.program.model.symbol.Symbol;
+import resources.ResourceManager;
 
 public class DeleteAction extends SymbolTreeContextAction {
 
@@ -60,7 +58,6 @@ public class DeleteAction extends SymbolTreeContextAction {
 			}
 		}
 
-		setEnabled(true);
 		return true;
 	}
 

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/MoreNode.java

@@ -0,0 +1,118 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.app.plugin.core.symboltree.nodes;
+
+import java.awt.datatransfer.DataFlavor;
+import java.util.List;
+
+import javax.swing.Icon;
+
+import docking.widgets.tree.GTreeNode;
+import ghidra.program.model.symbol.Namespace;
+import ghidra.util.exception.CancelledException;
+import ghidra.util.task.TaskMonitor;
+import resources.Icons;
+
+/**
+ * Node to represent nodes that are not shown. After showing a handful of symbol nodes
+ * with the same name, this node will be used in place of the rest of the nodes and
+ * will display "xx more..." where xx is the number of nodes that are not being shown. 
+ * 
+ */
+public class MoreNode extends SymbolTreeNode {
+	private static Icon ICON = Icons.MAKE_SELECTION_ICON;
+	private int count;
+	private String name;
+
+	MoreNode(String name, int count) {
+		this.name = name;
+		this.count = count;
+	}
+
+	@Override
+	public String getName() {
+		return count + " more...";
+	}
+
+	@Override
+	public boolean canCut() {
+		return false;
+	}
+
+	@Override
+	public boolean canPaste(List<GTreeNode> pastedNodes) {
+		return false;
+	}
+
+	@Override
+	public void setNodeCut(boolean isCut) {
+		throw new UnsupportedOperationException("Cannot cut an organization node");
+	}
+
+	@Override
+	public boolean isCut() {
+		return false;
+	}
+
+	@Override
+	public DataFlavor getNodeDataFlavor() {
+		return null;
+	}
+
+	@Override
+	public boolean supportsDataFlavors(DataFlavor[] dataFlavors) {
+		return false;
+	}
+
+	@Override
+	public Namespace getNamespace() {
+		return null;
+	}
+
+	@Override
+	public List<GTreeNode> generateChildren(TaskMonitor monitor) throws CancelledException {
+		// not used, children generated in constructor
+		return null;
+	}
+
+	@Override
+	public Icon getIcon(boolean expanded) {
+		return ICON;
+	}
+
+	@Override
+	public String getToolTip() {
+		return "There are " + count + " nodes named \"" +
+			name + "\" not being shown";
+	}
+
+	@Override
+	public boolean isLeaf() {
+		return true;
+	}
+
+	void incrementCount() {
+		count++;
+	}
+
+	void decrementCount() {
+		count = Math.max(0, --count);
+	}
+
+	boolean isEmpty() {
+		return count == 0;
+	}
+}

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/OrganizationNode.java

@@ -22,16 +22,18 @@ import javax.swing.Icon;
 
 import docking.widgets.tree.GTreeNode;
 import ghidra.program.model.symbol.Namespace;
-import ghidra.util.datastruct.IntArray;
 import ghidra.util.exception.CancelledException;
 import ghidra.util.task.TaskMonitor;
 import resources.ResourceManager;
 
 /**
- * See {@link #computeChildren(List, int, GTreeNode, int, TaskMonitor)} for details on 
+ * These nodes are used to organize large lists of nodes into a hierachical structure based on 
+ * the node names. See {@link #organize(List, int, TaskMonitor)} for details on 
  * how this class works.
  */
 public class OrganizationNode extends SymbolTreeNode {
+	public static final int MAX_SAME_NAME = 10;
+
 	static final Comparator<GTreeNode> COMPARATOR = new OrganizationNodeComparator();
 
 	private static Icon OPEN_FOLDER_GROUP_ICON =
@@ -40,45 +42,38 @@ public class OrganizationNode extends SymbolTreeNode {
 		ResourceManager.loadImage("images/closedFolderGroup.png");
 
 	private String baseName;
+	private int totalCount;
 
-	/**
+	private MoreNode moreNode;
-	 * You cannot instantiate this class directly, instead use the factory method below
+
-	 * {@link #organize(List, int, TaskMonitor)}
+	private OrganizationNode(List<GTreeNode> list, int maxGroupSize, TaskMonitor monitor)
-	 * @throws CancelledException if the operation is cancelled
-	 */
-	private OrganizationNode(List<GTreeNode> list, int max, int parentLevel, TaskMonitor monitor)
 			throws CancelledException {
+		totalCount = list.size();
+		// organize children further if the list is too big
+		List<GTreeNode> children = organize(list, maxGroupSize, monitor);
 
-		doSetChildren(computeChildren(list, max, this, parentLevel, monitor));
+		// if all the entries have the same name and we have more than a handful, show only
+		// a few and add a special "More" node
+		if (children.size() > MAX_SAME_NAME && hasSameName(children)) {
+			// they all have the same name, so just use that as this nodes name
+			baseName = children.get(0).getName();
 
-		GTreeNode child = getChild(0);
+			children = new ArrayList<>(children.subList(0, MAX_SAME_NAME));
-		baseName = child.getName().substring(0, getPrefixSizeForGrouping(getChildren(), 1) + 1);
+			moreNode = new MoreNode(baseName, totalCount - MAX_SAME_NAME);
+			children.add(moreNode);
+		}
+		else {
+			// name this node the prefix that all children nodes have in common
+			baseName = getCommonPrefix(children);
+		}
+		doSetChildren(children);
 	}
 
 	/**
-	 * A factory method for creating OrganizationNode objects. 
+	 * Subdivide the given list of nodes recursively such that there are generally not more
-	 * See {@link #computeChildren(List, int, GTreeNode, int, TaskMonitor)}
+	 * than maxGroupSize number of nodes at any level. Also, if there are ever many
-	 *
+	 * nodes of the same name, a group for them will be created and only a few will be shown with
-	 * @param nodes the original list of child nodes to be subdivided.
+	 * an "xx more..." node to indicate there are additional nodes that are not shown.
-	 * @param max The max number of child nodes per parent node at any node level.
-	 * @param monitor the task monitor used to cancel this operation
-	 * @return A list of nodes that is based upon the given list, but subdivided as needed.
-	 * @throws CancelledException if the operation is cancelled
-	 * @see #computeChildren(List, int, GTreeNode, int, TaskMonitor)
-	 */
-	public static List<GTreeNode> organize(List<GTreeNode> nodes, int max, TaskMonitor monitor)
-			throws CancelledException {
-		return organize(nodes, null, max, monitor);
-	}
-
-	private static List<GTreeNode> organize(List<GTreeNode> nodes, GTreeNode parent, int max,
-			TaskMonitor monitor) throws CancelledException {
-		return computeChildren(nodes, max, parent, 0, monitor);
-	}
-
-	/**
-	 * Subdivide the given list of nodes such that the list or no new parent created will have
-	 * more than <tt>maxNodes</tt> children.
 	 * <p>
 	 * This algorithm uses the node names to group nodes based upon common prefixes.  For example,
 	 * if a parent node contained more than <tt>maxNodes</tt> children then a possible grouping
@@ -98,103 +93,47 @@ public class OrganizationNode extends SymbolTreeNode {
 	 *  g
 	 * </pre>
 	 * <p>
-	 * The algorithm prefers to group nodes that have the longest common prefixes.
+	 * @param list list of child nodes of to breakup into smaller groups
-	 * <p>
+	 * @param maxGroupSize the max number of nodes to allow before trying to organize into
-	 * <b>Note: the given data must be sorted for this method to work properly.</b>
+	 * smaller groups
-	 *
+	 * @param monitor the TaskMonitor to be checked for canceling this operation
-	 * @param list list of child nodes of <tt>parent</tt> to breakup into smaller groups.
+	 * @return the given <tt>list</tt> sub-grouped as outlined above
-	 * @param maxNodes The max number of child nodes per parent node at any node level.
-	 * @param parent The parent of the given <tt>children</tt>
-	 * @param parentLevel node depth in the tree of <b>Organization</b> nodes.
-	 * @return the given <tt>list</tt> sub-grouped as outlined above.
 	 * @throws CancelledException if the operation is cancelled
 	 */
-	private static List<GTreeNode> computeChildren(List<GTreeNode> list, int maxNodes,
+	public static List<GTreeNode> organize(List<GTreeNode> list, int maxGroupSize,
-			GTreeNode parent, int parentLevel, TaskMonitor monitor) throws CancelledException {
+			TaskMonitor monitor) throws CancelledException {
-		List<GTreeNode> children;
+
-		if (list.size() <= maxNodes) {
+		Map<String, List<GTreeNode>> prefixMap = partition(list, maxGroupSize, monitor);
-			children = new ArrayList<>(list);
+
+		// if they didn't partition, just add all given nodes as children
+		if (prefixMap == null) {
+			return new ArrayList<>(list);
 		}
-		else {
-			int characterOffset = getPrefixSizeForGrouping(list, maxNodes);
 
-			characterOffset = Math.max(characterOffset, parentLevel + 1);
+		// otherwise, the nodes have been partitioned into groups with a common prefix
+		// loop through and create organization nodes for groups larger than one element
+		List<GTreeNode> children = new ArrayList<>();
+		for (String prefix : prefixMap.keySet()) {
+			monitor.checkCanceled();
 
-			children = new ArrayList<>();
+			List<GTreeNode> nodesSamePrefix = prefixMap.get(prefix);
-			String prevStr = list.get(0).getName();
+
-			int start = 0;
+			// all the nodes that don't have a common prefix get added directly
-			int end = list.size();
+			if (prefix.isEmpty()) {
-			for (int i = 1; i < end; i++) {
+				children.addAll(nodesSamePrefix);
-				monitor.checkCanceled();
+			}
-				String str = list.get(i).getName();
+			// groups with one entry, just add in the element directly
-				if (stringsDiffer(prevStr, str, characterOffset)) {
+			else if (nodesSamePrefix.size() == 1) {
-					addNode(children, list, start, i - 1, maxNodes, characterOffset, monitor);
+				children.addAll(nodesSamePrefix);
-					start = i;
+			}
-				}
+			else {
-				prevStr = str;
+				// add an organization node for each unique prefix
+				children.add(new OrganizationNode(nodesSamePrefix, maxGroupSize, monitor));
 			}
-			addNode(children, list, start, end - 1, maxNodes, characterOffset, monitor);
 		}
 		return children;
 	}
 
-	private static boolean stringsDiffer(String s1, String s2, int diffLevel) {
-		if (s1.length() <= diffLevel || s2.length() <= diffLevel) {
-			return true;
-		}
-		return s1.substring(0, diffLevel + 1)
-				.compareToIgnoreCase(s2.substring(0, diffLevel + 1)) != 0;
-	}
-
-	private static void addNode(List<GTreeNode> children, List<GTreeNode> list, int start, int end,
-			int max, int diffLevel, TaskMonitor monitor) throws CancelledException {
-		if (end - start > 0) {
-			children.add(
-				new OrganizationNode(list.subList(start, end + 1), max, diffLevel, monitor));
-		}
-		else {
-			GTreeNode node = list.get(start);
-			children.add(node);
-		}
-	}
-
-	/**
-	 * Returns the longest prefix size such that the list of nodes can be grouped by
-	 * those prefixes while not exceeding <tt>maxNodes</tt> number of children.
-	 */
-	private static int getPrefixSizeForGrouping(List<GTreeNode> list, int maxNodes) {
-		IntArray prefixSizeCountBins = new IntArray();
-		Iterator<GTreeNode> it = list.iterator();
-		String previousNodeName = it.next().getName();
-		prefixSizeCountBins.put(0, 1);
-		while (it.hasNext()) {
-			String currentNodeName = it.next().getName();
-			int prefixSize = getCommonPrefixSize(previousNodeName, currentNodeName);
-			prefixSizeCountBins.put(prefixSize, prefixSizeCountBins.get(prefixSize) + 1);
-			previousNodeName = currentNodeName;
-		}
-
-		int binContentsTotal = 0;
-		for (int i = 0; i <= prefixSizeCountBins.getLastNonEmptyIndex(); i++) {
-			binContentsTotal += prefixSizeCountBins.get(i);
-			if (binContentsTotal > maxNodes) {
-				return Math.max(0, i - 1);  // we've crossed the max; take a step back
-			}
-		}
-
-		return prefixSizeCountBins.getLastNonEmptyIndex(); // all are allowed; use max prefix size
-	}
-
-	private static int getCommonPrefixSize(String s1, String s2) {
-		int maxCompareLength = Math.min(s1.length(), s2.length());
-		for (int i = 0; i < maxCompareLength; i++) {
-			if (Character.toUpperCase(s1.charAt(i)) != Character.toUpperCase(s2.charAt(i))) {
-				return i;
-			}
-		}
-		return maxCompareLength; // one string is a subset of the other (or the same)
-	}
-
 	@Override
 	public boolean equals(Object o) {
 		if (this == o) {
@@ -226,10 +165,6 @@ public class OrganizationNode extends SymbolTreeNode {
 		return false;
 	}
 
-	public boolean isModifiable() {
-		return false;
-	}
-
 	@Override
 	public void setNodeCut(boolean isCut) {
 		throw new UnsupportedOperationException("Cannot cut an organization node");
@@ -245,12 +180,12 @@ public class OrganizationNode extends SymbolTreeNode {
 
 	@Override
 	public String getName() {
-		return baseName + "...";
+		return baseName;
 	}
 
 	@Override
 	public String getToolTip() {
-		return getName();
+		return "Contains labels that start with \"" + getName() + "\" (" + totalCount + ")";
 	}
 
 	@Override
@@ -282,6 +217,11 @@ public class OrganizationNode extends SymbolTreeNode {
 	 * @param newNode the node to insert.
 	 */
 	public void insertNode(GTreeNode newNode) {
+		if (moreNode != null) {
+			moreNode.incrementCount();
+			return;
+		}
+
 		int index = Collections.binarySearch(getChildren(), newNode, getChildrenComparator());
 		if (index >= 0) {
 			// found a match
@@ -309,6 +249,23 @@ public class OrganizationNode extends SymbolTreeNode {
 			return null;
 		}
 
+		// special case: all symbols in this group have the same name.
+		if (moreNode != null) {
+			if (!symbolName.equals(baseName)) {
+				return null;
+			}
+			// The node either belongs to this node's children or it is represented by the
+			// 'more' node
+			for (GTreeNode child : children()) {
+				SymbolTreeNode symbolTreeNode = (SymbolTreeNode) child;
+				if (symbolTreeNode.getSymbol() == key.getSymbol()) {
+					return child;
+				}
+			}
+
+			return moreNode;
+		}
+
 		//
 		// Note: The 'key' node used for searching will find us the parent node of the symbol
 		//       that has changed if it is an org node (this is because the org node searches
@@ -339,6 +296,112 @@ public class OrganizationNode extends SymbolTreeNode {
 		return COMPARATOR;
 	}
 
+	// We are being tricky here. The findSymbolTreeNode above returns the 'more' node
+	// if the searched node is one of the nodes not being shown, so then the removeNode gets
+	// called with the 'more' node, which just means to decrement the count.
+	@Override
+	public void removeNode(GTreeNode node) {
+		if (node == moreNode) {
+			moreNode.decrementCount();
+			if (!moreNode.isEmpty()) {
+				return;
+			}
+			// The 'more' node is empty, just let it be removed
+			moreNode = null;
+		}
+		super.removeNode(node);
+	}
+
+	@Override
+	public List<GTreeNode> generateChildren(TaskMonitor monitor) throws CancelledException {
+		// not used, children generated in constructor
+		return null;
+	}
+
+	private String getCommonPrefix(List<GTreeNode> children) {
+		int commonPrefixSize = getCommonPrefixSize(children);
+		return children.get(0).getName().substring(0, commonPrefixSize);
+	}
+
+	/**
+	 * This is the algorithm for partitioning a list of nodes into a hierarchical structure based
+	 * on common prefixes 
+	 * @param nodeList the list of nodes to be partitioned
+	 * @param maxGroupSize the maximum number of nodes in a group before an organization is attempted
+	 * @param monitor {@link TaskMonitor} so the operation can be cancelled
+	 * @return a map of common prefixes to lists of nodes that have that common prefix. Returns null
+	 * if the size is less than maxGroupSize or the partition didn't reduce the number of nodes
+	 * @throws CancelledException if the operation was cancelled
+	 */
+	private static Map<String, List<GTreeNode>> partition(List<GTreeNode> nodeList,
+			int maxGroupSize, TaskMonitor monitor) throws CancelledException {
+
+		// no need to partition of the number of nodes is small enough
+		if (nodeList.size() <= maxGroupSize) {
+			return null;
+		}
+		int commonPrefixSize = getCommonPrefixSize(nodeList);
+		int uniquePrefixSize = commonPrefixSize + 1;
+		Map<String, List<GTreeNode>> map = new LinkedHashMap<>();
+		for (GTreeNode node : nodeList) {
+			monitor.checkCanceled();
+			String prefix = getPrefix(node, uniquePrefixSize);
+			List<GTreeNode> list = map.computeIfAbsent(prefix, k -> new ArrayList<GTreeNode>());
+			list.add(node);
+		}
+		if (map.size() == 1) {
+			return null;
+		}
+		if (map.size() >= nodeList.size()) {
+			return null;	// no reduction
+		}
+
+		return map;
+	}
+
+	private static String getPrefix(GTreeNode gTreeNode, int uniquePrefixSize) {
+		String name = gTreeNode.getName();
+		if (name.length() <= uniquePrefixSize) {
+			return name;
+		}
+		return name.substring(0, uniquePrefixSize);
+	}
+
+	private static int getCommonPrefixSize(List<GTreeNode> list) {
+		GTreeNode node = list.get(0);
+		String first = node.getName();
+		int inCommonSize = first.length();
+		for (int i = 1; i < list.size(); i++) {
+			String next = list.get(i).getName();
+			inCommonSize = Math.min(inCommonSize, getCommonPrefixSize(first, next, inCommonSize));
+		}
+		return inCommonSize;
+	}
+
+	private static int getCommonPrefixSize(String base, String candidate, int max) {
+		int maxCompareLength = Math.min(max, candidate.length());
+		for (int i = 0; i < maxCompareLength; i++) {
+			if (base.charAt(i) != candidate.charAt(i)) {
+				return i;
+			}
+		}
+		return maxCompareLength; // one string is a subset of the other (or the same)
+	}
+
+	private static boolean hasSameName(List<GTreeNode> list) {
+		if (list.size() < 2) {
+			return false;
+		}
+		String name = list.get(0).getName();
+		for (GTreeNode node : list) {
+			if (!name.equals(node.getName())) {
+				return false;
+			}
+		}
+		return true;
+
+	}
+
 	static class OrganizationNodeComparator implements Comparator<GTreeNode> {
 		@Override
 		public int compare(GTreeNode g1, GTreeNode g2) {
@@ -355,9 +418,4 @@ public class OrganizationNode extends SymbolTreeNode {
 		}
 	}
 
-	@Override
-	public List<GTreeNode> generateChildren(TaskMonitor monitor) throws CancelledException {
-		// not used, children generated in constructor
-		return null;
-	}
 }

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/SymbolCategoryNode.java

@@ -28,6 +28,7 @@ import ghidra.util.task.TaskMonitor;
 import ghidra.util.task.TaskMonitorAdapter;
 
 public abstract class SymbolCategoryNode extends SymbolTreeNode {
+	private static final int MAX_NODES = 40;
 	protected SymbolCategory symbolCategory;
 	protected SymbolTable symbolTable;
 	protected GlobalNamespace globalNamespace;
@@ -53,10 +54,7 @@ public abstract class SymbolCategoryNode extends SymbolTreeNode {
 		SymbolType symbolType = symbolCategory.getSymbolType();
 		List<GTreeNode> list = getSymbols(symbolType, monitor);
 		monitor.checkCanceled();
-		if (list.size() > MAX_CHILD_NODES) {
+		return OrganizationNode.organize(list, MAX_NODES, monitor);
-			list = OrganizationNode.organize(list, MAX_CHILD_NODES, monitor);
-		}
-		return list;
 	}
 
 	public Program getProgram() {

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/nodes/SymbolTreeNode.java

@@ -41,8 +41,6 @@ import ghidra.util.task.TaskMonitor;
  */
 public abstract class SymbolTreeNode extends GTreeSlowLoadingNode {
 
-	public static final int MAX_CHILD_NODES = 40;
-
 	public static final Comparator<Symbol> SYMBOL_COMPARATOR = (s1, s2) -> {
 		// note: not really sure if we care about the cases where 'symbol' is null, as that 
 		//       implies the symbol was deleted and the node will go away.  Just be consistent.

Ghidra/Features/Base/src/test/java/ghidra/app/plugin/core/symboltree/nodes/OrganizationNodeTest.java

@@ -0,0 +1,193 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.app.plugin.core.symboltree.nodes;
+
+import static org.junit.Assert.*;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.Test;
+
+import docking.test.AbstractDockingTest;
+import docking.widgets.tree.GTreeNode;
+import ghidra.program.model.symbol.Symbol;
+import ghidra.program.model.symbol.StubSymbol;
+import ghidra.util.Swing;
+import ghidra.util.exception.AssertException;
+import ghidra.util.exception.CancelledException;
+import ghidra.util.task.TaskMonitor;
+
+public class OrganizationNodeTest extends AbstractDockingTest {
+
+	@Test
+	public void testOrganizeDoesNothingIfBelowMaxGroupSize() {
+		List<GTreeNode> nodeList =
+			nodes("AAA", "AAB", "AAB", "AABA", "BBA", "BBB", "BBC", "CCC", "DDD");
+		List<GTreeNode> result = organize(nodeList, 10);
+		assertEquals(nodeList, result);
+
+		result = organize(nodeList, 5);
+		assertNotEquals(nodeList, result);
+	}
+
+	@Test
+	public void testBasicPartitioning() {
+		List<GTreeNode> nodeList = nodes("AAA", "AAB", "AAC", "BBA", "BBB", "BBC", "CCC", "DDD");
+		List<GTreeNode> result = organize(nodeList, 5);
+		assertEquals(4, result.size());
+		assertEquals("AA", result.get(0).getName());
+		assertEquals("BB", result.get(1).getName());
+		assertEquals("CCC", result.get(2).getName());
+		assertEquals("DDD", result.get(3).getName());
+
+		GTreeNode aaGroup = result.get(0);
+		assertEquals(3, aaGroup.getChildCount());
+		assertEquals("AAA", aaGroup.getChild(0).getName());
+		assertEquals("AAB", aaGroup.getChild(1).getName());
+		assertEquals("AAC", aaGroup.getChild(2).getName());
+
+		GTreeNode bbGroup = result.get(1);
+		assertEquals(3, bbGroup.getChildCount());
+	}
+
+	@Test
+	public void testMultiLevel() {
+		List<GTreeNode> nodeList = nodes("A", "B", "CAA", "CAB", "CAC", "CAD", "CAE", "CAF", "CBA");
+		List<GTreeNode> result = organize(nodeList, 5);
+		assertEquals(3, result.size());
+		assertEquals("A", result.get(0).getName());
+		assertEquals("B", result.get(1).getName());
+		assertEquals("C", result.get(2).getName());
+
+		GTreeNode cGroup = result.get(2);
+		assertEquals(2, cGroup.getChildCount());
+		assertEquals("CA", cGroup.getChild(0).getName());
+		assertEquals("CBA", cGroup.getChild(1).getName());
+
+		GTreeNode caGroup = cGroup.getChild(0);
+		assertEquals(6, caGroup.getChildCount());
+		assertEquals("CAA", caGroup.getChild(0).getName());
+		assertEquals("CAF", caGroup.getChild(5).getName());
+	}
+
+	@Test
+	public void testManySameLabels() {
+		List<GTreeNode> nodeList =
+			nodes("A", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP",
+				"DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP");
+
+		List<GTreeNode> result = organize(nodeList, 5);
+		assertEquals(2, result.size());
+		assertEquals("A", result.get(0).getName());
+		assertEquals("DUP", result.get(1).getName());
+
+		GTreeNode dupNode = result.get(1);
+		assertEquals(OrganizationNode.MAX_SAME_NAME + 1, dupNode.getChildCount());
+		assertEquals("11 more...", dupNode.getChild(OrganizationNode.MAX_SAME_NAME).getName());
+
+	}
+
+	@Test
+	public void testRemoveNotShownNode() {
+		List<GTreeNode> nodeList =
+			nodes("A", "D1", "D2", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP",
+				"DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP");
+
+		List<GTreeNode> result = organize(nodeList, 5);
+
+		SymbolTreeNode dNode = (SymbolTreeNode) result.get(1);
+		GTreeNode dupNode = dNode.getChild(2);
+
+		assertEquals(OrganizationNode.MAX_SAME_NAME + 1, dupNode.getChildCount());
+		assertEquals("11 more...", dupNode.getChild(OrganizationNode.MAX_SAME_NAME).getName());
+
+		SymbolTreeNode node = (SymbolTreeNode) nodeList.get(nodeList.size() - 1);
+		simulateSmbolDeleted(dNode, node.getSymbol());
+
+		assertEquals("10 more...", dupNode.getChild(dupNode.getChildCount() - 1).getName());
+	}
+
+	@Test
+	public void testRemoveShownNode() {
+		List<GTreeNode> nodeList =
+			nodes("A", "D1", "D2", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP",
+				"DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP");
+
+		List<GTreeNode> result = organize(nodeList, 5);
+
+		SymbolTreeNode dNode = (SymbolTreeNode) result.get(1);
+		GTreeNode dupNode = dNode.getChild(2);
+
+		assertEquals(OrganizationNode.MAX_SAME_NAME + 1, dupNode.getChildCount());
+		assertEquals("11 more...", dupNode.getChild(OrganizationNode.MAX_SAME_NAME).getName());
+
+		SymbolTreeNode node = (SymbolTreeNode) nodeList.get(4);
+		simulateSmbolDeleted(dNode, node.getSymbol());
+
+		assertEquals(OrganizationNode.MAX_SAME_NAME, dupNode.getChildCount());
+		assertEquals("11 more...", dupNode.getChild(dupNode.getChildCount() - 1).getName());
+	}
+
+	@Test
+	public void testAddDupNodeJustIncrementsCount() {
+		List<GTreeNode> nodeList =
+			nodes("A", "D1", "D2", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP",
+				"DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP", "DUP");
+
+		List<GTreeNode> result = organize(nodeList, 5);
+
+		SymbolTreeNode dNode = (SymbolTreeNode) result.get(1);
+		GTreeNode dupNode = dNode.getChild(2);
+
+		assertEquals(OrganizationNode.MAX_SAME_NAME + 1, dupNode.getChildCount());
+		assertEquals("11 more...", dupNode.getChild(OrganizationNode.MAX_SAME_NAME).getName());
+
+		((OrganizationNode) dNode).insertNode(node("DUP"));
+
+		assertEquals(OrganizationNode.MAX_SAME_NAME + 1, dupNode.getChildCount());
+		assertEquals("12 more...", dupNode.getChild(OrganizationNode.MAX_SAME_NAME).getName());
+
+	}
+
+	private void simulateSmbolDeleted(SymbolTreeNode root, Symbol symbolToDelete) {
+		SymbolNode key = SymbolNode.createKeyNode(symbolToDelete, symbolToDelete.getName(), null);
+		GTreeNode found = root.findSymbolTreeNode(key, false, TaskMonitor.DUMMY);
+		Swing.runNow(() -> found.getParent().removeNode(found));
+	}
+
+	private List<GTreeNode> organize(List<GTreeNode> list, int size) {
+		try {
+			return OrganizationNode.organize(list, size, TaskMonitor.DUMMY);
+		}
+		catch (CancelledException e) {
+			throw new AssertException("Can't happen");
+		}
+	}
+
+	private List<GTreeNode> nodes(String... names) {
+		List<GTreeNode> list = new ArrayList<>();
+		for (String name : names) {
+			list.add(node(name));
+		}
+		return list;
+	}
+
+	private GTreeNode node(String name) {
+		return new CodeSymbolNode(null, new StubSymbol(name, null));
+	}
+
+}

Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/model/symbol/StubSymbol.java

@@ -0,0 +1,228 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.program.model.symbol;
+
+import ghidra.program.model.address.Address;
+import ghidra.program.model.listing.CircularDependencyException;
+import ghidra.program.model.listing.Program;
+import ghidra.program.util.ProgramLocation;
+import ghidra.util.exception.DuplicateNameException;
+import ghidra.util.exception.InvalidInputException;
+import ghidra.util.task.TaskMonitor;
+
+// Simple symbol test implementation
+public class StubSymbol implements Symbol {
+	private static long nextId = 0;
+
+	private long id;
+	private String name;
+	private Address address;
+
+	public StubSymbol(String name, Address address) {
+		this.name = name;
+		this.address = address;
+		id = nextId++;
+	}
+
+	@Override
+	public Address getAddress() {
+		return address;
+	}
+
+	@Override
+	public String getName() {
+		return name;
+	}
+
+	@Override
+	public String[] getPath() {
+		return new String[] { name };
+	}
+
+	@Override
+	public Program getProgram() {
+		return null;
+	}
+
+	@Override
+	public String getName(boolean includeNamespace) {
+		return name;
+	}
+
+	@Override
+	public Namespace getParentNamespace() {
+		return null;
+	}
+
+	@Override
+	public Symbol getParentSymbol() {
+		return null;
+	}
+
+	@Override
+	public boolean isDescendant(Namespace namespace) {
+		return false;
+	}
+
+	@Override
+	public boolean isValidParent(Namespace parent) {
+		return false;
+	}
+
+	@Override
+	public SymbolType getSymbolType() {
+		return SymbolType.LABEL;
+	}
+
+	@Override
+	public int getReferenceCount() {
+		return 0;
+	}
+
+	@Override
+	public boolean hasMultipleReferences() {
+		return false;
+	}
+
+	@Override
+	public boolean hasReferences() {
+		return false;
+	}
+
+	@Override
+	public Reference[] getReferences(TaskMonitor monitor) {
+		return null;
+	}
+
+	@Override
+	public Reference[] getReferences() {
+		return null;
+	}
+
+	@Override
+	public ProgramLocation getProgramLocation() {
+		return null;
+	}
+
+	@Override
+	public void setName(String newName, SourceType source)
+			throws DuplicateNameException, InvalidInputException {
+		this.name = newName;
+	}
+
+	@Override
+	public void setNamespace(Namespace newNamespace)
+			throws DuplicateNameException, InvalidInputException, CircularDependencyException {
+		// do nothing
+	}
+
+	@Override
+	public void setNameAndNamespace(String newName, Namespace newNamespace, SourceType source)
+			throws DuplicateNameException, InvalidInputException, CircularDependencyException {
+		this.name = newName;
+	}
+
+	@Override
+	public boolean delete() {
+		return false;
+	}
+
+	@Override
+	public boolean isPinned() {
+		return false;
+	}
+
+	@Override
+	public void setPinned(boolean pinned) {
+		// nothing
+	}
+
+	@Override
+	public boolean isDynamic() {
+		return false;
+	}
+
+	@Override
+	public boolean isExternal() {
+		return false;
+	}
+
+	@Override
+	public boolean isPrimary() {
+		return true;
+	}
+
+	@Override
+	public boolean setPrimary() {
+		return false;
+	}
+
+	@Override
+	public boolean isExternalEntryPoint() {
+		return false;
+	}
+
+	@Override
+	public long getID() {
+		return name.hashCode();
+	}
+
+	@Override
+	public Object getObject() {
+		return null;
+	}
+
+	@Override
+	public boolean isGlobal() {
+		return true;
+	}
+
+	@Override
+	public void setSource(SourceType source) {
+		// nothing
+	}
+
+	@Override
+	public SourceType getSource() {
+		return SourceType.USER_DEFINED;
+	}
+
+	@Override
+	public boolean isDeleted() {
+		return false;
+	}
+
+	@Override
+	public int hashCode() {
+		return (int) id;
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		if (this == obj) {
+			return true;
+		}
+		if (obj == null) {
+			return false;
+		}
+		if (getClass() != obj.getClass()) {
+			return false;
+		}
+		StubSymbol other = (StubSymbol) obj;
+		return id == other.id;
+	}
+
+}