On Fri, 18 Oct 2024 15:54:24 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 1: > 1: /* Does this test has a value at all? Does this test verifies the functionality of the `Robot` class? Many other tests rely on `Robot.createScreenCapture` to capture part of the screen. If `Robot` captures something unexpected, all such tests should fail. test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 43: > 41: * @summary Verify that the image captured from the screen using a Robot > 42: * and the source image are same. > 43: * @run main ScreenCaptureRobotTest Suggestion: * @run main/othervm -Dsun.java2d.uiScale=1 ScreenCaptureRobotTest Otherwise the test would fail if run in a High DPI environment. [Phil mentioned it](https://github.com/openjdk/jdk/pull/21524#discussion_r1805263931). test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 69: > 67: > 68: Graphics g = realImage.createGraphics(); > 69: g.setColor(Color.yellow); Please use upper-case color constants. test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 72: > 70: g.fillRect(0, 0, 200, 100); > 71: g.setColor(Color.red); > 72: g.setFont(new Font("SansSerif", Font.BOLD, 20)); Suggestion: g.setFont(new Font(Font.SANS_SERIF, Font.BOLD, 20)); Use the constant for the logical font. test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 82: > 80: frame.add(canvas); > 81: frame.setSize(300, 200); > 82: frame.setLocation(100, 100); Is it important? I'd rather put the frame into the centre of the screen as most new tests do. test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 84: > 82: frame.setLocation(100, 100); > 83: frame.setVisible(true); > 84: canvas.requestFocus(); Is `canvas` focusable? I guess it's not, then this statement doesn't make sense. test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 94: > 92: > 93: Point pnt = canvas.getLocationOnScreen(); > 94: Rectangle rect = new Rectangle(pnt.x + 10, pnt.y + 10, 200, 100); Why are coordinates of the start of the canvas are offset by 10 pixels but the size isn't. The image is 200×100, then you capture 10 pixels on the right and bottom which belong to whatever is on the screen in this area. test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 137: > 135: @Override > 136: public void paint(Graphics g) { > 137: g.drawImage(displayImage, 10, 10, this); You can use `realImage` here. Both fields point to the same object anyway. ------------- PR Review: https://git.openjdk.org/jdk/pull/21524#pullrequestreview-2382115196 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808870689 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808874757 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808826430 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808829862 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808832519 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808833268 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808839354 PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808862076
