On Thu, 30 Jan 2025 09:59:33 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.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> optimise
I am also for generating an `ImageIcon` on the fly _to avoid adding a binary
file_.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
line 887:
> 885: }
> 886: if (icon != null) {
> 887: if (!((AbstractButton) c).isSelected()) {
This new block of code 887–918 copies that from above 855–884.
You should create a helper method instead copying the same code in the two
branches of code, or the code should be re-arranged somehow.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
line 891:
> 889: Part part;
> 890: if (type == JRadioButtonMenuItem.class) {
> 891: part = Part.BP_RADIOBUTTON;
The part `Part.BP_RADIOBUTTON` doesn't look right to me.
The selected case above always uses the part `Part.MP_POPUPCHECK` — and it
looks correct because this code paints a menu item (and radio or check menu
items are usually in popup menus rather than on the menu bar).
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
line 894:
> 892: } else {
> 893: part = Part.MP_POPUPCHECK;
> 894: }
Suggestion:
Part part = (type == JRadioButtonMenuItem.class)
? Part.BP_RADIOBUTTON
: Part.MP_POPUPCHECK;
Moreover, you use this syntax below.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
line 907:
> 905: (type == JRadioButtonMenuItem.class)
> 906: ? State.BULLETDISABLED
> 907: : State.CHECKMARKDISABLED;
Suggestion:
state = (type == JRadioButtonMenuItem.class)
? State.BULLETDISABLED
: State.CHECKMARKDISABLED;
For consistency with the code in the block above `else`.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/23324#pullrequestreview-2583633590
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1935463611
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1935467489
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1935452320
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r1935454098
