GT-3411 - Actions - fixed key bindings being removed from options after

tool restart

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

@@ -119,16 +119,19 @@ public class KeyEntryDialog extends DialogComponentProvider {
 	}
 
 	private JPanel createCollisionPanel() {
-		JPanel p = new JPanel(new BorderLayout());
+		JPanel parent = new JPanel(new BorderLayout());
+
+		JPanel noWrapPanel = new JPanel(new BorderLayout());
 		collisionPane = new JTextPane();
 		collisionPane.setEditable(false);
 		collisionPane.setBackground(bgColor);
 		doc = collisionPane.getStyledDocument();
-		JScrollPane sp = new JScrollPane(collisionPane);
+		noWrapPanel.add(collisionPane, BorderLayout.CENTER);
+		JScrollPane sp = new JScrollPane(noWrapPanel);
 		Dimension d = defaultPanel.getPreferredSize();
 		sp.setPreferredSize(new Dimension(sp.getPreferredSize().width, d.height));
-		p.add(sp, BorderLayout.CENTER);
-		return p;
+		parent.add(sp, BorderLayout.CENTER);
+		return parent;
 	}
 
 	/**
@@ -194,13 +197,19 @@ public class KeyEntryDialog extends DialogComponentProvider {
 			return;
 		}
 
+		list.sort((a1, a2) -> {
+			String s1 = a1.getName() + a1.getOwnerDescription();
+			String s2 = a2.getName() + a2.getOwnerDescription();
+			return s1.compareToIgnoreCase(s2);
+		});
+
 		String ksName = KeyBindingUtils.parseKeyStroke(ks);
 		try {
 			doc.insertString(0, "Actions mapped to " + ksName + "\n\n", textAttrSet);
 			for (int i = 0; i < list.size(); i++) {
 				DockingActionIf a = list.get(i);
 
-				String collisionStr = "\t" + a.getName() + "  (" + a.getOwnerDescription() + ")\n";
+				String collisionStr = "\t" + a.getName() + " (" + a.getOwnerDescription() + ")\n";
 				int offset = doc.getLength();
 				doc.insertString(offset, collisionStr, textAttrSet);
 				doc.setParagraphAttributes(offset, 1, tabAttrSet, false);

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

@@ -34,6 +34,6 @@ public class SharedActionRegistry {
 	 * @param toolActions the tool action manager
 	 */
 	public static void installSharedActions(DockingTool tool, ToolActions toolActions) {
-		GTable.createSharedActions(tool, toolActions, ToolConstants.TOOL_OWNER);
+		GTable.createSharedActions(tool, toolActions, ToolConstants.SHARED_OWNER);
 	}
 }

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

@@ -0,0 +1,64 @@
+/* ###
+ * 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.actions;
+
+import javax.swing.KeyStroke;
+
+import docking.tool.ToolConstants;
+
+/**
+ * A marker interface to signal that the implementing action serves as an action that should
+ * not be itself used in the tool, but should only be used to register and manager keybindings.
+ * 
+ * 
+ * <p>This action is merely a tool by which transient components can ensure that their actions
+ * are correctly managed when the component is created.  Normal actions will get registered when
+ * the tool first starts-up.  Alternatively, transient components only appear when called upon
+ * by some event, such as a user request.  The issue heretofore was that the tool will remove
+ * any options that are not longer used. Thus, if an action belonging to a transient component
+ * does not get registered every time the tool is used, then the options (and key bindings) for
+ * that action are removed from the too.   This interface allows a second-party to register 
+ * an action on behalf of a transient provider, thus preventing the tool from removing any 
+ * previously applied options.
+ */
+public interface SharedDockingActionPlaceholder {
+
+	/**
+	 * The action name.  This name must exactly match the name of the action represented by 
+	 * this placeholder.
+	 * @return the name
+	 */
+	public String getName();
+
+	/**
+	 * Returns an owner name to use in place of {@value ToolConstants#SHARED_OWNER}.  
+	 * This should only be used when the client knows for certain that all shared actions are 
+	 * shared by a single owner.  This is not typical for shared actions.  This can happen when one
+	 * owner (such as a plugin) has multiple component providers that share action key  bindings.
+	 * @return the owner
+	 */
+	public default String getOwner() {
+		return ToolConstants.SHARED_OWNER;
+	}
+
+	/**
+	 * The default key binding for the action represented by this placeholder
+	 * @return the key binding; may be null
+	 */
+	public default KeyStroke getKeyBinding() {
+		return null;
+	}
+}

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

@@ -20,6 +20,8 @@ import java.util.Map.Entry;
 
 import javax.swing.KeyStroke;
 
+import org.apache.commons.collections4.Bag;
+import org.apache.commons.collections4.bag.HashBag;
 import org.apache.commons.lang3.StringUtils;
 
 import docking.ActionContext;
@@ -34,11 +36,25 @@ import ghidra.framework.options.ToolOptions;
  * allows plugins to create actions that share keybindings without having to manage those 
  * keybindings themselves.
  * 
+ * <p>Some ways this class is used:
+ * <ol>
+ * 	<li>As a central action to manage key bindings for multiple actions from different clients 
+ *      (plugins) that are conceptually the same.  When the plugins are loaded 
+ *      these actions get registered and are wired to listen to key binding changes to this stub. 
+ * 	</li>
+ *  <li>As a placeholder action to manage key bindings for actions that have not yet been 
+ *      registered and may not get registered during the lifetime of a single tool session.
+ *      This can happen when a plugin has transient component providers that only get shown
+ *      upon a user request.  This stub allows the key binding for those actions to be managed,
+ *      even if they do not get registered when the tool is shown.
+ *  </li>
+ * </ol>
+ * 
  * <p>Clients should not be using this class directly.
  */
 public class SharedStubKeyBindingAction extends DockingAction implements OptionsChangeListener {
 
-	static final String SHARED_OWNER = ToolConstants.TOOL_OWNER;
+	static final String SHARED_OWNER = ToolConstants.SHARED_OWNER;
 
 	/**
 	 * We save the client actions for later validate and options updating.  We also need the
@@ -50,27 +66,54 @@ public class SharedStubKeyBindingAction extends DockingAction implements Options
 	private WeakHashMap<DockingActionIf, KeyStroke> clientActions = new WeakHashMap<>();
 
 	private ToolOptions keyBindingOptions;
+	private Bag<String> actionOwners = new HashBag<String>();
 
 	/**
 	 * Creates a new dummy action by the given name and default keystroke value
 	 * 
 	 * @param name The name of the action--this will be displayed in the options as the name of
 	 *             key binding's action
+	 * @param defaultKs the default key stroke for this stub.  The key stroke will be validated
+	 *        each time an action is added to this stub to ensure that the defaults are in sync.
 	 * @param options the tool's key binding options
 	 */
-	SharedStubKeyBindingAction(String name, ToolOptions options) {
-		super(name, SHARED_OWNER);
+	SharedStubKeyBindingAction(String name, KeyStroke defaultKs, ToolOptions options) {
+		// Note: we need to have this stub registered to use key bindings so that the options will
+		//       restore the saved key binding to this class, which will then notify any of the
+		//       shared actions using this stub.
+		super(name, SHARED_OWNER, KeyBindingType.INDIVIDUAL);
 		this.keyBindingOptions = options;
 
 		// Dummy keybinding actions don't have help--the real action does
 		DockingWindowManager.getHelpService().excludeFromHelp(this);
 
+		setUnvalidatedKeyBindingData(new KeyBindingData(defaultKs));
+
 		// A listener to keep the shared, stub keybindings in sync with their clients 
 		options.addOptionsChangeListener(this);
 	}
 
+	/**
+	 * Adds the given owner name to this stub.  This is used to display all known clients of
+	 * the action represented by this stub.  Normally, when this class has actions, the names
+	 * of each action's owner would be used directly.  However, this class can also be used as 
+	 * a placeholder, when no actions have yet been registered.  In that case, the owner has 
+	 * to be set directly on this stub.
+	 *  
+	 * @param owner the name of the client that owns the actions that may get registered with
+	 *        this stub
+	 */
+	void addActionOwner(String owner) {
+		if (DockingWindowManager.DOCKING_WINDOWS_OWNER.equals(owner)) {
+			// Special case: special system-level action owner; the user does not need to see
+			return;
+		}
+		actionOwners.add(owner);
+	}
+
 	void removeClientAction(DockingActionIf action) {
 		clientActions.remove(action);
+		actionOwners.remove(action.getOwner());
 	}
 
 	void addClientAction(DockingActionIf action) {
@@ -88,44 +131,20 @@ public class SharedStubKeyBindingAction extends DockingAction implements Options
 
 	@Override
 	public String getOwnerDescription() {
-		List<String> owners = getDistinctOwners();
+		List<String> owners = new LinkedList<>(actionOwners.uniqueSet());
 		if (owners.size() == 1) {
 			return owners.get(0);
 		}
 
-		boolean hasTool = owners.remove(ToolConstants.TOOL_OWNER);
+		boolean hasTool = owners.remove(SHARED_OWNER);
 		Collections.sort(owners);
 		if (hasTool) {
-			owners.add(0, ToolConstants.TOOL_OWNER);
+			owners.add(0, SHARED_OWNER);
 		}
 
 		return StringUtils.join(owners, ", ");
 	}
 
-	private List<String> getDistinctOwners() {
-		List<String> results = new ArrayList<>();
-		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);
-			}
-		}
-
-		if (results.isEmpty()) {
-			// This implies we have an action owned by the DockingWindowManager 
-			// (the DOCKING_WINDOWS_OWNER).  In this case, use the Tool as the owner.
-			results.add(SHARED_OWNER);
-		}
-
-		return results;
-	}
-
 	private KeyStroke validateActionsHaveTheSameDefaultKeyStroke(DockingActionIf newAction) {
 
 		// this value may be null

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

@@ -18,6 +18,7 @@ package docking.actions;
 import java.beans.PropertyChangeEvent;
 import java.beans.PropertyChangeListener;
 import java.util.*;
+import java.util.Map.Entry;
 
 import javax.swing.Action;
 import javax.swing.KeyStroke;
@@ -142,6 +143,13 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		}
 
 		KeyStroke ks = action.getKeyBinding();
+		loadKeyBindingFromOptions(action, ks);
+
+		keyBindingsManager.addAction(provider, action);
+	}
+
+	private void loadKeyBindingFromOptions(DockingActionIf action, KeyStroke ks) {
+
 		String description = "Keybinding for " + action.getFullName();
 		keyBindingOptions.registerOption(action.getFullName(), OptionType.KEYSTROKE_TYPE, ks, null,
 			description);
@@ -149,8 +157,6 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		if (!Objects.equals(ks, newKs)) {
 			action.setUnvalidatedKeyBindingData(new KeyBindingData(newKs));
 		}
-
-		keyBindingsManager.addAction(provider, action);
 	}
 
 	private void installSharedKeyBinding(ComponentProvider provider, DockingActionIf action) {
@@ -161,11 +167,13 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		SharedStubKeyBindingAction stub = sharedActionMap.computeIfAbsent(name, key -> {
 
 			SharedStubKeyBindingAction newStub =
-				new SharedStubKeyBindingAction(name, keyBindingOptions);
+				new SharedStubKeyBindingAction(name, defaultKeyStroke, keyBindingOptions);
 			registerStub(newStub, defaultKeyStroke);
 			return newStub;
 		});
 
+		String owner = action.getOwner();
+		stub.addActionOwner(owner);
 		stub.addClientAction(action);
 
 		if (!(action instanceof AutoGeneratedDockingAction)) {
@@ -176,16 +184,12 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 
 	private void registerStub(SharedStubKeyBindingAction stub, KeyStroke defaultKeyStroke) {
 		stub.addPropertyChangeListener(this);
-		String description = "Keybinding for Stub action: " + stub.getFullName();
-		keyBindingOptions.registerOption(stub.getFullName(), OptionType.KEYSTROKE_TYPE,
-			defaultKeyStroke, null, description);
+
+		loadKeyBindingFromOptions(stub, defaultKeyStroke);
+
 		keyBindingsManager.addAction(null, stub);
 	}
 
-	/**
-	 * Removes the given action from the tool
-	 * @param action the action to be removed.
-	 */
 	@Override
 	public synchronized void removeGlobalAction(DockingActionIf action) {
 		action.removePropertyChangeListener(this);
@@ -230,12 +234,6 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		}
 	}
 
-	/**
-	 * 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
-	 */
 	@Override
 	public synchronized Set<DockingActionIf> getActions(String owner) {
 
@@ -245,18 +243,18 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 			result.addAll(actions);
 		}
 
-		if (SharedStubKeyBindingAction.SHARED_OWNER.equals(owner)) {
-			result.addAll(sharedActionMap.values());
+		Set<Entry<String, SharedStubKeyBindingAction>> entries = sharedActionMap.entrySet();
+		for (Entry<String, SharedStubKeyBindingAction> entry : entries) {
+			SharedStubKeyBindingAction stub = entry.getValue();
+			String stubOwner = stub.getOwner();
+			if (stubOwner.equals(owner)) {
+				result.add(stub);
+			}
 		}
 
 		return result;
 	}
 
-	/**
-	 * Get a set of all actions in the tool
-	 * 
-	 * @return a new set of the existing actions
-	 */
 	@Override
 	public synchronized Set<DockingActionIf> getAllActions() {
 
@@ -418,4 +416,31 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		return sharedActionMap.get(name);
 	}
 
+	/**
+	 * Allows clients to register an action by using a placeholder.  This is useful when 
+	 * an API wishes to have a central object (like a plugin) register actions for transient
+	 * providers, that may not be loaded until needed.
+	 * 
+	 * <p>This method may be called multiple times with the same conceptual placeholder--the
+	 * placeholder will only be added once.
+	 * 
+	 * @param placeholder the placeholder containing information related to the action it represents
+	 */
+	public void registerSharedActionPlaceholder(SharedDockingActionPlaceholder placeholder) {
+
+		String name = placeholder.getName();
+		KeyStroke defaultKeyStroke = placeholder.getKeyBinding();
+
+		SharedStubKeyBindingAction stub = sharedActionMap.computeIfAbsent(name, key -> {
+
+			SharedStubKeyBindingAction newStub =
+				new SharedStubKeyBindingAction(name, defaultKeyStroke, keyBindingOptions);
+			registerStub(newStub, defaultKeyStroke);
+			return newStub;
+		});
+
+		String owner = placeholder.getOwner();
+		stub.addActionOwner(owner);
+	}
+
 }

Ghidra/Framework/Docking/src/main/java/docking/tool/ToolConstants.java

@@ -15,6 +15,7 @@
  */
 package docking.tool;
 
+import docking.action.KeyBindingType;
 import docking.tool.util.DockingToolConstants;
 
 /**
@@ -93,6 +94,13 @@ public interface ToolConstants extends DockingToolConstants {
 	 */
 	public static final String TOOL_OWNER = "Tool";
 
+	/**
+	 * This is used when many actions wish to share a key binding.  
+	 * 
+	 * @see KeyBindingType#SHARED
+	 */
+	public static final String SHARED_OWNER = "Shared";
+
 	/**
 	 * Name of options for a tool
 	 */