On Wed, 3 May 2023 16:00:39 GMT, Tejesh R <t...@openjdk.org> wrote:
>> Open source few AWT ScrollPane related tests.
>
> Tejesh R has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed whitespace error
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/ScrollPane/ComponentScrollTest.java line 42:
> 40: import java.awt.event.AdjustmentListener;
> 41:
> 42: import java.lang.reflect.InvocationTargetException;
`InvocationTargetException` is unused, remove unused imports, please.
test/jdk/java/awt/ScrollPane/ComponentScrollTest.java line 72:
> 70: @Override
> 71: public void adjustmentValueChanged(AdjustmentEvent
> adjustmentEvent) {
> 72: count++;
IDE warns that we're changing a volatile field in a non-atomic fashion… but
it's probably okay here.
test/jdk/java/awt/ScrollPane/ComponentScrollTest.java line 87:
> 85: });
> 86:
> 87: Thread.sleep(5000);
Sleeping for 5 seconds could be too much.
It would be great to ensure the updated test reproduces the problem that was
fixed by [JDK-4342129](https://bugs.openjdk.org/browse/JDK-4342129) in 1.3.0
and 1.4.0. Otherwise, these 5 seconds could be a waste of time.
test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 52:
> 50:
> 51: public class ScrollPaneLimitation {
> 52: final static int SCROLL_POS = 50000;
Suggestion:
static final int SCROLL_POS = 50000;
Address the missorted modifiers.
test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 91:
> 89: lock.notify();
> 90: }
> 91: }
Suggestion:
if (e.getID() == MouseEvent.MOUSE_PRESSED
&& e.getSource() == ScrollPaneLimitation.child
&& e.getY() > SCROLL_POS) {
go.countDown();
}
I meant replacing synchronisation with `lock` by `CountDownLatch`. You created
a mix of them.
(Here, I moved the first `&&` operator to wrapped line — you should be
consistent: either move binary operators to wrapped line or leave them on the
previous line.
test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 106:
> 104:
> 105: EventQueue.invokeAndWait(() -> {
> 106: p = child.getLocation();
The synchronization here doesn't look correct.
`p` is later used on main thread. It is safer to create a local variable inside
the lambda expression, print the positions etc. As the last statement create a
new `Point` instance with the computed coordinates for mouse click and assign
it to `p` which will be used on the main thread to move the mouse cursor.
It is not necessary to mark the field as `volatile` because `invokeAndWait`
serves a the synchronisation point between the two thread.
test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 144:
> 142: if (!mouseWasPressed) {
> 143: throw new RuntimeException("mouse was not pressed");
> 144: }
Suggestion:
if (!go.await(3, TimeUnit.SECONDS)) {
throw new RuntimeException("mouse was not pressed");
}
If mouse is clicked and the event is sent, the `go` reaches zero and `await`
returns `true`; otherwise `await` returns `false` and the test fails.
The `lock` and `mouseWasPressed` are to be removed.
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 39:
> 37: import java.awt.Robot;
> 38: import java.awt.ScrollPane;
> 39: import java.awt.Toolkit;
`BorderLayout` and `Toolkit` are unused, remove unused imports, please.
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 71:
> 69: button.addActionListener(new ActionListener() {
> 70: public void actionPerformed(ActionEvent e) {
> 71: actionSema.raise();
The entire thing with a custom `Semaphore` is overly complicated and
unnecessary. What it verifies is that the button is clicked, isn't it. Use the
standard `CountDownLatch` for it.
So, here it'll be:
Suggestion:
latch.countDown();
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 111:
> 109: throw new RuntimeException("ScrollPane doesn't handle " +
> 110: "correctly add after remove");
> 111: }
All this code will be as simple as this:
Suggestion:
if (!latch.await(1, TimeUnit.SECONDS)) {
throw new RuntimeException("ScrollPane doesn't handle " +
"correctly add after remove");
}
No need to `waitForIdle` because the latch will be released only after the
clicked event gets handled.
Then remove the `Semaphore` class which is not implemented correctly.
test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 98:
> 96:
> 97: robot.mouseMove(sp.getLocationOnScreen().x + sp.getWidth() -
> paneInsets.right / 2,
> 98: sp.getLocationOnScreen().y + sp.getHeight() / 2);
You should get the location on the scroll pane and its size inside
`invokeAndWait` and use these two new fields to move the mouse.
test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 174:
> 172: } catch (InterruptedException e) {
> 173: throw new RuntimeException("Test interrupted while keys
> being pressed.", e);
> 174: }
You can remove the try-catch block here, the throws clause in the method
declaration allows throwing `InterruptedException`. Since this exception is
unlikely to be thrown ever, the additional detail message isn't as useful.
test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 178:
> 176:
> 177: public void adjustmentValueChanged(AdjustmentEvent e) {
> 178: notifyReceived = true;
`notifyReceived` should be modified and read inside `synchronized
(ScrollPaneWindowsTest.LOCK)` block.
test/jdk/java/awt/ScrollPane/ScrollPositionIntact.java line 74:
> 72: robot.delay(1000);
> 73: robot.waitForIdle();
> 74: EventQueue.invokeAndWait(() -> {
Suggestion:
});
Robot robot = new Robot();
robot.delay(1000);
robot.waitForIdle();
EventQueue.invokeAndWait(() -> {
Let's add blank lines to separate the code portions visually.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13621#pullrequestreview-1414573417
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185984003
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185994523
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1186039141
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1186052205
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185957430
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185966007
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185961134
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185984651
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185986092
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1185988584
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1186046722
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1186051071
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1186048121
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1186056045
