On Fri, 12 Jun 2026 03:01:06 GMT, Prasanta Sadhukhan <[email protected]>
wrote:
>> 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
It doesn't seem right to me. Scanning should be performed whenever menu layout
is performed.
If the result of scanning is cached, it should be per popup.
>> 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.
Have you tested such a scenario?
>> 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
Fair point.
Still, I'd like to avoid code duplication if possible. I haven't thought it
through if the code duplication can be eliminated or not. It may be possible to
pull out the body into a helper method that gets called from from both branches.
>> 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..
If a set of `@test` tags is added, I suggest in another comment, I'd like to
ensure this test won't even start on macOS with its default Aqua L&F (or system
one).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3692663453
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3692671165
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3692413812
PR Review Comment: https://git.openjdk.org/jdk/pull/29730#discussion_r3692764889
