GT-2724,2216 - Table Chooser Dialog - Improvements: 1) objects instead

of row numbers are used to track work items, 2) added API methods for
things like removing items, and getting a dialog closed notification;
added tests

Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java

@@ -15,34 +15,58 @@
  */
 package ghidra.app.tablechooser;
 
-import java.awt.BorderLayout;
+import java.awt.*;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import javax.swing.*;
-import javax.swing.event.ListSelectionEvent;
+import javax.swing.table.TableCellRenderer;
-import javax.swing.event.ListSelectionListener;
 
 import docking.*;
 import docking.action.*;
+import docking.widgets.table.*;
+import docking.widgets.table.threaded.ThreadedTableModel;
 import ghidra.app.nav.Navigatable;
 import ghidra.app.nav.NavigatableRemovalListener;
 import ghidra.app.services.GoToService;
 import ghidra.app.util.HelpTopics;
 import ghidra.framework.plugintool.PluginTool;
+import ghidra.generic.function.Callback;
 import ghidra.program.model.listing.Program;
 import ghidra.program.util.ProgramLocation;
 import ghidra.program.util.ProgramSelection;
 import ghidra.util.HelpLocation;
-import ghidra.util.Msg;
+import ghidra.util.SystemUtilities;
+import ghidra.util.datastruct.WeakDataStructureFactory;
+import ghidra.util.datastruct.WeakSet;
 import ghidra.util.table.*;
 import ghidra.util.task.TaskMonitor;
 import resources.ResourceManager;
 
-public class TableChooserDialog extends DialogComponentProvider implements
+/**
-		NavigatableRemovalListener {
+ * Dialog to show a table of items.  If the dialog is constructed with a non-null 
+ * {@link TableChooserExecutor}, then a button will be placed in the dialog, allowing the user
+ * to perform the action defined by the executor.
+ * 
+ * <p>Each button press will use the selected items as the items to be processed.  While the 
+ * items are schedule to be processed, they will still be in the table, painted light gray.  
+ * Attempting to reschedule any of these pending items will have no effect.   Each time the 
+ * button is pressed, a new {@link SwingWorker} is created, which will put the processing into
+ * a background thread.   Further, by using multiple workers, the work will be performed in
+ * parallel.
+ */
+public class TableChooserDialog extends DialogComponentProvider
+		implements NavigatableRemovalListener {
+
+	// thread-safe data structures
+	private WeakSet<ExecutorSwingWorker> workers =
+		WeakDataStructureFactory.createCopyOnReadWeakSet();
+	private Set<AddressableRowObject> sharedPending = ConcurrentHashMap.newKeySet();
 
 	private final TableChooserExecutor executor;
-	private Set<ExecutorSwingWorker> workers = new HashSet<ExecutorSwingWorker>();
+	private WrappingCellRenderer wrappingRenderer = new WrappingCellRenderer();
 
 	private GhidraTable table;
 	private TableChooserTableModel model;
@@ -50,6 +74,8 @@ public class TableChooserDialog extends DialogComponentProvider implements
 	private final PluginTool tool;
 	private Navigatable navigatable;
 
+	private Callback closedCallback = Callback.dummy();
+
 	public TableChooserDialog(PluginTool tool, TableChooserExecutor executor, Program program,
 			String title, Navigatable navigatable, boolean isModal) {
 
@@ -66,7 +92,6 @@ public class TableChooserDialog extends DialogComponentProvider implements
 		addDismissButton();
 		createActions();
 		setOkEnabled(false);
-
 	}
 
 	public TableChooserDialog(PluginTool tool, TableChooserExecutor executor, Program program,
@@ -77,8 +102,7 @@ public class TableChooserDialog extends DialogComponentProvider implements
 	private JPanel buildMainPanel() {
 		JPanel panel = new JPanel(new BorderLayout());
 		createTableModel();
-		GhidraThreadedTablePanel<AddressableRowObject> tablePanel =
+		TableChooserDialogPanel tablePanel = new TableChooserDialogPanel(model);
-			new GhidraThreadedTablePanel<AddressableRowObject>(model, 50, 2000);
 
 		table = tablePanel.getTable();
 		GoToService goToService = tool.getService(GoToService.class);
@@ -87,12 +111,8 @@ public class TableChooserDialog extends DialogComponentProvider implements
 			navigatable.addNavigatableListener(this);
 			table.installNavigation(goToService, navigatable);
 		}
-		table.getSelectionModel().addListSelectionListener(new ListSelectionListener() {
+		table.getSelectionModel().addListSelectionListener(
-			@Override
+			e -> setOkEnabled(table.getSelectedRowCount() > 0));
-			public void valueChanged(ListSelectionEvent e) {
-				setOkEnabled(table.getSelectedRowCount() > 0);
-			}
-		});
 
 		GhidraTableFilterPanel<AddressableRowObject> filterPanel =
 			new GhidraTableFilterPanel<AddressableRowObject>(table, model);
@@ -101,22 +121,38 @@ public class TableChooserDialog extends DialogComponentProvider implements
 		return panel;
 	}
 
+	/**
+	 * Sets the given listener that will get notified when this dialog is closed
+	 * @param callback the callback to notify
+	 */
+	public void setClosedListener(Callback callback) {
+		this.closedCallback = Callback.dummyIfNull(callback);
+	}
+
+	/**
+	 * Adds the given object to this dialog.  This method can be called from any thread.
+	 * 
+	 * @param rowObject the object to add
+	 */
 	public void add(AddressableRowObject rowObject) {
 		model.addObject(rowObject);
 	}
 
+	/**
+	 * Removes the given object from this dialog.  Nothing will happen if the given item is not 
+	 * in this dialog.  This method can be called from any thread.
+	 * 
+	 * @param rowObject the object to remove
+	 */
+	public void remove(AddressableRowObject rowObject) {
+		model.removeObject(rowObject);
+	}
+
 	private void createTableModel() {
-		try {
+
-			SwingUtilities.invokeAndWait(new Runnable() {
+		// note: the task monitor is installed later when this model is added to the threaded panel
-				@Override
+		SystemUtilities.runSwingNow(
-				public void run() {
+			() -> model = new TableChooserTableModel("Test", tool, program, null));
-					model = new TableChooserTableModel("Test", tool, program, null /* set later*/);
-				}
-			});
-		}
-		catch (Exception e) {
-			Msg.showError(this, null, "Error Creating Table", "Error Creating Table", e);
-		}
 	}
 
 	private void createActions() {
@@ -140,8 +176,8 @@ public class TableChooserDialog extends DialogComponentProvider implements
 		selectAction.setHelpLocation(new HelpLocation(HelpTopics.SEARCH, "Make_Selection"));
 
 		DockingAction selectionNavigationAction = new SelectionNavigationAction(owner, table);
-		selectionNavigationAction.setHelpLocation(new HelpLocation(HelpTopics.SEARCH,
+		selectionNavigationAction.setHelpLocation(
-			"Selection_Navigation"));
+			new HelpLocation(HelpTopics.SEARCH, "Selection_Navigation"));
 
 		addAction(selectAction);
 		addAction(selectionNavigationAction);
@@ -172,61 +208,90 @@ public class TableChooserDialog extends DialogComponentProvider implements
 		}
 	}
 
+	@Override
+	protected void dialogClosed() {
+		closedCallback.call();
+	}
+
 	@Override
 	protected void okCallback() {
 
-		TaskMonitor monitor = showTaskMonitorComponent(executor.getButtonName(), true, true);
+		List<AddressableRowObject> rowObjects = getSelectedRowObjects();
-
+		rowObjects.removeAll(sharedPending); // only keep selected items not being processed
-		try {
+		if (rowObjects.isEmpty()) {
-			ExecutorSwingWorker worker = new ExecutorSwingWorker(monitor);
+			return;
-			worker.execute();
-			workers.add(worker);
 		}
-		finally {
+
+		clearSelection(); // prevent odd behavior with selection around as the table changes
+		sharedPending.addAll(rowObjects);
+
+		TaskMonitor monitor = getTaskMonitorComponent();
+		ExecutorSwingWorker worker = new ExecutorSwingWorker(rowObjects, monitor);
+		workers.add(worker);
+
+		showProgressBar("Working", true, true, 0);
+		worker.execute();
+	}
+
+	private void workerDone(ExecutorSwingWorker worker) {
+		workers.remove(worker);
+		if (workers.isEmpty()) {
 			hideTaskMonitorComponent();
 		}
 	}
 
 	public boolean isBusy() {
-		ExecutorSwingWorker[] threadSafeArray =
+		for (ExecutorSwingWorker worker : workers) {
-			workers.toArray(new ExecutorSwingWorker[workers.size()]);
-		for (ExecutorSwingWorker worker : threadSafeArray) {
 			if (!worker.isDone()) {
 				return true;
 			}
 		}
-		return false;
+
+		return model.isBusy();
 	}
 
-	private void doExecute(TaskMonitor monitor) {
+	private void doExecute(List<AddressableRowObject> rowObjects, TaskMonitor monitor) {
-		int[] selectedRows = table.getSelectedRows();
 
-		monitor.initialize(selectedRows.length);
+		monitor.initialize(rowObjects.size());
 
-		List<AddressableRowObject> deletedRowObjects = new ArrayList<AddressableRowObject>();
+		try {
-		for (int selectedRow : selectedRows) {
+			List<AddressableRowObject> deleted = doProcessRowObjects(rowObjects, monitor);
+			for (AddressableRowObject rowObject : deleted) {
+				model.removeObject(rowObject);
+			}
+		}
+		finally {
+			// Note: the code below this comment needs to happen, even if the monitor is cancelled
+			sharedPending.removeAll(rowObjects);
+			model.fireTableDataChanged();
+			setStatusText("");
+		}
+	}
+
+	private List<AddressableRowObject> doProcessRowObjects(List<AddressableRowObject> rowObjects,
+			TaskMonitor monitor) {
+		List<AddressableRowObject> deleted = new ArrayList<AddressableRowObject>();
+		for (AddressableRowObject rowObject : rowObjects) {
 			if (monitor.isCancelled()) {
-				return;
+				break;
 			}
 
-			AddressableRowObject rowObject = model.getRowObject(selectedRow);
+			if (!model.containsObject(rowObject)) {
+				// this implies the item has been programmatically removed
+				monitor.incrementProgress(1);
+				continue;
+			}
 
 			monitor.setMessage("Processing item at address " + rowObject.getAddress());
-
 			if (executor.execute(rowObject)) {
-				deletedRowObjects.add(rowObject);
+				deleted.add(rowObject);
 			}
 
 			monitor.incrementProgress(1);
 			table.repaint(); // in case the data is updated while processing
 		}
 
-		for (AddressableRowObject addressableRowObject : deletedRowObjects) {
+		return deleted;
-			model.removeObject(addressableRowObject);
-		}
-
-		model.fireTableDataChanged();
-		setStatusText("");
 	}
 
 	public void addCustomColumn(ColumnDisplay<?> columnDisplay) {
@@ -246,31 +311,119 @@ public class TableChooserDialog extends DialogComponentProvider implements
 		return model.getRowCount();
 	}
 
+	public void clearSelection() {
+		table.clearSelection();
+	}
+
+	public void selectRows(int... rows) {
+
+		ListSelectionModel selectionModel = table.getSelectionModel();
+		for (int row : rows) {
+			selectionModel.addSelectionInterval(row, row);
+		}
+	}
+
+	public int[] getSelectedRows() {
+		int[] selectedRows = table.getSelectedRows();
+		return selectedRows;
+	}
+
+	public List<AddressableRowObject> getSelectedRowObjects() {
+		int[] selectedRows = table.getSelectedRows();
+		List<AddressableRowObject> rowObjects = model.getRowObjects(selectedRows);
+		return rowObjects;
+	}
+
 //==================================================================================================
 // Inner Classes
 //==================================================================================================
 
+	private class TableChooserDialogPanel extends GhidraThreadedTablePanel<AddressableRowObject> {
+
+		public TableChooserDialogPanel(ThreadedTableModel<AddressableRowObject, ?> model) {
+			super(model, 50, 2000);
+		}
+
+		@Override
+		protected GTable createTable(ThreadedTableModel<AddressableRowObject, ?> tm) {
+			return new TableChooserDialogGhidraTable(tm);
+		}
+	}
+
+	private class TableChooserDialogGhidraTable extends GhidraTable {
+
+		public TableChooserDialogGhidraTable(ThreadedTableModel<AddressableRowObject, ?> tm) {
+			super(tm);
+		}
+
+		@Override
+		public TableCellRenderer getCellRenderer(int row, int col) {
+			TableCellRenderer tableRenderer = super.getCellRenderer(row, col);
+			wrappingRenderer.setDelegate(tableRenderer);
+			return wrappingRenderer;
+		}
+	}
+
+	private class WrappingCellRenderer extends GhidraTableCellRenderer {
+
+		private Color pendingColor = new Color(192, 192, 192, 75);
+		private TableCellRenderer delegate;
+
+		@Override
+		public Component getTableCellRendererComponent(GTableCellRenderingData data) {
+
+			Component superRenderer;
+			if (delegate instanceof GTableCellRenderer) {
+				superRenderer = super.getTableCellRendererComponent(data);
+			}
+			else {
+				superRenderer = super.getTableCellRendererComponent(data.getTable(),
+					data.getValue(), data.isSelected(), data.hasFocus(), data.getRowViewIndex(),
+					data.getColumnViewIndex());
+			}
+
+			AddressableRowObject ro = (AddressableRowObject) data.getRowObject();
+			if (sharedPending.contains(ro)) {
+				superRenderer.setBackground(pendingColor);
+				superRenderer.setForeground(data.getTable().getSelectionForeground());
+				superRenderer.setForeground(Color.BLACK);
+			}
+
+			return superRenderer;
+		}
+
+		void setDelegate(TableCellRenderer delegate) {
+			this.delegate = delegate;
+		}
+	}
+
 	/**
-	 * Runs our work off the Swing thread, so that the GUI updates as the task is being 
+	 * Runs our work off the Swing thread, so that the GUI updates as the task is being executed
-	 * executed.
 	 */
 	private class ExecutorSwingWorker extends SwingWorker<Object, Object> {
 
 		private final TaskMonitor monitor;
+		private List<AddressableRowObject> rowObjects;
 
-		ExecutorSwingWorker(TaskMonitor monitor) {
+		ExecutorSwingWorker(List<AddressableRowObject> rowObjects, TaskMonitor monitor) {
+			this.rowObjects = rowObjects;
 			this.monitor = monitor;
 		}
 
 		@Override
 		protected Object doInBackground() throws Exception {
-			doExecute(monitor);
+			doExecute(rowObjects, monitor);
 			return null;
 		}
 
 		@Override
 		protected void done() {
-			workers.remove(this);
+			workerDone(this);
+		}
+
+		@Override
+		public String toString() {
+			return rowObjects.toString();
 		}
 	}
 }

Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserExecutor.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.
@@ -30,9 +29,8 @@ public interface TableChooserExecutor {
 	 * Applies this executors action to the given rowObject.  Return true if the given object 
 	 * should be removed from the table. 
 	 * 
-	 * @param rowObject the AddressRowObject to be executed upon.
+	 * @param rowObject the AddressRowObject to be executed upon
-	 * @param monitor The monitor you can use to set status messages.
+	 * @return true if the rowObject should be removed from the table, false otherwise
-	 * @return true if the rowObject should be removed from the table, false otherwise.
 	 */
 	public boolean execute(AddressableRowObject rowObject);
 }

Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.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,6 +15,9 @@
  */
 package ghidra.app.tablechooser;
 
+import java.util.*;
+
+import docking.widgets.table.*;
 import ghidra.framework.plugintool.ServiceProvider;
 import ghidra.program.model.address.Address;
 import ghidra.program.model.listing.Program;
@@ -25,11 +27,10 @@ import ghidra.util.table.AddressBasedTableModel;
 import ghidra.util.table.field.AddressTableColumn;
 import ghidra.util.task.TaskMonitor;
 
-import java.util.*;
-
-import docking.widgets.table.*;
-
 public class TableChooserTableModel extends AddressBasedTableModel<AddressableRowObject> {
+
+	// we maintain this list so that any future reload operations can load the original user data
+	// (this downside of this is that two lists are maintained)
 	Set<AddressableRowObject> myPrivateList = new HashSet<AddressableRowObject>();
 
 	public TableChooserTableModel(String title, ServiceProvider serviceProvider, Program program,
@@ -49,6 +50,11 @@ public class TableChooserTableModel extends AddressBasedTableModel<AddressableRo
 		super.removeObject(obj);
 	}
 
+	public synchronized boolean containsObject(AddressableRowObject obj) {
+		// checking this list allows us to work around the threaded nature of our parent
+		return myPrivateList.contains(obj);
+	}
+
 	@Override
 	public Address getAddress(int row) {
 		return getRowObject(row).getAddress();

Ghidra/Features/Base/src/test/java/ghidra/app/tablechooser/TableChooserDialogTest.java

@@ -0,0 +1,394 @@
+/* ###
+ * 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.tablechooser;
+
+import static org.junit.Assert.*;
+
+import java.util.*;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.*;
+
+import ghidra.app.nav.Navigatable;
+import ghidra.framework.plugintool.DummyPluginTool;
+import ghidra.program.model.address.Address;
+import ghidra.program.model.address.TestAddress;
+import ghidra.program.model.listing.Program;
+import ghidra.test.AbstractGhidraHeadedIntegrationTest;
+import ghidra.test.ToyProgramBuilder;
+import util.CollectionUtils;
+
+public class TableChooserDialogTest extends AbstractGhidraHeadedIntegrationTest {
+
+	private static final String OK_BUTTON_TEXT = "Do Work";
+	private static final TestExecutorDecision DEFAULT_DECISION = r -> true;
+
+	private DummyPluginTool tool;
+	private TableChooserDialog dialog;
+	private SpyTableChooserExecutor executor;
+
+	/** Interface for tests to signal what is expected of the executor */
+	private TestExecutorDecision testDecision = DEFAULT_DECISION;
+
+	@Before
+	public void setUp() throws Exception {
+		executor = new SpyTableChooserExecutor();
+		createDialog(executor);
+	}
+
+	@After
+	public void tearDown() {
+		runSwing(() -> {
+			tool.close();
+			//dialog.close();
+		});
+	}
+
+	private void createDialog(SpyTableChooserExecutor dialogExecutor) throws Exception {
+		executor = dialogExecutor;
+
+		tool = new DummyPluginTool();
+		tool.setVisible(true);
+		Program program = new ToyProgramBuilder("Test", true).getProgram();
+		Navigatable navigatable = null;
+		dialog = new TableChooserDialog(tool, executor, program, "Title", navigatable);
+		dialog.show();
+		loadData();
+	}
+
+	private void reCreateDialog(SpyTableChooserExecutor dialogExecutor) throws Exception {
+		runSwing(() -> dialog.close());
+		createDialog(dialogExecutor);
+	}
+
+	private void loadData() {
+		for (int i = 0; i < 7; i++) {
+			dialog.add(new TestStubRowObject());
+		}
+
+		waitForDialog();
+	}
+
+	@Test
+	public void testClosedListener() {
+
+		AtomicBoolean called = new AtomicBoolean();
+		dialog.setClosedListener(() -> called.set(true));
+
+		runSwing(() -> dialog.close());
+
+		assertTrue("Dialog 'closed' listener not called", called.get());
+	}
+
+	@Test
+	public void testNullExecutor() throws Exception {
+		reCreateDialog(null);  // null executor
+
+		assertNull("OK button should not be showing",
+			findComponentByName(dialog.getComponent(), "OK"));
+	}
+
+	@Test
+	public void testButtonCallbabck() {
+
+		int rowCount = getRowCount();
+		TestStubRowObject rowObject = selectRow(0);
+
+		pressExecuteButton();
+		waitForDialog();
+
+		assertNotInDialog(rowObject);
+		assertRowCount(rowCount - 1);
+	}
+
+	@Test
+	public void testCallbackWithoutRemoval() {
+
+		int rowCount = getRowCount();
+		TestStubRowObject rowObject = selectRow(0);
+
+		testDecision = r -> false; // don't remove
+
+		pressExecuteButton();
+		waitForDialog();
+
+		assertInDialog(rowObject);
+		assertOnlyExecutedOnce(rowObject);
+		assertRowCount(rowCount);
+	}
+
+	@Test
+	public void testCalllbackRemovesItems_OtherItemSelected() {
+		/*
+		 	Select multiple items.  
+		 	Have the first callback remove one of the remaining *unselected* items.
+		 	The removed item should not itself get a callback.
+		 */
+
+		int rowCount = getRowCount();
+		List<TestStubRowObject> selected = selectRows(0, 2);
+		List<TestStubRowObject> toRemove = new ArrayList<>(toRowObjects(1, 3));
+
+		List<TestStubRowObject> removedButNotExecuted = new ArrayList<>();
+		testDecision = r -> {
+
+			// remove the non-selected items
+			for (TestStubRowObject other : toRemove) {
+				removedButNotExecuted.add(other);
+				dialog.remove(other);
+			}
+			toRemove.clear(); // only do this one time
+
+			return true; // remove 'r'
+		};
+
+		pressExecuteButton();
+		waitForDialog();
+		assertEquals("Did not remove all items", 2, removedButNotExecuted.size());
+
+		assertNotInDialog(selected);
+		assertNotInDialog(removedButNotExecuted);
+		assertRowCount(rowCount - (selected.size() + removedButNotExecuted.size()));
+		assertNotExecuted(removedButNotExecuted);
+	}
+
+	@Test
+	public void testCalllbackRemovesItems_OtherItemNotSelected() {
+
+		/*
+		 	Select multiple items.  
+		 	Have the first callback remove one of the remaining *selected* items.
+		 	The removed item should not itself get a callback.
+		 */
+
+		int rowCount = getRowCount();
+		List<TestStubRowObject> selected = selectRows(0, 1, 3);
+		List<TestStubRowObject> toProcess = new ArrayList<>(selected);
+
+		List<TestStubRowObject> removedButNotExecuted = new ArrayList<>();
+		testDecision = r -> {
+			toProcess.remove(r);
+
+			// if not empty, remove one of the remaining items
+			if (!toProcess.isEmpty()) {
+				TestStubRowObject other = toProcess.remove(0);
+				removedButNotExecuted.add(other);
+				dialog.remove(other);
+			}
+			return true; // remove 'r'
+		};
+
+		pressExecuteButton();
+		waitForDialog();
+		assertTrue(toProcess.isEmpty());
+
+		assertNotInDialog(selected);
+		assertRowCount(rowCount - selected.size());
+		assertNotExecuted(removedButNotExecuted);
+	}
+
+	@Test
+	public void testItemsRepeatedlyRequestedToBeProcessed() {
+
+		/*
+		 	The execution step of the dialog can be slow, depending upon what work the user is
+		 	doing in the callback.  Due to this, the UI allows the user to select the same item
+		 	while it is schedule to be processed.   This test ensures that an item processed and
+		 	removed in one scheduled request will not be processed again later.
+		 */
+
+		List<TestStubRowObject> selected1 = selectRows(0, 1, 2);
+
+		CountDownLatch startLatch = new CountDownLatch(1);
+		CountDownLatch continueLatch = new CountDownLatch(1);
+		testDecision = r -> {
+
+			// 
+			// Signal that we have started and wait to continue
+			//
+			startLatch.countDown();
+			waitFor(continueLatch);
+
+			return true; // remove 'r'
+		};
+
+		pressExecuteButton();
+		waitFor(startLatch);
+
+		List<TestStubRowObject> selected2 = selectRows(1);
+		pressExecuteButton();      // schedule the second request
+		continueLatch.countDown(); // release the first scheduled request
+
+		waitForDialog();
+
+		assertNotInDialog(selected1);
+		assertNotInDialog(selected2);
+		assertOnlyExecutedOnce(selected2);
+	}
+
+//==================================================================================================
+// Private Methods
+//==================================================================================================
+
+	private void assertRowCount(int expected) {
+		int actual = getRowCount();
+		assertEquals("Table model row count is not as expected", expected, actual);
+	}
+
+	private void assertInDialog(TestStubRowObject... rowObject) {
+		assertInDialog(Arrays.asList(rowObject));
+	}
+
+	private void assertInDialog(List<TestStubRowObject> rowObjects) {
+		TableChooserTableModel model = getModel();
+		for (TestStubRowObject rowObject : rowObjects) {
+			int index = runSwing(() -> model.getRowIndex(rowObject));
+			assertTrue("Row object is not in the dialog", index >= 0);
+		}
+	}
+
+	private void assertNotInDialog(TestStubRowObject... rowObjects) {
+		assertNotInDialog(Arrays.asList(rowObjects));
+	}
+
+	private void assertNotInDialog(List<TestStubRowObject> rowObjects) {
+		TableChooserTableModel model = getModel();
+		for (TestStubRowObject rowObject : rowObjects) {
+			int index = runSwing(() -> model.getRowIndex(rowObject));
+			assertFalse("Row object is still in the dialog", index >= 0);
+		}
+	}
+
+	private void assertNotExecuted(List<TestStubRowObject> removedButNotExecuted) {
+		for (TestStubRowObject rowObject : removedButNotExecuted) {
+			assertFalse("Row object was unexpectedly processed by the Executor",
+				executor.wasExecuted(rowObject));
+		}
+	}
+
+	private void assertOnlyExecutedOnce(TestStubRowObject... rowObjects) {
+		assertOnlyExecutedOnce(Arrays.asList(rowObjects));
+	}
+
+	private void assertOnlyExecutedOnce(List<TestStubRowObject> rowObjects) {
+		for (TestStubRowObject rowObject : rowObjects) {
+			assertEquals("Row object was unexpectedly processed by the Executor", 1,
+				executor.getExecutedCount(rowObject));
+		}
+	}
+
+	private List<TestStubRowObject> toRowObjects(int... rows) {
+
+		List<TestStubRowObject> results = new ArrayList<>();
+		for (int row : rows) {
+			AddressableRowObject r = runSwing(() -> getModel().getRowObject(row));
+			results.add((TestStubRowObject) r);
+		}
+		return results;
+	}
+
+	private void waitForDialog() {
+		waitForCondition(() -> !dialog.isBusy());
+	}
+
+	private void pressExecuteButton() {
+		pressButtonByName(dialog.getComponent(), "OK");
+	}
+
+	private int getRowCount() {
+		return runSwing(() -> dialog.getRowCount());
+	}
+
+	private TestStubRowObject selectRow(int row) {
+		List<TestStubRowObject> selected = selectRows(row);
+		return selected.get(0);
+	}
+
+	private List<TestStubRowObject> selectRows(int... row) {
+		runSwing(() -> dialog.clearSelection());
+		runSwing(() -> dialog.selectRows(row));
+		List<AddressableRowObject> selected = runSwing(() -> dialog.getSelectedRowObjects());
+		return CollectionUtils.asList(selected, TestStubRowObject.class);
+	}
+
+	private TableChooserTableModel getModel() {
+		return (TableChooserTableModel) getInstanceField("model", dialog);
+	}
+
+//==================================================================================================
+// Inner Classes
+//==================================================================================================	
+
+	private interface TestExecutorDecision {
+		public boolean decide(AddressableRowObject rowObject);
+	}
+
+	private class SpyTableChooserExecutor implements TableChooserExecutor {
+
+		private Map<AddressableRowObject, AtomicInteger> callbacks = new HashMap<>();
+
+		@Override
+		public String getButtonName() {
+			return OK_BUTTON_TEXT;
+		}
+
+		int getExecutedCount(TestStubRowObject rowObject) {
+			AtomicInteger counter = callbacks.get(rowObject);
+			if (counter == null) {
+				return 0;
+			}
+			return counter.get();
+		}
+
+		@Override
+		public boolean execute(AddressableRowObject rowObject) {
+
+			callbacks.merge(rowObject, new AtomicInteger(1), (k, v) -> {
+				v.incrementAndGet();
+				return v;
+			});
+
+			boolean result = testDecision.decide(rowObject);
+			return result;
+		}
+
+		boolean wasExecuted(AddressableRowObject rowObject) {
+			return callbacks.containsKey(rowObject);
+		}
+	}
+
+	private static class TestStubRowObject implements AddressableRowObject {
+
+		private static int counter;
+		private long addr;
+
+		TestStubRowObject() {
+			addr = ++counter;
+		}
+
+		@Override
+		public Address getAddress() {
+			return new TestAddress(addr);
+		}
+
+		@Override
+		public String toString() {
+			return getAddress().toString();
+		}
+	}
+}

Ghidra/Framework/Generic/src/main/java/generic/platform/AbstractMacHandler.java

@@ -1,43 +0,0 @@
-/* ###
- * 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 generic.platform;
-
-import java.lang.reflect.InvocationHandler;
-
-import org.apache.commons.lang3.reflect.MethodUtils;
-
-/**
- * A general interface for handle Mac Application callbacks.  Some possible callbacks are:
- * <ul>
- * 	<li>quit</li>
- * 	<li>about</li>
- * 	<li>preferences</li>
- * 	<li>file handling</li>
- * </ul>
- * 
- * see com.apple.eawt.Application
- */
-abstract class AbstractMacHandler implements InvocationHandler {
-
-
-
-	protected Object getApplication() throws Exception {
-		Class<?> clazz = Class.forName("com.apple.eawt.Application");
-		Object application = MethodUtils.invokeExactStaticMethod(clazz, "getApplication");
-		return application;
-	}
-
-}

Ghidra/Framework/Generic/src/main/java/generic/platform/MacAboutHandler.java

@@ -1,77 +0,0 @@
-/* ###
- * 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 generic.platform;
-
-import java.lang.reflect.Method;
-import java.lang.reflect.Proxy;
-
-import org.apache.commons.lang3.reflect.MethodUtils;
-
-import ghidra.framework.OperatingSystem;
-import ghidra.framework.Platform;
-import ghidra.util.Msg;
-
-/**
- * A base implementation for creating an 'About' menu action callback.  This is executed when
- * the user presses the Dock's 'Ghidra->About' menu action.
- * <p>
- * Simply constructing this class will register it.
- * <p>
- * See 
- * com.apple.eawt.Application.setAboutHandler(AboutHandler)
- * com.apple.eawt.AboutHandler.handleAbout(AboutEvent)
- */
-public abstract class MacAboutHandler extends AbstractMacHandler {
-
-	public MacAboutHandler() {
-		addAboutApplicationListener();
-	}
-
-	public abstract void about();
-
-	private void addAboutApplicationListener() {
-		if (Platform.CURRENT_PLATFORM.getOperatingSystem() != OperatingSystem.MAC_OS_X) {
-			return;
-		}
-
-		try {
-			Object application = getApplication();
-			setAboutHandler(application);
-		}
-		catch (Exception e) {
-			Msg.error(this, "Unable to install Mac quit handler", e);
-		}
-	}
-
-	private void setAboutHandler(Object application) throws Exception {
-
-		Class<?> aboutHandlerClass = Class.forName("com.apple.eawt.AboutHandler");
-		Object aboutHandler = Proxy.newProxyInstance(getClass().getClassLoader(),
-			new Class[] { aboutHandlerClass }, this);
-		MethodUtils.invokeMethod(application, "setAboutHandler", aboutHandler);
-	}
-
-	@Override
-	public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
-
-		// Args: AboutEvent
-
-		about(); // call our about() callback, ignoring the Application API
-
-		// the handleAbout() is void--return null
-		return null;
-	}
-}

Ghidra/Framework/Generic/src/main/java/generic/platform/MacQuitHandler.java

@@ -1,83 +0,0 @@
-/* ###
- * 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 generic.platform;
-
-import java.lang.reflect.Method;
-import java.lang.reflect.Proxy;
-
-import org.apache.commons.lang3.reflect.MethodUtils;
-
-import ghidra.framework.OperatingSystem;
-import ghidra.framework.Platform;
-import ghidra.util.Msg;
-
-/**
- * A base implementation for creating an 'Quit' menu action callback.  This is executed when
- * the user presses the Dock's 'Ghidra->Quit' menu action.
- * <p>
- * Simply constructing this class will register it.
- * <p>
- * See 
- * com.apple.eawt.Application.setQuitHandler(QuitHandler)
- * com.apple.eawt.AboutHandler.handleQuitRequestWith(QuitEvent, QuitResponse)
- */
-public abstract class MacQuitHandler extends AbstractMacHandler {
-
-	public MacQuitHandler() {
-		addQuitApplicationListener(this);
-	}
-
-	public abstract void quit();
-
-	private void addQuitApplicationListener(MacQuitHandler macQuitHandler) {
-		if (Platform.CURRENT_PLATFORM.getOperatingSystem() != OperatingSystem.MAC_OS_X) {
-			return;
-		}
-
-		try {
-			Object application = getApplication();
-			setQuitHandler(application);
-		}
-		catch (Exception e) {
-			Msg.error(this, "Unable to install Mac quit handler", e);
-		}
-	}
-
-	private void setQuitHandler(Object application) throws Exception {
-
-		Class<?> quitHandlerClass = Class.forName("com.apple.eawt.QuitHandler");
-		Object quitHandler = Proxy.newProxyInstance(getClass().getClassLoader(),
-			new Class[] { quitHandlerClass }, this);
-		MethodUtils.invokeMethod(application, "setQuitHandler", quitHandler);
-	}
-
-	@Override
-	public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
-
-		// Args: QuitEvent event, QuitResponse response
-
-		// Call QuitResponse.cancelQuit(), as we will allow our tool to quit the application
-		// instead of the OS.
-		Object response = args[1];
-		MethodUtils.invokeExactMethod(response, "cancelQuit");
-
-		quit();
-
-		// the handleQuitRequestWith() is void--return null
-		return null;
-	}
-
-}

Ghidra/Framework/Generic/src/main/java/generic/test/AbstractGTest.java

@@ -20,12 +20,15 @@ import static org.junit.Assert.fail;
 
 import java.io.File;
 import java.util.*;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import java.util.function.BooleanSupplier;
 import java.util.function.Supplier;
 
 import org.junit.Assert;
 import org.junit.rules.TestName;
 
+import ghidra.framework.Application;
 import ghidra.framework.TestApplicationUtils;
 import ghidra.util.SystemUtilities;
 import ghidra.util.UniversalIdGenerator;
@@ -341,7 +344,24 @@ public abstract class AbstractGTest {
 	}
 
 	/**
-	 * Waits for the given condition to return true.
+	 * Waits for the given latch to be counted-down
+	 * 
+	 * @param latch the latch to await
+	 * @throws AssertionFailedError if the condition is not met within the timeout period
+	 */
+	public static void waitFor(CountDownLatch latch) {
+		try {
+			if (!latch.await(DEFAULT_WAIT_TIMEOUT, TimeUnit.MILLISECONDS)) {
+				throw new AssertionFailedError("Timed-out waiting for CountDownLatch");
+			}
+		}
+		catch (InterruptedException e) {
+			fail("Interrupted waiting for CountDownLatch");
+		}
+	}
+
+	/**
+	 * Waits for the given condition to return true
 	 *
 	 * @param condition the condition that returns true when satisfied
 	 * @throws AssertionFailedError if the condition is not met within the timeout period
@@ -351,7 +371,7 @@ public abstract class AbstractGTest {
 	}
 
 	/**
-	 * Waits for the given condition to return true.
+	 * Waits for the given condition to return true
 	 *
 	 * @param condition the condition that returns true when satisfied
 	 * @throws AssertionFailedError if the condition is not met within the timeout period
@@ -380,7 +400,7 @@ public abstract class AbstractGTest {
 	 *
 	 * <P>Most clients should use {@link #waitForCondition(BooleanSupplier)}.
 	 *
-	 * @param condition the condition that returns true when satisfied
+	 * @param supplier the supplier that returns true when satisfied
 	 */
 	public static void waitForConditionWithoutFailing(BooleanSupplier supplier) {
 		waitForCondition(supplier, false /*failOnTimeout*/, null /*failure message*/);
@@ -465,7 +485,6 @@ public abstract class AbstractGTest {
 	 * throwing an exception if that does not happen by the given timeout.
 	 *
 	 * @param supplier the supplier of the value
-	 * @param timeoutMillis the timeout
 	 * @param failureMessage the message to print upon the timeout being reached
 	 * @param failOnTimeout if true, an exception will be thrown if the timeout is reached
 	 * @return the value

Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakDataStructureFactory.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,13 +15,15 @@
  */
 package ghidra.util.datastruct;
 
+/**
+ * Factory for creating containers to use in various threading environments
+ */
 public class WeakDataStructureFactory {
 
 	/**
 	 * Use when all access are on a single thread, such as the Swing thread.
 	 * 
 	 * @return a new WeakSet
-	 * @see CopyOnWriteReadWeakSet
 	 */
 	public static <T> WeakSet<T> createSingleThreadAccessWeakSet() {
 		return new ThreadUnsafeWeakSet<T>();
@@ -32,7 +33,7 @@ public class WeakDataStructureFactory {
 	 * Use when mutations outweigh iterations.
 	 * 
 	 * @return a new WeakSet
-	 * @see CopyOnWriteReadWeakSet
+	 * @see CopyOnReadWeakSet
 	 */
 	public static <T> WeakSet<T> createCopyOnReadWeakSet() {
 		return new CopyOnReadWeakSet<T>();
@@ -47,5 +48,4 @@ public class WeakDataStructureFactory {
 	public static <T> WeakSet<T> createCopyOnWriteWeakSet() {
 		return new CopyOnWriteWeakSet<T>();
 	}
-
 }

Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakSet.java

@@ -76,17 +76,21 @@ public abstract class WeakSet<T> implements Iterable<T> {
 //==================================================================================================	
 
 	/**
-	 * Add the given object to the set.
+	 * Add the given object to the set
+	 * @param t the object to add
 	 */
 	public abstract void add(T t);
 
 	/**
 	 * Remove the given object from the data structure
+	 * @param t the object to remove
+	 * 
 	 */
 	public abstract void remove(T t);
 
 	/**
 	 * Returns true if the given object is in this data structure
+	 * @return true if the given object is in this data structure
 	 */
 	public abstract boolean contains(T t);
 
@@ -97,11 +101,13 @@ public abstract class WeakSet<T> implements Iterable<T> {
 
 	/**
 	 * Return the number of objects contained within this data structure
+	 * @return the size
 	 */
 	public abstract int size();
 
 	/**
-	 * Return whether this data structure is empty. 
+	 * Return whether this data structure is empty
+	 * @return whether this data structure is empty
 	 */
 	public abstract boolean isEmpty();
 
@@ -119,4 +125,9 @@ public abstract class WeakSet<T> implements Iterable<T> {
 	public Stream<T> stream() {
 		return values().stream();
 	}
+
+	@Override
+	public String toString() {
+		return values().toString();
+	}
 }

Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/AboutToolMacQuitHandler.java

@@ -1,60 +0,0 @@
-/* ###
- * 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.framework.plugintool;
-
-import docking.DockingWindowManager;
-import docking.framework.AboutDialog;
-import generic.platform.MacAboutHandler;
-import ghidra.framework.OperatingSystem;
-import ghidra.framework.Platform;
-import ghidra.util.SystemUtilities;
-
-/**
- * A plugin-level 'About' handler that serves as the callback from the Dock's 'About' popup action.
- */
-public class AboutToolMacQuitHandler extends MacAboutHandler {
-
-	// Note: we only want this handle to be installed once globally for the entire application 
-	//       (otherwise, multiple prompts will be displayed).
-	private static AboutToolMacQuitHandler INSTANCE = null;
-
-	public static void install() {
-
-		if (Platform.CURRENT_PLATFORM.getOperatingSystem() != OperatingSystem.MAC_OS_X) {
-			return;
-		}
-
-		// These calls should all be in the Swing thread; thus, no need for locking.
-		SystemUtilities.assertThisIsTheSwingThread("Must install quit handler in the Swing thread");
-		if (INSTANCE != null) {
-			return;
-		}
-
-		// just creating the instance will install it
-		AboutToolMacQuitHandler instance = new AboutToolMacQuitHandler();
-		INSTANCE = instance;
-	}
-
-	private AboutToolMacQuitHandler() {
-		// only we can construct
-	}
-
-	@Override
-	public void about() {
-		DockingWindowManager.showDialog(new AboutDialog());
-	}
-
-}

Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacAboutHandler.java

@@ -21,7 +21,6 @@ import docking.DockingWindowManager;
 import docking.framework.AboutDialog;
 import ghidra.framework.OperatingSystem;
 import ghidra.framework.Platform;
-import ghidra.util.SystemUtilities;
 
 /**
  * A plugin-level about handler that serves as the callback from the Dock's 'About' popup action.
@@ -35,22 +34,18 @@ public class PluginToolMacAboutHandler {
 	 * 
 	 * @param winMgr The docking window manager to use to install the about dialog.
 	 */
-	public static void install(DockingWindowManager winMgr) {
+	public static synchronized void install(DockingWindowManager winMgr) {
 
 		if (installed) {
 			return;
 		}
+		installed = true;
 
 		if (Platform.CURRENT_PLATFORM.getOperatingSystem() != OperatingSystem.MAC_OS_X) {
 			return;
 		}
 
-		// These calls should all be in the Swing thread; thus, no need for locking.
+		Desktop.getDesktop().setAboutHandler(
-		SystemUtilities.assertThisIsTheSwingThread(
+			e -> DockingWindowManager.showDialog(new AboutDialog()));
-			"Must install about handler in the Swing thread");
-
-		Desktop.getDesktop().setAboutHandler(e -> winMgr.showDialog(new AboutDialog()));
-
-		installed = true;
 	}
 }

Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacQuitHandler.java

@@ -19,7 +19,6 @@ import java.awt.Desktop;
 
 import ghidra.framework.OperatingSystem;
 import ghidra.framework.Platform;
-import ghidra.util.SystemUtilities;
 
 /**
  * A plugin-level quit handler that serves as the callback from the Dock's 'Quit' popup action.
@@ -41,24 +40,20 @@ public class PluginToolMacQuitHandler {
 	 * 
 	 * @param tool The tool to close, which should result in the desired quit behavior.
 	 */
-	public static void install(PluginTool tool) {
+	public static synchronized void install(PluginTool tool) {
 
 		if (installed) {
 			return;
 		}
+		installed = true;
 
 		if (Platform.CURRENT_PLATFORM.getOperatingSystem() != OperatingSystem.MAC_OS_X) {
 			return;
 		}
 
-		// These calls should all be in the Swing thread; thus, no need for locking.
-		SystemUtilities.assertThisIsTheSwingThread("Must install quit handler in the Swing thread");
-
 		Desktop.getDesktop().setQuitHandler((evt, response) -> {
-			response.cancelQuit(); // we will allow our tool to quit the application instead of the OS
+			response.cancelQuit(); // allow our tool to quit the application instead of the OS
 			tool.close();
 		});
-
-		installed = true;
 	}
 }

Ghidra/Framework/Project/src/test/java/ghidra/framework/plugintool/DummyPluginTool.java

@@ -38,7 +38,8 @@ public class DummyPluginTool extends PluginTool {
 
 		@Override
 		public void closeTool(Tool t) {
-			System.exit(0);
+			// If we call this, then the entire test VM will exit, which is bad
+			// System.exit(0);
 		}
 	}
 }