From f8bd49b4beb9f5d51d2291de6861e10e231bf974 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Tue, 11 Jun 2019 13:30:42 -0400 Subject: [PATCH] GT-2869 - Key Bindings - review fixes --- .../core/calltree/CallTreeProvider.java | 6 -- .../CompositeEditorProvider.java | 5 -- .../LocationReferencesProvider.java | 8 +- .../PropertyManagerProvider.java | 1 - ...kageHelper.java => ActionToGuiHelper.java} | 31 ++++--- .../java/docking/DockingWindowManager.java | 65 ++++---------- .../java/docking/actions/KeyBindingUtils.java | 2 +- .../actions/SharedStubKeyBindingAction.java | 4 + .../java/docking/actions/ToolActions.java | 70 ++++++++------- .../SharedKeyBindingDockingActionTest.java | 4 +- .../docking/DockingActionKeyBindingTest.java | 90 ------------------- .../plugintool/dialog/KeyBindingsPanel.java | 2 +- 12 files changed, 84 insertions(+), 204 deletions(-) rename Ghidra/Framework/Docking/src/main/java/docking/{DockingActionPackageHelper.java => ActionToGuiHelper.java} (70%) delete mode 100644 Ghidra/Framework/Docking/src/test/java/docking/DockingActionKeyBindingTest.java diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java index a1e9e64fc2..39fd756f18 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/calltree/CallTreeProvider.java @@ -834,12 +834,6 @@ public class CallTreeProvider extends ComponentProviderAdapter implements Domain currentProgram = null; } - tool.removeLocalAction(this, recurseDepthAction); - tool.removeLocalAction(this, refreshAction); - tool.removeLocalAction(this, filterDuplicates); - tool.removeLocalAction(this, navigationOutgoingAction); - tool.removeLocalAction(this, navigateIncomingToggleAction); - recurseDepthAction.dispose(); refreshAction.dispose(); filterDuplicates.dispose(); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/CompositeEditorProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/CompositeEditorProvider.java index ad8107e3e3..090824993d 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/CompositeEditorProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/CompositeEditorProvider.java @@ -206,11 +206,6 @@ public abstract class CompositeEditorProvider extends ComponentProviderAdapter @Override public void dispose() { - CompositeEditorTableAction[] allActions = actionMgr.getAllActions(); - for (CompositeEditorTableAction allAction : allActions) { - tool.removeLocalAction(this, allAction); - } - tool.showComponentProvider(this, false); tool.removeComponentProvider(this); for (EditorListener el : listeners) { el.closed(this); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/locationreferences/LocationReferencesProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/locationreferences/LocationReferencesProvider.java index 5ca4b5f0c1..c6fb3e222e 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/locationreferences/LocationReferencesProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/locationreferences/LocationReferencesProvider.java @@ -38,7 +38,8 @@ import ghidra.program.model.address.AddressSet; import ghidra.program.model.listing.Program; import ghidra.program.util.ProgramLocation; import ghidra.util.HelpLocation; -import ghidra.util.table.*; +import ghidra.util.table.GhidraTable; +import ghidra.util.table.SelectionNavigationAction; import ghidra.util.table.actions.DeleteTableRowAction; import ghidra.util.task.SwingUpdateManager; import resources.Icons; @@ -208,11 +209,6 @@ public class LocationReferencesProvider extends ComponentProviderAdapter tool.removeComponentProvider(this); - tool.removeLocalAction(this, homeAction); - tool.removeLocalAction(this, refreshAction); - tool.removeLocalAction(this, selectionAction); - tool.removeLocalAction(this, highlightAction); - homeAction.dispose(); refreshAction.dispose(); highlightAction.dispose(); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/debug/propertymanager/PropertyManagerProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/debug/propertymanager/PropertyManagerProvider.java index 3da662eff0..85f3856976 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/debug/propertymanager/PropertyManagerProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/debug/propertymanager/PropertyManagerProvider.java @@ -93,7 +93,6 @@ public class PropertyManagerProvider extends ComponentProviderAdapter { } void dispose() { - tool.removeLocalAction(this, deleteAction); tool.removeComponentProvider(this); } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/DockingActionPackageHelper.java b/Ghidra/Framework/Docking/src/main/java/docking/ActionToGuiHelper.java similarity index 70% rename from Ghidra/Framework/Docking/src/main/java/docking/DockingActionPackageHelper.java rename to Ghidra/Framework/Docking/src/main/java/docking/ActionToGuiHelper.java index acf18d07b8..1400d776da 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/DockingActionPackageHelper.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/ActionToGuiHelper.java @@ -15,7 +15,7 @@ */ package docking; -import java.util.Set; +import java.util.Iterator; import docking.action.DockingActionIf; @@ -24,11 +24,11 @@ import docking.action.DockingActionIf; * {@link DockingWindowManager}. This allows the manager's interface to hide methods that * don't make sense for public consumption. */ -public class DockingActionPackageHelper { +public class ActionToGuiHelper { private DockingWindowManager windowManager; - public DockingActionPackageHelper(DockingWindowManager windowManager) { + public ActionToGuiHelper(DockingWindowManager windowManager) { this.windowManager = windowManager; } @@ -49,14 +49,6 @@ public class DockingActionPackageHelper { windowManager.removeToolAction(action); } - /** - * Returns all actions registered with this manager - * @return the actions - */ - public Set getAllActions() { - return windowManager.getAllActions(); - } - /** * Adds an action that will be associated with the given provider. These actions will * appear in the local header for the component as a toolbar button or a drop-down menu @@ -69,4 +61,21 @@ public class DockingActionPackageHelper { windowManager.addLocalAction(provider, action); } + /** + * Get an iterator over the actions for the given provider + * @param provider the component provider for which to iterate over all its owned actions + * @return null if the provider does not exist in the window manager + */ + public Iterator getComponentActions(ComponentProvider provider) { + return windowManager.getComponentActions(provider); + } + + /** + * Removes the action from the given provider's header bar. + * @param provider the provider whose header bar from which the action should be removed. + * @param action the action to be removed from the provider's header bar. + */ + public void removeProviderAction(ComponentProvider provider, DockingActionIf action) { + windowManager.removeProviderAction(provider, action); + } } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java b/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java index 2ec9097e08..7723085e4b 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java @@ -60,8 +60,6 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder final static String COMPONENT_MENU_NAME = "Window"; - private final static List EMPTY_LIST = Collections.emptyList(); - private static DockingActionIf actionUnderMouse; private static Object objectUnderMouse; @@ -643,19 +641,6 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder placeholderManager.removeComponent(provider); } - /** - * Get an iterator over the actions for the given provider. - * @param provider the component provider for which to iterate over all its owned actions. - * @return null if the provider does not exist in this window manager - */ - public Iterator getComponentActions(ComponentProvider provider) { - ComponentPlaceholder placeholder = getActivePlaceholder(provider); - if (placeholder != null) { - return placeholder.getActions(); - } - return EMPTY_LIST.iterator(); - } - /** * Removes all components and actions associated with the given owner. * @param owner the name of the owner whose associated component and actions should be removed. @@ -666,12 +651,21 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder scheduleUpdate(); } - /** - * Removes the action from the given provider's header bar. - * @param provider the provider whose header bar from which the action should be removed. - * @param action the action to be removed from the provider's header bar. - */ - public void removeProviderAction(ComponentProvider provider, DockingActionIf action) { +//================================================================================================== +// Package-level Action Methods +//================================================================================================== + + Iterator getComponentActions(ComponentProvider provider) { + ComponentPlaceholder placeholder = getActivePlaceholder(provider); + if (placeholder != null) { + return placeholder.getActions(); + } + + List emptyList = Collections.emptyList(); + return emptyList.iterator(); + } + + void removeProviderAction(ComponentProvider provider, DockingActionIf action) { ComponentPlaceholder placeholder = getActivePlaceholder(provider); if (placeholder != null) { actionManager.removeLocalAction(action); @@ -679,17 +673,6 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder } } -//================================================================================================== -// Package-level Action Methods -//================================================================================================== - - /** - * Adds an action that will be associated with the given provider. These actions will - * appear in the local header for the component as a toolbar button or a drop-down menu - * item if it has an icon and menu path respectively. - * @param provider the provider whose header on which the action is to be placed. - * @param action the action to add to the providers header bar. - */ void addLocalAction(ComponentProvider provider, DockingActionIf action) { ComponentPlaceholder placeholder = getActivePlaceholder(provider); if (placeholder == null) { @@ -699,34 +682,16 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder actionManager.addLocalAction(action, provider); } - /** - * Adds an action to the global menu or toolbar which appear in the main frame. If - * the action has a menu path, it will be in the menu. If it has an icon, it will - * appear in the toolbar. - * @param action the action to be added. - */ void addToolAction(DockingActionIf action) { actionManager.addToolAction(action); scheduleUpdate(); } - /** - * Removes the given action from the global menu and toolbar. - * @param action the action to be removed. - */ void removeToolAction(DockingActionIf action) { actionManager.removeToolAction(action); scheduleUpdate(); } - /** - * Returns all actions registered with this manager - * @return the actions - */ - public Set getAllActions() { - return actionManager.getAllActions(); - } - //================================================================================================== // End Package-level Methods //================================================================================================== diff --git a/Ghidra/Framework/Docking/src/main/java/docking/actions/KeyBindingUtils.java b/Ghidra/Framework/Docking/src/main/java/docking/actions/KeyBindingUtils.java index a809f889ae..026782d5bf 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/actions/KeyBindingUtils.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/actions/KeyBindingUtils.java @@ -336,7 +336,7 @@ public class KeyBindingUtils { * @param tool the tool containing the actions * @return the actions mapped by their full name (e.g., 'Name (OwnerName)') */ - public static Map getAllKeyBindingActionsByFullName(DockingTool tool) { + public static Map getAllActionsByFullName(DockingTool tool) { Map deduper = new HashMap<>(); Set actions = tool.getAllActions(); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/actions/SharedStubKeyBindingAction.java b/Ghidra/Framework/Docking/src/main/java/docking/actions/SharedStubKeyBindingAction.java index ce2c3efd2b..f8362089da 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/actions/SharedStubKeyBindingAction.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/actions/SharedStubKeyBindingAction.java @@ -66,6 +66,10 @@ public class SharedStubKeyBindingAction extends DockingAction implements Options options.addOptionsChangeListener(this); } + void removeClientAction(DockingActionIf action) { + clientActions.remove(action); + } + void addClientAction(DockingActionIf action) { // 1) Validate new action keystroke against existing actions diff --git a/Ghidra/Framework/Docking/src/main/java/docking/actions/ToolActions.java b/Ghidra/Framework/Docking/src/main/java/docking/actions/ToolActions.java index da354228ee..545f20a659 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/actions/ToolActions.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/actions/ToolActions.java @@ -18,7 +18,6 @@ package docking.actions; import java.beans.PropertyChangeEvent; import java.beans.PropertyChangeListener; import java.util.*; -import java.util.stream.Collectors; import javax.swing.KeyStroke; @@ -29,6 +28,7 @@ import docking.action.*; import docking.tool.util.DockingToolConstants; import ghidra.framework.options.*; import ghidra.util.exception.AssertException; +import util.CollectionUtils; /** * An class to manage actions registered with the tool @@ -36,7 +36,7 @@ import ghidra.util.exception.AssertException; public class ToolActions implements PropertyChangeListener { private DockingWindowManager winMgr; - private DockingActionPackageHelper actionGuiHelper; + private ActionToGuiHelper actionGuiHelper; /* Map of Maps of Sets @@ -62,7 +62,7 @@ public class ToolActions implements PropertyChangeListener { public ToolActions(DockingTool tool, DockingWindowManager windowManager) { this.dockingTool = tool; this.winMgr = windowManager; - this.actionGuiHelper = new DockingActionPackageHelper(winMgr); + this.actionGuiHelper = new ActionToGuiHelper(winMgr); keyBindingOptions = tool.getOptions(DockingToolConstants.KEY_BINDINGS); } @@ -78,10 +78,6 @@ public class ToolActions implements PropertyChangeListener { actions.add(action); } - private void removeActionFromMap(DockingActionIf action) { - getActionStorage(action).remove(action); - } - /** * Add an action that works specifically with a component provider. * @param provider provider associated with the action @@ -150,7 +146,7 @@ public class ToolActions implements PropertyChangeListener { */ public synchronized void removeToolAction(DockingActionIf action) { action.removePropertyChangeListener(this); - removeActionFromMap(action); + removeAction(action); actionGuiHelper.removeToolAction(action); } @@ -160,16 +156,19 @@ public class ToolActions implements PropertyChangeListener { */ public synchronized void removeToolActions(String owner) { + // remove from the outer map first, to prevent concurrent modification exceptions + Map> toCleanup = actionsByNameByOwner.remove(owner); + if (toCleanup == null) { + return; // no actions registered for this owner + } + //@formatter:off - Set toRemove = actionsByNameByOwner.get(owner).values() + toCleanup.values() .stream() .flatMap(set -> set.stream()) - .collect(Collectors.toSet()) + .forEach(action -> removeToolAction(action)) ; //@formatter:on - - // must do this later to avoid concurrent modification exceptions - toRemove.forEach(action -> removeToolAction(action)); } private void checkForAlreadyAddedAction(ComponentProvider provider, DockingActionIf action) { @@ -180,19 +179,7 @@ public class ToolActions implements PropertyChangeListener { } /** - * Remove an action that works specifically with a component provider. - * @param provider provider associated with the action - * @param action local action to the provider - */ - public synchronized void removeProviderAction(ComponentProvider provider, - DockingActionIf action) { - action.removePropertyChangeListener(this); - removeActionFromMap(action); - winMgr.removeProviderAction(provider, action); - } - - /** - * Get all actions for the given owner. + * Get all actions for the given owner * @param owner owner of the actions * @return array of actions; zero length array is returned if no * action exists with the given name @@ -250,14 +237,37 @@ public class ToolActions implements PropertyChangeListener { } } + /** + * Remove an action that works specifically with a component provider. + * @param provider provider associated with the action + * @param action local action to the provider + */ + public synchronized void removeProviderAction(ComponentProvider provider, + DockingActionIf action) { + action.removePropertyChangeListener(this); + removeAction(action); + actionGuiHelper.removeProviderAction(provider, action); + } + /** * Get the actions for the given provider and remove them from the action map * @param provider provider whose actions are to be removed */ - public void removeComponentActions(ComponentProvider provider) { - Iterator iterator = winMgr.getComponentActions(provider); - while (iterator.hasNext()) { - removeActionFromMap(iterator.next()); + public synchronized void removeComponentActions(ComponentProvider provider) { + Iterator it = actionGuiHelper.getComponentActions(provider); + Set set = CollectionUtils.asSet(it); + for (DockingActionIf action : set) { + removeProviderAction(provider, action); + } + } + + private void removeAction(DockingActionIf action) { + getActionStorage(action).remove(action); + if (action.usesSharedKeyBinding()) { + SharedStubKeyBindingAction stub = sharedActionMap.get(action.getName()); + if (stub != null) { + stub.removeClientAction(action); + } } } diff --git a/Ghidra/Framework/Docking/src/test.slow/java/docking/actions/SharedKeyBindingDockingActionTest.java b/Ghidra/Framework/Docking/src/test.slow/java/docking/actions/SharedKeyBindingDockingActionTest.java index 2792ba6d33..3e387be964 100644 --- a/Ghidra/Framework/Docking/src/test.slow/java/docking/actions/SharedKeyBindingDockingActionTest.java +++ b/Ghidra/Framework/Docking/src/test.slow/java/docking/actions/SharedKeyBindingDockingActionTest.java @@ -290,7 +290,6 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest { assertActionNotInTool(action1); assertActionNotInTool(action2); - String sharedName = action1.getFullName(); assertNoSharedKeyBindingStubInstalled(action1); } @@ -357,8 +356,7 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest { //================================================================================================== private void assertSharedStubInTool() { - ToolActions actionManager = - (ToolActions) getInstanceField("actionMgr", tool); + ToolActions actionManager = (ToolActions) getInstanceField("actionMgr", tool); DockingActionIf action = actionManager.getSharedStubKeyBindingAction(SHARED_NAME); assertNotNull("Shared action stub is not in the tool", action); } diff --git a/Ghidra/Framework/Docking/src/test/java/docking/DockingActionKeyBindingTest.java b/Ghidra/Framework/Docking/src/test/java/docking/DockingActionKeyBindingTest.java deleted file mode 100644 index afe8d22ca8..0000000000 --- a/Ghidra/Framework/Docking/src/test/java/docking/DockingActionKeyBindingTest.java +++ /dev/null @@ -1,90 +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 docking; - -import static org.junit.Assert.assertEquals; - -import java.awt.event.InputEvent; -import java.awt.event.KeyEvent; - -import javax.swing.KeyStroke; - -import org.junit.Test; - -import generic.test.AbstractGenericTest; - -public class DockingActionKeyBindingTest extends AbstractGenericTest { - - @Test - public void testKeybinding_Unmodified() { - - KeyStroke javaKeyStroke = KeyStroke.getKeyStroke(KeyEvent.VK_C, 0); - KeyStroke dockingKeyStroke = DockingKeyBindingAction.parseKeyStroke("C"); - assertEquals(javaKeyStroke, dockingKeyStroke); - } - - @Test - public void testKeybinding_Unmodified_MixedCase() { - - KeyStroke javaKeyStroke = KeyStroke.getKeyStroke(KeyEvent.VK_C, 0); - KeyStroke dockingKeyStroke = DockingKeyBindingAction.parseKeyStroke("C"); - assertEquals(javaKeyStroke, dockingKeyStroke); - } - - @Test - public void testKeybinding_Modifier() { - - KeyStroke javaKeyStroke = - KeyStroke.getKeyStroke(KeyEvent.VK_COMMA, InputEvent.CTRL_DOWN_MASK); - KeyStroke dockingKeyStroke = DockingKeyBindingAction.parseKeyStroke("ctrl COMMA"); - assertEquals(javaKeyStroke, dockingKeyStroke); - } - - @Test - public void testKeybinding_MultipleModifiers() { - - KeyStroke javaKeyStroke = KeyStroke.getKeyStroke(KeyEvent.VK_COMMA, - InputEvent.CTRL_DOWN_MASK | InputEvent.SHIFT_DOWN_MASK); - KeyStroke dockingKeyStroke = DockingKeyBindingAction.parseKeyStroke("ctrl shift COMMA"); - assertEquals(javaKeyStroke, dockingKeyStroke); - } - - @Test - public void testKeybinding_MultipleModifiersOutOfOrder() { - - KeyStroke javaKeyStroke = KeyStroke.getKeyStroke(KeyEvent.VK_COMMA, - InputEvent.CTRL_DOWN_MASK | InputEvent.SHIFT_DOWN_MASK); - KeyStroke dockingKeyStroke = DockingKeyBindingAction.parseKeyStroke("ctrl COMMA shift"); - assertEquals(javaKeyStroke, dockingKeyStroke); - } - - @Test - public void testKeybinding_MultipleModifiers_MixedCase() { - - KeyStroke javaKeyStroke = KeyStroke.getKeyStroke(KeyEvent.VK_COMMA, - InputEvent.CTRL_DOWN_MASK | InputEvent.SHIFT_DOWN_MASK); - KeyStroke dockingKeyStroke = DockingKeyBindingAction.parseKeyStroke("ctrl SHIFT comma"); - assertEquals(javaKeyStroke, dockingKeyStroke); - } - - @Test - public void testKeybinding_InvalidControl() { - - KeyStroke javaKeyStroke = KeyStroke.getKeyStroke(KeyEvent.VK_C, InputEvent.CTRL_DOWN_MASK); - KeyStroke dockingKeyStroke = DockingKeyBindingAction.parseKeyStroke("control C"); - assertEquals(javaKeyStroke, dockingKeyStroke); - } -} diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/dialog/KeyBindingsPanel.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/dialog/KeyBindingsPanel.java index 4d0eb63bba..9d6e39f9cc 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/dialog/KeyBindingsPanel.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/dialog/KeyBindingsPanel.java @@ -171,7 +171,7 @@ public class KeyBindingsPanel extends JPanel { originalValues = new HashMap<>(); String longestName = ""; - actionsByFullName = KeyBindingUtils.getAllKeyBindingActionsByFullName(tool); + actionsByFullName = KeyBindingUtils.getAllActionsByFullName(tool); Set> entries = actionsByFullName.entrySet(); for (Entry entry : entries) {