On Thu, 3 Oct 2024 17:20:59 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Jayathirth D V has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove not needed setVisible calls
>> - Update based on review comments
>
> test/jdk/java/awt/Window/OwnedWindowShowTest/OwnedWindowShowTest.java line 47:
>
>> 45: Window owner = new Window(parent);
>> 46: Window window = new Window(owner);
>> 47: window.setVisible(true);
>
> You may add a comment here that `setVisible(true)` should not throw any
> exception.
>
> Yes, it is stated in the test summary, yet repeating it here would make it
> clearer.
Updated.
> test/jdk/java/awt/Window/OwnedWindowShowTest/OwnedWindowShowTest.java line 51:
>
>> 49: if (parent != null) {
>> 50: parent.dispose();
>> 51: }
>
> The condition `parent != null` is always `true` when `if`-statement is
> reached.
> Suggestion:
>
> parent.dispose();
>
>
> You may also want to dispose of `window` and `owner` too.
Removed if check. If parent is disposed, i expect child components also to be
disposed.
I don't see any UI components after test is finished. Do we need to explicitly
dispose all child components?
> test/jdk/java/awt/Window/ShowWindowTest/ShowWindowTest.java line 52:
>
>> 50: 2. Click on the "Show" button. A Window with a "Hello World"
>> Label
>> 51: should appear
>> 52: 3. If a Window does not appear, the test failed.
>
> Suggestion:
>
> 2. Click on the "Show" button. A window with a "Hello World" Label
> should appear
> 3. If a Window does not appear, the test fails.
Updated and added more clarity.
> test/jdk/java/awt/Window/ShowWindowTest/ShowWindowTest.java line 71:
>
>> 69: frame.add(hideButton = new Button("Hide"));
>> 70: showButton.addActionListener(new ShowWindowTest());
>> 71: hideButton.addActionListener(new ShowWindowTest());
>
> Suggestion:
>
> frame.add(hideButton = new Button("Hide"));
>
> ActionListener handler = new ShowWindowTest();
> showButton.addActionListener(handler);
> hideButton.addActionListener(handler);
>
> You can use the same instance for both.
Updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21284#discussion_r1787236850
PR Review Comment: https://git.openjdk.org/jdk/pull/21284#discussion_r1787236083
PR Review Comment: https://git.openjdk.org/jdk/pull/21284#discussion_r1787236702
PR Review Comment: https://git.openjdk.org/jdk/pull/21284#discussion_r1787236996
