On Mon, 22 May 2023 07:06:37 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:
>
> Updated based on review comments
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/ScrollPane/ScrollPaneExtraScrollBar.java line 71:
> 69: try {
> 70: robot.delay(100);
> 71: robot.waitForIdle();
Optionally,
Suggestion:
robot = new Robot();
robot.waitForIdle();
robot.delay(100);
To be consistent with other tests in this review.
test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 115:
> 113: Container cp = child.getParent();
> 114: p = cp.getLocation();
> 115: if (p.y != -SCROLL_POS) {
Suggestion:
p = cp.getLocation();
System.out.println("Child's parent pos = " + p);
if (p.y != -SCROLL_POS) {
Just to make it clear that this branch is taken. Otherwise the test ends with
the message:
Child pos = java.awt.Point[x=0,y=0]
which is confusing. Yet no exception is thrown.
test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 125:
> 123: p = pane.getLocationOnScreen();
> 124: Dimension d = pane.getSize();
> 125: point = new Point(p.x += d.width / 2, p.y += d.height /
> 2);
Suggestion:
point = new Point(p.x + d.width / 2, p.y + d.height / 2);
There's no need to update the value stored in `p`.
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 76:
> 74: frame.pack();
> 75: frame.setLocationRelativeTo(null);
> 76: frame.setAlwaysOnTop(true);
I wonder why you decided to make the frame in this test always on top?
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 83:
> 81: public void start() throws Exception {
> 82: try {
> 83: Robot robot = new Robot();
Since you moved it as suggested, I see no reason why it's declared here rather
than at line before `robot.waitForIdle`, that is the first usage.
test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 85:
> 83: System.out.println("Adjustment Vertical Scroll Event
> called ");
> 84: }
> 85: });
Suggestion:
vScroll.addAdjustmentListener(ScrollPaneWindowsTest.this);
Isn't it more concise without introducing any duplicate code?
test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 115:
> 113:
> 114: robot.delay(100);
> 115: robot.waitForIdle();
Suggestion:
robot = new Robot();
robot.waitForIdle();
robot.delay(100);
test/jdk/java/awt/ScrollPane/ScrollPositionIntact.java line 77:
> 75: Robot robot = new Robot();
> 76: robot.delay(1000);
> 77: robot.waitForIdle();
Suggestion:
Robot robot = new Robot();
robot.waitForIdle();
robot.delay(1000);
Let's make this piece of code consistent in all the tests.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13621#pullrequestreview-1436387950
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200456271
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200463397
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200362994
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200364050
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200366170
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200370354
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200468052
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200373022
