Fix for shared global dialog escape action to appear in options without a dialog having been created

Ghidra/Features/Base/src/test.slow/java/docking/DialogComponentProviderActionsTest.java

@@ -0,0 +1,162 @@
+/* ###
+ * 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.*;
+
+import java.awt.Component;
+
+import javax.swing.*;
+
+import org.junit.*;
+
+import docking.action.DockingAction;
+import docking.action.KeyBindingData;
+import docking.actions.KeyBindingUtils;
+import ghidra.app.plugin.core.codebrowser.CodeBrowserPlugin;
+import ghidra.app.plugin.core.codebrowser.CodeViewerProvider;
+import ghidra.framework.plugintool.PluginTool;
+import ghidra.test.*;
+import ghidra.util.Msg;
+import ghidra.util.SpyErrorLogger;
+
+public class DialogComponentProviderActionsTest extends AbstractGhidraHeadedIntegrationTest {
+
+	private TestEnv env;
+	private PluginTool tool;
+	private DialogComponentProvider provider;
+	private SpyErrorLogger spyLogger = new SpyErrorLogger();
+
+	@Before
+	public void setUp() throws Exception {
+		env = new TestEnv();
+		tool = env.launchDefaultTool();
+		env.open(new ClassicSampleX86ProgramBuilder().getProgram());
+
+		provider = new TestDialogComponentProvider();
+
+		Msg.setErrorLogger(spyLogger);
+	}
+
+	@After
+	public void tearDown() {
+		env.dispose();
+	}
+
+	@Test
+	public void testKeyBinding() {
+		//
+		// Create an action for the dialog that has a keybinding.  Ensure that the action can be 
+		// triggered while the dialog is showing.
+		//
+		SpyAction spyAction = new SpyAction();
+
+		String ksText = "Control Y";
+		setKeyBinding(spyAction, ksText);
+		addAction(spyAction);
+
+		// the action should not work if the dialog is not showing
+		triggerKey(ksText);
+		assertFalse(spyAction.hasBeenCalled());
+
+		showDialogWithoutBlocking(tool, provider);
+		waitForDialogComponent(provider.getTitle());
+
+		triggerKey(ksText);
+		assertTrue(spyAction.hasBeenCalled());
+		close(provider);
+	}
+
+	@Test
+	public void testKeyBinding_SameAsGlobalKeyBinding() {
+		//
+		// Create an action for the dialog that has a keybinding.  Use a key binding that is the 
+		// same as a global tool action so that both could be triggered.  Verify that only the 
+		// dialog's action is executed while the dialog is showing.
+		//
+		SpyAction spyAction = new SpyAction();
+
+		String ksText = "G";
+		setKeyBinding(spyAction, ksText);
+		addAction(spyAction);
+
+		// verify the Go To dialog appears (and not Multiple Key Binding Dialog)
+		triggerKey(ksText);
+		DialogComponentProvider dialog = waitForDialogComponent("Go To ...");
+		close(dialog);
+
+		showDialogWithoutBlocking(tool, provider);
+		waitForDialogComponent(provider.getTitle());
+
+		triggerKey(ksText);
+		assertTrue(spyAction.hasBeenCalled());
+		close(provider);
+	}
+
+	private void addAction(DockingAction action) {
+		runSwing(() -> provider.addAction(action));
+	}
+
+	private void setKeyBinding(DockingAction action, String ksText) {
+		runSwing(() -> {
+			action.setKeyBindingData(new KeyBindingData(ksText));
+		});
+	}
+
+	private void triggerKey(String ksText) {
+		Component component = provider.getComponent();
+		if (!provider.isShowing()) {
+			CodeBrowserPlugin cbp = env.getPlugin(CodeBrowserPlugin.class);
+			CodeViewerProvider cvp = cbp.getProvider();
+			component = cvp.getComponent();
+		}
+		KeyStroke ks = KeyBindingUtils.parseKeyStroke(ksText);
+		triggerKey(component, ks);
+		waitForSwing();
+	}
+
+	private class SpyAction extends DockingAction {
+
+		private volatile boolean hasBeenCalled;
+
+		public SpyAction() {
+			super("Some Dialog Action", "Some Owner");
+		}
+
+		@Override
+		public void actionPerformed(ActionContext context) {
+			hasBeenCalled = true;
+		}
+
+		boolean hasBeenCalled() {
+			return hasBeenCalled;
+		}
+	}
+
+	private class TestDialogComponentProvider extends DialogComponentProvider {
+
+		private JComponent component = new JButton("Hey!");
+
+		protected TestDialogComponentProvider() {
+			super("Test Dialog");
+		}
+
+		@Override
+		public JComponent getComponent() {
+			return component;
+		}
+	}
+}

Ghidra/Features/Base/src/test.slow/java/ghidra/framework/plugintool/dialog/KeyBindingUtilsTest.java

@@ -133,6 +133,9 @@ public class KeyBindingUtilsTest extends AbstractGhidraHeadedIntegrationTest {
 
 	@After
 	public void tearDown() throws Exception {
+
+		closeAllWindows();
+
 		debug("tearDown()");
 		env.dispose();
 		debug("a");
@@ -243,7 +246,7 @@ public class KeyBindingUtilsTest extends AbstractGhidraHeadedIntegrationTest {
 		ToolOptions originalOptions = importOptions(saveFile);
 
 		assertOptionsMatch(
-			"The Options objects do not contain different data after changes have been made.",
+			"Options do not contain different data after changes have been made.",
 			toolKeyBindingOptions, originalOptions);
 
 		debug("c");
@@ -255,7 +258,7 @@ public class KeyBindingUtilsTest extends AbstractGhidraHeadedIntegrationTest {
 
 		// verify the changes are different than the original values
 		assertOptionsDontMatch(
-			"The Options objects do not contain different data after changes have been made.",
+			"Options does not contain different data after changes have been made.",
 			toolKeyBindingOptions, originalOptions);
 
 		debug("e");
@@ -269,7 +272,7 @@ public class KeyBindingUtilsTest extends AbstractGhidraHeadedIntegrationTest {
 
 		// verify the data is the same as it was before the changes
 		boolean same = compareOptionsWithKeyStrokeMap(originalOptions, keyStrokeMap);
-		assertTrue("The Options object contains different data than was imported.", same);
+		assertTrue("The exported options contains different data than were imported.", same);
 
 		debug("g");
 
@@ -626,23 +629,23 @@ public class KeyBindingUtilsTest extends AbstractGhidraHeadedIntegrationTest {
 	// keystrokes (the map is obtained from the key bindings panel after an
 	// import is done).
 	private boolean compareOptionsWithKeyStrokeMap(Options oldOptions,
-			Map<String, KeyStroke> panelKeyStrokeMap) {
+			Map<String, KeyStroke> currentKsMap) {
-		List<String> propertyNames = oldOptions.getOptionNames();
+		List<String> oldNames = oldOptions.getOptionNames();
-		for (String name : propertyNames) {
+		for (String oldName : oldNames) {
 
-			boolean match = panelKeyStrokeMap.containsKey(name);
+			boolean match = currentKsMap.containsKey(oldName);
-			ActionTrigger actionTrigger = oldOptions.getActionTrigger(name, null);
+			ActionTrigger oldTrigger = oldOptions.getActionTrigger(oldName, null);
-			KeyStroke optionsKs = null;
+			KeyStroke oldKs = null;
-			if (actionTrigger != null) {
+			if (oldTrigger != null) {
-				optionsKs = actionTrigger.getKeyStroke();
+				oldKs = oldTrigger.getKeyStroke();
 			}
 
-			KeyStroke panelKs = panelKeyStrokeMap.get(name);
+			KeyStroke currentKs = currentKsMap.get(oldName);
 
 			// if the value is null, then it would not have been placed into the options map 
 			// in the key bindings panel, so we only care about non-null values
-			if (optionsKs != null) {
+			if (oldKs != null) {
-				match &= (optionsKs.equals(panelKs));
+				match &= (oldKs.equals(currentKs));
 			}
 			else {
 				match = true;
@@ -650,6 +653,18 @@ public class KeyBindingUtilsTest extends AbstractGhidraHeadedIntegrationTest {
 
 			// short-circuit if there are any data that don't match
 			if (!match) {
+
+				boolean containsOption = currentKsMap.containsKey(oldName);
+
+				String message = """
+						Old key stroke does not match new key stroke.
+							Option: %s
+							Old: %s
+							New: %s
+							Option name in new options?: %s
+						""".formatted(oldName, oldKs, currentKs, containsOption);
+				Msg.debug(this, message);
+
 				return false;
 			}
 		}

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

@@ -0,0 +1,40 @@
+/* ###
+ * 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 java.awt.Component;
+
+import ghidra.util.Msg;
+
+/**
+ * Action context for {@link DialogComponentProvider}s.
+ */
+public class DialogActionContext extends DefaultActionContext {
+
+	public DialogActionContext(DialogComponentProvider dialogProvider, Component sourceComponent) {
+		super(null, dialogProvider, sourceComponent);
+	}
+
+	public DialogComponentProvider getDialogComponentProvider() {
+		Object contextObject = getContextObject();
+		if (contextObject instanceof DialogComponentProvider dcp) {
+			return dcp;
+		}
+
+		Msg.warn(this, "Found dialog context without a DialogComponentProvider context object");
+		return null;
+	}
+}

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

@@ -28,13 +28,14 @@ import org.jdesktop.animation.timing.TimingTargetAdapter;
 
 import docking.action.*;
 import docking.action.builder.ActionBuilder;
+import docking.actions.SharedActionRegistry;
+import docking.actions.ToolActions;
 import docking.event.mouse.GMouseListenerAdapter;
 import docking.menu.DialogToolbarButton;
 import docking.util.AnimationUtils;
 import docking.widgets.label.GDHtmlLabel;
 import generic.theme.GColor;
 import generic.theme.GThemeDefaults.Colors.Messages;
-import generic.util.WindowUtilities;
 import ghidra.util.*;
 import ghidra.util.exception.AssertException;
 import ghidra.util.task.*;
@@ -48,6 +49,7 @@ import utility.function.Callback;
 public class DialogComponentProvider
 		implements ActionContextProvider, StatusListener, TaskListener {
 
+	private static final String CLOSE_ACTION_NAME = "Close Dialog";
 	private static final Color FG_COLOR_ALERT = new GColor("color.fg.dialog.status.alert");
 	private static final Color FG_COLOR_ERROR = new GColor("color.fg.dialog.status.error");
 	private static final Color FG_COLOR_WARNING = new GColor("color.fg.dialog.status.warning");
@@ -82,7 +84,6 @@ public class DialogComponentProvider
 	private TaskMonitorComponent taskMonitorComponent;
 
 	private static final KeyStroke ESC_KEYSTROKE = KeyStroke.getKeyStroke(KeyEvent.VK_ESCAPE, 0);
-	private DockingAction closeAction;
 
 	private CardLayout progressCardLayout;
 	private JButton defaultButton;
@@ -182,36 +183,29 @@ public class DialogComponentProvider
 			rootPanel.add(panel, BorderLayout.SOUTH);
 		}
 
-		installEscapeAction();
-
 		doInitialize();
 	}
 
-	private void installEscapeAction() {
+	/**
-		closeAction = new ActionBuilder("Close Dialog", title)
+	 * Called by the framework during startup to register actions that are shared throughout the 
+	 * tool.  See {@link SharedActionRegistry}.
+	 * @param tool the tool
+	 * @param toolActions the class to which the actions should be added
+	 * @param owner the shared action owner
+	 */
+	public static void createSharedActions(Tool tool, ToolActions toolActions, String owner) {
+
+		DockingAction closeAction = new ActionBuilder(CLOSE_ACTION_NAME, owner)
 				.sharedKeyBinding()
 				.keyBinding(ESC_KEYSTROKE)
-				.enabledWhen(this::isMyDialog)
+				.withContext(DialogActionContext.class)
-				.onAction(c -> escapeCallback())
+				.enabledWhen(c -> c.getDialogComponentProvider() != null)
+				.onAction(c -> {
+					DialogComponentProvider dcp = c.getDialogComponentProvider();
+					dcp.escapeCallback();
+				})
 				.build();
-
+		toolActions.addGlobalAction(closeAction);
-		addAction(closeAction);
-	}
-
-	private boolean isMyDialog(ActionContext c) {
-		//
-		// Each dialog registers a shared action bound to Escape.  If all dialog actions are 
-		// enabled, then the user will get prompted to pick which dialog to close when pressing
-		// Escape.  Thus, we limit the enablement of each action to be the dialog that contains the
-		// focused component.  We use the action context to find out if this dialog is the active
-		// dialog.
-		//
-		Window window = WindowUtilities.windowForComponent(c.getSourceComponent());
-		if (!(window instanceof DockingDialog dockingDialog)) {
-			return false;
-		}
-
-		return dockingDialog.containsProvider(DialogComponentProvider.this);
 	}
 
 	/** a callback mechanism for children to do work */
@@ -220,13 +214,19 @@ public class DialogComponentProvider
 	}
 
 	/**
-	 * Returns true if the given keystroke is the trigger for this dialog's close action.
+	 * Returns true if the given action is one that has been registered by this dialog.
-	 * @param ks the keystroke
+	 * @param action the action 
-	 * @return true if the given keystroke is the trigger for this dialog's close action
+	 * @return true if the given action is one that has been registered by this dialog
 	 */
-	public boolean isCloseKeyStroke(KeyStroke ks) {
+	public boolean isDialogKeyBindingAction(DockingActionIf action) {
-		KeyStroke currentCloseKs = closeAction.getKeyBinding();
+		if (action instanceof DockingActionProxy proxy) {
-		return Objects.equals(ks, currentCloseKs);
+			return keyBindingProxyActions.contains(proxy);
+		}
+		String name = action.getName();
+		if (name.equals(CLOSE_ACTION_NAME)) {
+			return true;
+		}
+		return false;
 	}
 
 	public int getId() {
@@ -1254,14 +1254,14 @@ public class DialogComponentProvider
 		}
 
 		if (event == null) {
-			return new DefaultActionContext(null, c);
+			return new DialogActionContext(this, c);
 		}
 
 		Component sourceComponent = event.getComponent();
 		if (sourceComponent != null) {
 			c = sourceComponent;
 		}
-		return new DefaultActionContext(null, c).setSourceObject(event.getSource());
+		return new DialogActionContext(this, c).setSourceObject(event.getSource());
 	}
 
 	/**

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

@@ -4,9 +4,9 @@
  * 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.
@@ -61,6 +61,10 @@ public abstract class DockingKeyBindingAction extends AbstractAction {
 		dockingAction.actionPerformed(context);
 	}
 
+	public List<DockingActionIf> getValidActions(Object source) {
+		return getActions(); // the action for this class is always enabled and valid
+	}
+
 	protected ActionContext getLocalContext(ComponentProvider localProvider) {
 		if (localProvider == null) {
 			return new DefaultActionContext();

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

@@ -20,10 +20,12 @@ import static docking.KeyBindingPrecedence.*;
 import java.awt.*;
 import java.awt.event.KeyEvent;
 import java.awt.event.KeyListener;
+import java.util.List;
 
 import javax.swing.*;
 import javax.swing.text.JTextComponent;
 
+import docking.action.DockingActionIf;
 import docking.action.MultipleKeyAction;
 import docking.actions.KeyBindingUtils;
 import docking.menu.keys.MenuKeyProcessor;
@@ -133,8 +135,7 @@ public class KeyBindingOverrideKeyEventDispatcher implements KeyEventDispatcher
 		}
 
 		// some known special cases that we don't wish to process
-		KeyStroke ks = KeyStroke.getKeyStrokeForEvent(event);
+		if (!isValidContextForAction(event, action)) {
-		if (!isValidContextForKeyStroke(ks)) {
 			return false;
 		}
 
@@ -224,35 +225,33 @@ public class KeyBindingOverrideKeyEventDispatcher implements KeyEventDispatcher
 		return wasInProgress;
 	}
 
-	/**
+	private boolean isValidContextForAction(KeyEvent event, DockingKeyBindingAction kbAction) {
-	 * A check to see if a given keystroke is something that should not be processed, depending
-	 * upon the current state of the system.
-	 * 
-	 * @param keyStroke The keystroke to check.
-	 * @return true if the caller of this method should handle the keystroke; false if the
-	 *         keystroke should be ignored.
-	 */
-	private boolean isValidContextForKeyStroke(KeyStroke keyStroke) {
 		Window activeWindow = focusProvider.getActiveWindow();
-		if (activeWindow instanceof DockingDialog) {
+		if (!(activeWindow instanceof DockingDialog dialog)) {
-
+			return true; // allow all non-dialog windows to process events
-			// The choice to ignore modal dialogs was made long ago.  We cannot remember why the
-			// choice was made, but speculate that odd things can happen when keybindings are
-			// processed with modal dialogs open.  For now, do not let key bindings get processed
-			// for modal dialogs.  This can be changed in the future if needed.
-			DockingDialog dialog = (DockingDialog) activeWindow;
-			if (!dialog.isModal()) {
-				return true;
-			}
-
-			// Allow modal dialogs to process close keystrokes (e.g., ESCAPE) so they can be closed
-			DialogComponentProvider provider = dialog.getComponent();
-			if (provider.isCloseKeyStroke(keyStroke)) {
-				return true;
-			}
-			return false; // modal dialog; non-escape key
 		}
-		return true; // default case; allow it through
+
+		// The choice to ignore modal dialogs was made long ago.  We cannot remember why the
+		// choice was made, but speculate that odd things can happen when keybindings are
+		// processed with modal dialogs open.  For now, do not let key bindings get processed
+		// for modal dialogs.  This can be changed in the future if needed.
+		if (!dialog.isModal()) {
+			return true;
+		}
+
+		// Allow modal dialogs to process their own actions
+		DialogComponentProvider provider = dialog.getComponent();
+		List<DockingActionIf> actions = kbAction.getValidActions(event.getSource());
+		if (actions.isEmpty()) {
+			return false; // no actions; not a valid key stroke for this dialog
+		}
+		for (DockingActionIf action : actions) {
+			if (!provider.isDialogKeyBindingAction(action)) {
+				return false;
+			}
+		}
+
+		return true; // all actions belong to the active dialog; this is a valid action
 	}
 
 	private boolean isSettingKeyBindings(KeyEvent event) {

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

@@ -4,9 +4,9 @@
  * 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.
@@ -109,6 +109,22 @@ public class MultipleKeyAction extends DockingKeyBindingAction {
 		}
 	}
 
+	@Override
+	public List<DockingActionIf> getValidActions(Object source) {
+
+		if (ignoreActionWhileMenuShowing()) {
+			return List.of();
+		}
+
+		List<DockingActionIf> validActions = new ArrayList<>();
+		List<ExecutableAction> proxyActions = getActionsForCurrentOrDefaultContext(source);
+		for (ExecutableAction proxy : proxyActions) {
+			DockingActionIf action = proxy.getAction();
+			validActions.add(action);
+		}
+		return validActions;
+	}
+
 	@Override
 	public void actionPerformed(final ActionEvent event) {
 		// Build list of actions which are valid in current context

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

@@ -4,9 +4,9 @@
  * 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.
@@ -15,6 +15,7 @@
  */
 package docking.actions;
 
+import docking.DialogComponentProvider;
 import docking.Tool;
 import docking.action.DockingActionIf;
 import docking.tool.ToolConstants;
@@ -38,5 +39,7 @@ public class SharedActionRegistry {
 		GTable.createSharedActions(tool, toolActions, ToolConstants.SHARED_OWNER);
 
 		GTree.createSharedActions(tool, toolActions, ToolConstants.SHARED_OWNER);
+
+		DialogComponentProvider.createSharedActions(tool, toolActions, ToolConstants.SHARED_OWNER);
 	}
 }

Ghidra/Framework/Generic/src/main/resources/generic.log4jdev.xml

@@ -63,6 +63,7 @@
 		<logger name="ghidra.program.database" level="DEBUG" />
 		<logger name="ghidra.program.model.lang.xml" level="DEBUG"/>		
 		<logger name="ghidra.app.plugin.assembler" level="DEBUG" />		
+		<logger name="ghidra.app.plugin.core.debug" level="DEBUG" />
 		<logger name="ghidra.app.plugin.core.functiongraph" level="DEBUG" />	
 		<logger name="ghidra.app.plugin.core.string" level="DEBUG" />
 		<logger name="ghidra.app.plugin.core.libraryidentification" level="INFO"/>

Ghidra/Framework/Generic/src/main/resources/generic.log4jtest.xml

@@ -60,7 +60,8 @@
 		<logger name="ghidra.pcodeCPort.slgh_compile" level="INFO"/>
 		<logger name="ghidra.program.database" level="DEBUG" />
 		<logger name="ghidra.program.model.lang.xml" level="DEBUG"/>
-		<logger name="ghidra.app.plugin.assembler" level="DEBUG" />		
+		<logger name="ghidra.app.plugin.assembler" level="DEBUG" />	
+		<logger name="ghidra.app.plugin.core.debug" level="DEBUG" />	
 		<logger name="ghidra.app.plugin.core.functiongraph" level="DEBUG" />	
 		<logger name="ghidra.app.plugin.core.string" level="DEBUG" />
 		<logger name="ghidra.app.plugin.core.libraryidentification" level="INFO"/>