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