RFR: 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel [v30]
Alexey Ivanov
aivanov at openjdk.org
Thu Jun 26 15:28:43 UTC 2025
On Thu, 26 Jun 2025 12:17:52 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:
>> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is shown without radiobutton in WIndowsLookAndFeel as there was no provision of drawing the radiobutton alongside icon.
>> If icon is not there, the radiobutton is drawn. Added provision of drawing the radiobutton windows Skin even when imageIcon is present.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
> MenuItem with icon fix
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 77:
> 75: private static Color disabledForeground;
> 76: private static Color acceleratorSelectionForeground;
> 77: private static Color acceleratorForeground;
Why are these static? I'm pretty sure the colors can be menu item specific, although more commonly they would be L&F specific.
src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 167:
> 165: }
> 166:
> 167: public static void paintCheckIcon(Graphics g, MenuItemLayoutHelper lh,
I suggest creating a new `sun.swing.MenuItemRenderHelper` class similar to `sun.swing.MenuItemLayoutHelper` to keep both layout and renderer closer together.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java line 723:
> 721: acceleratorSelectionForeground);
> 722: SwingUtilities3.setAcceleratorForeground(acceleratorForeground);
> 723: SwingUtilities3.paintAccText(g, lh, lr);
This is a really weird way… Pass the colors explicitly as parameters to the `SwingUtilities3.paintAccText` method.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsCheckBoxMenuItemUI.java line 81:
> 79: * Paint MenuItem.
> 80: */
> 81: protected void paintMenuItem(Graphics g, JComponent c,
This javadoc doesn't add anything on top of what's in the overridden method, remove the javadoc.
Add `@Override` annotation.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java line 891:
> 889: // beside imageicon so this check is necessary to differentiate
> 890: boolean isWindows11OrLater = Integer.parseInt(System.getProperty("os.name")
> 891: .replaceAll("[^0-9]", "")) >= 11;
This value can be cached in a static field rather than evaluating it each time `WindowsIconFactory.VistaMenuItemCheckIconFactory.VistaMenuItemCheckIcon.paintIcon` is called.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java line 927:
> 925: skin.paintSkin(g, x - 2 * OFFSET, y,
> 926: getIconWidth(), getIconHeight(), backgroundState);
> 927: skinWidth = getIconWidth();
To achieve a better layout, the icon size, specifically its width, should increase whenever we detect a situation where both check mark / bullet and associated icon need to be painted together so that `VistaMenuItemCheckIconFactory` returns an instance of `VistaMenuItemCheckIcon` or `Windows11MenuItemCheckIcon` which reserves the space for both check mark / bullet and icon as well as a gap between them. This way, the exiting menu layout code would position the elements correctly.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 73:
> 71: private static Color disabledForeground;
> 72: private static Color acceleratorSelectionForeground;
> 73: private static Color acceleratorForeground;
Why are these needed?
`BasicMenuItemUI` has fields for all three colors: `disabledForeground`, `acceleratorSelectionForeground`, `acceleratorForeground`. Why do you need to create three *new static* fields instead?
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 157:
> 155: UIManager.getColor(prefix + ".disabledForeground");
> 156: }
> 157: }
I'm not convinced `installDefaults` needs overriding, `BasicMenuItemUI` already provides all these colors via its `BasicMenuItemUI.installDefaults` method and protected fields.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 184:
> 182: MenuItemLayoutHelper.LayoutResult lr, Color holdc) {
> 183: SwingUtilities3.paintIcon(g, lh, lr, holdc);
> 184: }
Why are these static methods needed? You can statically import these methods from `SwingUtilities3` (or wherever they end up) and use them as if they're declared locally.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 193:
> 191: SwingUtilities3.setAcceleratorForeground(acceleratorForeground);
> 192: SwingUtilities3.paintAccText(g, lh, lr);
> 193: }
Pass the colors explicitly as parameters to the SwingUtilities3.paintAccText method.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 201:
> 199: }
> 200:
> 201: protected void paintMenuItem(Graphics g, JComponent c,
Suggestion:
@Override
protected void paintMenuItem(Graphics g, JComponent c,
Add `@Override` annotation.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 234:
> 232: }
> 233: return iconPresent;
> 234: }
This should be really simple:
private static boolean checkIfImageIconPresent(JMenuItem mi) {
return (mi instanceof JCheckBoxMenuItem
|| mi instanceof JRadioButtonMenuItem)
&& mi.getIcon() != null;
}
That is
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 262:
> 260: acceleratorFont, MenuItemLayoutHelper.useCheckAndArrow(menuItem),
> 261: prefix);
> 262: MenuItemLayoutHelper.LayoutResult lr = lh.layoutMenuItem();
Can `acceleratorDelimiter`, `acceleratorFont` be fetched from any `MenuItemUI` classes?
Do I understand correctly that the original menu layout that was already performed for a class gets thrown away and is being re-evaluated here?
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 269:
> 267:
> 268: if ((Integer.parseInt(System.getProperty("os.name")
> 269: .replaceAll("[^0-9]", "")) >= 11)
You already have this code to detect whether it's Windows 11 or 10, put that code somewhere for re-use instead of duplicating it.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 306:
> 304: g.setColor(holdc);
> 305: g.setFont(holdf);
> 306: return;
Suggestion:
`return` as the last statement in a method is redundant.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuUI.java line 136:
> 134: * Paint MenuItem.
> 135: */
> 136: protected void paintMenuItem(Graphics g, JComponent c,
Suggestion:
@Override
protected void paintMenuItem(Graphics g, JComponent c,
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsRadioButtonMenuItemUI.java line 81:
> 79: * Paint MenuItem.
> 80: */
> 81: protected void paintMenuItem(Graphics g, JComponent c,
Suggestion:
@Override
protected void paintMenuItem(Graphics g, JComponent c,
-------------
PR Review: https://git.openjdk.org/jdk/pull/23324#pullrequestreview-2961758453
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2168941423
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169334895
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169199694
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169272729
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169282066
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169331667
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169217699
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169267877
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169263009
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169219765
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169285512
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169258704
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169310299
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169294403
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169296194
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169298130
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169302893
More information about the client-libs-dev
mailing list