On Tue, 28 Jan 2025 03:40:26 GMT, Prasanta Sadhukhan <psadhuk...@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.

src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
 line 860:

> 858:                         State state;
> 859:                         if (isEnabled(c, null)) {
> 860:                             backgroundState = State.NORMAL;

Do you happen to know why `State.BITMAP` was originally here? I tried locating 
where it originated from, but can't pinpoint exactly why it's either 
`State.BITMAP` or `State.NORMAL` based on the icon being `null`. Curious only 
because we're completely eliminating it here and there might be a dependency. 
However, I haven't encountered any issues when testing yet.

src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
 line 882:

> 880:                             } else {
> 881:                                 skin = xp.getSkin(c, part);
> 882:                                 skin.paintSkin(g, x - 3*OFFSET, y + 
> OFFSET, state);

Suggestion:

                                skin.paintSkin(g, x - 3 * OFFSET, y + OFFSET, 
state);

test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItem.java line 
51:

> 49:         Verify that for JRadioButtonMenuItem with imageicon,
> 50:         radiobutton is been shown alongside the imageicon.
> 51:         If radiobutton is shown, test passes else fails.""";

Suggestion:

        One JRadioButtonMenuItem is with imageicon and
        another one without imageicon.
        Verify that for JRadioButtonMenuItem with imageicon,
        radiobutton is been shown alongside the imageicon.
        If radiobutton is shown, test passes. Else fails.""";


Just for consistency, I see theres one capital `i`. I guess you can also just 
title-case all of the UI components. Ex: ImageIcon and RadioButton.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1931734330
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1931715704
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1931731265

Reply via email to