Fix for Escape action not working for some dialogs

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/label/LabelMgrPlugin.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,7 +61,7 @@ public class LabelMgrPlugin extends Plugin {
 		// Setup list of actions
 		setupActions();
 
-		addEditDialog = new AddEditDialog("", tool);
+		addEditDialog = new AddEditDialog("Add/Edit Label", tool);
 		addEditDialog.setReusable(true);
 	}
 
@@ -278,7 +278,7 @@ public class LabelMgrPlugin extends Plugin {
 			addr = loc.getAddress();
 		}
 		else if (location instanceof OperandFieldLocation) {
-			Address a = ((OperandFieldLocation) location).getRefAddress();
+			Address a = location.getRefAddress();
 			addr = (a == null) ? addr : a;
 		}
 

Ghidra/Features/Base/src/main/java/help/screenshot/AbstractScreenShotGenerator.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.
@@ -237,6 +237,7 @@ public abstract class AbstractScreenShotGenerator extends AbstractGhidraHeadedIn
 	public void performAction(String actionName, String owner, ComponentProvider contextProvider,
 			boolean wait) {
 		DockingActionIf action = getAction(tool, owner, actionName);
+		assertNotNull("Could not find action: " + actionName + " for owner " + owner, action);
 		performAction(action, contextProvider, wait);
 	}
 

Ghidra/Features/Base/src/main/java/help/screenshot/GhidraScreenShotGenerator.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.
@@ -225,7 +225,7 @@ public abstract class GhidraScreenShotGenerator extends AbstractScreenShotGenera
 		showDialog(dialog);
 	}
 
-	private void showDialog(final DialogComponentProvider dialogComponent) {
+	private void showDialog(DialogComponentProvider dialogComponent) {
 		runSwing(() -> {
 			DockingDialog dialog = DockingDialog.createDialog(null, dialogComponent, null);
 			dialog.setLocation(WindowUtilities.centerOnScreen(dialog.getSize()));

Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/misc/PropertyEditorTest.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.
@@ -25,7 +25,7 @@ import javax.swing.*;
 import org.junit.After;
 import org.junit.Test;
 
-import docking.DockingDialog;
+import docking.DockingWindowManager;
 import docking.options.editor.OptionsDialog;
 import docking.options.editor.StringWithChoicesEditor;
 import docking.widgets.tree.GTree;
@@ -34,26 +34,22 @@ import ghidra.test.AbstractGhidraHeadedIntegrationTest;
 
 public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 
-	private DockingDialog dialog;
+	private OptionsDialog dialog;
 
 	@After
 	public void tearDown() throws Exception {
 		if (dialog != null) {
-			dialog.setVisible(false);
+			close(dialog);
-			dialog.dispose();
 		}
 		waitForSwing();
 	}
 
-	private DockingDialog showEditor(Options options) {
+	private OptionsDialog showEditor(Options options) {
 		OptionsDialog dialogComponent =
 			new OptionsDialog("Test Properties", "Properties", new Options[] { options }, null);
-		final DockingDialog editorDialog =
+		runSwing(() -> DockingWindowManager.showDialog(dialogComponent), false);
-			runSwing(() -> DockingDialog.createDialog(null, dialogComponent, null));
-		SwingUtilities.invokeLater(() -> editorDialog.setVisible(true));
 
-		waitForJDialog("Test Properties");
+		OptionsDialog editorDialog = waitForDialogComponent(OptionsDialog.class);
-		assertNotNull("Dialog failed to launch", editorDialog);
 		waitForSwing();
 		waitForOptionsTree(dialogComponent);
 		return editorDialog;
@@ -65,10 +61,9 @@ public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 		waitForTree(tree);
 	}
 
-	private Component findPairedComponent(final Container container, final String labelText) {
+	private Component findPairedComponent(final String labelText) {
-		final Component[] box = new Component[1];
+		JComponent component = dialog.getComponent();
-		runSwing(() -> box[0] = doFindPairedComponent(container, labelText));
+		return runSwing(() -> doFindPairedComponent(component, labelText));
-		return box[0];
 	}
 
 	private Component doFindPairedComponent(Container container, String labelText) {
@@ -102,7 +97,7 @@ public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 
 		dialog = showEditor(options);
 
-		Component editor = findPairedComponent(dialog, "TestInt");
+		Component editor = findPairedComponent("TestInt");
 		assertNotNull("Could not find editor component", editor);
 		assertEquals(PropertyText.class, editor.getClass());
 		final PropertyText textField = (PropertyText) editor;
@@ -125,7 +120,7 @@ public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 
 		dialog = showEditor(options);
 
-		Component editor = findPairedComponent(dialog, "TestLong");
+		Component editor = findPairedComponent("TestLong");
 		assertNotNull("Could not find editor component", editor);
 		assertEquals(PropertyText.class, editor.getClass());
 		final PropertyText textField = (PropertyText) editor;
@@ -148,7 +143,7 @@ public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 
 		dialog = showEditor(options);
 
-		Component editor = findPairedComponent(dialog, "TestFloat");
+		Component editor = findPairedComponent("TestFloat");
 		assertNotNull("Could not find editor component", editor);
 		assertEquals(PropertyText.class, editor.getClass());
 		final PropertyText textField = (PropertyText) editor;
@@ -173,7 +168,7 @@ public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 
 		dialog = showEditor(options);
 
-		Component editor = findPairedComponent(dialog, "TestDouble");
+		Component editor = findPairedComponent("TestDouble");
 		assertNotNull("Could not find editor component", editor);
 		assertEquals(PropertyText.class, editor.getClass());
 		final PropertyText textField = (PropertyText) editor;
@@ -197,7 +192,7 @@ public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 
 		dialog = showEditor(options);
 
-		Component editor = findPairedComponent(dialog, "TestString");
+		Component editor = findPairedComponent("TestString");
 		assertNotNull("Could not find editor component", editor);
 		assertEquals(PropertyText.class, editor.getClass());
 		final PropertyText textField = (PropertyText) editor;
@@ -222,7 +217,7 @@ public class PropertyEditorTest extends AbstractGhidraHeadedIntegrationTest {
 
 		dialog = showEditor(options);
 
-		Component editor = findPairedComponent(dialog, "TestStringWithChoices");
+		Component editor = findPairedComponent("TestStringWithChoices");
 		assertNotNull("Could not find editor component", editor);
 		assertEquals(PropertySelector.class, editor.getClass());
 		final PropertySelector textSelector = (PropertySelector) editor;

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

@@ -91,11 +91,14 @@ public class DialogComponentProvider
 	private Component focusComponent;
 	private JPanel toolbar;
 
-	private final Map<DockingActionIf, DialogToolbarButton> actionMap = new HashMap<>();
+	private Map<DockingActionIf, DialogToolbarButton> toolbarButtonsByAction = new HashMap<>();
-	private final DialogComponentProviderPopupActionManager popupManager =
+	private DialogComponentProviderPopupActionManager popupManager =
 		new DialogComponentProviderPopupActionManager(this);
-	private final PopupHandler popupHandler = new PopupHandler();
+	private PopupHandler popupHandler = new PopupHandler();
-	private final Set<DockingActionIf> dialogActions = new HashSet<>();
+	private Set<DockingActionIf> dialogActions = new HashSet<>();
+
+	// we track these separately so that we can remove them on dispose
+	private Set<DialogActionProxy> keyBindingProxyActions = new HashSet<>();
 
 	private Point initialLocation;
 	private boolean resizeable = true;
@@ -106,6 +109,7 @@ public class DialogComponentProvider
 
 	private Dimension defaultSize;
 	private String accessibleDescription;
+	private Tool tool;
 
 	/**
 	 * Constructor for a DialogComponentProvider that will be modal and will include a status line and
@@ -177,13 +181,13 @@ public class DialogComponentProvider
 			panel.add(buttonPanel);
 			rootPanel.add(panel, BorderLayout.SOUTH);
 		}
+
 		installEscapeAction();
 
 		doInitialize();
 	}
 
 	private void installEscapeAction() {
-
 		closeAction = new ActionBuilder("Close Dialog", title)
 				.sharedKeyBinding()
 				.keyBinding(ESC_KEYSTROKE)
@@ -933,11 +937,16 @@ public class DialogComponentProvider
 
 		closeDialog();
 
+		if (tool != null) {
+			keyBindingProxyActions.forEach(a -> tool.removeAction(a));
+		}
+		keyBindingProxyActions.clear();
+
 		popupManager.dispose();
 
 		dialogActions.forEach(DockingActionIf::dispose);
 
-		actionMap.clear();
+		toolbarButtonsByAction.clear();
 		dialogActions.clear();
 	}
 
@@ -1153,6 +1162,38 @@ public class DialogComponentProvider
 		return ((dialog != null) && dialog.isShowing());
 	}
 
+	/**
+	 * Called each time the dialog is show.  The given tool is the parent tool of the dialog.
+	 * @param t the tool
+	 */
+	void dialogShown(Tool t) {
+		setTool(t);
+		dialogShown();
+	}
+
+	private void setTool(Tool tool) {
+
+		if (this.tool != null) {
+			return;
+		}
+
+		// initialize the first time we are shown
+		this.tool = tool;
+
+		if (tool == null) {
+			// The tool can be null for dialogs shown before the framework is initialized, like 
+			// dialogs shown over the splash screen.  Without a tool, we cannot add key binding
+			// actions.
+			return;
+		}
+
+		// Any actions in this list already were added before we had a tool.  Add them now.  Any
+		// future calls to addKeyBindingAction() will get added to the tool at that time.
+		for (DockingActionIf proxy : keyBindingProxyActions) {
+			tool.addAction(proxy);
+		}
+	}
+
 	/**
 	 * Override this method if you want to do something when the dialog is made visible
 	 */
@@ -1232,7 +1273,7 @@ public class DialogComponentProvider
 		if (context == null) {
 			context = new DefaultActionContext();
 		}
-		Set<DockingActionIf> keySet = actionMap.keySet();
+		Set<DockingActionIf> keySet = toolbarButtonsByAction.keySet();
 		for (DockingActionIf action : keySet) {
 			action.setEnabled(action.isEnabledForContext(context));
 		}
@@ -1255,7 +1296,7 @@ public class DialogComponentProvider
 
 		DialogToolbarButton button = new DialogToolbarButton(action, this);
 		toolbar.add(button);
-		actionMap.put(action, button);
+		toolbarButtonsByAction.put(action, button);
 	}
 
 	/**
@@ -1264,7 +1305,7 @@ public class DialogComponentProvider
 	 * the tool, as this dialog will do that for you.
 	 * @param action the action
 	 */
-	public void addAction(final DockingActionIf action) {
+	public void addAction(DockingActionIf action) {
 		dialogActions.add(action);
 		addToolbarAction(action);
 		popupManager.addAction(action);
@@ -1273,25 +1314,49 @@ public class DialogComponentProvider
 
 	private void addKeyBindingAction(DockingActionIf action) {
 
-		// add the action to the tool in order get key event management (key bindings
+		DialogActionProxy proxy = new DialogActionProxy(action);
-		// options and key event processing)
+		keyBindingProxyActions.add(proxy);
-		DockingWindowManager dwm = DockingWindowManager.getActiveInstance();
-		if (dwm == null) {
-			// This implies the client dialog has been shown outside of the plugin framework. In
-			// that case, the client will not get key event processing for dialog actions.
-			return;
-		}
 
-		Tool tool = dwm.getTool();
+		// The tool will be null when clients add actions to this dialog before it has been shown.
-		tool.addAction(new DialogActionProxy(action));
+		// This is different than ComponentProviders, which require a Tool at construction.  The
+		// DialogComponentProvider did not originally require a Tool at construction.  Rather than
+		// refactor the dialog to require a Tool, it was easier to simply delay adding actions until
+		// the Tool is set when the dialog is first shown.
+		if (tool != null) {
+			tool.addAction(proxy);
+		}
 	}
 
 	public void removeAction(DockingActionIf action) {
 		dialogActions.remove(action);
-		JButton button = actionMap.remove(action);
+		JButton button = toolbarButtonsByAction.remove(action);
 		if (button != null && toolbar != null) {
 			toolbar.remove(button);
 		}
+
+		popupManager.removeAction(action);
+
+		removeKeyBindingAction(action);
+	}
+
+	private void removeKeyBindingAction(DockingActionIf action) {
+
+		DialogActionProxy proxy = null;
+		for (DialogActionProxy actionProxy : keyBindingProxyActions) {
+			if (action == actionProxy.getAction()) {
+				proxy = actionProxy;
+				break;
+			}
+		}
+
+		if (proxy == null) {
+			return;
+		}
+
+		keyBindingProxyActions.remove(proxy);
+		if (tool != null) {
+			tool.removeAction(proxy);
+		}
 	}
 
 	/**

Ghidra/Framework/Docking/src/main/java/docking/DialogComponentProviderPopupActionManager.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.
@@ -49,6 +49,10 @@ public class DialogComponentProviderPopupActionManager {
 		popupActions.add(action);
 	}
 
+	void removeAction(DockingActionIf action) {
+		popupActions.remove(action);
+	}
+
 	void popupMenu(ActionContext actionContext, MouseEvent e) {
 		if (e.isConsumed()) {
 			return;

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

@@ -210,7 +210,11 @@ public class DockingDialog extends JDialog implements HelpDescriptor {
 
 			@Override
 			public void windowOpened(WindowEvent e) {
-				component.dialogShown();
+				Tool tool = null;
+				if (owningWindowManager != null) {
+					tool = owningWindowManager.getTool();
+				}
+				component.dialogShown(tool);
 			}
 
 			@Override

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

@@ -1814,7 +1814,8 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder
 				bestCenter = centeredOnComponent;
 			}
 
-			DockingDialog dialog = DockingDialog.createDialog(bestParent, provider, bestCenter);
+			DockingDialog dialog =
+				DockingDialog.createDialog(bestParent, provider, bestCenter);
 			dialog.setVisible(true);
 		};
 

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

@@ -216,8 +216,9 @@ public class KeyBindingOverrideKeyEventDispatcher implements KeyEventDispatcher
 			KeyStroke keyStroke = KeyStroke.getKeyStrokeForEvent(event);
 
 			// note: this call has no effect if 'action' is null
-			SwingUtilities.notifyAction(action, keyStroke, event, event.getSource(),
+			Object source = event.getSource();
-				event.getModifiersEx());
+			int modifiersEx = event.getModifiersEx();
+			SwingUtilities.notifyAction(action, keyStroke, event, source, modifiersEx);
 
 		}
 		return wasInProgress;

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

@@ -350,7 +350,6 @@ public abstract class DockingAction implements DockingActionIf {
 
 	@Override
 	public void setKeyBindingData(KeyBindingData newKeyBindingData) {
-
 		if (!supportsKeyBinding(newKeyBindingData)) {
 			return;
 		}
@@ -365,6 +364,11 @@ public abstract class DockingAction implements DockingActionIf {
 		firePropertyChanged(KEYBINDING_DATA_PROPERTY, oldData, keyBindingData);
 	}
 
+	// this allows framework classes to directly set the default value
+	protected void setDefaultKeyBindingData(KeyBindingData kbd) {
+		defaultKeyBindingData = kbd;
+	}
+
 	private boolean supportsKeyBinding(KeyBindingData kbData) {
 
 		KeyBindingType type = getKeyBindingType();

Ghidra/Framework/Docking/src/main/java/docking/actions/KeyBindings.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.
@@ -363,9 +363,14 @@ public class KeyBindings {
 				return;
 			}
 
+			// 1) update the options with the new value
 			ActionTrigger newTrigger = keyBinding.getActionTrigger();
 			String fullName = getFullName();
 			keyStrokeOptions.setActionTrigger(fullName, newTrigger);
+
+			// 2) update our state so the UI shows the new value
+			currentKeyStroke = newTrigger.getKeyStroke();
+			currentMouseBinding = newTrigger.getMouseBinding();
 		}
 
 		private boolean hasChanged() {
@@ -409,11 +414,11 @@ public class KeyBindings {
 			DockingActionIf action = getRepresentativeAction();
 			KeyBindingData defaultBinding = action.getDefaultKeyBindingData();
 
+			cancelChanges();
+
 			if (!matches(defaultBinding)) {
 				apply(options, defaultBinding);
 			}
-
-			cancelChanges();
 		}
 
 	}

Ghidra/Framework/Docking/src/main/java/docking/actions/SharedStubKeyBindingAction.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.
@@ -120,14 +120,21 @@ public class SharedStubKeyBindingAction extends DockingAction implements Options
 	void addClientAction(DockingActionIf action) {
 
 		// 1) Validate new action keystroke against existing actions
-		ActionTrigger defaultKs = validateActionsHaveTheSameDefaultKeyStroke(action);
+		ActionTrigger defaultTrigger = validateActionsHaveTheSameDefaultKeyStroke(action);
 
 		// 2) Add the action and the validated keystroke, as this is the default keystroke
-		clientActions.put(action, defaultKs);
+		clientActions.put(action, defaultTrigger);
 
 		// 3) Update the given action with the current option value.  This allows clients to 
 		//    add and remove actions after the tool has been initialized.
-		updateActionKeyStrokeFromOptions(action, defaultKs);
+		updateActionKeyStrokeFromOptions(action, defaultTrigger);
+
+		// 4) Store the default value for this stub so later requests to Restore Defaults will have
+		//    the correct value.
+		if (defaultTrigger != null) {
+			KeyBindingData defaultKbd = new KeyBindingData(defaultTrigger);
+			setDefaultKeyBindingData(defaultKbd);
+		}
 	}
 
 	@Override

Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsDialog.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.
@@ -48,7 +48,7 @@ public class OptionsDialog extends ReusableDialogComponentProvider {
 
 	public OptionsDialog(String title, String rootNodeName, Options[] options,
 			OptionsEditorListener listener, boolean showRestoreDefaultsButton) {
-		super("OptionsDialog.Foofoo", true, false, true, false);
+		super("Options Dialog", true, false, true, false);
 		this.listener = listener;
 		panel = new OptionsPanel(rootNodeName, options, showRestoreDefaultsButton,
 			new OptionsPropertyChangeListener());

Ghidra/Test/IntegrationTest/src/screen/java/help/screenshot/BookmarkPluginScreenShots.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.
@@ -52,14 +52,14 @@ public class BookmarkPluginScreenShots extends GhidraScreenShotGenerator {
 	public void testBookmarks() {
 		removeFlowArrows();
 		createBookmarks();
-		performAction("Show Bookmarks", "BookmarkPlugin", true);
+		performAction("Bookmarks", "BookmarkPlugin", true);
 		captureIsolatedProvider(BookmarkProvider.class, 900, 300);
 
 	}
 
 	@Test
 	public void testBookmarksFilter() {
-		performAction("Show Bookmarks", "BookmarkPlugin", true);
+		performAction("Bookmarks", "BookmarkPlugin", true);
 		performAction("Filter Bookmarks", "BookmarkPlugin", false);
 		waitForSwing();