On Fri, 2 Jan 2026 19:48:55 GMT, Alexey Ivanov <[email protected]> wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Alignment fix
>
> src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
> line 922:
>
>> 920: x + OFFSET,
>> 921: (icon.getIconHeight()
>> <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);
>> 922: } else if (icon.getIconWidth() <=
>> 16) {
>
> I don't understand why painting an icon and check marks / radio bullets in
> RTL case depends on the width of the icon but there's no such dependency in
> LTR case.
>
> Shouldn't the performed menu layout handle these cases?
As I see, MenuItemLayout handler's x position calculation for RTL depends on
icon rect width
https://github.com/openjdk/jdk/blob/6eaabed55ca4670d8c317f0a4323ccea4dd0b9ca/src/java.desktop/share/classes/sun/swing/MenuItemLayoutHelper.java#L737-L741
but LTR doesn't depend on rect width
https://github.com/openjdk/jdk/blob/6eaabed55ca4670d8c317f0a4323ccea4dd0b9ca/src/java.desktop/share/classes/sun/swing/MenuItemLayoutHelper.java#L726-L730
> src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
> line 923:
>
>> 921: (icon.getIconHeight()
>> <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);
>> 922: } else if (icon.getIconWidth() <=
>> 16) {
>> 923: if ((c instanceof JMenuItem mi)
>> && mi.getText().isEmpty()) {
>
> The condition `(c instanceof JMenuItem mi)` is always `true` according to the
> assert at line 881:
>
> https://github.com/openjdk/jdk/blob/2daf12edd24e641d4d7706d582994c2b3fe95e87/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java#L881
>
> where `menuItem` is a field in `VistaMenuItemCheckIcon`, the type of the
> field is `JMenuItem`.
>
> This means, you can use `menuItem` and ignore the passed in parameter `c`.
ok
> src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
> line 936:
>
>> 934: skin.paintSkin(g,
>> 935: (type ==
>> JRadioButtonMenuItem.class) ? (x + 3 * OFFSET) : (x + 4 * OFFSET),
>> 936: (icon.getIconHeight()
>> <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);
>
> I wonder why painting the skin for radio button menu requires such a fix up
> whereas it's not required for `JCheckBoxMenuItem`.
it is taken care via ternary operator, width/spread of check mark is usually
more than radio bullet so starting point of checkmark is considered a little
ahead compared to radio bullet
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2660479443
PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2660478835
PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2660480432
