On Tue, 17 Sep 2024 19:00:55 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comment fix
>
> 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.
Removed.
> 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?
Yeah.. updated now.
> 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);
> }
I gave a try to get PassFailFrame position but some error occured and then hard
coded. I agree hard-coding is not a best solution and thanks for suggesting the
other way around. A bit of correction in setting the testFrame location, y
co-ordinate should be firstFrame.getY() + firstFrame.getHeight() + 8 else the
gap is more between firstFrame and testFrame.
> 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.
Updated.
> 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.
Removed from all tests.
>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 Just left them as it was before conversion.
> 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`…
Ok.. Updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764371719
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764370259
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764371347
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764369981
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764372023
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764372474
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764372546
