diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/bookmark/BookmarkPluginTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/bookmark/BookmarkPluginTest.java index 903aca2d01..9f5ae523c1 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/bookmark/BookmarkPluginTest.java +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/bookmark/BookmarkPluginTest.java @@ -111,7 +111,7 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { BookmarkType[] types = bmMgr.getBookmarkTypes(); assertEquals(8, types.length); - checkBookmarkList(bookmarks); + checkBookmarkTable(bookmarks); DockingActionIf pa = getAction(plugin, "Filter Bookmarks"); performAction(pa, false); @@ -123,7 +123,7 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { }); Bookmark[] bms = new Bookmark[] { bookmarks[6], bookmarks[7], bookmarks[8] }; - checkBookmarkList(bms); + checkBookmarkTable(bms); } @Test @@ -131,7 +131,7 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { showBookmarkProvider(); - checkBookmarkList(bookmarks); + checkBookmarkTable(bookmarks); BookmarkTableModel model = (BookmarkTableModel) table.getModel(); DefaultTableTextFilterFactory factory = new DefaultTableTextFilterFactory<>(new FilterOptions()); @@ -141,16 +141,16 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { runSwing(() -> model.setTableFilter(factory.getTableFilter("help", transformer))); Bookmark[] bms = new Bookmark[] {}; - checkBookmarkList(bms); + checkBookmarkTable(bms); runSwing(() -> model.setTableFilter(factory.getTableFilter("Test20", transformer))); bms = new Bookmark[] { bookmarks[1], bookmarks[2] }; - checkBookmarkList(bms); + checkBookmarkTable(bms); runSwing(() -> model.setTableFilter(null)); - checkBookmarkList(bookmarks); + checkBookmarkTable(bookmarks); } @Test @@ -332,7 +332,7 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { Bookmark[] bms = new Bookmark[] { bookmarks[0], bookmarks[1], bookmarks[3], bookmarks[4], bookmarks[5], bookmarks[6], bookmarks[7], bookmarks[8] }; - checkBookmarkList(bms); + checkBookmarkTable(bms); selectRow(2, false); selectRow(5, true); @@ -340,7 +340,7 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { performAction(deleteAction, true); bms = new Bookmark[] { bookmarks[0], bookmarks[1], bookmarks[4], bookmarks[5], bookmarks[7], bookmarks[8] }; - checkBookmarkList(bms); + checkBookmarkTable(bms); } @@ -395,7 +395,7 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { @Test public void testAddBookmark() throws Exception { showBookmarkProvider(); - checkBookmarkList(bookmarks); + checkBookmarkTable(bookmarks); DockingActionIf pa = getAction(plugin, "Add Bookmark"); performAction(pa, codeViewerProvider, false); @@ -417,13 +417,13 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { Bookmark[] bks = new Bookmark[10]; list.toArray(bks); Arrays.sort(bks); - checkBookmarkList(bks); + checkBookmarkTable(bks); undo(program); - checkBookmarkList(bookmarks); + checkBookmarkTable(bookmarks); redo(program); - checkBookmarkList(bks); + checkBookmarkTable(bks); } @Test @@ -446,6 +446,37 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { assertFalse(list.contains(bookmark)); } + @Test + public void testDeleteBookmarkUpdatesTable() throws Exception { + + // + // This test catches a bug with removing a deleted bookmark from the table. The bug was + // triggered by a failure in the binary search. Specifically, if the binary search does + // not touch the item to be deleted, then the item would not get removed from the table. + // + + showBookmarkProvider(); + + List list = getBookmarks(program.getBookmarkManager()); + int n = list.size(); + int rowCount = n; + for (int i = 0; i < n; i++) { + + // cannot merely delete from the beginning to trigger this bug + int index = getRandomInt(0, list.size() - 1); + Bookmark bookmark = list.get(index); + Address address = bookmark.getAddress(); + DeleteBookmarkAction action = new DeleteBookmarkAction(plugin, bookmark, true); + MarkerLocation markerLocation = new MarkerLocation(null, program, address, 0, 0); + performAction(action, createContext(markerLocation), true); + + waitForTable(); + assertEquals(rowCount - 1, table.getRowCount()); + list = getBookmarks(program.getBookmarkManager()); + rowCount = list.size(); + } + } + @Test public void testMultipleDelete() throws Exception { showBookmarkProvider(); @@ -739,7 +770,7 @@ public class BookmarkPluginTest extends AbstractGhidraHeadedIntegrationTest { waitForTableModel((ThreadedTableModel) table.getModel()); } - private void checkBookmarkList(Bookmark[] bookmarksToCheck) { + private void checkBookmarkTable(Bookmark[] bookmarksToCheck) { waitForBusyTool(tool); waitForTable(); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/DefaultAddRemoveStrategy.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/DefaultAddRemoveStrategy.java index 17c085b223..c25957bfe2 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/DefaultAddRemoveStrategy.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/DefaultAddRemoveStrategy.java @@ -15,9 +15,10 @@ */ package docking.widgets.table.threaded; -import java.util.List; +import java.util.*; import docking.widgets.table.AddRemoveListItem; +import docking.widgets.table.TableSortingContext; import ghidra.util.exception.CancelledException; import ghidra.util.task.TaskMonitor; @@ -25,37 +26,70 @@ import ghidra.util.task.TaskMonitor; * A strategy that uses the table's sort state to perform a binary search of items to be added * and removed. * + *

This strategy is aware that some items may not be correctly removed, such as database items + * that have been deleted externally to the table. These items may require a brute force update + * to achieve removal. + * * @param the row type */ public class DefaultAddRemoveStrategy implements TableAddRemoveStrategy { @Override - public void process(List> addRemoveList, TableData updatedData, + public void process(List> addRemoveList, TableData tableData, TaskMonitor monitor) throws CancelledException { int n = addRemoveList.size(); monitor.setMessage("Adding/Removing " + n + " items..."); monitor.initialize(n); - // Note: this class does not directly perform a binary such, but instead relies on that + Set failedToRemove = new HashSet<>(); + + // Note: this class does not directly perform a binary search, but instead relies on that // work to be done by the call to TableData.remove() for (int i = 0; i < n; i++) { AddRemoveListItem item = addRemoveList.get(i); T value = item.getValue(); if (item.isChange()) { - updatedData.remove(value); - updatedData.insert(value); + tableData.remove(value); + tableData.insert(value); } else if (item.isRemove()) { - updatedData.remove(value); + if (!tableData.remove(value)) { + failedToRemove.add(value); + } } else if (item.isAdd()) { - updatedData.insert(value); + tableData.insert(value); } monitor.checkCanceled(); monitor.setProgress(i); } + + if (!failedToRemove.isEmpty()) { + int size = failedToRemove.size(); + String message = size == 1 ? "1 deleted item..." : size + " deleted items..."; + monitor.setMessage("Removing " + message); + + tableData.process((data, sortContext) -> { + return expungeLostItems(failedToRemove, data, sortContext); + }); + } + monitor.setMessage("Done adding/removing"); } + private List expungeLostItems(Set toRemove, List data, + TableSortingContext sortContext) { + + List newList = new ArrayList<>(data.size() - toRemove.size()); + for (int i = 0; i < data.size(); i++) { + T rowObject = data.get(i); + if (!toRemove.contains(rowObject)) { + newList.add(rowObject); + } + } + + return newList; + } + }