On Tue, 4 Jun 2024 13:45:44 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Code formatting update
>
> src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java line
> 1:
>
>> 1: /*
>
> You should update the copyright year.
Updated.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 64:
>
>> 62: } catch (Exception e) {
>> 63: throw new SkippedException("GTK LAF is not supported on this
>> system;" +
>> 64: " test passes");
>
> Suggestion:
>
> throw new SkippedException("GTK LAF is not supported on this
> system");
>
> I'd remove the additional message. I believe it was `println` before.
Updated.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 116:
>
>> 114: }
>> 115:
>> 116: private void onBackgroundThread20() {
>
> This method should run on EDT because it accesses the state of the components.
Updated to run this on EDT.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 133:
>
>> 131: Rectangle testRect =
>> 132: new Rectangle(loc.x, loc.y,
>> 133: test.getWidth(), test.getHeight());
>
> Suggestion:
>
> loc = test.getLocationOnScreen();
> Rectangle testRect = new Rectangle(test.getLocationOnScreen(),
> test.getSize());
>
> Choose one style and adjust both statements, `refRect` and `testRect`.
Updated to use same style for both places.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136:
>
>> 134: BufferedImage testimg =
>> robot.createScreenCapture(testRect);
>> 135:
>> 136: if (refimg.getWidth() != testimg.getWidth()
>
> I suggest moving the part that compares the images into a helper method.
> Alternatively, you can use
> [`Util.compareBufferedImages`](https://github.com/openjdk/jdk/blob/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L80).
Ok.. Updated
[Util.compareBufferedImages](https://github.com/openjdk/jdk/blob/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L80)
to compare images.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 137:
>
>> 135:
>> 136: if (refimg.getWidth() != testimg.getWidth()
>> 137: || refimg.getHeight() != testimg.getHeight())
>
> Suggestion:
>
> if (refimg.getWidth() != testimg.getWidth()
> || refimg.getHeight() != testimg.getHeight())
>
> One more space, it should be aligned to the inside of the parenthesis.
Used Util to compare images, so these lines are removed now.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 148:
>
>> 146: if (refPixel != testPixel) {
>> 147: fail("Image comparison failed at (" +
>> 148: x + "," + y + ") for image " + index);
>
> Adding `count` and `k` to diagnostic message could help identifying the
> failing case quicker.
Don't find adding `k` to diagnostic message useful.. failure message is more
relevant now about the images which are compared.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626152744
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626146072
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626148142
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626150340
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626150884
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626149792
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626152392
