On Tue, 20 May 2025 16:18:48 GMT, Manukumar V S <m...@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
