On Tue, 1 Apr 2025 12:02:49 GMT, Renjith Kannath Pariyangad
<rkannathp...@openjdk.org> wrote:
>> 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);
>> }
>> }
>
> Since its a test, I didn't expect any override of this class in future, my
> intention is add this feature with minimal change.
I agree with Renjith.
If you call `saveScreenCapture` once in the `main` method as I suggest, you can
pass `robot` to `saveScreenCapture`, in which case `robot` can remain local
variable.
>> 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?
>
> I don't think its a good idea
> 1. IOException was not the original intention of this test and its only for
> additional information
> 2. If we throw the error we will miss the actual exception, potentially that
> was the next instruction.
Yes, we don't care about `IOException`, it can occur only after the test
already failed, and it's important to preserve the original exception.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2026772299
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2026769752
