On Thu, 21 Mar 2024 16:51:36 GMT, Damon Nguyen <dngu...@openjdk.org> wrote:
>> Convert
>> java/awt/image/multiresolution/MultiDisplayTest/MultiDisplayTest.java applet
>> test to main using PassFailJFrame
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comments
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 62:
> 60: - 2nd display is non-HiDPI.
> 61:
> 62: In other cases please simply push "Pass".
These conditions can be detected. On the other hand, since it's a manual test,
the tester could enable or connect a second monitor, so it makes sense to
preserve the instructions.
test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 68:
> 66: Then drag parent / child to different displays and check
> 67: that the proper image is shown for every window
> 68: (must be "black 1x" for non-HiDPI and "blue 2x" for HiDPI).
What if the scale is not 200%? It's possible on Windows and it's not uncommon.
test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 79:
> 77: and Mission Control behavior.
> 78:
> 79: Close the windows.
Won't it fail the test?
test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 84:
> 82: """;
> 83:
> 84: private static final int W = 200, H = 200;
Suggestion:
private static final int W = 200;
private static final int H = 200;
Java Coding Guidelines do not recommend having several declarations on the same
line.
test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 141:
> 139: public ParentFrame() {
> 140: EventQueue.invokeLater(this::CreateUI);
> 141: }
This looks weird…
As far as I can see, the frame is created when Start button is clicked, which
implies this operation is running on EDT. There's no reason to use
`invokeLater`, all the code from `CreateUI` method could be placed into the
constructor.
test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 165:
> 163: super(f);
> 164: EventQueue.invokeLater(this::CreateUI);
> 165: }
The same comment, `invokeLater` seems redundant.
-------------
PR Review: https://git.openjdk.org/jdk/pull/18354#pullrequestreview-1952880721
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534322342
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534330435
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534327277
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534328684
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534338034
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534338996
