On Tue, 27 Aug 2024 18:24:26 GMT, Manukumar V S <m...@openjdk.org> wrote:
>> This PR creates a new test by stabilising and open sourcing a closed test.
>> This test verifies that the OpenGL pipeline does not create artifacts with
>> swing components after window is zoomed to maximum size and then resized
>> back to normal.
>> This test is run twice, with and without the flags "-Dsun.java2d.opengl=True
>> -Dsun.java2d.opengl.fbobject=false" .
>>
>> This is tested(15 times per platform) in all the available mach5 headful
>> platforms and found to be stable.
>
> Manukumar V S has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comment fixed : Added different @test tags for different platforms
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 2:
> 1: /*
> 2: * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights
> reserved.
I think the copyright year should be just 2024. Looks like the test is added in
the repo first time.
Suggestion:
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 29:
> 27: import javax.swing.SwingUtilities;
> 28: import java.awt.Color;
> 29: import java.awt.Dimension;
Please sort the imports. Usually `java.awt` imports are placed before
`javax.swing`.
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 53:
> 51: * resized back to normal. The test case simulates this operation using
> 52: * a JButton. A file image of the component will be saved before and
> after
> 53: * window resize. The test passes if both button images are the same.
Suggestion:
* resized back to normal. The test case simulates this operation using
* a JButton. A file image of the component will be saved before and after
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 99:
> 97: button = new JButton("Button A");
> 98: frame.setLocationRelativeTo(null);
> 99: frame.setLocation(200, 200);
Isn't it redundant to `setLocation` of frame after calling
`setLocationRelativeTo`?
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 133:
> 131: } else {
> 132: System.out.println("Button focus not gained...");
> 133: throw new RuntimeException("Can't gain focus on button
> even after waiting too long..");
Crosses the limit of 80 columns here and few more lines below. Please limit it
to 80 columns.
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 145:
> 143: if
> (frame.getToolkit().isFrameStateSupported(JFrame.MAXIMIZED_BOTH)) {
> 144: robot.waitForIdle();
> 145: //maximize frame from normal size
Suggestion:
// maximize frame from normal size
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 152:
> 150: if
> (frame.getToolkit().isFrameStateSupported(JFrame.NORMAL)) {
> 151: System.out.println("Frame is back to normal");
> 152: //resize from maximum size to normal
Suggestion:
// resize from maximum size to normal
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 159:
> 157: bimage2 = getButtonImage();
> 158: File file2 = new File("image2.png");
> 159: saveButtonImage(bimage2, file2);
We are saving these images even in the case where test passes. Generally, we
should save image only in case of test failure for debugging purpose.
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 168:
> 166: int numDiffPixels = di.getNumDiffPixels();
> 167: if (numDiffPixels != 0) {
> 168: throw new RuntimeException("Button renderings
> are different after window resize, num of Diff Pixels=" + numDiffPixels);
I think the `compare` method can be modified to return `true/false` based on
the nDiff value. If **nDiff is not equal to zero** which you are checking at
[L300](https://github.com/openjdk/jdk/blob/5694b6811e06793ddaf1984e909cc0e925d93047/test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java#L300)
then you can return `false`, evaluate the return value inside if condition and
throw RTE with the num of diff pixels value. If **nDiff is equal to zero** then
compare method should return `true` and test should pass.
Code snippet can be changed to something like this...
> System.out.println("Taking the diff of two images, image1 and image2");
> // results of image comparison
> if (!di.compare(bimage1, bimage2)) {
> throw new RuntimeException("Button renderings are
> different after window resize, num of Diff Pixels=" + di.getNumDiffPixels());
}
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 181:
> 179: }
> 180: } finally {
> 181: disposeFrame();
should be disposed on EDT.
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 185:
> 183: }
> 184:
> 185: //Capture button rendering as a BufferedImage
Suggestion:
// Capture button rendering as a BufferedImage
test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 203:
> 201: }
> 202:
> 203: //Save BufferedImage to PNG file
Suggestion:
// Save BufferedImage to PNG file
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733987202
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733967856
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733970039
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733971416
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733990904
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733988634
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733988706
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733997751
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1734007708
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733992703
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733991797
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733992309
