Merge remote-tracking branch 'origin/GP-61-dragonmacher-clear-keybindings'

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/codebrowser/actions/GotoNextFunctionAction.java

@@ -39,7 +39,7 @@ public class GotoNextFunctionAction extends NavigatableContextAction {
 	private PluginTool tool;
 
 	public GotoNextFunctionAction(PluginTool tool, String owner) {
-		super("Go to next function", owner);
+		super("Go To Next Function", owner);
 		this.tool = tool;
 
 		MenuData menuData =

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/codebrowser/actions/GotoPreviousFunctionAction.java

@@ -39,7 +39,7 @@ public class GotoPreviousFunctionAction extends NavigatableContextAction {
 	private PluginTool tool;
 
 	public GotoPreviousFunctionAction(PluginTool tool, String owner) {
-		super("Go to previous function", owner);
+		super("Go To Previous Function", owner);
 		this.tool = tool;
 
 		MenuData menuData =

Ghidra/Features/Base/src/main/java/ghidra/test/TestProgramManager.java

@@ -33,7 +33,8 @@ import ghidra.program.model.listing.Program;
 import ghidra.program.util.DefaultLanguageService;
 import ghidra.util.*;
 import ghidra.util.exception.*;
-import ghidra.util.task.*;
+import ghidra.util.task.ConsoleTaskMonitor;
+import ghidra.util.task.TaskMonitor;
 import utilities.util.FileUtilities;
 
 /**
@@ -123,11 +124,14 @@ public class TestProgramManager {
 	/**
 	 * Save a program to the cached program store.  A SaveAs will be performed on the
 	 * program to its cached storage location.
+	 * 
 	 * @param progName program name
 	 * @param program program object
 	 * @param replace if true any existing cached database with the same name will be replaced
 	 * @param monitor task monitor
+	 * @throws IOException if the database cannot be created
 	 * @throws DuplicateNameException if already cached
+	 * @throws CancelledException if the save operation is cancelled
 	 */
 	public void saveToCache(String progName, ProgramDB program, boolean replace,
 			TaskMonitor monitor) throws IOException, DuplicateNameException, CancelledException {
@@ -205,7 +209,9 @@ public class TestProgramManager {
 	 * the only reason to use this method vice openProgram().
 	 *
 	 * @param project the project into which the file will be restored
-	 * @param programName the name of the program zip file without the ".gzf" extension.
+	 * @param programName the name of the program zip file without the ".gzf" extension
+	 * @return the file
+	 * @throws FileNotFoundException if the file cannot be found 
 	 */
 	public DomainFile addProgramToProject(Project project, String programName)
 			throws FileNotFoundException {
@@ -221,6 +227,8 @@ public class TestProgramManager {
 	 *
 	 * @param folder the folder into which the domain file will be inserted
 	 * @param programName the name of the program zip file without the ".gzf" extension.
+	 * @return the file
+	 * @throws FileNotFoundException if the file cannot be found 
 	 */
 	public DomainFile addProgramToProject(DomainFolder folder, String programName)
 			throws FileNotFoundException {
@@ -234,7 +242,7 @@ public class TestProgramManager {
 		int oneUp = 0;
 		while (true) {
 			try {
-				DomainFile df = folder.createFile(name, gzf, TaskMonitorAdapter.DUMMY_MONITOR);
+				DomainFile df = folder.createFile(name, gzf, TaskMonitor.DUMMY);
 				AbstractGenericTest.waitForPostedSwingRunnables();
 				DomainObject dobj = df.getDomainObject(this, true, false, null);
 				try {
@@ -281,7 +289,7 @@ public class TestProgramManager {
 		DBHandle dbh = null;
 		boolean success = false;
 		try {
-			dbh = db.open(TaskMonitorAdapter.DUMMY_MONITOR);
+			dbh = db.open(TaskMonitor.DUMMY);
 			program = new ProgramDB(dbh, DBConstants.UPDATE, null, this);
 			success = true;
 		}
@@ -312,7 +320,7 @@ public class TestProgramManager {
 			}
 
 			Msg.info(this, message + (endTime - startTime));
-			dbh = db.open(TaskMonitorAdapter.DUMMY_MONITOR);
+			dbh = db.open(TaskMonitor.DUMMY);
 			program = new ProgramDB(dbh, DBConstants.UPDATE, null, this);
 			dbh = null;
 			success = true;
@@ -341,7 +349,7 @@ public class TestProgramManager {
 		File gzf = AbstractGenericTest.findTestDataFile(programName + ".gzf");
 		if (gzf != null && gzf.exists()) {
 			Msg.info(this, "Unpacking: " + gzf);
-			db = new PrivateDatabase(dbDir, gzf, TaskMonitorAdapter.DUMMY_MONITOR);
+			db = new PrivateDatabase(dbDir, gzf, TaskMonitor.DUMMY);
 			testPrograms.put(programName, db);
 			return db;
 		}
@@ -451,12 +459,12 @@ public class TestProgramManager {
 
 	private void upgradeDatabase(PrivateDatabase db) throws Exception {
 
-		DBHandle dbh = db.openForUpdate(TaskMonitorAdapter.DUMMY_MONITOR);
+		DBHandle dbh = db.openForUpdate(TaskMonitor.DUMMY);
 		try {
 			ProgramDB program =
-				new ProgramDB(dbh, DBConstants.UPGRADE, TaskMonitorAdapter.DUMMY_MONITOR, this);
+				new ProgramDB(dbh, DBConstants.UPGRADE, TaskMonitor.DUMMY, this);
 			if (dbh != null) {
-				dbh.save(null, null, TaskMonitorAdapter.DUMMY_MONITOR);
+				dbh.save(null, null, TaskMonitor.DUMMY);
 			}
 			dbh = null;
 			program.release(this);

Ghidra/Features/Base/src/test.slow/java/docking/action/KeyEntryDialogTest.java

@@ -19,19 +19,20 @@ import static org.junit.Assert.*;
 
 import java.awt.event.KeyEvent;
 import java.util.Map;
+import java.util.Set;
 
 import javax.swing.*;
 
 import org.junit.*;
 
 import docking.*;
-import docking.actions.DockingToolActions;
-import docking.actions.KeyEntryDialog;
+import docking.actions.*;
 import ghidra.app.plugin.core.codebrowser.CodeBrowserPlugin;
 import ghidra.app.plugin.core.navigation.GoToAddressLabelPlugin;
 import ghidra.framework.plugintool.PluginTool;
 import ghidra.test.AbstractGhidraHeadedIntegrationTest;
 import ghidra.test.TestEnv;
+import util.CollectionUtils;
 
 public class KeyEntryDialogTest extends AbstractGhidraHeadedIntegrationTest {
 
@@ -91,6 +92,39 @@ public class KeyEntryDialogTest extends AbstractGhidraHeadedIntegrationTest {
 		assertEquals(acceleratorKey.getKeyCode(), KeyEvent.VK_Q);
 	}
 
+	@Test
+	public void testClearDefaultKeyBinding() throws Exception {
+
+		DockingAction boundAction = getBoundAction_Shared();
+		showDialog(boundAction);
+
+		assertEquals("OPEN_BRACKET", keyEntryField.getText());
+		triggerBackspaceKey(keyEntryField);
+
+		pressDialogOK();
+		KeyStroke ks = boundAction.getKeyBinding();
+		assertNull(ks);
+	}
+
+	@Test
+	public void testClearDefaultKeyBinding_SharedKeybinding() throws Exception {
+
+		DockingAction boundAction = getBoundAction_Shared();
+		showDialog(boundAction);
+
+		KeyStroke oldKs = boundAction.getKeyBinding();
+		assertEquals("OPEN_BRACKET", keyEntryField.getText());
+		triggerBackspaceKey(keyEntryField);
+
+		pressDialogOK();
+		KeyStroke ks = boundAction.getKeyBinding();
+		assertNull(ks);
+
+		ToolActions toolActions = (ToolActions) tool.getToolActions();
+		Action toolAction = toolActions.getAction(oldKs);
+		assertNull("Shared actions' keybinding not cleared", toolAction);
+	}
+
 	@Test
 	public void testCollision() throws Exception {
 		// make sure we get a collision for a value that is already bound
@@ -185,6 +219,13 @@ public class KeyEntryDialogTest extends AbstractGhidraHeadedIntegrationTest {
 		return (DockingAction) getInstanceField("action", goToPlugin);
 	}
 
+	private DockingAction getBoundAction_Shared() {
+		Set<DockingActionIf> sharedActions =
+			getActionsByOwnerAndName(tool, "Shared", "Define Array");
+		assertFalse(sharedActions.isEmpty());
+		return (DockingAction) CollectionUtils.any(sharedActions);
+	}
+
 	private void pressDialogOK() {
 		JButton okButton = (JButton) getInstanceField("okButton", keyEntryDialog);
 		pressButton(okButton);

Ghidra/Features/Base/src/test.slow/java/ghidra/framework/options/ToolPluginOptionsTest.java

@@ -21,6 +21,8 @@ import java.awt.Color;
 import java.io.File;
 import java.util.*;
 
+import javax.swing.KeyStroke;
+
 import org.junit.*;
 
 import generic.stl.Pair;
@@ -182,11 +184,11 @@ public class ToolPluginOptionsTest extends AbstractGhidraHeadedIntegrationTest {
 		options = saveAndLoadOptions();
 		verifyStringOptionsStillChanged_UsingTheOptionsAPI(options, changedOption);
 
-		options.registerOption(NEW_TEST_OPTION_NAME, "default", null, null);
+		options.registerOption(NEW_TEST_OPTION_NAME, "default", null, "description");
 		options = saveAndLoadOptions();
 		verifyStringOptionsStillChanged_UsingTheOptionsAPI(options, changedOption);
 
-		options.registerOption(NEW_TEST_OPTION_NAME, "default", null, null);
+		options.registerOption(NEW_TEST_OPTION_NAME, "default", null, "description");
 		options = saveAndLoadOptions();
 		verifyStringOptionsStillChanged_UsingTheOptionsAPI(options, changedOption);
 
@@ -198,6 +200,28 @@ public class ToolPluginOptionsTest extends AbstractGhidraHeadedIntegrationTest {
 		verifyUnusedOptionNoLongerHasEntry(options, changedOption.first);
 	}
 
+	@Test
+	public void testClearingKeyBindingOption() {
+
+		Options options = loadKeyBindingOptions();
+
+		String optionName = clearKeyBinding(options);
+
+		options = saveAndLoadOptions();
+		verifyKeyBindingIsStillCleared(options, optionName);
+	}
+
+	@Test
+	public void testAccessingOptionWithoutRegistering() {
+
+		ToolOptions options = loadSearchOptions();
+		String defaultValue = "default";
+		Option option =
+			options.getOption(NEW_TEST_OPTION_NAME, OptionType.STRING_TYPE, defaultValue);
+		assertNotNull("unregistered option was not created on access", option);
+		assertEquals(defaultValue, option.getValue(defaultValue));
+	}
+
 	@Test
 	public void testSetFileOptionToNull() {
 		// 
@@ -309,12 +333,24 @@ public class ToolPluginOptionsTest extends AbstractGhidraHeadedIntegrationTest {
 		return tool.getOptions("Search");
 	}
 
+	private Options loadKeyBindingOptions() {
+		return tool.getOptions("Key Bindings");
+	}
+
 	private Pair<String, String> changeStringTestOption(Options options) {
-		options.registerOption(NEW_TEST_OPTION_NAME, "HEY", null, null);
+		options.registerOption(NEW_TEST_OPTION_NAME, "HEY", null, "description");
 		options.setString(NEW_TEST_OPTION_NAME, TEST_OPTION_STRING_VALUE);
 		return new Pair<>(NEW_TEST_OPTION_NAME, TEST_OPTION_STRING_VALUE);
 	}
 
+	private String clearKeyBinding(Options options) {
+		String keyBindingName = "Go To Next Function (CodeBrowserPlugin)";
+		KeyStroke ks = options.getKeyStroke(keyBindingName, null);
+		assertNotNull(ks);
+		options.setKeyStroke(keyBindingName, null);
+		return keyBindingName;
+	}
+
 	private void verifyStringOptionStillChanged_WithoutUsingOptionsAPI(Options options,
 			String optionName) {
 		assertTrue("Options does not have an entry for test option", options.contains(optionName));
@@ -340,6 +376,11 @@ public class ToolPluginOptionsTest extends AbstractGhidraHeadedIntegrationTest {
 		}
 	}
 
+	private void verifyKeyBindingIsStillCleared(Options options, String optionName) {
+		KeyStroke ksValue = options.getKeyStroke(optionName, null);
+		assertNull(ksValue);
+	}
+
 	private List<String> getDiffs(Map<String, Object> initialValues,
 			Map<String, Object> latestValues) {
 		List<String> diffs = new ArrayList<>();
@@ -354,20 +395,4 @@ public class ToolPluginOptionsTest extends AbstractGhidraHeadedIntegrationTest {
 		return diffs;
 	}
 
-	//	private OptionsPanel showSearchOptions() {
-//		List<DockingActionIf> actions = tool.getDockingActionsByFullActionName("Tool Options... (Tool)");
-//		assertEquals(1, actions.size());
-//		
-//		performAction(actions.get(0), false);
-//		
-//		OptionsDialog dialog = waitForDialogComponent(null, OptionsDialog.class, DEFAULT_WINDOW_TIMEOUT);
-//		
-//		OptionsPanel optionsPanel = (OptionsPanel) getInstanceField("panel", dialog);
-//		GTree tree = (GTree) getInstanceField("gTree", optionsPanel);
-//		tree.setSelectedNodeByNamePath(new String[] { "Options", "Search" });
-//		waitForTree(tree);
-//		
-//		return optionsPanel;
-//	}
-
 }

Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/ProgramUserDataTest.java

@@ -22,7 +22,7 @@ import java.util.Random;
 
 import org.junit.*;
 
-import generic.test.AbstractGenericTest;
+import generic.test.AbstractGTest;
 import ghidra.framework.data.ProjectFileManager;
 import ghidra.framework.model.*;
 import ghidra.program.model.address.Address;
@@ -35,12 +35,12 @@ import ghidra.test.AbstractGhidraHeadedIntegrationTest;
 import ghidra.test.ProjectTestUtils;
 import ghidra.util.Msg;
 import ghidra.util.exception.VersionException;
-import ghidra.util.task.TaskMonitorAdapter;
+import ghidra.util.task.TaskMonitor;
 import utilities.util.FileUtilities;
 
 public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 
-	private String TEMP = AbstractGenericTest.getTestDirectoryPath();
+	private String TEMP = AbstractGTest.getTestDirectoryPath();
 	private static Random RAND = new Random();
 
 	private ProjectLocator projectLocator;
@@ -60,20 +60,22 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 		userDir = new File(projectLocator.getProjectDir(), ProjectFileManager.USER_FOLDER_NAME);
 
 		ProgramBuilder builder = new ProgramBuilder("Test", ProgramBuilder._TOY);
-		df = project.getProjectData().getRootFolder().createFile("test", builder.getProgram(),
-			TaskMonitorAdapter.DUMMY_MONITOR);
+		df = project.getProjectData()
+				.getRootFolder()
+				.createFile("test", builder.getProgram(),
+					TaskMonitor.DUMMY);
 		builder.dispose();
 
 		Program program = null;
 		try {
 			program =
-				(Program) df.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+				(Program) df.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		}
 		catch (VersionException e) {
 			assertTrue(e.isUpgradable());
 			program =
-				(Program) df.getDomainObject(this, true, false, TaskMonitorAdapter.DUMMY_MONITOR);
-			program.save("upgrade", TaskMonitorAdapter.DUMMY_MONITOR);
+				(Program) df.getDomainObject(this, true, false, TaskMonitor.DUMMY);
+			program.save("upgrade", TaskMonitor.DUMMY);
 		}
 		finally {
 			program.release(this);
@@ -141,27 +143,27 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 		DomainFile df2;
 
 		Program program =
-			(Program) df.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+			(Program) df.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		space = program.getAddressFactory().getDefaultAddressSpace();
 		try {
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 			assertTrue(program.canSave());
 
 			// Modify program content - no user data should be saved
 			change(program);
 			assertTrue(program.isChanged());
-			program.save("save", TaskMonitorAdapter.DUMMY_MONITOR);
-			assertTrue(!program.isChanged());
+			program.save("save", TaskMonitor.DUMMY);
+			assertFalse(program.isChanged());
 			assertFalse("User data directory should be empty", userDataSubDir.isDirectory());
 
 			// Modify user data content
 			ProgramUserData userData = program.getProgramUserData();
 			change(userData, "STRING", space.getAddress(0), "Str0");
 			change(userData, "STRING", space.getAddress(10), "Str10");
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 
 			String newName = df.getName() + ".1";
-			df2 = df.getParent().createFile(newName, program, TaskMonitorAdapter.DUMMY_MONITOR);
+			df2 = df.getParent().createFile(newName, program, TaskMonitor.DUMMY);
 
 		}
 		finally {
@@ -169,9 +171,9 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 		}
 
 		program =
-			(Program) df2.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+			(Program) df2.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		try {
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 
 			// Verify user data content
 			ProgramUserData userData = program.getProgramUserData();
@@ -180,7 +182,7 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 			assertEquals("Str0", map.getString(space.getAddress(0)));
 			assertEquals("Str10", map.getString(space.getAddress(10)));
 			assertNull(map.getString(space.getAddress(20)));
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 		}
 		finally {
 			program.release(this);
@@ -203,17 +205,17 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 		int ver;
 
 		Program program =
-			(Program) df.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+			(Program) df.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		space = program.getAddressFactory().getDefaultAddressSpace();
 		try {
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 			assertTrue(program.canSave());
 
 			// Modify program content - no user data should be saved
 			change(program);
 			assertTrue(program.isChanged());
-			program.save("save", TaskMonitorAdapter.DUMMY_MONITOR);
-			assertTrue(!program.isChanged());
+			program.save("save", TaskMonitor.DUMMY);
+			assertFalse(program.isChanged());
 			assertFalse("User data directory should be empty", userDataSubDir.isDirectory());
 
 			ver = getLatestDbVersion(dbDir);
@@ -222,7 +224,7 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 			ProgramUserData userData = program.getProgramUserData();
 			change(userData, "STRING", space.getAddress(0), "Str0");
 			change(userData, "STRING", space.getAddress(10), "Str10");
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 
 		}
 		finally {
@@ -234,9 +236,9 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 			getLatestDbVersion(dbDir));
 
 		program =
-			(Program) df.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+			(Program) df.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		try {
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 
 			// Verify user data content
 			ProgramUserData userData = program.getProgramUserData();
@@ -245,21 +247,21 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 			assertEquals("Str0", map.getString(space.getAddress(0)));
 			assertEquals("Str10", map.getString(space.getAddress(10)));
 			assertNull(map.getString(space.getAddress(20)));
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 
 			// Modify user data content
 			change(userData, "STRING", space.getAddress(10), "Str10a");
 			change(userData, "STRING", space.getAddress(20), "Str20a");
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 		}
 		finally {
 			program.release(this);
 		}
 
 		program =
-			(Program) df.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+			(Program) df.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		try {
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 
 			// Verify user data content
 			ProgramUserData userData = program.getProgramUserData();
@@ -268,7 +270,7 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 			assertEquals("Str0", map.getString(space.getAddress(0)));
 			assertEquals("Str10a", map.getString(space.getAddress(10)));
 			assertEquals("Str20a", map.getString(space.getAddress(20)));
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 		}
 		finally {
 			program.release(this);
@@ -287,7 +289,7 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 		// TODO: Multi-user repository connect case not tested
 
 		Program program =
-			(Program) df.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+			(Program) df.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		space = program.getAddressFactory().getDefaultAddressSpace();
 		try {
 			// Create user data content
@@ -307,9 +309,9 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 		df = project.getProjectData().getFile("/test");
 
 		program =
-			(Program) df.getDomainObject(this, false, false, TaskMonitorAdapter.DUMMY_MONITOR);
+			(Program) df.getDomainObject(this, false, false, TaskMonitor.DUMMY);
 		try {
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 
 			// Verify user data content
 			ProgramUserData userData = program.getProgramUserData();
@@ -318,7 +320,7 @@ public class ProgramUserDataTest extends AbstractGhidraHeadedIntegrationTest {
 			assertEquals("Str0", map.getString(space.getAddress(0)));
 			assertEquals("Str10", map.getString(space.getAddress(10)));
 			assertNull(map.getString(space.getAddress(20)));
-			assertTrue(!program.isChanged());
+			assertFalse(program.isChanged());
 		}
 		finally {
 			program.release(this);

Ghidra/Features/Base/src/test/java/ghidra/framework/options/OptionsTest.java

@@ -43,10 +43,6 @@ public class OptionsTest extends AbstractGenericTest {
 
 	private ToolOptions options;
 
-	public OptionsTest() {
-		super();
-	}
-
 	@Before
 	public void setUp() throws Exception {
 		options = new ToolOptions("Test");

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

@@ -399,17 +399,14 @@ public class ToolActions implements DockingToolActions, PropertyChangeListener {
 		}
 
 		KeyBindingData newKeyBindingData = (KeyBindingData) evt.getNewValue();
-		KeyStroke newKeyStroke = null;
+		KeyStroke newKs = null;
 		if (newKeyBindingData != null) {
-			newKeyStroke = newKeyBindingData.getKeyBinding();
+			newKs = newKeyBindingData.getKeyBinding();
 		}
 
-		KeyStroke optKeyStroke = keyBindingOptions.getKeyStroke(action.getFullName(), null);
-		if (newKeyStroke == null) {
-			keyBindingOptions.removeOption(action.getFullName());
-		}
-		else if (!newKeyStroke.equals(optKeyStroke)) {
-			keyBindingOptions.setKeyStroke(action.getFullName(), newKeyStroke);
+		KeyStroke currentKs = keyBindingOptions.getKeyStroke(action.getFullName(), null);
+		if (!Objects.equals(currentKs, newKs)) {
+			keyBindingOptions.setKeyStroke(action.getFullName(), newKs);
 			keyBindingsChanged();
 		}
 	}

Ghidra/Framework/Docking/src/test.slow/java/docking/actions/SharedKeyBindingDockingActionTest.java

@@ -66,8 +66,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_SameDefaultKeyBindings() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 		tool.addAction(action2);
@@ -81,8 +81,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_OptionsChange() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 		tool.addAction(action2);
@@ -99,8 +99,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_DifferentDefaultKeyBindings() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_DIFFERENT_THAN_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_DIFFERENT_THAN_1);
 
 		tool.addAction(action1);
 		tool.addAction(action2);
@@ -115,8 +115,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_NoDefaultKeyBindings() {
 
-		TestAction action1 = new TestAction(OWNER_1, null);
-		TestAction action2 = new TestAction(OWNER_2, null);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, null);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, null);
 
 		tool.addAction(action1);
 		tool.addAction(action2);
@@ -130,8 +130,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 
 	@Test
 	public void testSharedKeyBinding_OneDefaultOneUndefinedDefaultKeyBinding() {
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, null);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, null);
 
 		tool.addAction(action1);
 		tool.addAction(action2);
@@ -146,8 +146,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_RemoveAction() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 		tool.addAction(action2);
@@ -166,7 +166,7 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_AddSameActionTwice() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 
@@ -188,8 +188,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_OnlyOneEntryInOptions() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 		tool.addAction(action2);
@@ -205,8 +205,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_AddActionAfterOptionHasChanged() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 		KeyStroke newKs = KeyStroke.getKeyStroke(KeyEvent.VK_Z, 0);
@@ -223,8 +223,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_AddActionAfterOptionHasChanged_RepeatAddRemove() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 		KeyStroke newKs = KeyStroke.getKeyStroke(KeyEvent.VK_Z, 0);
@@ -248,8 +248,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_SameDefaultKeyBindings_LocalAction() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		DummyComponentProvider provider = new DummyComponentProvider();
 		tool.addLocalAction(provider, action1);
@@ -264,8 +264,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_RemoveAction_LocalAction() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		DummyComponentProvider provider = new DummyComponentProvider();
 		tool.addLocalAction(provider, action1);
@@ -285,8 +285,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 	@Test
 	public void testSharedKeyBinding_RemoveComonentActions() {
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action2 = new TestAction(OWNER_2, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action2 = new SharedNameAction(OWNER_2, DEFAULT_KS_1);
 
 		DummyComponentProvider provider = new DummyComponentProvider();
 		tool.addLocalAction(provider, action1);
@@ -313,8 +313,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 		//       same name and owner.
 		//
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action1Copy = new TestAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action1Copy = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
 
 		tool.addAction(action1);
 		tool.addAction(action1Copy);
@@ -342,8 +342,8 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 		//       same name and owner.
 		//
 
-		TestAction action1 = new TestAction(OWNER_1, DEFAULT_KS_1);
-		TestAction action1Copy = new TestAction(OWNER_1, DEFAULT_KS_DIFFERENT_THAN_1);
+		SharedNameAction action1 = new SharedNameAction(OWNER_1, DEFAULT_KS_1);
+		SharedNameAction action1Copy = new SharedNameAction(OWNER_1, DEFAULT_KS_DIFFERENT_THAN_1);
 
 		tool.addAction(action1);
 		tool.addAction(action1Copy);
@@ -484,7 +484,7 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 		spyLogger.assertLogMessage("shared", "key", "binding", "actions", "different", "default");
 	}
 
-	private void assertKeyBinding(TestAction action, KeyStroke expectedKs) {
+	private void assertKeyBinding(SharedNameAction action, KeyStroke expectedKs) {
 		assertEquals(expectedKs, action.getKeyBinding());
 	}
 
@@ -492,9 +492,9 @@ public class SharedKeyBindingDockingActionTest extends AbstractDockingTest {
 // Inner Classes
 //==================================================================================================
 
-	private class TestAction extends DockingAction {
+	private class SharedNameAction extends DockingAction {
 
-		public TestAction(String owner, KeyStroke ks) {
+		public SharedNameAction(String owner, KeyStroke ks) {
 			super(SHARED_NAME, owner, KeyBindingType.SHARED);
 			setKeyBindingData(new KeyBindingData(ks));
 		}

Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/AbstractOptions.java

@@ -116,8 +116,8 @@ public abstract class AbstractOptions implements Options {
 				"Attempted to register an unsupported object: " + defaultValue.getClass());
 		}
 
-		registerOption(optionName, OptionType.getOptionType(defaultValue), defaultValue, help,
-			description);
+		OptionType type = OptionType.getOptionType(defaultValue);
+		registerOption(optionName, type, defaultValue, help, description);
 	}
 
 	@Override
@@ -150,16 +150,41 @@ public abstract class AbstractOptions implements Options {
 			valueMap.put(optionName, option);
 			return;
 		}
-		else if (!currentOption.isRegistered()) {
-			Option option =
+
+		Option newOption = null;
+		if (currentOption.isRegistered()) {
+			// Registered again
+			newOption =
+				copyRegisteredOption(currentOption, type, description, defaultValue, help, editor);
+		}
+		else {
+			// option was accessed, but not registered
+			newOption =
 				createRegisteredOption(optionName, type, description, help, defaultValue, editor);
-			option.setCurrentValue(currentOption.getCurrentValue());
-			valueMap.put(optionName, option);
-			return;
 		}
 
-		// TODO: We probably don't need to do anything special if we are re-registering an
-		// option, which is what the below code handles.
+		copyCurrentValue(currentOption, newOption);
+		valueMap.put(optionName, newOption);
+	}
+
+	private void copyCurrentValue(Option currentOption, Option newOption) {
+
+		Object currentValue = currentOption.getCurrentValue();
+		OptionType type = currentOption.getOptionType();
+		if (!isNullable(type) && currentValue == null) {
+			return; // not allowed to be null
+		}
+
+		// null is allowed; null can represent a valid 'cleared' state
+		newOption.setCurrentValue(currentValue);
+
+	}
+
+	private Option copyRegisteredOption(Option currentOption, OptionType type,
+			String description, Object defaultValue, HelpLocation help, PropertyEditor editor) {
+
+		// We probably don't need to do anything special if we  are re-registering an option, 
+		// which is what the below code handles
 		String oldDescription = currentOption.getDescription();
 		HelpLocation oldHelp = currentOption.getHelpLocation();
 		Object oldDefaultValue = currentOption.getDefaultValue();
@@ -170,13 +195,9 @@ public abstract class AbstractOptions implements Options {
 		Object newDefaultValue = oldDefaultValue == null ? defaultValue : oldDefaultValue;
 		PropertyEditor newEditor = oldEditor == null ? editor : oldEditor;
 
-		Option newOption = createRegisteredOption(optionName, type, newDescripiton, newHelpLocation,
+		String optionName = currentOption.getName();
+		return createRegisteredOption(optionName, type, newDescripiton, newHelpLocation,
 			newDefaultValue, newEditor);
-		Object currentValue = currentOption.getCurrentValue();
-		if (currentValue != null) {
-			newOption.setCurrentValue(currentValue);
-		}
-		valueMap.put(optionName, newOption);
 	}
 
 	@Override
@@ -249,6 +270,10 @@ public abstract class AbstractOptions implements Options {
 		}
 
 		OptionType type = option.getOptionType();
+		return isNullable(type);
+	}
+
+	private boolean isNullable(OptionType type) {
 		switch (type) {
 
 			// objects can be null
@@ -396,8 +421,8 @@ public abstract class AbstractOptions implements Options {
 	public Font getFont(String optionName, Font defaultValue) {
 		Option option = getOption(optionName, OptionType.FONT_TYPE, defaultValue);
 		try {
-		return (Font) option.getValue(defaultValue);
-	}
+			return (Font) option.getValue(defaultValue);
+		}
 		catch (ClassCastException e) {
 			return defaultValue;
 		}

Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/Option.java

@@ -1,6 +1,5 @@
 /* ###
  * IP: GHIDRA
- * REVIEWED: YES
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,10 +15,12 @@
  */
 package ghidra.framework.options;
 
+import java.beans.PropertyEditor;
+import java.util.Objects;
+
 import ghidra.util.HelpLocation;
 import ghidra.util.SystemUtilities;
-
-import java.beans.PropertyEditor;
+import utilities.util.reflection.ReflectionUtilities;
 
 public abstract class Option {
 	private final String name;
@@ -68,23 +69,19 @@ public abstract class Option {
 		return helpLocation;
 	}
 
-	public boolean hasValue() {
-		return defaultValue != null || getCurrentValue() != null;
-	}
-
 	public String getDescription() {
 		return description == null ? "Unregistered Option" : description;
 	}
 
 	public Object getValue(Object passedInDefaultValue) {
 		Object value = getCurrentValue();
-		if (value != null) {
-			return value;
+		if (value == null && defaultValue == null) {
+			// Assume that both values being null is an indication that there is no value
+			// to use.  Otherwise, a null current value implies that the user has cleared
+			// the default value.
+			return passedInDefaultValue;
 		}
-		if (defaultValue != null) {
-			return defaultValue;
-		}
-		return passedInDefaultValue;
+		return value;
 	}
 
 	public boolean isRegistered() {
@@ -97,11 +94,7 @@ public abstract class Option {
 
 	public boolean isDefault() {
 		Object value = getCurrentValue();
-		if (value == null) {
-			return true;
-		}
-
-		return value.equals(defaultValue);
+		return Objects.equals(value, defaultValue);
 	}
 
 	@Override
@@ -125,46 +118,12 @@ public abstract class Option {
 		Throwable throwable = new Throwable();
 		StackTraceElement[] stackTrace = throwable.getStackTrace();
 
-		String information = getInceptionInformationFromTheFirstClassThatIsNotUs(stackTrace);
-		inceptionInformation = information;
-	}
-
-	private String getInceptionInformationFromTheFirstClassThatIsNotUs(
-			StackTraceElement[] stackTrace) {
-
-		// To find our creation point we can use a simple algorithm: find the name of our class, 
-		// which is in the first stack trace element and then keep walking backwards until that
-		// name is not ours.
-		//         
-		String myClassName = getClass().getName();
-		int myClassNameStartIndex = -1;
-		for (int i = 1; i < stackTrace.length; i++) { // start at 1, because we are the first item
-			StackTraceElement stackTraceElement = stackTrace[i];
-			String elementClassName = stackTraceElement.getClassName();
-			if (myClassName.equals(elementClassName)) {
-				myClassNameStartIndex = i;
-				break;
-			}
-		}
-
-		// Finally, go backwards until we find a non-options class in the stack, in order
-		// to remove infrastructure code from the client that called the options API.
-		int creatorIndex = myClassNameStartIndex;
-		for (int i = myClassNameStartIndex; i < stackTrace.length; i++) { // start at 1, because we are the first item
-			StackTraceElement stackTraceElement = stackTrace[i];
-			String elementClassName = stackTraceElement.getClassName();
-
-			if (elementClassName.toLowerCase().indexOf("option") == -1) {
-				creatorIndex = i;
-				break;
-			}
-		}
-
-		return stackTrace[creatorIndex].toString();
+		StackTraceElement[] filteredTrace =
+			ReflectionUtilities.filterStackTrace(stackTrace, "option", "OptionsManager");
+		inceptionInformation = filteredTrace[0].toString();
 	}
 
 	public OptionType getOptionType() {
 		return optionType;
 	}
-
 }

Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/ToolOptions.java

@@ -41,6 +41,10 @@ import ghidra.util.exception.AssertException;
  * The Options Dialog shows the hierarchy in tree format.
  */
 public class ToolOptions extends AbstractOptions {
+	private static final String CLASS_ATTRIBUTE = "CLASS";
+	private static final String NAME_ATTRIBUTE = "NAME";
+	private static final String WRAPPED_OPTION_NAME = "WRAPPED_OPTION";
+	private static final String CLEARED_VALUE_ELEMENT_NAME = "CLEARED_VALUE";
 	public static final Set<Class<?>> PRIMITIVE_CLASSES = buildPrimitiveClassSet();
 	public static final Set<Class<?>> WRAPPABLE_CLASSES = buildWrappableClassSet();
 
@@ -81,34 +85,57 @@ public class ToolOptions extends AbstractOptions {
 	 * @param root XML that contains the set of options to restore
 	 */
 	public ToolOptions(Element root) {
-		this(root.getAttributeValue("NAME"));
+		this(root.getAttributeValue(NAME_ATTRIBUTE));
 
 		SaveState saveState = new SaveState(root);
 
+		readNonWrappedOptions(saveState);
+
+		try {
+			readWrappedOptions(root);
+		}
+		catch (ReflectiveOperationException exc) {
+			Msg.error(this, "Unexpected Exception: " + exc.getMessage(), exc);
+		}
+	}
+
+	private void readNonWrappedOptions(SaveState saveState) {
 		for (String optionName : saveState.getNames()) {
 			Object object = saveState.getObject(optionName);
 			Option option =
 				createUnregisteredOption(optionName, OptionType.getOptionType(object), null);
 			option.doSetCurrentValue(object);  // use doSet versus set so that it is not registered
 			valueMap.put(optionName, option);
-
 		}
+	}
 
-		Iterator<?> iter = root.getChildren("WRAPPED_OPTION").iterator();
-		while (iter.hasNext()) {
-			try {
-				Element elem = (Element) iter.next();
-				String optionName = elem.getAttributeValue("NAME");
-				Class<?> c = Class.forName(elem.getAttributeValue("CLASS"));
-				Constructor<?> constructor = c.getDeclaredConstructor();
-				WrappedOption wo = (WrappedOption) constructor.newInstance();
-				wo.readState(new SaveState(elem));
-				Option option = createUnregisteredOption(optionName, wo.getOptionType(), null);
-				option.doSetCurrentValue(wo.getObject());// use doSet versus set so that it is not registered
-				valueMap.put(optionName, option);
+	private void readWrappedOptions(Element root) throws ReflectiveOperationException {
+
+		Iterator<?> it = root.getChildren(WRAPPED_OPTION_NAME).iterator();
+		while (it.hasNext()) {
+
+			Element element = (Element) it.next();
+			List<?> children = element.getChildren();
+			if (children.isEmpty()) {
+				continue; // shouldn't happen
 			}
-			catch (Exception exc) {
-				Msg.error(this, "Unexpected Exception: " + exc.getMessage(), exc);
+
+			String optionName = element.getAttributeValue(NAME_ATTRIBUTE);
+			Class<?> c = Class.forName(element.getAttributeValue(CLASS_ATTRIBUTE));
+			Constructor<?> constructor = c.getDeclaredConstructor();
+			WrappedOption wo = (WrappedOption) constructor.newInstance();
+			Option option = createUnregisteredOption(optionName, wo.getOptionType(), null);
+			valueMap.put(optionName, option);
+
+			Element child = (Element) children.get(0);
+			String elementName = child.getName();
+			if (CLEARED_VALUE_ELEMENT_NAME.equals(elementName)) {
+				// a signal that the default option value has been cleared
+				option.doSetCurrentValue(null); // use doSet so that it is not registered
+			}
+			else {
+				wo.readState(new SaveState(element));
+				option.doSetCurrentValue(wo.getObject()); // use doSet so that it is not registered
 			}
 		}
 	}
@@ -122,8 +149,20 @@ public class ToolOptions extends AbstractOptions {
 	 * @return the xml root element
 	 */
 	public Element getXmlRoot(boolean includeDefaultBindings) {
+
 		SaveState saveState = new SaveState(XML_ELEMENT_NAME);
 
+		writeNonWrappedOptions(includeDefaultBindings, saveState);
+
+		Element root = saveState.saveToXml();
+		root.setAttribute(NAME_ATTRIBUTE, name);
+
+		writeWrappedOptions(includeDefaultBindings, root);
+
+		return root;
+	}
+
+	private void writeNonWrappedOptions(boolean includeDefaultBindings, SaveState saveState) {
 		for (String optionName : valueMap.keySet()) {
 			Option optionValue = valueMap.get(optionName);
 			if (includeDefaultBindings || !optionValue.isDefault()) {
@@ -133,27 +172,43 @@ public class ToolOptions extends AbstractOptions {
 				}
 			}
 		}
+	}
 
-		Element root = saveState.saveToXml();
-		root.setAttribute("NAME", name);
-
+	private void writeWrappedOptions(boolean includeDefaultBindings, Element root) {
 		for (String optionName : valueMap.keySet()) {
-			Option optionValue = valueMap.get(optionName);
-			if (includeDefaultBindings || !optionValue.isDefault()) {
-				Object value = optionValue.getValue(null);
-				if (value != null && !isSupportedBySaveState(value)) {
-					WrappedOption wrappedOption = wrapOption(value);
-					SaveState ss = new SaveState("WRAPPED_OPTION");
-					wrappedOption.writeState(ss);
-					Element elem = ss.saveToXml();
-					elem.setAttribute("NAME", optionName);
-					elem.setAttribute("CLASS", wrappedOption.getClass().getName());
-					root.addContent(elem);
+			Option option = valueMap.get(optionName);
+			if (includeDefaultBindings || !option.isDefault()) {
+
+				Object value = option.getCurrentValue();
+				if (isSupportedBySaveState(value)) {
+					// handled above
+					continue;
 				}
+
+				WrappedOption wrappedOption = wrapOption(option);
+				if (wrappedOption == null) {
+					// cannot write an option without a value to determine its type
+					continue;
+				}
+
+				SaveState ss = new SaveState(WRAPPED_OPTION_NAME);
+				Element elem = null;
+				if (value == null) {
+					// Handle the null case ourselves, not using the wrapped option (and when 
+					// reading from xml) so that the logic does not need to in each wrapped option
+					elem = ss.saveToXml();
+					elem.addContent(new Element(CLEARED_VALUE_ELEMENT_NAME));
+				}
+				else {
+					wrappedOption.writeState(ss);
+					elem = ss.saveToXml();
+				}
+
+				elem.setAttribute(NAME_ATTRIBUTE, optionName);
+				elem.setAttribute(CLASS_ATTRIBUTE, wrappedOption.getClass().getName());
+				root.addContent(elem);
 			}
 		}
-
-		return root;
 	}
 
 	private boolean isSupportedBySaveState(Object obj) {
@@ -170,7 +225,19 @@ public class ToolOptions extends AbstractOptions {
 
 	}
 
-	private WrappedOption wrapOption(Object value) {
+	private WrappedOption wrapOption(Option option) {
+
+		Object value = null;
+		value = option.getCurrentValue();
+		if (value == null) {
+			value = option.getDefaultValue();
+		}
+
+		if (value == null) {
+			// nothing to wrap
+			return null;
+		}
+
 		if (value instanceof CustomOption) {
 			return new WrappedCustomOption((CustomOption) value);
 		}
@@ -232,44 +299,6 @@ public class ToolOptions extends AbstractOptions {
 		}
 	}
 
-	////////////////////////////////////////////////////////////////
-
-	private class NotifyListenersRunnable implements Runnable {
-		private String optionName;
-		private Object oldValue;
-		private Object newValue;
-		private boolean vetoed;
-
-		NotifyListenersRunnable(String optionName, Object oldValue, Object newValue) {
-			this.optionName = optionName;
-			this.oldValue = oldValue;
-			this.newValue = newValue;
-		}
-
-		@Override
-		public void run() {
-			List<OptionsChangeListener> notifiedListeners = new ArrayList<>();
-			try {
-				for (OptionsChangeListener listener : listeners) {
-					listener.optionsChanged(ToolOptions.this, optionName, oldValue, newValue);
-					notifiedListeners.add(listener);
-				}
-			}
-			catch (OptionsVetoException e) {
-				vetoed = true;
-				for (OptionsChangeListener notifiedListener : notifiedListeners) {
-					notifiedListener.optionsChanged(ToolOptions.this, optionName, newValue,
-						oldValue);
-				}
-			}
-		}
-
-		public boolean wasVetoed() {
-			return vetoed;
-		}
-
-	}
-
 	/**
 	 * Adds all the options name/value pairs to this Options.
 	 * @param newOptions the new options into which the current options values will be placed
@@ -356,6 +385,8 @@ public class ToolOptions extends AbstractOptions {
 		ToolOption(String name, OptionType type, String description, HelpLocation helpLocation,
 				Object defaultValue, boolean isRegistered, PropertyEditor editor) {
 			super(name, type, description, helpLocation, defaultValue, isRegistered, editor);
+
+			this.currentValue = defaultValue;
 		}
 
 		@Override
@@ -385,7 +416,44 @@ public class ToolOptions extends AbstractOptions {
 	protected boolean notifyOptionChanged(String optionName, Object oldValue, Object newValue) {
 		NotifyListenersRunnable runnable =
 			new NotifyListenersRunnable(optionName, oldValue, newValue);
-		SystemUtilities.runSwingNow(runnable);
+		Swing.runNow(runnable);
 		return !runnable.wasVetoed();
 	}
+
+	private class NotifyListenersRunnable implements Runnable {
+		private String optionName;
+		private Object oldValue;
+		private Object newValue;
+		private boolean vetoed;
+
+		NotifyListenersRunnable(String optionName, Object oldValue, Object newValue) {
+			this.optionName = optionName;
+			this.oldValue = oldValue;
+			this.newValue = newValue;
+		}
+
+		@Override
+		public void run() {
+			List<OptionsChangeListener> notifiedListeners = new ArrayList<>();
+			try {
+				for (OptionsChangeListener listener : listeners) {
+					listener.optionsChanged(ToolOptions.this, optionName, oldValue, newValue);
+					notifiedListeners.add(listener);
+				}
+			}
+			catch (OptionsVetoException e) {
+				vetoed = true;
+				for (OptionsChangeListener notifiedListener : notifiedListeners) {
+					notifiedListener.optionsChanged(ToolOptions.this, optionName, newValue,
+						oldValue);
+				}
+			}
+		}
+
+		public boolean wasVetoed() {
+			return vetoed;
+		}
+
+	}
+
 }

Ghidra/Framework/Project/src/main/java/ghidra/framework/data/OptionsDB.java

@@ -26,8 +26,7 @@ import ghidra.util.SystemUtilities;
 import ghidra.util.exception.ClosedException;
 
 /**
- * 
- *
+ * Database implementation of {@link Option}
  */
 class OptionsDB extends AbstractOptions {
 
@@ -42,8 +41,6 @@ class OptionsDB extends AbstractOptions {
 	private Table propertyTable;
 	private DomainObjectAdapterDB domainObj;
 
-	/**
-	 */
 	OptionsDB(DomainObjectAdapterDB domainObj) {
 		super("");
 		this.domainObj = domainObj;
@@ -59,7 +56,7 @@ class OptionsDB extends AbstractOptions {
 	 * the corresponding oldPath properties will be removed.
 	 * @throws IllegalStateException if list has been manipulated since construction
 	 * @throws IllegalArgumentException if invalid property alterations are provided
-	 * @throws IOException
+	 * @throws IOException if there is an exception moving or deleting a property 
 	 */
 	synchronized void performAlterations(Map<String, String> propertyAlterations)
 			throws IOException {
@@ -108,7 +105,7 @@ class OptionsDB extends AbstractOptions {
 		}
 
 		// move records
-		ArrayList<Record> list = new ArrayList<Record>();
+		ArrayList<Record> list = new ArrayList<>();
 		rec = propertyTable.getRecord(new StringField(oldPath));
 		if (rec != null) {
 			propertyTable.deleteRecord(new StringField(oldPath));
@@ -182,7 +179,7 @@ class OptionsDB extends AbstractOptions {
 
 	@Override
 	public synchronized List<String> getOptionNames() {
-		Set<String> names = new HashSet<String>(valueMap.keySet());
+		Set<String> names = new HashSet<>(valueMap.keySet());
 		names.addAll(aliasMap.keySet());
 		try {
 			if (propertyTable != null) {
@@ -196,7 +193,7 @@ class OptionsDB extends AbstractOptions {
 		catch (IOException e) {
 			domainObj.dbError(e);
 		}
-		List<String> optionNames = new ArrayList<String>(names);
+		List<String> optionNames = new ArrayList<>(names);
 		Collections.sort(optionNames);
 		return optionNames;
 	}
@@ -253,16 +250,6 @@ class OptionsDB extends AbstractOptions {
 		}
 	}
 
-//	Property getProperty(String propertyName) {
-//		Record rec = getPropertyRecord(propertyName);
-//		if (rec == null) {
-//			return null;
-//		}
-//		int propertyOrdinal = rec.getByteValue(TYPE_COL);
-//		OptionType propertyType = OptionType.values()[propertyOrdinal];
-//		return createProperty(propertyName, propertyType, null);
-//	}
-
 	class DBOption extends Option {
 		private Object value = null;
 		private boolean isCached = false;
@@ -270,6 +257,8 @@ class OptionsDB extends AbstractOptions {
 		protected DBOption(String name, OptionType type, String description, HelpLocation help,
 				Object defaultValue, boolean isRegistered, PropertyEditor editor) {
 			super(name, type, description, help, defaultValue, isRegistered, editor);
+
+			getCurrentValue(); // initialize our defaults
 		}
 
 		@Override

Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/dialog/KeyBindingsPanel.java

@@ -123,19 +123,9 @@ public class KeyBindingsPanel extends JPanel {
 			return;
 		}
 
-		if (newKeyStroke != null) {
-			options.setKeyStroke(fullActionName, newKeyStroke);
-		}
-		else {
-			options.removeOption(fullActionName);
-		}
+		options.setKeyStroke(fullActionName, newKeyStroke);
 		originalValues.put(fullActionName, newKeyStroke);
 		keyStrokesByFullName.put(fullActionName, newKeyStroke);
-
-		List<DockingActionIf> actions = actionsByFullName.get(fullActionName);
-		for (DockingActionIf action : actions) {
-			action.setUnvalidatedKeyBindingData(new KeyBindingData(newKeyStroke));
-		}
 	}
 
 	public void cancel() {