On Thu, 4 Jul 2024 10:25:38 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: Review comments fixed
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 56:
> 54:
> 55: private static Frame frame;
> 56: private static int DELAY = 500;
Suggestion:
private static final int DELAY = 500;
It's a constant, isn't it?
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 58:
> 56: private static int DELAY = 500;
> 57: private static Robot robot;
> 58: private volatile static Component[] components;
Why does the `components` array need to be `volatile`?
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 62:
> 60: private volatile static boolean componentShown = false;
> 61: private volatile static boolean componentMoved = false;
> 62: private volatile static boolean componentResized = false;
Assigning the default value is redundant…
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 66:
> 64: private volatile static Dimension compSize;
> 65: private volatile static ArrayList<ComponentEvent> events =
> 66: new ArrayList<>();
Marking a list as `volatile` is not enough to access the contents of the list
from different threads, you have to use a synchronized collection and iterate
over `events` while holding the lock.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 141:
> 139: EventQueue.invokeAndWait(ComponentEventTest::initializeGUI);
> 140: robot.delay(DELAY);
> 141: robot.waitForIdle();
Suggestion:
robot.waitForIdle();
robot.delay(DELAY);
Delaying before `waitForIdle` doesn't make much sense… The tests use
`waitForIdle` followed by `delay` to ensure all the events are handled and then
to add additional time… just in case.
If you call `waitForIdle` after `delay`, the chances are high that
`waitForIdle` returns immediately because there are no pending events already
(they're handled when the thread has been sleeping).
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 164:
> 162: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 163:
> 164: // Hide all components and check if the ComponentEvent is
> triggered
Is it really what's going on here? Where do you hide all the components?
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 181:
> 179: System.err.print("Events triggered are: ");
> 180: for (int j = 0; j < events.size();
> 181: System.err.print(events.get(j) + "; "), j++);
This is very confusing. Move printing into the body of the `for`-loop:
Suggestion:
for (int j = 0; j < events.size(); j++) {
System.err.print(events.get(j) + "; ");
}
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 184:
> 182: System.err.println();
> 183: throw new RuntimeException(
> 184: "FAIL: ComponentEvent triggered when frame is
> iconified");
You're repeating these lines of code over and over again — create a helper
method for printing the events.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 282:
> 280: throw new RuntimeException(
> 281: "FAIL: ComponentResized not triggered for "
> 282: + components[i].getClass());
Choose one style… At line 267, you pass the message on the same line; here you
wrap the message to next line immediately.
It's more common to keep the first part of the string on the same line and wrap
the string as necessary.
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 291:
> 289: resetFrame();
> 290: });
> 291: robot.delay(DELAY);
So, you're repeating the same tests as above? Why can't you use a loop?
for (boolean state : new boolean[] {true, false}) {
currentComponent.setEnabled(state);
doTests();
}
test/jdk/java/awt/event/ComponentEvent/ComponentEventTest.java line 377:
> 375: }
> 376:
> 377: private static void resetFrame() {
Suggestion:
private static void revalidateFrame() {
`revalidateFrame` is a better name, it's more descriptive: you *invalidate* and
then *validate* the frame.
-------------
PR Review: https://git.openjdk.org/jdk/pull/19521#pullrequestreview-2161195355
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667103248
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667103770
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667114986
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667102414
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667106160
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667107307
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667100737
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667101210
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667111579
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667097272
PR Review Comment: https://git.openjdk.org/jdk/pull/19521#discussion_r1667114236
