From 2a4758caa0fc9575231c77202cf4e7476d659041 Mon Sep 17 00:00:00 2001 From: ghidragon <106987263+ghidragon@users.noreply.github.com> Date: Wed, 23 Nov 2022 11:01:43 -0500 Subject: [PATCH] GP-2862 fixing table selection colors --- .../options/editor/ColorPropertyEditor.java | 6 +-- .../widgets/AbstractGCellRenderer.java | 42 ++++++++++++++++ .../widgets/tree/support/GTreeRenderer.java | 49 ++++++++++++------- .../theme/ApplicationThemeManager.java | 20 +++----- .../java/generic/theme/GColorUIResource.java | 10 ++++ .../java/generic/theme/StubThemeManager.java | 10 ---- .../main/java/generic/theme/ThemeManager.java | 22 --------- .../generic/theme/laf/LookAndFeelManager.java | 2 +- .../theme/laf/NimbusLookAndFeelManager.java | 5 ++ .../theme/ApplicationThemeManagerTest.java | 12 +---- 10 files changed, 99 insertions(+), 79 deletions(-) diff --git a/Ghidra/Framework/Docking/src/main/java/docking/options/editor/ColorPropertyEditor.java b/Ghidra/Framework/Docking/src/main/java/docking/options/editor/ColorPropertyEditor.java index 7fb6a09f39..203fe0f324 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/options/editor/ColorPropertyEditor.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/options/editor/ColorPropertyEditor.java @@ -36,10 +36,8 @@ public class ColorPropertyEditor extends PropertyEditorSupport { @Override public Component getCustomEditor() { - if (colorChooser != null) { - return colorChooser; - } - + // always create a new one. Holding on to closed dialogs causes issues if the + // theme changes colorChooser = new GhidraColorChooser(); colorChooser.getSelectionModel().addChangeListener(e -> colorChanged()); return colorChooser; diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/AbstractGCellRenderer.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/AbstractGCellRenderer.java index e7e201fd91..1deffcca57 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/AbstractGCellRenderer.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/AbstractGCellRenderer.java @@ -20,9 +20,11 @@ import java.awt.*; import javax.swing.BorderFactory; import javax.swing.JComponent; import javax.swing.border.Border; +import javax.swing.plaf.UIResource; import docking.widgets.label.GDHtmlLabel; import generic.theme.GColor; +import generic.theme.GColorUIResource; import generic.theme.GThemeDefaults.Colors.Palette; /** @@ -268,4 +270,44 @@ public abstract class AbstractGCellRenderer extends GDHtmlLabel { public void firePropertyChange(String propertyName, boolean oldValue, boolean newValue) { // stub } + + /** + * Overrides this method to ensure that the new foreground color is not + * a {@link GColorUIResource}. Some Look and Feels will ignore color values that extend + * {@link UIResource}, choosing instead their own custom painting behavior. By not using a + * UIResource, we prevent the Look and Feel from overriding this renderer's color value. + * + * @param fg the new foreground color + */ + @Override + public void setForeground(Color fg) { + super.setForeground(fromUiResource(fg)); + } + + /** + * Overrides this method to ensure that the new background color is not + * a {@link GColorUIResource}. Some Look and Feels will ignore color values that extend + * {@link UIResource}, choosing instead their own custom painting behavior. By not using a + * UIResource, we prevent the Look and Feel from overriding this renderer's color value. + * + * @param bg the new background color + */ + @Override + public void setBackground(Color bg) { + super.setBackground(fromUiResource(bg)); + } + + /** + * Checks and converts any {@link GColorUIResource} to a {@link GColor} + * @param color the color to check if it is a {@link UIResource} + * @return either the given color or if it is a {@link GColorUIResource}, then a plain + * {@link GColor} instance referring to the same theme color property id. + */ + private Color fromUiResource(Color color) { + if (color instanceof GColorUIResource uiResource) { + return uiResource.toGColor(); + } + return color; + } + } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/support/GTreeRenderer.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/support/GTreeRenderer.java index c76a67e2f7..3413f16769 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/support/GTreeRenderer.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/support/GTreeRenderer.java @@ -19,13 +19,14 @@ import java.awt.*; import javax.swing.Icon; import javax.swing.JTree; -import javax.swing.plaf.ColorUIResource; +import javax.swing.plaf.UIResource; import javax.swing.tree.DefaultTreeCellRenderer; import docking.widgets.GComponent; import docking.widgets.tree.GTree; import docking.widgets.tree.GTreeNode; import generic.theme.GColor; +import generic.theme.GColorUIResource; public class GTreeRenderer extends DefaultTreeCellRenderer implements GComponent { @@ -87,31 +88,43 @@ public class GTreeRenderer extends DefaultTreeCellRenderer implements GComponent return this; } + /** + * Overrides this method to ensure that the new background selection color is not + * a {@link GColorUIResource}. Some Look and Feels will ignore color values that extend + * {@link UIResource}, choosing instead their own custom painting behavior. By not using a + * UIResource, we prevent the Look and Feel from overriding this renderer's color value. + * + * @param newColor the new background selection color + */ @Override public void setBackgroundSelectionColor(Color newColor) { - super.setBackgroundSelectionColor(fromUiResource(newColor, "Tree.selectionBackground")); - } - - @Override - public void setBackgroundNonSelectionColor(Color newColor) { - super.setBackgroundNonSelectionColor(fromUiResource(newColor, "Tree.textBackground")); + super.setBackgroundSelectionColor(fromUiResource(newColor)); } /** - * Converts the given color from a {@link ColorUIResource} to a {@link Color}. This is used - * to deal with the issue that some Look and Feels will not correctly paint with this - * renderer when using UI resource objects. This behavior can be changed by overriding this - * method. + * Overrides this method to ensure that the new background non-selection color is not + * a {@link GColorUIResource}. Some Look and Feels will ignore color values that extend + * {@link UIResource}, choosing instead their own custom painting behavior. By not using a + * UIResource, we prevent the Look and Feel from overriding this renderer's color value. * - * @param c the source color - * @param defaultKey the GColor key to use if the given color is a ColorUIResource - * @return the new color + * @param newColor the new background non-selection color */ - protected Color fromUiResource(Color c, String defaultKey) { - if (c instanceof ColorUIResource) { - return new GColor(defaultKey); + @Override + public void setBackgroundNonSelectionColor(Color newColor) { + super.setBackgroundNonSelectionColor(fromUiResource(newColor)); + } + + /** + * Checks and converts any {@link GColorUIResource} to a {@link GColor} + * @param color the color to check if it is a {@link UIResource} + * @return either the given color or if it is a {@link GColorUIResource}, then a plain + * {@link GColor} instance referring to the same theme color property id. + */ + protected Color fromUiResource(Color color) { + if (color instanceof GColorUIResource uiResource) { + return uiResource.toGColor(); } - return c; + return color; } protected void updateIconTextGap(Icon icon, int minWidth) { diff --git a/Ghidra/Framework/Generic/src/main/java/generic/theme/ApplicationThemeManager.java b/Ghidra/Framework/Generic/src/main/java/generic/theme/ApplicationThemeManager.java index 0c682d1462..92e621619e 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/theme/ApplicationThemeManager.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/theme/ApplicationThemeManager.java @@ -41,7 +41,6 @@ public class ApplicationThemeManager extends ThemeManager { protected ThemePreferences themePreferences = new ThemePreferences(); private Map gColorMap = new HashMap<>(); - private Map gIconMap = new HashMap<>(); // stores the original value for ids whose value has changed from the current theme private GThemeValueMap changedValuesMap = new GThemeValueMap(); @@ -237,7 +236,13 @@ public class ApplicationThemeManager extends ThemeManager { lookAndFeelManager.iconsChanged(changedIconIds, newIcon); } - @Override + /** + * Gets a UIResource version of the GColor for the given id. Using this method ensures that + * the same instance is used for a given id. This fixes an issue with some + * {@link LookAndFeel}s that internally use '==' comparisons. + * @param id the id to get a GColorUIResource for + * @return a GColorUIResource for the given id + */ public GColorUIResource getGColorUiResource(String id) { GColorUIResource gColor = gColorMap.get(id); if (gColor == null) { @@ -247,17 +252,6 @@ public class ApplicationThemeManager extends ThemeManager { return gColor; } - @Override - public GIconUIResource getGIconUiResource(String id) { - - GIconUIResource gIcon = gIconMap.get(id); - if (gIcon == null) { - gIcon = new GIconUIResource(id); - gIconMap.put(id, gIcon); - } - return gIcon; - } - /** * Sets specially defined system UI values. These values are created by the application as a * convenience for mapping generic concepts to values that differ by Look and Feel. This allows diff --git a/Ghidra/Framework/Generic/src/main/java/generic/theme/GColorUIResource.java b/Ghidra/Framework/Generic/src/main/java/generic/theme/GColorUIResource.java index 7c93e21c38..afc07e55aa 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/theme/GColorUIResource.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/theme/GColorUIResource.java @@ -15,6 +15,8 @@ */ package generic.theme; +import java.awt.Color; + import javax.swing.UIDefaults; import javax.swing.plaf.UIResource; @@ -30,4 +32,12 @@ public class GColorUIResource extends GColor implements UIResource { super(id); } + /** + * Returns a non-UIResource GColor for this GColorUiResource's id + * @return a non-UIResource GColor for this GColorUiResource's id + */ + public Color toGColor() { + return new GColor(getId()); + } + } diff --git a/Ghidra/Framework/Generic/src/main/java/generic/theme/StubThemeManager.java b/Ghidra/Framework/Generic/src/main/java/generic/theme/StubThemeManager.java index 36799bc022..f9c6febb59 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/theme/StubThemeManager.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/theme/StubThemeManager.java @@ -160,16 +160,6 @@ public class StubThemeManager extends ThemeManager { currentValues.addIcon(newValue); } - @Override - public GColorUIResource getGColorUiResource(String id) { - throw new UnsupportedOperationException(); - } - - @Override - public GIconUIResource getGIconUiResource(String id) { - throw new UnsupportedOperationException(); - } - @Override public GThemeValueMap getJavaDefaults() { throw new UnsupportedOperationException(); diff --git a/Ghidra/Framework/Generic/src/main/java/generic/theme/ThemeManager.java b/Ghidra/Framework/Generic/src/main/java/generic/theme/ThemeManager.java index be3e98dcfc..dd33802f5e 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/theme/ThemeManager.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/theme/ThemeManager.java @@ -397,28 +397,6 @@ public abstract class ThemeManager { throw new UnsupportedOperationException(); } - /** - * gets a UIResource version of the GColor for the given id. Using this method ensures that - * the same instance is used for a given id. This combats some poor code in some of the - * {@link LookAndFeel}s where the use == in some places to test for equals. - * @param id the id to get a GColorUIResource for - * @return a GColorUIResource for the given id - */ - public GColorUIResource getGColorUiResource(String id) { - throw new UnsupportedOperationException(); - } - - /** - * gets a UIResource version of the GIcon for the given id. Using this method ensures that - * the same instance is used for a given id. This combats some poor code in some of the - * {@link LookAndFeel}s where the use == in some places to test for equals. - * @param id the id to get a {@link GIconUIResource} for - * @return a GIconUIResource for the given id - */ - public GIconUIResource getGIconUiResource(String id) { - throw new UnsupportedOperationException(); - } - /** * Returns the {@link GThemeValueMap} containing all the default theme values defined by the * current {@link LookAndFeel}. diff --git a/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/LookAndFeelManager.java b/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/LookAndFeelManager.java index f9e1e67289..91fad08373 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/LookAndFeelManager.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/LookAndFeelManager.java @@ -275,7 +275,7 @@ public abstract class LookAndFeelManager { return new ThemeGrouper(); } - private void installPropertiesBackIntoUiDefaults(GThemeValueMap javaDefaults) { + protected void installPropertiesBackIntoUiDefaults(GThemeValueMap javaDefaults) { UIDefaults defaults = UIManager.getDefaults(); GTheme theme = themeManager.getActiveTheme(); diff --git a/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/NimbusLookAndFeelManager.java b/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/NimbusLookAndFeelManager.java index e2d9414c31..a36006ce57 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/NimbusLookAndFeelManager.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/theme/laf/NimbusLookAndFeelManager.java @@ -104,4 +104,9 @@ public class NimbusLookAndFeelManager extends LookAndFeelManager { // (see NimbusDefaults for key values that can be changed here) } + @Override + protected void installPropertiesBackIntoUiDefaults(GThemeValueMap javaDefaults) { + // do nothing, this was handled when we overrode the getDefaults() method in the + // GNimubusLookAndFeel + } } diff --git a/Ghidra/Framework/Generic/src/test/java/generic/theme/ApplicationThemeManagerTest.java b/Ghidra/Framework/Generic/src/test/java/generic/theme/ApplicationThemeManagerTest.java index 1d5bf0b725..9413e87f89 100644 --- a/Ghidra/Framework/Generic/src/test/java/generic/theme/ApplicationThemeManagerTest.java +++ b/Ghidra/Framework/Generic/src/test/java/generic/theme/ApplicationThemeManagerTest.java @@ -49,7 +49,7 @@ public class ApplicationThemeManagerTest { private GTheme NIMBUS_THEME = new NimbusTheme(); private GTheme WINDOWS_THEME = new WindowsTheme(); private GTheme MAC_THEME = new MacTheme(); - private ThemeManager themeManager; + private ApplicationThemeManager themeManager; private boolean errorsExpected; @@ -291,16 +291,6 @@ public class ApplicationThemeManagerTest { assertTrue(color == color2); } - @Test - public void testGetGIconUiResource() { - Icon icon = themeManager.getGIconUiResource("icon.test.foo"); - assertTrue(icon instanceof UIResource); - - // make sure there is only one instance for an id; - Icon gIcon2 = themeManager.getGIconUiResource("icon.test.foo"); - assertTrue(icon == gIcon2); - } - @Test public void testGetApplicationLightDefaults() { assertEquals(defaultValues, themeManager.getApplicationLightDefaults());