From e3b7d11eeabda3a81ca104ce718dd48b9ffb27de Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Fri, 18 Nov 2022 12:29:25 -0500 Subject: [PATCH] GP-2843 - Update Options Dialog to allow veto exceptions to trigger a dialog for the user and to keep the options dialog open GP-2843 - Updated options to show the user when an option is vetoed. --- .../navigation/NavigationHistoryPlugin.java | 6 +-- .../bean/opteditor/OptionsDialogTest.java | 43 ++++++++++++++++--- .../docking/options/editor/OptionsDialog.java | 8 ++-- .../docking/options/editor/OptionsPanel.java | 9 ++-- .../ghidra/framework/options/EditorState.java | 30 ++++++------- .../ghidra/framework/options/ToolOptions.java | 26 ++++++----- 6 files changed, 80 insertions(+), 42 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/NavigationHistoryPlugin.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/NavigationHistoryPlugin.java index 8c220cc162..e35c40c081 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/NavigationHistoryPlugin.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/navigation/NavigationHistoryPlugin.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. @@ -76,7 +76,7 @@ public class NavigationHistoryPlugin extends Plugin private static final String MEMENTO_CLASS = "MEMENTO_CLASS"; private Map historyListMap = new HashMap<>(); - private static final int ABSOLUTE_MAX_HISTORY_SIZE = 100; + private static final int ABSOLUTE_MAX_HISTORY_SIZE = 400; private static final int ABSOLUTE_MIN_HISTORY_SIZE = 10; final static int MAX_HISTORY_SIZE = 30; private int maxHistorySize = MAX_HISTORY_SIZE; diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/util/bean/opteditor/OptionsDialogTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/util/bean/opteditor/OptionsDialogTest.java index 48eff24326..e9cb5362fd 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/util/bean/opteditor/OptionsDialogTest.java +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/util/bean/opteditor/OptionsDialogTest.java @@ -33,6 +33,7 @@ import javax.swing.tree.TreePath; import org.apache.commons.lang3.StringUtils; import org.junit.*; +import docking.DialogComponentProvider; import docking.action.DockingActionIf; import docking.actions.KeyBindingUtils; import docking.options.editor.*; @@ -705,9 +706,41 @@ public class OptionsDialogTest extends AbstractGhidraHeadedIntegrationTest { } + @Test + public void testOptionsVeto() throws Exception { + + Object root = treeModel.getRoot(); + Object toolNode = getGTreeNode(root, TOOL_NODE_NAME); + selectNode(toolNode); + assertTrue(!defaultPanel.isShowing()); + + ScrollableOptionsEditor simpleOptionsPanel = + (ScrollableOptionsEditor) getEditorPanel(toolNode); + assertNotNull(simpleOptionsPanel); + JComponent comp = simpleOptionsPanel.getComponent(); + assertTrue(comp.isShowing()); + + JTextField field = (JTextField) findPairedComponent(comp, "Max Navigation History Size"); + assertNotNull(field); + String text = getText(field); + assertEquals("30", text); + + // + // This field has a hard-coded max value of 400. Set the value higher to trigger a veto + // exception. + // + setText(field, "1000"); + pressOptionsOk(); + DialogComponentProvider warningDialog = waitForDialogComponent("Invalid Option Value"); + pressButtonByText(warningDialog, "OK"); + + text = getText(field); + assertEquals("30", text); + } + //================================================================================================= // Inner Classes -//================================================================================================= +//================================================================================================= private KeyStroke getKeyBinding(String actionName) throws Exception { OptionsEditor editor = seleNodeWithCustomEditor("Key Bindings"); @@ -876,7 +909,7 @@ public class OptionsDialogTest extends AbstractGhidraHeadedIntegrationTest { } private void pressOptionsOk() { - pressButtonByName(dialog.getComponent(), "OK", true); + pressButtonByName(dialog.getComponent(), "OK", false); waitForSwing(); } @@ -1105,7 +1138,8 @@ public class OptionsDialogTest extends AbstractGhidraHeadedIntegrationTest { name = "Favorite Color"; - options.registerOption(name, Palette.RED, null, "description"); + options.registerThemeColorBinding(name, "color.bg", null, "description"); + //options.registerOption(name, Palette.RED, null, "description"); name = "Favorite String"; options.registerOption(name, "Foo", null, "description"); @@ -1114,8 +1148,7 @@ public class OptionsDialogTest extends AbstractGhidraHeadedIntegrationTest { name = "Mouse Buttons" + Options.DELIMITER + "Mouse Button To Activate"; options.registerOption(name, GhidraOptions.CURSOR_MOUSE_BUTTON_NAMES.MIDDLE, null, "description"); - options.setEnum(name, - GhidraOptions.CURSOR_MOUSE_BUTTON_NAMES.MIDDLE); + options.setEnum(name, GhidraOptions.CURSOR_MOUSE_BUTTON_NAMES.MIDDLE); } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsDialog.java b/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsDialog.java index 824ba792ec..973c012b7a 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsDialog.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsDialog.java @@ -35,7 +35,7 @@ public class OptionsDialog extends DialogComponentProvider { /** * Construct a new OptionsDialog. - * + * * @param title dialog title * @param rootNodeName name to display for the root node in the tree * @param options editable options @@ -95,7 +95,9 @@ public class OptionsDialog extends DialogComponentProvider { return; } if (result == OptionDialog.YES_OPTION) { - applyChanges(); + if (!applyChanges()) { + return; + } } } close(); @@ -142,7 +144,7 @@ public class OptionsDialog extends DialogComponentProvider { //========================================================= // Inner Classes -//========================================================= +//========================================================= class OptionsPropertyChangeListener implements PropertyChangeListener { @Override diff --git a/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsPanel.java b/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsPanel.java index a7a14d5206..c60ea829ff 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsPanel.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/options/editor/OptionsPanel.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. @@ -212,6 +212,7 @@ public class OptionsPanel extends JPanel { catch (OptionsVetoException ove) { Msg.showWarn(this, this, "Invalid Option Value", "Attempted to set an option to an invalid value:\n" + ove.getMessage()); + status = false; } catch (Exception e) { status = false; @@ -372,7 +373,7 @@ public class OptionsPanel extends JPanel { //================================================================================================== // Inner Classes -//================================================================================================== +//================================================================================================== public class OptionsDataTransformer extends DefaultGTreeDataTransformer { @Override @@ -390,7 +391,7 @@ public class OptionsPanel extends JPanel { } private void addDetails(Options options, String optionName, List results) { - // some options use property editor classes to handle options editing and not + // some options use property editor classes to handle options editing and not // EditableOptions objects directly PropertyEditor propertyEditor = options.getRegisteredPropertyEditor(optionName); if (propertyEditor instanceof CustomOptionsEditor) { diff --git a/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/EditorState.java b/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/EditorState.java index 1c79064c95..6977680d8f 100644 --- a/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/EditorState.java +++ b/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/EditorState.java @@ -116,19 +116,17 @@ public class EditorState implements PropertyChangeListener { if (Objects.equals(currentValue, originalValue)) { return; } - boolean success = false; - try { - options.putObject(name, currentValue); - Object newValue = options.getObject(name, null); + + options.putObject(name, currentValue); + Object newValue = options.getObject(name, null); + boolean success = Objects.equals(currentValue, newValue); + if (success) { originalValue = newValue; currentValue = newValue; - success = true; } - finally { - if (!success) { - editor.setValue(originalValue); - currentValue = originalValue; - } + else { + editor.setValue(originalValue); + currentValue = originalValue; } } @@ -145,8 +143,8 @@ public class EditorState implements PropertyChangeListener { public Component getEditorComponent() { if (editor == null) { // can occur if support has been dropped for custom state/option - editor = new ErrorPropertyEditor( - "Ghidra does not know how to render state: " + name, null); + editor = + new ErrorPropertyEditor("Ghidra does not know how to render state: " + name, null); return editor.getCustomEditor(); } if (editor.supportsCustomEditor()) { @@ -165,16 +163,14 @@ public class EditorState implements PropertyChangeListener { Class clazz = editor.getClass(); String clazzName = clazz.getSimpleName(); if (clazzName.startsWith("String")) { - // Most likely some kind of string editor with a null value. Just use a string + // Most likely some kind of string editor with a null value. Just use a string // property and let the value be empty. return new PropertyText(editor); } editor.removePropertyChangeListener(this); - editor = new ErrorPropertyEditor( - Application.getName() + " does not know how to use PropertyEditor: " + - editor.getClass().getName(), - null); + editor = new ErrorPropertyEditor(Application.getName() + + " does not know how to use PropertyEditor: " + editor.getClass().getName(), null); return editor.getCustomEditor(); } diff --git a/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/ToolOptions.java b/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/ToolOptions.java index 9566a60d1d..c21f855e5b 100644 --- a/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/ToolOptions.java +++ b/Ghidra/Framework/Generic/src/main/java/ghidra/framework/options/ToolOptions.java @@ -32,16 +32,16 @@ import ghidra.util.exception.AssertException; /** * Class to manage a set of option name/value pairs for a category. - * + * *

The values may be primitives or {@link WrappedOption}s that are containers for primitive * components. - * + * *

The name/value pair has an owner so that the option name can be removed from the Options * object when it is no longer being used. - * + * *

Note: Property Names can have {@link Options#DELIMITER} characters to create a hierarchy. * So too can sub-options accessed via {@link #getOptions(String)}. - * + * *

The Options Dialog shows the delimited hierarchy in tree format. */ public class ToolOptions extends AbstractOptions { @@ -148,7 +148,7 @@ public class ToolOptions extends AbstractOptions { * Return an XML element for the option names and values. * Note: only those options which have been explicitly set * will be included. - * + * * @param includeDefaultBindings true to include default key binding values in the xml * @return the xml root element */ @@ -421,14 +421,20 @@ public class ToolOptions extends AbstractOptions { NotifyListenersRunnable runnable = new NotifyListenersRunnable(optionName, oldValue, newValue); Swing.runNow(runnable); - return !runnable.wasVetoed(); + + OptionsVetoException veto = runnable.getVetoException(); + if (veto != null) { + throw veto; + } + + return true; } private class NotifyListenersRunnable implements Runnable { private String optionName; private Object oldValue; private Object newValue; - private boolean vetoed; + private OptionsVetoException veto; NotifyListenersRunnable(String optionName, Object oldValue, Object newValue) { this.optionName = optionName; @@ -446,7 +452,7 @@ public class ToolOptions extends AbstractOptions { } } catch (OptionsVetoException e) { - vetoed = true; + veto = e; for (OptionsChangeListener notifiedListener : notifiedListeners) { notifiedListener.optionsChanged(ToolOptions.this, optionName, newValue, oldValue); @@ -454,8 +460,8 @@ public class ToolOptions extends AbstractOptions { } } - public boolean wasVetoed() { - return vetoed; + OptionsVetoException getVetoException() { + return veto; } }