On Fri, 25 Oct 2024 15:01:49 GMT, Naveen Narayanan <[email protected]> wrote:
>> This testcase checks for the following:
>>
>> 1. An image is drawn on the screen and a portion of it is captured using a
>> Robot. It is tested by comparing whether the captured image is same as the
>> source image.
>>
>> Testing:
>> Tested using Mach5(20 times per platform) in MacOS, Linux and Windows. Seen
>> all tests passing.
>
> Naveen Narayanan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8342098: Updated review comments
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 70:
> 68: realImage = GraphicsEnvironment.getLocalGraphicsEnvironment()
> 69: .getDefaultScreenDevice().getDefaultConfiguration()
> 70: .createCompatibleImage(IMAGE_WIDTH, IMAGE_HEIGHT);
Suggestion:
realImage = GraphicsEnvironment.getLocalGraphicsEnvironment()
.getDefaultScreenDevice()
.getDefaultConfiguration()
.createCompatibleImage(IMAGE_WIDTH, IMAGE_HEIGHT);
Don't hide a call and wrap at each chained call — it's easier to track what's
happening.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 81:
> 79:
> 80: canvas = new ImageCanvas();
> 81: canvas.setBackground(Color.YELLOW);
I recommend setting the preferred size of the canvas instead of setting a size
of the frame:
canvas.setPreferredSize(new Dimension(IMAGE_WIDTH + OFFSET * 2,
IMAGE_HEIGHT + OFFSET * 2));
By using `OFFSET * 2` you add a margin on either side of the image on the
canvas.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 84:
> 82: frame.setLayout(new BorderLayout());
> 83: frame.add(canvas);
> 84: frame.setSize(300, 200);
Suggestion:
frame.pack();
If you set the preferred size for the canvas, just pack the frame.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 99:
> 97:
> 98: Rectangle rect = new Rectangle(point.x + 10, point.y + 10,
> IMAGE_WIDTH,
> 99: IMAGE_HEIGHT);
Suggestion:
Rectangle rect = new Rectangle(point.x + OFFSET, point.y + OFFSET,
IMAGE_WIDTH, IMAGE_HEIGHT);
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 106:
> 104: String errorMessage = "FAIL : Captured Image is different
> from "
> 105: + "the real image";
> 106: System.err.println("Test failed");
I agree with @kumarabhi006, `"Test failed"` could be redundant.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 118:
> 116: int realPixel;
> 117: int imgWidth = capturedImg.getWidth(null);
> 118: int imgHeight = capturedImg.getHeight(null);
Suggestion:
int imgWidth = capturedImg.getWidth();
int imgHeight = capturedImg.getHeight();
Both images are of type `BufferedImage` which has `getWidth()` and
`getHeight()` without parameters.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 133:
> 131: System.out.println("Captured pixel ("
> 132: + Integer.toHexString(capturedPixel) + ") at
> (" + i
> 133: + ", " + j + ") is not equal to real pixel ("
Suggestion:
+ Integer.toHexString(capturedPixel) + ") at "
+ "(" + i + ", " + j + ") is not equal to real
pixel ("
Keeping the coordinates close in the code improves readability, doesn't it?
I'd also keep the opening and closing parentheses for pixels on the same line.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 145:
> 143: @Override
> 144: public void paint(Graphics g) {
> 145: g.drawImage(realImage, 10, 10, this);
Suggestion:
g.drawImage(realImage, OFFSET, OFFSET, this);
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 150:
> 148:
> 149: private static void saveImage(BufferedImage image, String fileName) {
> 150: // Save BufferedImage to PNG file
Suggestion:
I think the comment is redundant.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 155:
> 153: System.out.println("Saving image : " + image + " to \n"
> 154: + file.getAbsolutePath());
> 155: ImageIO.write(image, "PNG", file);
Suggestion:
ImageIO.write(image, "png", new File(fileName));
The body can be shortened.
(Usually, the format is in lower case; that's what I've seen the most in tests
which save images.)
Does printing the format of the image and the absolute path help? I see no
value.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 158:
> 156: } catch (IOException ioe) {
> 157: throw new RuntimeException(
> 158: "Image save failed : " + ioe.getMessage());
I would refrain from throwing this exception: it is not important, it is not
the reason why the test fails. If you throw this exception here, you'll hide
the real reason, that is that _the images are different_.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 165:
> 163: if (frame != null) {
> 164: frame.dispose();
> 165: frame = null;
Assigning `null` is not necessary.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21524#pullrequestreview-2405211906
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822806853
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822814897
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822816416
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822818768
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822822354
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822826053
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822836347
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822837524
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822839304
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822845059
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822849256
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822850523
