GP-5620 - Fixed a bug that introduced duplicate data when renaming

functions with a filter

Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/functionwindow/FunctionWindowPluginTest.java

@@ -4,9 +4,9 @@
  * 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.
@@ -18,25 +18,32 @@ package ghidra.app.plugin.core.functionwindow;
 import static org.hamcrest.CoreMatchers.*;
 import static org.junit.Assert.*;
 
+import java.util.List;
+
 import javax.swing.*;
 import javax.swing.event.ChangeEvent;
 import javax.swing.table.*;
 
 import org.junit.*;
 
-import docking.*;
+import docking.ActionContext;
+import docking.DefaultActionContext;
 import docking.action.DockingActionIf;
 import docking.tool.ToolConstants;
 import docking.widgets.combobox.GComboBox;
 import docking.widgets.dialogs.SettingsDialog;
-import docking.widgets.table.GTable;
+import docking.widgets.table.*;
 import docking.widgets.table.threaded.ThreadedTableModel;
+import ghidra.app.cmd.label.RenameLabelCmd;
 import ghidra.app.plugin.core.clear.ClearCmd;
 import ghidra.app.plugin.core.clear.ClearOptions;
 import ghidra.app.services.ProgramManager;
 import ghidra.framework.cmd.CompoundCmd;
 import ghidra.framework.plugintool.PluginTool;
+import ghidra.program.model.address.Address;
+import ghidra.program.model.address.AddressFactory;
 import ghidra.program.model.listing.*;
+import ghidra.program.model.symbol.*;
 import ghidra.test.*;
 
 public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTest {
@@ -45,7 +52,7 @@ public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTes
 	private Program program;
 	private FunctionWindowPlugin plugin;
 	private GTable functionTable;
-	private ComponentProvider provider;
+	private FunctionWindowProvider provider;
 
 	@Before
 	public void setUp() throws Exception {
@@ -57,7 +64,7 @@ public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTes
 
 		plugin.showFunctions();
 		waitForSwing();
-		provider = tool.getComponentProvider("Functions Window");
+		provider = (FunctionWindowProvider) tool.getComponentProvider("Functions Window");
 		functionTable = (GTable) findComponentByName(provider.getComponent(), "Functions Table");
 	}
 
@@ -76,7 +83,7 @@ public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTes
 		ProgramManager pm = tool.getService(ProgramManager.class);
 		pm.closeProgram(program, true);
 		waitForSwing();
-		waitForNotBusy(functionTable);
+		waitForTable();
 	}
 
 	@Test
@@ -91,12 +98,12 @@ public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTes
 			cmd.add(new ClearCmd(f.getBody(), new ClearOptions()));
 		}
 		applyCmd(program, cmd);
-		waitForNotBusy(functionTable);
+		waitForTable();
 
 		assertEquals(0, functionTable.getRowCount());
 
 		undo(program);
-		waitForNotBusy(functionTable);
+		waitForTable();
 
 		assertEquals(numData, functionTable.getRowCount());
 	}
@@ -104,7 +111,7 @@ public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTes
 	@Test
 	public void testProgramClose() throws Exception {
 		closeProgram();
-		waitForNotBusy(functionTable);
+		waitForTable();
 		assertEquals(functionTable.getRowCount(), 0);
 	}
 
@@ -165,6 +172,117 @@ public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTes
 		assertThat(copyText, containsString(signatureText));
 	}
 
+	@Test
+	public void testChange_WithFitler() throws Exception {
+
+		//
+		// This tests a regression with changed items.  Normally a changed item is handled by a 
+		// remove of the existing row object, with a re-add of that object.  This allows us to avoid
+		// duplicates and to sort the item.  We had a bug that prevented the item from being 
+		// removed.
+		//
+
+		// the bug was only present when sorted on the name column, since the sort was no longer
+		// correct when the name had changed
+		sort("Name");
+		int startRowCount = functionTable.getRowCount();
+
+		// verify the function we will rename is in the table
+		assertFunctionInTable("FUN_010058b8");
+
+		// apply a filter that will hide an item we will rename
+		filter("entry");
+		assertEquals(1, functionTable.getRowCount());
+		assertFunctionInTable("entry");
+
+		// rename a function not showing, using a name that will pass the filter		
+		// FUN_010058b8 -> entry2
+		renameFunction(addr("010058b8"), "entry2");
+
+		// verify the new item appears
+		assertEquals(2, functionTable.getRowCount());
+		assertFunctionInTable("entry2");
+
+		// remove the filter
+		filter("");
+
+		// verify the old item is gone and the new item is still there
+		assertFunctionInTable("entry2");
+		assertFunctionNotInTable("FUN_010058b8");
+		assertEquals("Table row count should not have changed for a function rename", startRowCount,
+			functionTable.getRowCount());
+	}
+
+	private void sort(String columnName) {
+
+		int column = getColumn(columnName);
+		TableSortState descendingSortState = TableSortState.createDefaultSortState(column, false);
+		FunctionTableModel model = (FunctionTableModel) functionTable.getModel();
+		runSwing(() -> model.setTableSortState(descendingSortState));
+		waitForTable();
+	}
+
+	private int getColumn(String columnName) {
+		int n = functionTable.getColumnCount();
+		for (int i = 0; i < n; i++) {
+			String name = functionTable.getColumnName(i);
+			if (name.equals(columnName)) {
+				return i;
+			}
+		}
+
+		fail("Could not find column '%s'".formatted(columnName));
+		return 0;
+	}
+
+	private void assertFunctionNotInTable(String expectedName) {
+		FunctionTableModel model = (FunctionTableModel) functionTable.getModel();
+		List<FunctionRowObject> data = model.getModelData();
+		for (FunctionRowObject rowObject : data) {
+			Function f = rowObject.getFunction();
+			String name = f.getName();
+			if (name.equals(expectedName)) {
+				fail("The table should not have a function by name '%s'".formatted(expectedName));
+			}
+		}
+	}
+
+	private void assertFunctionInTable(String expectedName) {
+		FunctionTableModel model = (FunctionTableModel) functionTable.getModel();
+		List<FunctionRowObject> data = model.getModelData();
+		for (FunctionRowObject rowObject : data) {
+			Function f = rowObject.getFunction();
+			String name = f.getName();
+			if (name.equals(expectedName)) {
+				return;
+			}
+		}
+		fail("The table should have a function by name '%s'".formatted(expectedName));
+	}
+
+	private void renameFunction(Address entry, String newName) {
+
+		FunctionManager fm = program.getFunctionManager();
+		Function f = fm.getFunctionAt(entry);
+		Symbol symbol = f.getSymbol();
+		Namespace namespace = f.getParentNamespace();
+		RenameLabelCmd cmd =
+			new RenameLabelCmd(symbol, newName, namespace, SourceType.USER_DEFINED);
+		applyCmd(program, cmd);
+		waitForTable();
+	}
+
+	private Address addr(String s) {
+		AddressFactory af = program.getAddressFactory();
+		return af.getAddress(s);
+	}
+
+	private void filter(String text) {
+		GTableFilterPanel<?> filterPanel = functionTable.getTableFilterPanel();
+		runSwing(() -> filterPanel.setFilterText(text));
+		waitForTable();
+	}
+
 	private String getCopyText(int row, int column) {
 		Object value = runSwing(() -> functionTable.getValueAt(row, column));
 		assertNotNull(value);
@@ -227,8 +345,8 @@ public class FunctionWindowPluginTest extends AbstractGhidraHeadedIntegrationTes
 		runSwing(() -> table.editingStopped(new ChangeEvent(table)));
 	}
 
-	private void waitForNotBusy(GTable table) {
+	private void waitForTable() {
-		waitForTableModel((ThreadedTableModel<?, ?>) table.getModel());
+		waitForTableModel((ThreadedTableModel<?, ?>) functionTable.getModel());
 	}
 
 }

Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/CoalescingAddRemoveStrategy.java

@@ -4,22 +4,23 @@
  * 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 docking.widgets.table;
+package docking.widgets.table.threaded;
 
 import static docking.widgets.table.AddRemoveListItem.Type.*;
 
 import java.util.*;
 
-import docking.widgets.table.threaded.*;
+import docking.widgets.table.AddRemoveListItem;
+import docking.widgets.table.TableSortingContext;
 import ghidra.util.exception.CancelledException;
 import ghidra.util.task.TaskMonitor;
 
@@ -76,6 +77,10 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 	public void process(List<AddRemoveListItem<T>> addRemoveList, TableData<T> tableData,
 			TaskMonitor monitor) throws CancelledException {
 
+		if (addRemoveList.isEmpty()) {
+			return;
+		}
+
 		Set<AddRemoveListItem<T>> items = coalesceAddRemoveItems(addRemoveList);
 
 		//
@@ -85,11 +90,7 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 		// existing item still has the data used to sort it.  If the sort data has changed, then
 		// even this step will not allow the TableData to find the item in a search.
 		//
-		Map<T, T> hashed = new HashMap<>();
+		Map<T, T> hashed = hashAllTableData(tableData, monitor);
-		for (T t : tableData) {
-			hashed.put(t, t);
-		}
-
 		Set<T> failedToRemove = new HashSet<>();
 
 		int n = items.size();
@@ -103,14 +104,13 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 			if (item.isChange()) {
 				T toRemove = hashed.remove(value);
 				remove(tableData, toRemove, failedToRemove);
-				monitor.incrementProgress(1);
 			}
 			else if (item.isRemove()) {
 				T toRemove = hashed.remove(value);
 				remove(tableData, toRemove, failedToRemove);
 				it.remove();
 			}
-			monitor.checkCancelled();
+			monitor.increment();
 		}
 
 		if (!failedToRemove.isEmpty()) {
@@ -119,7 +119,7 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 			monitor.setMessage("Removing " + message);
 
 			tableData.process((data, sortContext) -> {
-				return expungeLostItems(failedToRemove, data, sortContext);
+				return expungeLostItems(failedToRemove, data, sortContext, monitor);
 			});
 		}
 
@@ -137,8 +137,7 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 				tableData.insert(value);
 				hashed.put(value, value);
 			}
-			monitor.checkCancelled();
+			monitor.increment();
-			monitor.incrementProgress(1);
 		}
 
 		monitor.setMessage("Done adding/removing");
@@ -165,6 +164,17 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 		return new HashSet<>(map.values());
 	}
 
+	private Map<T, T> hashAllTableData(TableData<T> tableData, TaskMonitor monitor)
+			throws CancelledException {
+		TableData<T> allData = tableData.getRootData();
+		Map<T, T> hashed = new HashMap<>();
+		for (T t : allData) {
+			monitor.checkCancelled();
+			hashed.put(t, t);
+		}
+		return hashed;
+	}
+
 	private void handleAdd(AddRemoveListItem<T> item, Map<T, AddRemoveListItem<T>> map) {
 
 		T rowObject = item.getValue();
@@ -241,7 +251,7 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 	 * against the entire list of table data, locating the item to be removed.
 	 */
 	private List<T> expungeLostItems(Set<T> toRemove, List<T> data,
-			TableSortingContext<T> sortContext) {
+			TableSortingContext<T> sortContext, TaskMonitor monitor) {
 
 		if (sortContext.isUnsorted()) {
 			// this can happen if the data is unsorted and we were asked to remove an item that
@@ -249,10 +259,14 @@ public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T>
 			return data;
 		}
 
-		// Copy to a new list those items that are not marked for removal.  This saves the
+		// Copy to a new list those items that are not marked for removal.  This saves a list move 
-		// list move its items every time a remove takes place
+		// time a remove takes place
 		List<T> newList = new ArrayList<>(data.size() - toRemove.size());
 		for (int i = 0; i < data.size(); i++) {
+			if (monitor.isCancelled()) {
+				return newList;
+			}
+
 			T rowObject = data.get(i);
 			if (!toRemove.contains(rowObject)) {
 				newList.add(rowObject);

Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java

@@ -170,7 +170,7 @@ public class TableData<ROW_OBJECT> implements Iterable<ROW_OBJECT> {
 			return true;
 		}
 
-		// We used to have code that pass proxy objects to this class to remove items.  That code
+		// We used to have code that passed proxy objects to this class to remove items.  That code
 		// has been updated to no longer pass proxy objects.  Leaving this code here for a while
 		// just in case we find another client doing the same thing.
 		// return data.remove(t);
@@ -189,6 +189,10 @@ public class TableData<ROW_OBJECT> implements Iterable<ROW_OBJECT> {
 	public void process(
 			BiFunction<List<ROW_OBJECT>, TableSortingContext<ROW_OBJECT>, List<ROW_OBJECT>> function) {
 
+		if (!isSorted()) {
+			return;
+		}
+
 		if (source != null) {
 			source.process(function);
 		}

Ghidra/Framework/Docking/src/test/java/docking/widgets/table/threaded/CoalescingAddRemoveStrategyTest.java

@@ -4,16 +4,16 @@
  * 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 docking.widgets.table;
+package docking.widgets.table.threaded;
 
 import static docking.widgets.table.AddRemoveListItem.Type.*;
 import static org.junit.Assert.*;
@@ -23,7 +23,8 @@ import java.util.*;
 import org.junit.Before;
 import org.junit.Test;
 
-import docking.widgets.table.threaded.TestTableData;
+import docking.widgets.table.*;
+import docking.widgets.table.threaded.CoalescingAddRemoveStrategy;
 import ghidra.util.task.TaskMonitor;
 
 public class CoalescingAddRemoveStrategyTest {