GT-2925 - Key Bindings - Support Window Menu Provider Key Bindings -

test and review fixes

Ghidra/Framework/Docking/src/main/java/docking/AbstractDockingTool.java

@@ -67,6 +67,7 @@ public abstract class AbstractDockingTool implements DockingTool {
 	@Override
 	public void removeComponentProvider(ComponentProvider provider) {
 		Runnable r = () -> {
+			toolActions.removeGlobalAction(provider.getShowProviderAction());
 			toolActions.removeActions(provider);
 			winMgr.removeComponent(provider);
 		};

Ghidra/Framework/Docking/src/main/java/docking/DockableToolBarManager.java

@@ -168,7 +168,8 @@ class DockableToolBarManager {
 		private DockableComponent dockableComponent;
 
 		ToolBarCloseAction(DockableComponent dockableComponent) {
-			super("Close Window", DockingWindowManager.DOCKING_WINDOWS_OWNER);
+			super("Close Window", DockingWindowManager.DOCKING_WINDOWS_OWNER,
+				KeyBindingType.UNSUPPORTED);
 			this.dockableComponent = dockableComponent;
 			setDescription("Close Window");
 			setToolBarData(new ToolBarData(closeIcon, null));

Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java

@@ -47,16 +47,14 @@ import util.CollectionUtils;
  * Manages the "Docking" arrangement of a set of components and actions. The components can be "docked" 
  * together or exist in their own window.  Actions can be associated with components so they
  * "move" with the component as it moved from one location to another.
- * 
+ * <P>
  * Components are added via ComponentProviders.  A ComponentProvider is an interface for getting
  * a component and its related information.  The docking window manager will get the component
  * from the provider as needed.  It is up to the provider if it wants to reuse the component or
- * recreate a new one when the component is requested.  When the user "hides" (by using 
- * the x button on the component area) a component, the docking window manager removes all
+ * recreate a new one when the component is requested.  When the user hides a component (by using 
+ * the x button on the component header), the docking window manager removes all
  * knowledge of the component and will request it again from the provider if the component
- * becomes "unhidden".  The provider is also notified whenever a component is hidden.   Some
- * providers will use the notification to remove the provider from the docking window manager so
- * that the can not "unhide" the component using the built-in window menu.
+ * is again shown.  The provider is also notified whenever a component is hidden and shown. 
  */
 public class DockingWindowManager implements PropertyChangeListener, PlaceholderInstaller {
 
@@ -712,8 +710,11 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder
 		ComponentPlaceholder placeholder = getActivePlaceholder(provider);
 		if (placeholder != null) {
 			showComponent(placeholder, visibleState, true);
+			return;
 		}
-		else {
+
+		if (visibleState) {
+
 			// a null placeholder implies the client is trying to show a provider that has not
 			// been added to the tool
 			Msg.warn(this, "Attempting to show an unknown Component Provider '" +

Ghidra/Framework/Docking/src/main/java/docking/ShowComponentAction.java

@@ -76,11 +76,7 @@ class ShowComponentAction extends DockingAction implements Comparable<ShowCompon
 
 		// keybinding data used to show the binding in the menu
 		ComponentProvider provider = placeholder.getProvider();
-		DockingActionIf action = provider.getShowProviderAction();
-		KeyBindingData kbData = action.getKeyBindingData();
-		if (kbData != null) {
-			setKeyBindingData(kbData);
-		}
+		synchronizeKeyBinding(provider);
 
 		// Use provider Help for this action		
 		HelpLocation helpLocation = provider.getHelpLocation();
@@ -94,6 +90,21 @@ class ShowComponentAction extends DockingAction implements Comparable<ShowCompon
 		}
 	}
 
+	/**
+	 * Ensures that the given provider's key binding matches this class's key binding
+	 * @param provider the provider
+	 */
+	private void synchronizeKeyBinding(ComponentProvider provider) {
+		DockingActionIf action = provider.getShowProviderAction();
+		KeyBindingData defaultBinding = action.getDefaultKeyBindingData();
+		setKeyBindingData(defaultBinding);
+
+		KeyBindingData kbData = action.getKeyBindingData();
+		if (kbData != null) {
+			setUnvalidatedKeyBindingData(kbData);
+		}
+	}
+
 	private static KeyBindingType createKeyBindingType(boolean isTransient,
 			ComponentPlaceholder placeholder) {
 

Ghidra/Framework/Docking/src/main/java/docking/action/DockingActionIf.java

@@ -55,6 +55,16 @@ public interface DockingActionIf extends HelpDescriptor {
 	 */
 	public String getOwner();
 
+	/**
+	 * Returns a description of this actions owner.  For most actions this will return the 
+	 * same value as {@link #getOwner()}.
+	 * 
+	 * @return the description
+	 */
+	public default String getOwnerDescription() {
+		return getOwner();
+	}
+
 	/**
 	 * Returns a short description of this action. Generally used for a tooltip
 	 * @return the description

Ghidra/Framework/Docking/src/main/java/docking/action/KeyBindingsManager.java

@@ -39,7 +39,7 @@ public class KeyBindingsManager implements PropertyChangeListener {
 		actionToProviderMap = new HashMap<>();
 	}
 
-	public void addAction(DockingActionIf action, ComponentProvider optionalProvider) {
+	public void addAction(ComponentProvider optionalProvider, DockingActionIf action) {
 		action.addPropertyChangeListener(this);
 		if (optionalProvider != null) {
 			actionToProviderMap.put(action, optionalProvider);
@@ -48,7 +48,7 @@ public class KeyBindingsManager implements PropertyChangeListener {
 		KeyStroke keyBinding = action.getKeyBinding();
 
 		if (keyBinding != null) {
-			addKeyBinding(action, optionalProvider, keyBinding);
+			addKeyBinding(optionalProvider, action, keyBinding);
 		}
 	}
 
@@ -67,7 +67,7 @@ public class KeyBindingsManager implements PropertyChangeListener {
 		removeKeyBinding(action.getKeyBinding(), action);
 	}
 
-	private void addKeyBinding(DockingActionIf action, ComponentProvider provider,
+	private void addKeyBinding(ComponentProvider provider, DockingActionIf action,
 			KeyStroke keyStroke) {
 		if (ReservedKeyBindings.isReservedKeystroke(keyStroke)) {
 			throw new AssertException("Cannot assign action to a reserved keystroke.  " +
@@ -145,7 +145,7 @@ public class KeyBindingsManager implements PropertyChangeListener {
 		if (newKeyData != null) {
 			KeyStroke ks = newKeyData.getKeyBinding();
 			if (ks != null) {
-				addKeyBinding(action, actionToProviderMap.get(action), ks);
+				addKeyBinding(actionToProviderMap.get(action), action, ks);
 			}
 		}
 	}

Ghidra/Framework/Docking/src/main/java/docking/action/MultipleKeyAction.java

@@ -63,6 +63,7 @@ public class MultipleKeyAction extends DockingKeyBindingAction {
 			throw new IllegalArgumentException(
 				"KeyStrokes don't match - was: " + keyStroke + " new: " + keyBinding);
 		}
+
 		actions.add(new ActionData(action, provider));
 	}
 
@@ -273,5 +274,10 @@ public class MultipleKeyAction extends DockingKeyBindingAction {
 		boolean isMyProvider(ComponentProvider otherProvider) {
 			return provider == otherProvider;
 		}
+
+		@Override
+		public String toString() {
+			return provider.toString() + " - " + action;
+		}
 	}
 }

Ghidra/Framework/Docking/src/main/java/docking/actions/DockingToolActions.java

@@ -34,6 +34,15 @@ public interface DockingToolActions {
 	 */
 	public void addLocalAction(ComponentProvider provider, DockingActionIf action);
 
+	/**
+	 * Gets the provider action by the given name
+	 * 
+	 * @param provider the provider
+	 * @param actionName the action name
+	 * @return the action
+	 */
+	public DockingActionIf getLocalAction(ComponentProvider provider, String actionName);
+
 	/**
 	 * Removes the given provider's local action
 	 * 

Ghidra/Framework/Docking/src/main/java/docking/actions/KeyEntryDialog.java

@@ -155,6 +155,7 @@ public class KeyEntryDialog extends DialogComponentProvider {
 
 		KeyStroke existingKeyStroke = action.getKeyBinding();
 		if (Objects.equals(existingKeyStroke, newKeyStroke)) {
+			setStatusText("Key binding unchanged");
 			return;
 		}
 
@@ -177,11 +178,18 @@ public class KeyEntryDialog extends DialogComponentProvider {
 	}
 
 	private void updateCollisionPane(KeyStroke ks) {
+		clearStatusText();
 		collisionPane.setText("");
 		if (ks == null) {
 			return;
 		}
 
+		KeyStroke existingKeyStroke = action.getKeyBinding();
+		if (Objects.equals(existingKeyStroke, ks)) {
+			setStatusText("Key binding unchanged");
+			return;
+		}
+
 		List<DockingActionIf> list = getManagedActionsForKeyStroke(ks);
 		if (list.size() == 0) {
 			return;
@@ -193,7 +201,7 @@ public class KeyEntryDialog extends DialogComponentProvider {
 			for (int i = 0; i < list.size(); i++) {
 				DockingActionIf a = list.get(i);
 
-				String collisionStr = "\t" + a.getName() + "  (" + a.getOwner() + ")\n";
+				String collisionStr = "\t" + a.getName() + "  (" + a.getOwnerDescription() + ")\n";
 				int offset = doc.getLength();
 				doc.insertString(offset, collisionStr, textAttrSet);
 				doc.setParagraphAttributes(offset, 1, tabAttrSet, false);
@@ -211,6 +219,7 @@ public class KeyEntryDialog extends DialogComponentProvider {
 		if (multiAction == null) {
 			return Collections.emptyList();
 		}
+
 		List<DockingActionIf> list = multiAction.getActions();
 		Map<String, DockingActionIf> nameMap = new HashMap<>(list.size());
 

Ghidra/Framework/Docking/src/main/java/docking/actions/SharedStubKeyBindingAction.java

@@ -85,13 +85,14 @@ public class SharedStubKeyBindingAction extends DockingAction implements Options
 		updateActionKeyStrokeFromOptions(action, defaultKs);
 	}
 
-	public String getOwnersDescription() {
+	@Override
+	public String getOwnerDescription() {
 		List<String> owners = getDistinctOwners();
 		Collections.sort(owners);
 		if (owners.size() == 1) {
 			return owners.get(0);
 		}
-		return '(' + StringUtils.join(owners, ", ") + ')';
+		return StringUtils.join(owners, ", ");
 	}
 
 	private List<String> getDistinctOwners() {
@@ -99,6 +100,11 @@ public class SharedStubKeyBindingAction extends DockingAction implements Options
 		Set<DockingActionIf> actions = clientActions.keySet();
 		for (DockingActionIf action : actions) {
 			String owner = action.getOwner();
+			if (DockingWindowManager.DOCKING_WINDOWS_OWNER.equals(owner)) {
+				// special case: this is the owner for special system-level actions
+				continue;
+			}
+
 			if (!results.contains(owner)) {
 				results.add(owner);
 			}
@@ -139,12 +145,16 @@ public class SharedStubKeyBindingAction extends DockingAction implements Options
 
 	private void updateActionKeyStrokeFromOptions(DockingActionIf action, KeyStroke defaultKs) {
 
+		KeyStroke stubKs = defaultKs;
 		KeyStroke optionsKs = getKeyStrokeFromOptions(defaultKs);
 		if (!Objects.equals(defaultKs, optionsKs)) {
 			// we use the 'unvalidated' call since this value is provided by the user--we assume
 			// that user input is correct; we only validate programmer input
 			action.setUnvalidatedKeyBindingData(new KeyBindingData(optionsKs));
+			stubKs = optionsKs;
 		}
+
+		setUnvalidatedKeyBindingData(new KeyBindingData(stubKs));
 	}
 
 	private KeyStroke getKeyStrokeFromOptions(KeyStroke validatedKeyStroke) {

Ghidra/Framework/Docking/src/main/java/docking/actions/ToolActions.java

@@ -115,27 +115,21 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 
 		action.addPropertyChangeListener(this);
 		addActionToMap(action);
-		setKeyBindingOption(action);
-		keyBindingsManager.addAction(action, provider);
+		initializeKeyBinding(provider, action);
 		actionGuiHelper.addLocalAction(provider, action);
 	}
 
-	/**
-	 * Adds the action to the tool.
-	 * @param action the action to be added.
-	 */
 	@Override
 	public synchronized void addGlobalAction(DockingActionIf action) {
 		checkForAlreadyAddedAction(null, action);
 
 		action.addPropertyChangeListener(this);
 		addActionToMap(action);
-		setKeyBindingOption(action);
-		keyBindingsManager.addAction(action, null);
+		initializeKeyBinding(null, action);
 		actionGuiHelper.addToolAction(action);
 	}
 
-	private void setKeyBindingOption(DockingActionIf action) {
+	private void initializeKeyBinding(ComponentProvider provider, DockingActionIf action) {
 
 		KeyBindingType type = action.getKeyBindingType();
 		if (!type.supportsKeyBindings()) {
@@ -143,7 +137,7 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		}
 
 		if (type.isShared()) {
-			installSharedKeyBinding(action);
+			installSharedKeyBinding(provider, action);
 			return;
 		}
 
@@ -154,9 +148,11 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		if (!Objects.equals(ks, newKs)) {
 			action.setUnvalidatedKeyBindingData(new KeyBindingData(newKs));
 		}
+
+		keyBindingsManager.addAction(provider, action);
 	}
 
-	private void installSharedKeyBinding(DockingActionIf action) {
+	private void installSharedKeyBinding(ComponentProvider provider, DockingActionIf action) {
 		String name = action.getName();
 		KeyStroke defaultKeyStroke = action.getKeyBinding();
 
@@ -172,6 +168,9 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		});
 
 		stub.addClientAction(action);
+
+		// note: only put the stub in the manager, not the actual action
+		keyBindingsManager.addAction(provider, stub);
 	}
 
 	/**
@@ -318,10 +317,12 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		for (DockingActionIf action : set) {
 			removeLocalAction(provider, action);
 		}
-
 	}
 
 	private void removeAction(DockingActionIf action) {
+
+		keyBindingsManager.removeAction(action);
+
 		getActionStorage(action).remove(action);
 		if (!action.getKeyBindingType().isShared()) {
 			return;
@@ -370,6 +371,19 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		}
 	}
 
+	@Override
+	public DockingActionIf getLocalAction(ComponentProvider provider, String actionName) {
+
+		Iterator<DockingActionIf> it = actionGuiHelper.getComponentActions(provider);
+		while (it.hasNext()) {
+			DockingActionIf action = it.next();
+			if (action.getName().equals(actionName)) {
+				return action;
+			}
+		}
+		return null;
+	}
+
 	public Action getAction(KeyStroke ks) {
 		return keyBindingsManager.getDockingKeyAction(ks);
 	}

Ghidra/Framework/Docking/src/main/java/docking/test/AbstractDockingTest.java

@@ -43,6 +43,7 @@ import com.google.common.collect.Sets;
 import docking.*;
 import docking.action.DockingActionIf;
 import docking.action.ToggleDockingActionIf;
+import docking.actions.DockingToolActions;
 import docking.dnd.GClipboard;
 import docking.framework.DockingApplicationConfiguration;
 import docking.menu.DockingToolbarButton;
@@ -1195,8 +1196,18 @@ public abstract class AbstractDockingTest extends AbstractGenericTest {
 		return CollectionUtils.any(actions);
 	}
 
+	/**
+	 * Returns the action by the given name that belongs to the given provider
+	 * 
+	 * @param provider the provider
+	 * @param actionName the action name
+	 * @return the action
+	 */
 	public static DockingActionIf getLocalAction(ComponentProvider provider, String actionName) {
-		return getAction(provider.getTool(), provider.getName(), actionName);
+		DockingTool tool = provider.getTool();
+		DockingToolActions toolActions = tool.getToolActions();
+		DockingActionIf action = toolActions.getLocalAction(provider, actionName);
+		return action;
 	}
 
 	/**
@@ -1850,10 +1861,12 @@ public abstract class AbstractDockingTest extends AbstractGenericTest {
 	 *
 	 * @param tool the tool in which the provider lives
 	 * @param name the name of the provider to show
+	 * @return the newly shown provider
 	 */
-	public void showProvider(DockingTool tool, String name) {
+	public ComponentProvider showProvider(DockingTool tool, String name) {
 		ComponentProvider provider = tool.getComponentProvider(name);
 		tool.showComponentProvider(provider, true);
+		return provider;
 	}
 
 	/**