On Fri, 5 Jun 2026 03:21:46 GMT, Prasanta Sadhukhan <[email protected]> wrote:
>> [JDK-8348760](https://bugs.openjdk.org/browse/JDK-8348760) fixed an issue in >> Windows L&F JMenuItem layout whereby radio bullet/checkmark was rendered in >> different columnspace than menuitem imageicon so radiobullet/checkmark is >> rendered in first column and imageicon is rendered in 2nd column but this >> rendering of imageicon in 2nd columnspace was done invariably for all >> JMenuItem irrespective of if it is JRadioButtonMenuItem or JCheckBoxMenuItem >> or JMenuItem, which is wrong. >> >> Normal JMenuItem (which are not JRadioButtonMenuItem or JCheckBoxMenuItem) >> imageicon rendering should be done in first columnspace as was done before >> JDK-8348760 fix because there is no radiobullet/checkmark to render for >> those menuitems so no need to reserve columnspace for those bullet/checkmark >> icon >> >> Before fix >> >> <img width="205" height="127" alt="image" >> src="https://github.com/user-attachments/assets/13a1e352-5e8d-4251-b7a7-032935eab74e"; >> /> >> >> >> After fix >> >> <img width="195" height="131" alt="image" >> src="https://github.com/user-attachments/assets/84ec3ee6-2823-4bf7-840d-b53f2e9d44c3"; >> /> >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Test applicable for all L&F Do we support placing `JCheckBoxMenuItem` and `JRadioButtonMenuItem` directly to menu bar? Windows doesn't allow doing it, but Swing allows, so we should ensure the layout works correctly when such menu items are present in the menu bar. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 182: > 180: if (afterCheckIconGapObject instanceof Integer) { > 181: afterCheckIconGap = (Integer) > afterCheckIconGapObject; > 182: } Suggestion: int afterCheckIconGap = UIManager.getInt(getPropertyPrefix() + ".afterCheckIconGap"); [`UIManager.getInt`](https://docs.oracle.com/en/java/javase/26/docs/api/java.desktop/javax/swing/UIManager.html#getInt(java.lang.Object)) performs exactly this logic. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 194: > 192: boolean checkBulletPresent; > 193: boolean iconPresent; > 194: JMenu menu; `menu` is never used. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 249: > 247: } > 248: } > 249: return null; Why top-level menu? This property should apply to a popup menu only. Each popup should display its own children based on whether there is at least one `JCheckBoxMenuItem` or `JRadioButtonMenuItem` that also have an icon. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 252: > 250: } > 251: static final String CHECK_BULLET_AND_ICON_PRESENT = > 252: "WindowsMenuItemUI.checkBulletAndIconPresent"; Suggestion: } static final String CHECK_BULLET_AND_ICON_PRESENT = "WindowsMenuItemUI.checkBulletAndIconPresent"; A blank line between methods and fields makes it easier to quickly parse the source code. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 290: > 288: if (c instanceof JMenu menu && menu.isTopLevelMenu()) { > 289: MenuScanResult scan = new MenuScanResult(); > 290: scan.scanMenuForCheckBulletAndIcon(menu); So, you can pass `menu` to the `MenuScanResult` constructor which, in its turn, should call `scanMenuForCheckBulletAndIcon(menu)` to initialise the two fields of the `MenuScanResult` object. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 296: > 294: Boolean.valueOf(scan.checkBulletAndIconPresent())); > 295: } > 296: if (mi instanceof JRadioButtonMenuItem || mi instanceof > JCheckBoxMenuItem || mi instanceof JMenuItem) { This condition is always `true`: the local variable `mi` is of type `JMenuItem`. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java line 300: > 298: lh.allocateIconTextGap(textGap); > 299: } > 300: } What if the menu is modified and a menu item that triggers two-column check + icon rendering is added or removed? I still think the logic for scanning menu items in a popup belongs in `MenuItemLayoutHelper`… but it could be unfeasible to pull it there. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsPopupMenuUI.java line 33: > 31: import java.awt.Insets; > 32: import java.awt.Window; > 33: import javax.swing.*; Can we avoid using wildcard imports? There are quite a few single class imports from the `javax.swing` package below. Expanding the wildcard adds just four menu-related imports. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsPopupMenuUI.java line 216: > 214: > 215: if (size != null) { > 216: size = new Dimension(size); Is creating new copy necessary? src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsPopupMenuUI.java line 223: > 221: if (afterCheckIconGapObject instanceof Integer) { > 222: afterCheckIconGap = (Integer) > afterCheckIconGapObject; > 223: } Use `UIManager.getInt`. src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsPopupMenuUI.java line 233: > 231: > 232: private static boolean hasCheckBulletAndIconPresent(JPopupMenu > popupMenu) { > 233: for (Component child : popupMenu.getComponents()) { This method looks very similar to what you have in `WindowsMenuItemUI.scanMenuComponent`, is it possible to re-use the logic? test/jdk/javax/swing/JMenuItem/TestRadioAndCheckMenuItemWithIcon.java line 31: > 29: * JRadioButtonMenuItem and JCheckboxMenuItem > 30: * is rendered with ImageIcon in WindowsLookAndFeel > 31: * @requires (os.family == "windows") Why are we removing the restriction to Windows? test/jdk/javax/swing/JMenuItem/TestRadioAndCheckMenuItemWithIcon.java line 104: > 102: If bullet and checkmark is shown in "Menu" > 103: and "MenuItem2" image icon location and > 104: and "Menu2" rendering is as decribed, test passes else fails."""; Suggestion: and "Menu2" rendering is as described, test passes else fails."""; test/jdk/javax/swing/JMenuItem/TestRadioAndCheckMenuItemWithIcon.java line 107: > 105: > 106: public static void main(String[] args) throws Exception { > 107: if ((UIManager.getLookAndFeel().getID()).equals("Aqua")) { Is there a reason why `getLookAndFeel().getID()` is used here instead of `getLookAndFeel().getName()` that's used below? test/jdk/javax/swing/JMenuItem/TestRadioAndCheckMenuItemWithIcon.java line 112: > 110: > 111: String INSTRUCTIONS = null; > 112: int colWidth = 0; The initialisers are redundant—the `if` statement always initialises both `INSTRUCTIONS` and `colWidth`. I'd prefer lower-case `instructions` here—it's a regular local variable. test/jdk/javax/swing/JMenuItem/TestRadioAndCheckMenuItemWithIcon.java line 117: > 115: INSTRUCTIONS = INSTRUCTIONS2 + INSTRUCTIONS3 + INSTRUCTIONS4; > 116: colWidth = 75; > 117: } else { Since this test was specifically targeted to Windows Look-and-Feel, we should provide an easy way to run it in Windows L&F instead of the default (Metal) L&F. ------------- PR Review: https://git.openjdk.org/jdk/pull/29730#pullrequestreview-4438475583 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364461573 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364558891 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364502013 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364507360 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364611997 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364599172 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364526548 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364652264 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364532740 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364534203 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364544868 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364666316 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364703491 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364670687 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364636919 PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3364676444
