RFR: 8357305: Compilation failure in javax/swing/JMenuItem/bug6197830.java [v2]

Alexey Ivanov aivanov at openjdk.org
Tue May 20 18:10:59 UTC 2025 

On Tue, 20 May 2025 16:18:48 GMT, Manukumar V S <mvs at openjdk.org> wrote:

>> There are some compilation failures noticed in the recently open sourced test javax/swing/JMenuItem/bug6197830.java. The compilation failures are due to missing import statements and a missing dependency file MenuItemTest.java.
>> 
>> Fix: I have added the required import statements and added the code-piece from MenuItemTest.java as a method getMenuItemTestFrame().
>
> Manukumar V S has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments fixed : Rearranged and reused code by creating a new Helper MenuItemTestHelper.java

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 2:

> 1: /*
> 2:  * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

I'd keep 2005 year as the start of the copyright — it's not a brand new class, you just pulled it out of another class.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 43:

> 41: import javax.swing.UIManager;
> 42: 
> 43: class MenuItemTestHelper {

I would've kept `MenuItemTest` as the name of the class. It's find either way.

I'd also mark this class as `final`. It may be `public` or be left with the default access.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 53:

> 51:             } catch (Exception e) {
> 52:                 e.printStackTrace();
> 53:             }

I wonder if we should actually wrap the exception into `RuntimeException` and let it propagate to fail the test. I think we should; if Look and Feel wasn't set as expected, the test won't test that part of the functionality.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 65:

> 63: 
> 64:             public int getIconWidth() { return 10; }
> 65:             public int getIconHeight() { return 10; }

I suggest keeping the multiline version of the methods, I find it easier to read. Let IDE collapse the code for you.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 69:

> 67: 
> 68:         Icon myIcon2 = new Icon() {
> 69:             public void paintIcon(Component c, Graphics g, int x, int y) {

I'd add the `@Override` annotations to all the overridden methods.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 74:

> 72:                 g.fillRect(x, y, 15, 10);
> 73:                 g.setColor(color);
> 74:             }

Perhaps, both `Icon` instances could even be refactored into its own class that accepts color, width and height.

However, this is close to over-engineering the test code…

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 85:

> 83:         menuBar.add(createSomeIconsMenu(myIcon, myIcon2));
> 84: 
> 85:         String title = "Menu Item Test " + (isLeft ? "(Left-to-right)" : "(Right-to-left)");

Suggestion:

        String title = (isLeft ? "(Left-to-right)" : "(Right-to-left)") + " - Menu Item Test";

I suggest flipping the title so that the most relevant part is always visible rather than truncated.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 87:

> 85:         String title = "Menu Item Test " + (isLeft ? "(Left-to-right)" : "(Right-to-left)");
> 86:         JFrame frame = new JFrame(title);
> 87:         frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

Closing the frame is handled by `PassFailJFrame`, so `setDefaultCloseOperation` is redundant.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 89:

> 87:         frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
> 88:         frame.setJMenuBar(menuBar);
> 89:         frame.applyComponentOrientation(isLeft ? ComponentOrientation.LEFT_TO_RIGHT : ComponentOrientation.RIGHT_TO_LEFT);

Suggestion:

        frame.applyComponentOrientation(isLeft
                                        ? ComponentOrientation.LEFT_TO_RIGHT
                                        : ComponentOrientation.RIGHT_TO_LEFT);

This avoids a long line.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 100:

> 98:         frame.setSize(300, 300);
> 99:         frame.setLocation(isLeft ? 0 : 600, frameY);
> 100:         frame.setVisible(true);

`setLocation` shouldn't be need if you use `PassFailJFrame` layouts for positioning. Then `frameY` becomes redundant.

`setVisible(true)` isn't need either.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 120:

> 118: 
> 119:         JRadioButtonMenuItem rm2 = new JRadioButtonMenuItem("And icon.");
> 120:         rm2.setAccelerator(KeyStroke.getKeyStroke(KeyEvent.VK_F1, KeyEvent.SHIFT_MASK));

Suggestion:

        rm2.setAccelerator(KeyStroke.getKeyStroke(KeyEvent.VK_F1, KeyEvent.SHIFT_DOWN_MASK));

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 139:

> 137: 
> 138:     private static JMenu createNoNothingMenu() {
> 139:         final JMenu menu2 = new JMenu("No nothing");

Suggestion:

        final JMenu noMenu = new JMenu("No nothing");

I'd rename it to `noMenu` to represent `NoNothing` from the method name. Otherwise, it can be just `menu`, there's no other `menu` in scope any more.

test/jdk/javax/swing/JMenuItem/MenuItemTest/MenuItemTestHelper.java line 148:

> 146:                     PassFailJFrame.log("menu.width = " + width);
> 147:                 }
> 148:             });

Suggestion:

            item.addActionListener((e) ->
                    PassFailJFrame.log("menu.width = "
                                       + menu2.getPopupMenu().getWidth()));

Let's be concise.

test/jdk/javax/swing/JMenuItem/MenuItemTest/bug4729669.java line 49:

> 47:                 .columns(35)
> 48:                 .testUI(bug4729669::createTestUI)
> 49:                 .position(PassFailJFrame.Position.TOP_LEFT_CORNER)

Suggestion:

                .positionTestUIRightColumn()

test/jdk/javax/swing/JMenuItem/MenuItemTest/bug6197830.java line 46:

> 44: 
> 45:     public static void main(String[] args) throws Exception {
> 46:         PassFailJFrame.builder()

Use `.positionTestUIBottomRowCentered()`, it works perfectly well.

Then remove all the `.setLocation` calls from `createTestUI`.

-------------

PR Review: https://git.openjdk.org/jdk/pull/25319#pullrequestreview-2854867797
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098398095
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098469033
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098479317
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098411293
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098429701
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098416620
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098533147
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098431170
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098545578
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098445060
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098588166
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098549320
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098543901
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098443874
PR Review Comment: https://git.openjdk.org/jdk/pull/25319#discussion_r2098560543

More information about the client-libs-dev mailing list