On Wed, 26 Apr 2023 14:25:42 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Tejesh R has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated ScrollPaneWindowsTest.java
>
> test/jdk/java/awt/ScrollPane/ComponentScrollTest.java line 80:
>
>> 78: Thread.sleep(5000);
>> 79: } catch (InterruptedException ie) {
>> 80: }
>
> This `sleep` blocks EDT. Is it intended?
Yeah, I guess 5000 might not be required 1000 might be fine, but either delay
or sleep is required I feel.
> test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 87:
>
>> 85:
>> 86: Point buttonLoc = button.getLocationOnScreen();
>> 87: Dimension buttonSize = button.getSize();
>
> Both `getLocationOnScreen` and `getSize` should also be called on EDT?
Updated.
> test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 53:
>
>> 51: Frame frame;
>> 52: Insets paneInsets;
>> 53: public static Object LOCK = new Object();
>
> The lock object should be `final`.
Updated.
> test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 62:
>
>> 60: System.out.println("This test is for Windows 2000 and
>> above.");
>> 61: return;
>> 62: }
>
> Shall this be dropped too? Java won't start on Windows versions before Vista
> (6.0), so does it serve a purpose other than a historical reasons?
Updated.
> test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 70:
>
>> 68: public void init() throws Exception {
>> 69: EventQueue.invokeAndWait(() -> {
>> 70: frame = new Frame();
>
> Suggestion:
>
> frame = new Frame("ScrollPaneWindowsTest");
Updated.
> test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 81:
>
>> 79: hScroll = (ScrollPaneAdjustable) sp.getHAdjustable();
>> 80: vScroll.addAdjustmentListener(this);
>> 81: hScroll.addAdjustmentListener(this);
>
> ScrollPane should be created inside `invokeAndWait` block, listeners should
> be added on EDT too.
Updated.
> test/jdk/java/awt/ScrollPane/ScrollPositionIntact.java line 31:
>
>> 29: */
>> 30:
>> 31: import java.awt.*;
>
> Avoid wildcard imports.
Updated.
> test/jdk/java/awt/ScrollPane/ScrollPositionIntact.java line 61:
>
>> 59: pa.add("East", new Label("East", Label.RIGHT));
>> 60: sp.add(pa);
>> 61: frame = new Frame();
>
> Suggestion:
>
> frame = new Frame("ScrollPositionIntact");
Updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178149780
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178149893
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178149988
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178150098
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178150275
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178150178
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178150376
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178150724
