diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/functionwindow/FunctionWindowPluginTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/functionwindow/FunctionWindowPluginTest.java index ec408d1fbc..690e4e285f 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/functionwindow/FunctionWindowPluginTest.java +++ b/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 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 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) { - waitForTableModel((ThreadedTableModel) table.getModel()); + private void waitForTable() { + waitForTableModel((ThreadedTableModel) functionTable.getModel()); } } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/CoalescingAddRemoveStrategy.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/CoalescingAddRemoveStrategy.java similarity index 92% rename from Ghidra/Framework/Docking/src/main/java/docking/widgets/table/CoalescingAddRemoveStrategy.java rename to Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/CoalescingAddRemoveStrategy.java index 8f13c73523..31ed1cf671 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/CoalescingAddRemoveStrategy.java +++ b/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 implements TableAddRemoveStrategy public void process(List> addRemoveList, TableData tableData, TaskMonitor monitor) throws CancelledException { + if (addRemoveList.isEmpty()) { + return; + } + Set> items = coalesceAddRemoveItems(addRemoveList); // @@ -85,11 +90,7 @@ public class CoalescingAddRemoveStrategy implements TableAddRemoveStrategy // 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 hashed = new HashMap<>(); - for (T t : tableData) { - hashed.put(t, t); - } - + Map hashed = hashAllTableData(tableData, monitor); Set failedToRemove = new HashSet<>(); int n = items.size(); @@ -103,14 +104,13 @@ public class CoalescingAddRemoveStrategy implements TableAddRemoveStrategy 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 implements TableAddRemoveStrategy 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 implements TableAddRemoveStrategy tableData.insert(value); hashed.put(value, value); } - monitor.checkCancelled(); - monitor.incrementProgress(1); + monitor.increment(); } monitor.setMessage("Done adding/removing"); @@ -165,6 +164,17 @@ public class CoalescingAddRemoveStrategy implements TableAddRemoveStrategy return new HashSet<>(map.values()); } + private Map hashAllTableData(TableData tableData, TaskMonitor monitor) + throws CancelledException { + TableData allData = tableData.getRootData(); + Map hashed = new HashMap<>(); + for (T t : allData) { + monitor.checkCancelled(); + hashed.put(t, t); + } + return hashed; + } + private void handleAdd(AddRemoveListItem item, Map> map) { T rowObject = item.getValue(); @@ -241,7 +251,7 @@ public class CoalescingAddRemoveStrategy implements TableAddRemoveStrategy * against the entire list of table data, locating the item to be removed. */ private List expungeLostItems(Set toRemove, List data, - TableSortingContext sortContext) { + TableSortingContext 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 implements TableAddRemoveStrategy return data; } - // Copy to a new list those items that are not marked for removal. This saves the - // list move its items every time a remove takes place + // Copy to a new list those items that are not marked for removal. This saves a list move + // time a remove takes place List 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); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java index 1e93a0c8ab..6cfc4f8884 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/TableData.java @@ -170,7 +170,7 @@ public class TableData implements Iterable { 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 implements Iterable { public void process( BiFunction, TableSortingContext, List> function) { + if (!isSorted()) { + return; + } + if (source != null) { source.process(function); } diff --git a/Ghidra/Framework/Docking/src/test/java/docking/widgets/table/CoalescingAddRemoveStrategyTest.java b/Ghidra/Framework/Docking/src/test/java/docking/widgets/table/threaded/CoalescingAddRemoveStrategyTest.java similarity index 98% rename from Ghidra/Framework/Docking/src/test/java/docking/widgets/table/CoalescingAddRemoveStrategyTest.java rename to Ghidra/Framework/Docking/src/test/java/docking/widgets/table/threaded/CoalescingAddRemoveStrategyTest.java index c2eae82f58..1ff41f4ccf 100644 --- a/Ghidra/Framework/Docking/src/test/java/docking/widgets/table/CoalescingAddRemoveStrategyTest.java +++ b/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 {