On Fri, 24 Jan 2025 00:25:14 GMT, Alisen Chung <ach...@openjdk.org> wrote:
>> Currently on macOS when mouseMove is given an offscreen coordinate to move
>> the mouse to, mouseMove will physically clamp to the edge of the screen, but
>> if you try to grab the mouse location immediately after by using
>> MouseInfo.getPointerInfo().getLocation() you will get the value of the
>> offscreen point.
>>
>> Windows and linux do this clamping and coordinate handling for us, but new
>> distributions may not necessarily handle clamping the same way, so Robot
>> should be checking for clamping rather than delegating it to native.
>>
>> This fix updates shared code to cache the screen bounds and adds a check to
>> not exceed the bounds in mouseMove. The caching is done in the Robot
>> constructor, so if the screen bounds changes the constructor must be called
>> again to update to the new bounds.
>
> Alisen Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> init finX, finY
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/Robot.java line 1:
> 1: /*
Revert all the unrelated formatting changes.
src/java.desktop/share/classes/java/awt/Robot.java line 109:
> 107: *
> 108: * @author Robi Khan
> 109: * @since 1.3
Do we have to modify these lines?
src/java.desktop/share/classes/java/awt/Robot.java line 133:
> 131: init(GraphicsEnvironment.getLocalGraphicsEnvironment()
> 132: .getDefaultScreenDevice());
> 133: }
Nothing's changed here… These look as unrelated to the problem that's being
addressed.
src/java.desktop/share/classes/java/awt/Robot.java line 157:
> 155: * GraphicsDevice.
> 156: * @see java.awt.GraphicsEnvironment#isHeadless
> 157: * @see GraphicsDevice
It's exactly the same here: the formatting is changed, yet nothing's touched in
this method.
You should avoid changing or reformatting lines of code which you don't touch…
unless the purpose of a PR is to update formatting and perform some clean-ups.
src/java.desktop/share/classes/java/awt/Robot.java line 175:
> 173: for (int i = 0; i < gs.length; i++) {
> 174: allScreenBounds[i] =
> gs[i].getDefaultConfiguration().getBounds();
> 175: }
This doesn't look right to me… `init` is an instance method, yet it initialises
`allScreenBounds` which is shared among all instances.
In a way it makes sense to share the graphics environment / configuration. On
the other hand how much would it affect the operation of robot if the bounds
are fetched from `GraphicsEnvironment` only when needed?
What happens if the graphics configuration changes? Does the user need to
create a new instance of the `Robot` class?
src/java.desktop/share/classes/java/awt/Robot.java line 234:
> 232: for (Rectangle screenBounds : allScreenBounds) {
> 233: int closestX = Math.min(Math.max(x, screenBounds.x),
> screenBounds.x + screenBounds.width);
> 234: int closestY = Math.min(Math.max(y, screenBounds.y),
> screenBounds.y + screenBounds.height);
Shouldn't the width and height have `- 1`? I mean if the screen resolution is
1920×1080, the highest valid coordinates are 1919 and 1079 for <var>x</var> and
<var>y</var> correspondingly.
src/java.desktop/share/classes/java/awt/Robot.java line 253:
> 251: leastXDiff = currXDiff;
> 252: leastYDiff = currYDiff;
> 253: }
Both branches must be executed at the same time — you have adjust both
<var>x</var> and <var>y</var> if the diff isn't 0.
Probably, min/max would work better here too.
test/jdk/java/awt/Robot/MouseMoveOffScreen.java line 2:
> 1: /*
> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
Suggestion:
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
It hasn't been integrated in 2024, so the year needs bumping up to 2025.
test/jdk/java/awt/Robot/MouseMoveOffScreen.java line 44:
> 42: robot.mouseMove(STARTING_LOC.x, STARTING_LOC.y);
> 43: robot.delay(500);
> 44: robot.mouseMove(OFF_SCREEN_LOC.x, OFF_SCREEN_LOC.y);
You should validate to ensure your `OFF_SCREEN_LOC` is actually off screen.
test/jdk/java/awt/Robot/MouseMoveOffScreen.java line 50:
> 48: throw new RuntimeException("Test Failed, getLocation returned
> null.");
> 49: }
> 50: Point currLoc = MouseInfo.getPointerInfo().getLocation();
Suggestion:
Point currLoc = MouseInfo.getPointerInfo().getLocation();
if (currLoc == null) {
throw new RuntimeException("Test Failed, getLocation returned
null.");
}
You could save the the mouse location right away.
-------------
PR Review: https://git.openjdk.org/jdk/pull/22781#pullrequestreview-2573308137
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929109081
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929070058
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929071748
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929073747
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929100875
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929103712
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929107458
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929110307
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929111847
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929113298
