On Tue, 17 Sep 2024 04:31:09 GMT, Abhishek Kumar <abhis...@openjdk.org> wrote:
> Few AWT MenuItem related tests are converted from applet to manual and moved
> to open.
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/MenuItem/GiantFontTest.java line 72:
> 70: mb.add(m);
> 71: f.setMenuBar(mb);
> 72: f.setSize (450, 400);
Suggestion:
f.setSize(450, 400);
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 45:
> 43: */
> 44:
> 45: public class LotsOfMenuItemsTest implements ComponentListener {
Suggestion:
public class LotsOfMenuItemsTest extends ComponentAdapter {
You use only one method from `ComponentListener`, override only the one you
need and remove the ones you don't use.
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 47:
> 45: public class LotsOfMenuItemsTest implements ComponentListener {
> 46: private static final int NUM_WINDOWS = 400;
> 47: private static TestFrame firstFrame, testFrame;
It is not recommended to declare several fields or variables on one line in
Java.
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 48:
> 46: private static final int NUM_WINDOWS = 400;
> 47: private static TestFrame firstFrame, testFrame;
> 48: private static Rectangle rect;
`rect` is never used.
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 53:
> 51: LotsOfMenuItemsTest obj = new LotsOfMenuItemsTest();
> 52: String INSTRUCTIONS = """
> 53: This test creates a lots of frames with menubars.
Suggestion:
This test creates lots of frames with menu bars.
Either _“a lot of”_ or _“lots of”_.
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 58:
> 56:
> 57: If everything seems to work - test passed.
> 58: Click "Done" button in the test harness window.
There's no **Done** button.
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 73:
> 71: }
> 72:
> 73: private List<? extends Frame> createAndShowUI() {
It can be simplified to
Suggestion:
private List<Frame> createAndShowUI() {
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 86:
> 84: testFrame.setTitle("Last Frame");
> 85: }
> 86: }
Suggestion:
for (int i = 1; i < NUM_WINDOWS - 1; ++i) {
testFrame = new TestFrame("Running(" + i + ")...");
testFrame.setVisible(false);
testFrame.dispose();
}
testFrame = new TestFrame("Last Frame");
Doesn't it make the code clearer and shorter?
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 89:
> 87: list.add(firstFrame);
> 88: list.add(testFrame);
> 89: return list;
You can return a list like this:
Suggestion:
return List.of(firstFrame, testFrame);
No need to explicitly create a list, no need to explicit add items one by one.
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 98:
> 96: firstFrame.setLocation(970, 350);
> 97: testFrame.setLocation(970, 510);
> 98: }
Hard-coding coordinates is not the best solution. The workaround is to position
the first frame using `PassFailJFrame` and then use the location of the first
frame to position the second (test) frame.
Suggestion:
@Override
public void componentShown(ComponentEvent e) {
PassFailJFrame.positionTestWindow(firstFrame,
PassFailJFrame.Position.HORIZONTAL);
testFrame.setLocation(firstFrame.getX(),
firstFrame.getY() + firstFrame.getWidth() + 8);
}
test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 103:
> 101: }
> 102:
> 103: class TestFrame extends Frame {
Could you please make `TestFrame` a static nested class inside
`LotsOfMenuItemsTest`.
I avoids conflicts when you mark a folder as Test Source in an IDE and that
folder contains multiple tests.
In fact, you don't need to extend `Frame` at all: create an instance of the
`Frame` class and add menu bar and set the size of the frame. However, I agree,
keeping this stuff in constructor simplifies test.
test/jdk/java/awt/MenuItem/MenuSetFontTest.java line 47:
> 45: Click on the "File" menu. You will see "menu item"
> item.
> 46: Press pass if menu item is displayed using bold and
> large font else fail.
> 47: If you do not see menu at all, press fail.""";
Suggestion:
Press Pass if menu item is displayed using bold and large
font, otherwise press Fail.
If you do not see menu at all, press Fail.""";
Capitalising the names of the buttons makes it easier to recognise it's a UI
element.
test/jdk/java/awt/MenuItem/MenuSetFontTest.java line 54:
> 52: .rows((int) INSTRUCTIONS.lines().count() + 2)
> 53: .columns(40)
> 54: .testTimeOut(5)
You may omit the default timeout.
test/jdk/java/awt/MenuItem/MenuSetFontTest.java line 67:
> 65: menu.add(item);
> 66: menuBar.add(menu);
> 67: menuBar.setFont(new Font("Monospaced", 1, 24));
Use the constants from the `Font` class:
Suggestion:
menuBar.setFont(new Font(Font.MONOSPACED, Font.BOLD, 24));
test/jdk/java/awt/MenuItem/NullOrEmptyStringLabelTest.java line 47:
> 45: The bug is reproducible under Win32 and Solaris.
> 46: Setting 'null' and "" as a label of menu item
> 47: should by the spec set blank label on all platforms.
Suggestion:
Setting 'null' and "" as a label of menu item
should set blank label on all platforms according to the
spec(ification).
It is uncommon to insert a phrase between an auxiliary verb and the main verb,
even though adverbs are usually placed between them.
test/jdk/java/awt/MenuItem/NullOrEmptyStringLabelTest.java line 76:
> 74: Button button3 = new Button("Set MenuItem label to 'text'");
> 75: button1.addActionListener(new ActionListener() {
> 76: public void actionPerformed(ActionEvent ev) {
Maybe add `@Override` annotation?
test/jdk/java/awt/MenuItem/NullOrEmptyStringLabelTest.java line 97:
> 95: frame.add("North", button1);
> 96: frame.add("South", button2);
> 97: frame.add("Center", button3);
Wouldn't it be better to keep the logical and visual order of the buttons
consistent? Or is the intention to set the label to `"text"` after `null`
followed by `""`?
I also recommend using newer syntax (order of parameters) and constants from
`BorderLayout`:
Suggestion:
frame.add(button1, BorderLayout.NORTH);
frame.add(button2, BorderLayout.CENTER);
frame.add(button3, BorderLayout.SOUTH);
test/jdk/java/awt/MenuItem/UnicodeMenuItemTest.java line 81:
> 79:
> 80: MenuItem mi6 = new MenuItem("\u012d");
> 81: m.add(mi6);
The numbering is weird here: 2, 6, 7 — but there's no `mi1` or `m3`…
-------------
PR Review: https://git.openjdk.org/jdk/pull/21029#pullrequestreview-2310512618
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763692026
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763745882
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763708736
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763749772
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763710647
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763727356
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763756892
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763732588
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763707460
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763744407
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763705638
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763748726
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763768377
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763753615
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763764904
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763774205
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763784379
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1763788616
