Merge remote-tracking branch

'origin/GP-989-dragonmacher-bookmark-table-delete' into Ghidra_10.0
(closes #3066)

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<BookmarkRowObject> 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<Bookmark> 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<Bookmark, ?>) table.getModel());
 	}
 
-	private void checkBookmarkList(Bookmark[] bookmarksToCheck) {
+	private void checkBookmarkTable(Bookmark[] bookmarksToCheck) {
 		waitForBusyTool(tool);
 		waitForTable();
 

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.   
  *
+ * <P>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 <T> the row type
  */
 public class DefaultAddRemoveStrategy<T> implements TableAddRemoveStrategy<T> {
 
 	@Override
-	public void process(List<AddRemoveListItem<T>> addRemoveList, TableData<T> updatedData,
+	public void process(List<AddRemoveListItem<T>> addRemoveList, TableData<T> 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<T> 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<T> item = addRemoveList.get(i);
 			T value = item.getValue();
 			if (item.isChange()) {
-				updatedData.remove(value);
+				tableData.remove(value);
-				updatedData.insert(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<T> expungeLostItems(Set<T> toRemove, List<T> data,
+			TableSortingContext<T> sortContext) {
+
+		List<T> 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;
+	}
+
 }