On Wed, 30 Oct 2024 14:53:43 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> 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: 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.
What about this suggestion? Do you disagree?
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 122:
>
>> 120: if (imgWidth != IMAGE_WIDTH || imgHeight != IMAGE_HEIGHT) {
>> 121: System.out
>> 122: .println("Captured and real images are different in
>> size");
>
> Suggestion:
>
> System.out.println("Captured and real images are different in
> size");
>
> Please, don't wrap calls to `println` like this, instead wrap the message if
> required.
>
> In this case, wrapping is unnecessary. The line is 81-column long, wrapping
> it doesn't improve the code readability, quite the opposite.
I'm for changing this and getting rid of wrapping here.
> 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_.
You still throw `IOException`… without wrapping it into `RuntimeException`.
The suggestion was to ignore `IOException`, maybe printing diagnostic message
to `System.err`:
private static void saveImage(BufferedImage image, String fileName) {
try {
ImageIO.write(image, "png", new File(fileName));
} catch (IOException ignored) {
}
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1838473018
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1838470504
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1838480434
