On Fri, 2 Jan 2026 09:58:56 GMT, Prasanta Sadhukhan <[email protected]> wrote:
>> Check/radiobutton icon are not aligned properly in RTL. `WindowsMenuItemUI >> `uses `MenuItemLayoutHelper.layoutMenuItem` to do the layout which calls >> `doRTLColumnLayout `which calculates x positions in `calcXPositionsRTL `and >> then again aligns in `alignRects`. However, since in Windows historically >> radiobutton/check icon was not drawn or drawn below the menuitem image icon >> (since image icon and check icon was drawn in the same layout space and not >> separately) the aligned x position of check icons returned from >> `MenuItemLayoutHelper` was not correct but since `MenuItemLayoutHelper` >> alignment is used in other L&Fs also so we need to realign it in windows >> specific class i.e in WindowsIconFactory in paintIcon >> >> Before fix >> >> <img width="425" height="646" alt="image" >> src="https://github.com/user-attachments/assets/6aac649d-b099-4e11-ba9a-83c623034287"; >> /> >> >> After fix >> >> <img width="430" height="641" alt="image" >> src="https://github.com/user-attachments/assets/e0ea7e3e-d6cb-44a6-aa4f-78435f85d6fb"; >> /> > > 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? 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`. 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`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2658309916 PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2658290233 PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2658311962
