On Tue, 17 Sep 2024 16:19:27 GMT, Ravi Gupta <rgu...@openjdk.org> wrote:
>> This testcase checks for the following assertions for Component events:
>>
>> 1. When components are resized, moved, hidden and shown the respective
>> events are triggered.
>> 2. When the components are hidden/disabled also,the component events like
>> resized/moved are triggered.
>> 3. When a hidden component is hidden again, or a visible component is shown
>> again, the events should not be fired.
>> 4. When a window is minimized/restored then hidden and shown component
>> events should be triggered.
>>
>> Testing:
>> Tested using Mach5(20 times per platform) in macos,linux and windows and got
>> all pass.
>
> Ravi Gupta has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8333403: Whitespace Fixed
I am still for moving the test into `java/awt/Component` folder.
test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 50:
> 48: * @bug 8333403
> 49: * @summary Test performs various operations to check components events
> are triggered properly.
> 50: * @run main ComponentEventTest
Add the library and build tags to use the `Platform` class.
Suggestion:
* @summary Test performs various operations to check components events are
triggered properly.
* @library /test/lib
* @build jdk.test.lib.Platform
* @run main ComponentEventTest
And don't forget to add `jdk.test.lib.Platform` to the list of imports.
test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 149:
> 147: private static void doTest()
> 148: throws InvocationTargetException, InterruptedException {
> 149: // Click on the Frame to ensure its gain Focus
Suggestion:
// Click the frame to ensure it gains focus
I see no reason why you capitalise “frame” and “focus”.
test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 161:
> 159: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 160:
> 161: robot.delay(DELAY);
Does it make sense to move this part into a new method `clickFrame`?
Does anyone else have any opinions on this?
test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 171:
> 169: robot.delay(DELAY);
> 170:
> 171: System.out.println("Iconify frame");
Similarly, iconify and deiconify could be moved into a separate method.
Shorter, logically complete pieces improve the code quality and the overall
readability of the code.
test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 196:
> 194: * the events from the native system.
> 195: * Please check the JDK-6754618
> 196: */
Suggestion:
/*
* Because of the different behavior between MS Windows and other OS,
* we receive native events WM_SIZE and WM_MOVE on Windows
* when the frame state changes from iconified to normal.
* AWT sends these events to components when it receives
* the events from the native system.
* See JDK-6754618 for more information.
*/
test/jdk/java/awt/ComponentEvent/ComponentEventTest.java line 208:
> 206: throw new RuntimeException(
> 207: "ComponentEvent triggered when frame set to normal");
> 208: }
I suggest using the `Platform` class to determine whether the test runs on
Windows or not and splitting the common condition and platform-dependent
conditions:
if (componentShown || componentHidden) {
throw new RuntimeException(
"FAIL: componentShown or componentHidden triggered "
+ "when frame set to normal");
}
if (Platform.isWindows() && (!componentMoved || !componentResized)) {
throw new RuntimeException(
"FAIL: componentMoved or componentResized wasn't triggered "
+ "when frame set to normal");
}
if (!Platform.isWindows() && (componentMoved || componentResized)) {
throw new RuntimeException(
"FAIL: componentMoved or componentResized triggered "
+ "when frame set to normal");
}
This allows you to report a clearer error message.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19521#pullrequestreview-2310234320
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763592072
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763532753
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763582314
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763583953
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763545672
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1763563500
