On Fri, 18 Oct 2024 15:54:24 GMT, Naveen Narayanan <d...@openjdk.org> 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
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 70:
> 68: Graphics g = realImage.createGraphics();
> 69: g.setColor(Color.yellow);
> 70: g.fillRect(0, 0, 200, 100);
You may define `rect_width` and `rect_height` variable with 200 and 100
respectively.
These numbers are used more than once and you are trying to avoid magic numbers
for better practice.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 78:
> 76:
> 77: canvas = new ImageCanvas();
> 78: canvas.setBackground(Color.YELLOW);
Use either `Color.yellow` or `Color.YELLOW` for consistency.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 91:
> 89: robot = new Robot();
> 90: robot.delay(delay);
> 91: robot.waitForIdle();
I guess `robot.waitForIdle()` should be followed by `robot.delay(delay)`.
I agree with @TejeshR13 that you can get rid of `delay` variable, just used
once.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 93:
> 91: robot.waitForIdle();
> 92:
> 93: Point pnt = canvas.getLocationOnScreen();
should be accessed on EDT.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 102:
> 100: String errorMessage = "FAIL : Captured Image is different
> from "
> 101: + "the actual image";
> 102: System.err.println("Test failed");
I think you should save the images if image comparison fails, they can be used
for diagnostic purpose.
No need to define a `errorMessage` variable, can throw the error message
directly.
`System.err.println("Test failed");` look redundant to me.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 107:
> 105: System.out.println("\nCaptured Image is same as actual
> Image");
> 106: System.out.println("Test passed");
> 107: }
May not be required if test passes.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 111:
> 109:
> 110: private static boolean compareImages(BufferedImage capturedImg,
> 111: BufferedImage realImg) {
`realImg` is a class variable, not required to pass as an argument.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 119:
> 117:
> 118: imgWidth = capturedImg.getWidth(null);
> 119: imgHeight = capturedImg.getHeight(null);
These variables can be removed and `capturedImg.getWidth(null)` should replace
`imgWidth` in for-loop condition. same for `imgHeight` also.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 125:
> 123: capturedPixel = capturedImg.getRGB(i, j);
> 124: realPixel = realImg.getRGB(i, j);
> 125: if (capturedPixel != realPixel) {
To optimize further, `capturedImg.getRGB(i, j)` and `realImg.getRGB(i, j)` can
be directly evaluated inside if condition.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 127:
> 125: if (capturedPixel != realPixel) {
> 126: result = false;
> 127: return result;
can be replace by `return false`, no need of `result` variable then.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 131:
> 129: }
> 130: }
> 131: return result;
Suggestion:
return true;
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808081942
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808080720
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808084152
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808084265
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808085750
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808086184
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808086674
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808088631
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808089744
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808089131
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808089179
