On Fri, 5 Jun 2026 18:52:28 GMT, Alexey Ivanov <[email protected]> wrote:
> 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.
It works as per my testing
> 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.
ok
> 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.
ok..removed..
> 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.
It is done so that scanning happen only once for top level menu and not
everytime for each menuitem
> 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.
ok
> 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.
ok
> 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`.
ok..removed..
> 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.
MenuItemUI is invoked which will scan again.
> 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.
ok
> 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?
removed..
> 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`.
ok
> 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?
For menu we need to get the children from getMenuComponents while for JPopMenu
we need to get the children using getComponents..so again instance check will
be necessary if combined and also code is not exactly same so needed in both
place
> 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.""";
ok
> 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?
getName returns Mac OS X while getID returns Aqua which is easier to compare
and also the former may look like we are restricting the whole platform, which
is not the case..
> 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.
it will fail to compile
> 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.
Added a line in the instruction..
-------------
PR Comment: https://git.openjdk.org/jdk/pull/29730#issuecomment-4686929219
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400378829
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400380122
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400379086
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400379392
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400380461
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400380279
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400379536
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400380838
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400379759
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400379868
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400380013
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400381425
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400381111
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400380646
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3400381279
