RFR: 8294535 : Add screen capture functionality to PassFailJFrame

[Alexey Ivanov]

On Tue, 23 May 2023 02:52:03 GMT, lawrence.andrews <duke at openjdk.org> wrote:

> 1) Used builder pattern 
> 2) Tested AWT tests and it passed

I really like your idea with *builder pattern* for creating `PassFailJFrame`. Well done!

Is the purpose of screenshot in `PrintLatinCJKTest` to capture how the printed document looks in a PDF viewer?

@honkar-jdk could you take a look? You've been involved in early development and updating of the manual testing framework, and you've used it extensively.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 271:

> 269:             } catch (AWTException e) {
> 270:                 String errorMsg = "Failed to create the " +
> 271:                         "instance of Robot.";

Suggestion:

                String errorMsg = "Failed to create an " +
                        "instance of Robot.";

Maybe make it a constant?

Or wrap the entire string to avoid breaking it?

Suggestion:

                String errorMsg =
                        "Failed to create an instance of Robot.";

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 281:

> 279:         List<Image> imageList =
> 280:                 multiResolutionImage.getResolutionVariants();
> 281:         Image image = imageList.get(imageList.size() - 1);

Suggestion:

        MultiResolutionImage mri = robot.createMultiResolutionScreenCapture(bounds);
        List<Image> imageList = mri.getResolutionVariants();
        Image image = imageList.get(imageList.size() - 1);

`mri` is a common abbreviation of `MultiResolutionImage`, it's much shorter.

Alternatively, you may inline the two calls:
Suggestion:

        List<Image> imageList = robot.createMultiResolutionScreenCapture(bounds)
                                     .getResolutionVariants();
        Image image = imageList.get(imageList.size() - 1);

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 287:

> 285:             ImageIO.write((RenderedImage) image, "png", file);
> 286:         } catch (IOException e) {
> 287:             throw new RuntimeException(e.getMessage());

You shouldn't throw away the original exception, it may be useful in diagnosing the problem.
Suggestion:

            throw new RuntimeException(e);


Show message to the user? Or, add a try-catch inside `captureScreen` to show this message.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 297:

> 295:                     .map(GraphicsDevice::getDefaultConfiguration)
> 296:                     .map(GraphicsConfiguration::getBounds)
> 297:                     .forEach(PassFailJFrame::captureScreen);

Suggestion:

            Arrays.stream(GraphicsEnvironment.getLocalGraphicsEnvironment()
                                             .getScreenDevices())
                  .map(GraphicsDevice::getDefaultConfiguration)
                  .map(GraphicsConfiguration::getBounds)
                  .forEach(PassFailJFrame::captureScreen);

Align the dots of the chained calls, which is especially important for `getScreenDevices` where it's called on the result of `getLocalGraphicsEnvironment()` so that the stream is created on `getScreenDevices`.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 304:

> 302:                     .filter(Window::isShowing)
> 303:                     .map(Window::getBounds)
> 304:                     .forEach(PassFailJFrame::captureScreen);

Suggestion:

            windowList.stream()
                      .filter(Window::isShowing)
                      .map(Window::getBounds)
                      .forEach(PassFailJFrame::captureScreen);

Align the dots for wrapped calls.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 306:

> 304:                     .forEach(PassFailJFrame::captureScreen);
> 305:         }
> 306: 

Maybe use `switch` statement with a `default` case which throws an exception rather than shows success message. (`if else if else` will also do.)

Yeah, it's a private method yet it's better to handle an unexpected value.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 309:

> 307:         JOptionPane.showMessageDialog(frame, "Screen Captured " +
> 308:                         "Successfully", "Screen Capture",
> 309:                 JOptionPane.INFORMATION_MESSAGE);

Suggestion:

        JOptionPane.showMessageDialog(frame,
                "Screen captured successfully",
                "Screen Capture",
                JOptionPane.INFORMATION_MESSAGE);

It's better not to split strings unnecessarily. The messages usually use regular capitalisation as opposed to title capitalisation.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 601:

> 599:             if (this.instructions == null || this.instructions.length() == 0) {
> 600:                 throw new RuntimeException("Please provide the test " +
> 601:                         "instruction for this manual test");

Is `IllegalStateException` or `IllegalArgumentException` better than a generic `RuntimeException`?

-------------

Changes requested by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14094#pullrequestreview-1442592961
PR Comment: https://git.openjdk.org/jdk/pull/14094#issuecomment-1561829381
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204655826
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204659400
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204662983
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204665042
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204665704
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204667803
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204669333
PR Review Comment: https://git.openjdk.org/jdk/pull/14094#discussion_r1204670709