From d474d83166774941a5db9f82dadd7239d52c12b5 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Tue, 9 Apr 2019 18:22:01 -0400 Subject: [PATCH 1/2] 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 --- .../app/tablechooser/TableChooserDialog.java | 275 +++++++++--- .../tablechooser/TableChooserExecutor.java | 6 +- .../tablechooser/TableChooserTableModel.java | 16 +- .../tablechooser/TableChooserDialogTest.java | 394 ++++++++++++++++++ .../generic/platform/AbstractMacHandler.java | 43 -- .../generic/platform/MacAboutHandler.java | 77 ---- .../java/generic/platform/MacQuitHandler.java | 83 ---- .../main/java/generic/test/AbstractGTest.java | 27 +- .../datastruct/WeakDataStructureFactory.java | 8 +- .../java/ghidra/util/datastruct/WeakSet.java | 15 +- .../plugintool/AboutToolMacQuitHandler.java | 60 --- .../plugintool/PluginToolMacAboutHandler.java | 13 +- .../plugintool/PluginToolMacQuitHandler.java | 11 +- .../framework/plugintool/DummyPluginTool.java | 3 +- 14 files changed, 670 insertions(+), 361 deletions(-) create mode 100644 Ghidra/Features/Base/src/test/java/ghidra/app/tablechooser/TableChooserDialogTest.java delete mode 100644 Ghidra/Framework/Generic/src/main/java/generic/platform/AbstractMacHandler.java delete mode 100644 Ghidra/Framework/Generic/src/main/java/generic/platform/MacAboutHandler.java delete mode 100644 Ghidra/Framework/Generic/src/main/java/generic/platform/MacQuitHandler.java delete mode 100644 Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/AboutToolMacQuitHandler.java diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java b/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java index 8162f54899..0678716791 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java +++ b/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.util.*; +import java.awt.*; +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.event.ListSelectionListener; +import javax.swing.table.TableCellRenderer; 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. + * + *

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 workers = + WeakDataStructureFactory.createCopyOnReadWeakSet(); + private Set sharedPending = ConcurrentHashMap.newKeySet(); private final TableChooserExecutor executor; - private Set workers = new HashSet(); + 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 tablePanel = - new GhidraThreadedTablePanel(model, 50, 2000); + TableChooserDialogPanel tablePanel = new TableChooserDialogPanel(model); 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() { - @Override - public void valueChanged(ListSelectionEvent e) { - setOkEnabled(table.getSelectedRowCount() > 0); - } - }); + table.getSelectionModel().addListSelectionListener( + e -> setOkEnabled(table.getSelectedRowCount() > 0)); GhidraTableFilterPanel filterPanel = new GhidraTableFilterPanel(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() { - @Override - public void run() { - model = new TableChooserTableModel("Test", tool, program, null /* set later*/); - } - }); - } - catch (Exception e) { - Msg.showError(this, null, "Error Creating Table", "Error Creating Table", e); - } + + // note: the task monitor is installed later when this model is added to the threaded panel + SystemUtilities.runSwingNow( + () -> model = new TableChooserTableModel("Test", tool, program, null)); } 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, - "Selection_Navigation")); + selectionNavigationAction.setHelpLocation( + 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); - - try { - ExecutorSwingWorker worker = new ExecutorSwingWorker(monitor); - worker.execute(); - workers.add(worker); + List rowObjects = getSelectedRowObjects(); + rowObjects.removeAll(sharedPending); // only keep selected items not being processed + if (rowObjects.isEmpty()) { + return; } - 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 = - workers.toArray(new ExecutorSwingWorker[workers.size()]); - for (ExecutorSwingWorker worker : threadSafeArray) { + for (ExecutorSwingWorker worker : workers) { if (!worker.isDone()) { return true; } } - return false; + + return model.isBusy(); } - private void doExecute(TaskMonitor monitor) { - int[] selectedRows = table.getSelectedRows(); + private void doExecute(List rowObjects, TaskMonitor monitor) { - monitor.initialize(selectedRows.length); + monitor.initialize(rowObjects.size()); - List deletedRowObjects = new ArrayList(); - for (int selectedRow : selectedRows) { + try { + List 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 doProcessRowObjects(List rowObjects, + TaskMonitor monitor) { + List deleted = new ArrayList(); + 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) { - model.removeObject(addressableRowObject); - } - - model.fireTableDataChanged(); - setStatusText(""); + return deleted; } 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 getSelectedRowObjects() { + int[] selectedRows = table.getSelectedRows(); + List rowObjects = model.getRowObjects(selectedRows); + return rowObjects; + } + //================================================================================================== // Inner Classes //================================================================================================== + private class TableChooserDialogPanel extends GhidraThreadedTablePanel { + + public TableChooserDialogPanel(ThreadedTableModel model) { + super(model, 50, 2000); + } + + @Override + protected GTable createTable(ThreadedTableModel tm) { + return new TableChooserDialogGhidraTable(tm); + } + } + + private class TableChooserDialogGhidraTable extends GhidraTable { + + public TableChooserDialogGhidraTable(ThreadedTableModel 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 - * executed. + * Runs our work off the Swing thread, so that the GUI updates as the task is being executed */ private class ExecutorSwingWorker extends SwingWorker { private final TaskMonitor monitor; + private List rowObjects; - ExecutorSwingWorker(TaskMonitor monitor) { + ExecutorSwingWorker(List 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(); } } } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserExecutor.java b/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserExecutor.java index be559bcece..e1e7f537df 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserExecutor.java +++ b/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 monitor The monitor you can use to set status messages. - * @return true if the rowObject should be removed from the table, false otherwise. + * @param rowObject the AddressRowObject to be executed upon + * @return true if the rowObject should be removed from the table, false otherwise */ public boolean execute(AddressableRowObject rowObject); } diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.java index c296f9d10c..ff13222f5b 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.java +++ b/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 { + + // 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 myPrivateList = new HashSet(); public TableChooserTableModel(String title, ServiceProvider serviceProvider, Program program, @@ -49,6 +50,11 @@ public class TableChooserTableModel extends AddressBasedTableModel 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 selected = selectRows(0, 2); + List toRemove = new ArrayList<>(toRowObjects(1, 3)); + + List 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 selected = selectRows(0, 1, 3); + List toProcess = new ArrayList<>(selected); + + List 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 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 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 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 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 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 rowObjects) { + for (TestStubRowObject rowObject : rowObjects) { + assertEquals("Row object was unexpectedly processed by the Executor", 1, + executor.getExecutedCount(rowObject)); + } + } + + private List toRowObjects(int... rows) { + + List 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 selected = selectRows(row); + return selected.get(0); + } + + private List selectRows(int... row) { + runSwing(() -> dialog.clearSelection()); + runSwing(() -> dialog.selectRows(row)); + List 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 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(); + } + } +} diff --git a/Ghidra/Framework/Generic/src/main/java/generic/platform/AbstractMacHandler.java b/Ghidra/Framework/Generic/src/main/java/generic/platform/AbstractMacHandler.java deleted file mode 100644 index f443f6726f..0000000000 --- a/Ghidra/Framework/Generic/src/main/java/generic/platform/AbstractMacHandler.java +++ /dev/null @@ -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: - *

- * - * 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; - } - -} diff --git a/Ghidra/Framework/Generic/src/main/java/generic/platform/MacAboutHandler.java b/Ghidra/Framework/Generic/src/main/java/generic/platform/MacAboutHandler.java deleted file mode 100644 index 42d1727800..0000000000 --- a/Ghidra/Framework/Generic/src/main/java/generic/platform/MacAboutHandler.java +++ /dev/null @@ -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. - *

- * Simply constructing this class will register it. - *

- * 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; - } -} diff --git a/Ghidra/Framework/Generic/src/main/java/generic/platform/MacQuitHandler.java b/Ghidra/Framework/Generic/src/main/java/generic/platform/MacQuitHandler.java deleted file mode 100644 index 07f048a259..0000000000 --- a/Ghidra/Framework/Generic/src/main/java/generic/platform/MacQuitHandler.java +++ /dev/null @@ -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. - *

- * Simply constructing this class will register it. - *

- * 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; - } - -} diff --git a/Ghidra/Framework/Generic/src/main/java/generic/test/AbstractGTest.java b/Ghidra/Framework/Generic/src/main/java/generic/test/AbstractGTest.java index 0e44963019..66757b9c8f 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/test/AbstractGTest.java +++ b/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 { * *

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 diff --git a/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakDataStructureFactory.java b/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakDataStructureFactory.java index 7ab3b72de7..16304d9aab 100644 --- a/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakDataStructureFactory.java +++ b/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 WeakSet createSingleThreadAccessWeakSet() { return new ThreadUnsafeWeakSet(); @@ -32,7 +33,7 @@ public class WeakDataStructureFactory { * Use when mutations outweigh iterations. * * @return a new WeakSet - * @see CopyOnWriteReadWeakSet + * @see CopyOnReadWeakSet */ public static WeakSet createCopyOnReadWeakSet() { return new CopyOnReadWeakSet(); @@ -47,5 +48,4 @@ public class WeakDataStructureFactory { public static WeakSet createCopyOnWriteWeakSet() { return new CopyOnWriteWeakSet(); } - } diff --git a/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakSet.java b/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakSet.java index 3f3249c9b6..6b80e9de1c 100644 --- a/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakSet.java +++ b/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakSet.java @@ -76,17 +76,21 @@ public abstract class WeakSet implements Iterable { //================================================================================================== /** - * 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 implements Iterable { /** * 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 implements Iterable { public Stream stream() { return values().stream(); } + + @Override + public String toString() { + return values().toString(); + } } diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/AboutToolMacQuitHandler.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/AboutToolMacQuitHandler.java deleted file mode 100644 index 12aff5f33f..0000000000 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/AboutToolMacQuitHandler.java +++ /dev/null @@ -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()); - } - -} diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacAboutHandler.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacAboutHandler.java index c0abd9c485..cba9547736 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacAboutHandler.java +++ b/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. - SystemUtilities.assertThisIsTheSwingThread( - "Must install about handler in the Swing thread"); - - Desktop.getDesktop().setAboutHandler(e -> winMgr.showDialog(new AboutDialog())); - - installed = true; + Desktop.getDesktop().setAboutHandler( + e -> DockingWindowManager.showDialog(new AboutDialog())); } } diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacQuitHandler.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacQuitHandler.java index ec55e84cc2..4055361289 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/PluginToolMacQuitHandler.java +++ b/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; } } diff --git a/Ghidra/Framework/Project/src/test/java/ghidra/framework/plugintool/DummyPluginTool.java b/Ghidra/Framework/Project/src/test/java/ghidra/framework/plugintool/DummyPluginTool.java index 6af31093f2..01db7ae1ed 100644 --- a/Ghidra/Framework/Project/src/test/java/ghidra/framework/plugintool/DummyPluginTool.java +++ b/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); } } } From c65c60a042238162e13deef22f6588b05f25df8e Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Wed, 10 Apr 2019 17:38:33 -0400 Subject: [PATCH 2/2] GT-2724 - review fixes --- .../java/ghidra/app/tablechooser/TableChooserDialog.java | 6 +++--- .../ghidra/app/tablechooser/TableChooserTableModel.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java b/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java index 0678716791..f47db591e0 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserDialog.java @@ -51,7 +51,7 @@ import resources.ResourceManager; * to perform the action defined by the executor. * *

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. + * items are scheduled 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 @@ -115,7 +115,7 @@ public class TableChooserDialog extends DialogComponentProvider e -> setOkEnabled(table.getSelectedRowCount() > 0)); GhidraTableFilterPanel filterPanel = - new GhidraTableFilterPanel(table, model); + new GhidraTableFilterPanel<>(table, model); panel.add(tablePanel, BorderLayout.CENTER); panel.add(filterPanel, BorderLayout.SOUTH); return panel; @@ -270,7 +270,7 @@ public class TableChooserDialog extends DialogComponentProvider private List doProcessRowObjects(List rowObjects, TaskMonitor monitor) { - List deleted = new ArrayList(); + List deleted = new ArrayList<>(); for (AddressableRowObject rowObject : rowObjects) { if (monitor.isCancelled()) { break; diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.java index ff13222f5b..93bb78659c 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/tablechooser/TableChooserTableModel.java @@ -30,7 +30,7 @@ import ghidra.util.task.TaskMonitor; public class TableChooserTableModel extends AddressBasedTableModel { // 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) + // (the downside of this is that two lists are maintained) Set myPrivateList = new HashSet(); public TableChooserTableModel(String title, ServiceProvider serviceProvider, Program program,