On Fri, 28 Mar 2025 06:00:51 GMT, Renjith Kannath Pariyangad
<rkannathp...@openjdk.org> wrote:
> Hi Reviewers,
>
> Added screen capture in case of test failure using Robot.
>
> Please review and let me know your suggestion if any.
Just a few minor questions.
Also, it seems that the screenshot code is repeated all over the place.
Wouldn't it make sense to make a utility/add it to an existing utility class
for further use to standardise it and make it simpler to maintain in time? I
know this is well beyond the scope of this ticket, but just thought to mention
the idea 😃
BufferedImage image = robot.createScreenCapture(bounds);
try {
ImageIO.write(image,"png", new File("Screenshot.png"));
} catch (IOException e) {
...
test/jdk/javax/swing/Popup/TaskbarPositionTest.java line 78:
> 76: public class TaskbarPositionTest implements ActionListener {
> 77:
> 78: private static JFrame frame;
Nit: I don't think `* @run main TaskbarPositionTest` is needed. But it's fine
as it is too, don't have any strong feelings
test/jdk/javax/swing/Popup/TaskbarPositionTest.java line 78:
> 76: public class TaskbarPositionTest implements ActionListener {
> 77:
> 78: private static JFrame frame;
Nit: copyright year
test/jdk/javax/swing/Popup/TaskbarPositionTest.java line 81:
> 79: private static JPopupMenu popupMenu;
> 80: private static JPanel panel;
> 81: private static Robot robot;
Do you think it would be better to have Robot initialisation in this way? IMO
it will make the robot final, so there is no way to overwrite it in the future
as well as it would be easier to debug and not be initialised in the main
method of the class.
private static final Robot robot;
static {
try {
robot = new Robot();
} catch (AWTException e) {
throw new RuntimeException(e);
}
}
test/jdk/javax/swing/Popup/TaskbarPositionTest.java line 223:
> 221: ImageIO.write(image,"png", new File("Screenshot.png"));
> 222: } catch (IOException e) {
> 223: e.printStackTrace();
I'm not sure it's the best idea to hide the error and just print it into
stdout. Wouldn't it be better to just throw the error, what do you think?
-------------
PR Review: https://git.openjdk.org/jdk/pull/24286#pullrequestreview-2726076897
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2018993990
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2018994882
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2018981226
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2018984420
