-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8150564: Migrate useful ExtendedRobot methods into awt.Robot #22044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back achung! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@alisenchung this pull request can not be integrated into git checkout 8150564
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
@alisenchung The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
Please resolve the merge conflict. |
/csr |
/reviewers reviewers 2 |
@prrace has indicated that a compatibility and specification (CSR) request is needed for this pull request. @alisenchung please create a CSR request for issue JDK-8150564 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
@prrace Usage: |
/reviewers 2 reviewers |
@alisenchung This pull request is now open |
@@ -126,6 +126,17 @@ public class Robot { | |||
|
|||
private DirectColorModel screenCapCM = null; | |||
|
|||
/** | |||
* Default 20 milliseconds delay for mouse {@code click} and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Default 20 milliseconds delay for mouse {@code click} and | |
* Default delay for mouse {@code click} and |
There is no need to specify the exact value in the documentation, in case of something it will be much easier to change it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be aware that values of static fields are automatically documented.
eg see
(https://docs.oracle.com/en/java/javase/21/docs/api/constant-values.html#java.awt.Font.BOLD)
And an app really could use knowing what the default is.
And these defaults have been battle-tested in ExtendedRobot
public static final int DEFAULT_DELAY = 20; | ||
|
||
/** | ||
* Default 2 pixel step length for mouse {@code glide}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Default 2 pixel step length for mouse {@code glide}. | |
* Default pixel step length for mouse {@code glide}. |
Same here
* A convenience method that simulates clicking a mouse button by calling {@code mousePress} | ||
* and {@code mouseRelease}. Invokes {@code waitForIdle} with a default {@link #DEFAULT_DELAY delay} after | ||
* {@code mousePress} and {@code mouseRelease} calls. For specifics on valid inputs please see | ||
* {@link java.awt.Robot#mousePress(int)}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's discussable, and I may be wrong, but I'm not a fan of documentation that is very specific about its implementation.
I prefer the one that was before in the ExtendedRobot
.
Clicks mouse button(s) by calling {@link #mousePress(int)} and {@link #mouseRelease(int)} methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the concern but in this case it is more or less explanation of what the method does. Perhaps it can be adjusted or made in to an apiNote or whatever is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this method doesn't call waitForIdle there's no reason to document here that you'll get IllegalThreadStateException if called on the EDT, and waitForIdle is the sole source of ITSE.
So since we are obliged to document the IllegalThreadStateException I don't think it does any harm to say it calls waitForIdle.
*/ | ||
@Override | ||
public synchronized void waitForIdle() { | ||
waitForIdle(syncDelay); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This waitForIdle(500)
is no longer called by tests(as it uses regular java.awt.Robot#waitForIdle()
, so I assume you have verified that automated testing looks good. (I haven't gone through all the tests yet)
/** | ||
* Default 2 pixel step length for mouse {@code glide}. | ||
*/ | ||
public static final int DEFAULT_STEP_LENGTH = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to make the DEFAULT_DELAY
and DEFAULT_STEP_LENGTH
configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"glide has over-rides step length"
Sorry, that text was not clear. What I mean is that glide(..) accepts delay and step parameters, and so at least in that case you can over-ride with your preferred values.
Other than that, it isn't configurable in the ExtendedRobot, so the need isn't clear.
Even so, we could do this but whether now or later, but in either case,
we ought to think now as to what it would look like so it can be compatibly added.
In ExtendedRobot the equivalent fields are private but here they are public.
Tests might start to use them directly. I'm not sure what for, but they might.
But if it is configurable tests probably should instead call a getter() for the currently set delay/step.
Then all the methods that now mention DEFAULT_ would probably need to mention something like
getMouseDelay() and getStepLength() instead.
So it may be better to do it now ? Thoughts ?
stepNum++) { | ||
x += tDx; | ||
y += tDy; | ||
mouseMove((int)x, (int)y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mouseMove
can throw an IllegalThreadStateException
under certain circumstances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes should be added to all glide API methods
* @see #glide(int, int, int, int, int, int) | ||
* @since 25 | ||
*/ | ||
public void glide(int x, int y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the public void glide(Point dest)
and public void glide(Point src, Point dest)
added, it may be convenient in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a look at all the tests running glide and very few of them actually used glide(Point dest) and glide(Point src, Point dest) so I decided to remove them from them from the migration
@@ -48,7 +49,6 @@ | |||
@summary Test Component.paintAll() method | |||
@author [email protected]: area=awt.component | |||
@library /lib/client/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed anymore, is it?
@library /lib/client/ |
* @see #type(int) | ||
* @see java.awt.event.KeyEvent | ||
*/ | ||
public void type(char c) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those type(char...
are not migrated also.
At least one test uses it:
With your change, it now calls type(int)
directly, which has a different implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've now also migrated type(int)
@alisenchung This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@alisenchung This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open |
@alisenchung This pull request is now open |
@alisenchung This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see couple of tests that use ExtendedRobot which can be changed to use Robot. Can you double check these tests.
java/awt/Frame/DisposeParentGC/DisposeParentGC.java
java/awt/List/SetBackgroundTest/SetBackgroundTest.java
java/awt/Modal/ToBack/ToBackFDFTest.java
public void glide(Point src, Point dest) { | ||
glide(src.x, src.y, dest.x, dest.y, DEFAULT_STEP_LENGTH, DEFAULT_SPEED); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't glide(Point src, Point dest)
and glide(Point dest)
be retained in ExtendedRobot
In previous comment it was mentioned that these methods won't be migrated so do they need to be retained in ExtendedRobot ?
I see MultipleMouseButtonsTest.java
using glide(Point src, Point dest)
. There are two options here:
-
Either update the test to use Robot's glide()
change the linerobot.glide(origin, center)
torobot.glide(origin.x, origin.y, center.x, center.y);
and completely remove ExtendedRobot versions of glide. -
Or retain these two convenience methods in ExtendedRobot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few other tests that use ExtendedRobot version of glide() so probably option 2 (retaining the two convenience methods in ExtendedRobot) might be a better option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally ER will be removed in the future so depending on the number of tests using glide(point), I will either change the tests or readd glide(point) to Robot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are couple of tests failing due to compilation error. I have added the list to JBS, please take a look.
@@ -61,7 +61,7 @@ | |||
public class TrayIconPopupTest { | |||
|
|||
TrayIcon icon; | |||
ExtendedRobot robot; | |||
Robot robot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test still builds ExtendedRobot which is no longer needed. Applicable for other tests as well.
@library /lib/client
@build ExtendedRobot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are many test changes it is good to run CI on latest update.
@@ -53,7 +54,6 @@ | |||
* java.desktop/java.awt.Helper | |||
* jdk.test.lib.Platform | |||
* jtreg.SkippedException | |||
* ExtendedRobot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ExtendedRobot is removed /lib/client is no longer required in @library
tag. Applicable for other tests as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are couple of tests with /lib/client in @library
tag which is no longer required. This cleanup can be handled in subsequent PR.
* @library ../helpers /lib/client/ | ||
* @library /test/lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here /lib/client is no longer required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes LGTM.
@alisenchung
Does CI run look good with the latest changes?
* A convenience method that simulates clicking a mouse button by calling {@code mousePress} | ||
* and {@code mouseRelease}. Invokes {@code waitForIdle} with a default {@link #DEFAULT_DELAY delay} after | ||
* {@code mousePress} and {@code mouseRelease} calls. For specifics on valid inputs please see | ||
* {@link java.awt.Robot#mousePress(int)}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this method doesn't call waitForIdle there's no reason to document here that you'll get IllegalThreadStateException if called on the EDT, and waitForIdle is the sole source of ITSE.
So since we are obliged to document the IllegalThreadStateException I don't think it does any harm to say it calls waitForIdle.
Some useful methods in ExtendedRobot should be migrated into Robot itself so that ExtendedRobot can be removed in the future. The tests using ExtendedRobot for these migrated methods are changed to use only Robot (removing unnecessary building of ExtendedRobot).
Progress
Issues
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22044/head:pull/22044
$ git checkout pull/22044
Update a local copy of the PR:
$ git checkout pull/22044
$ git pull https://git.openjdk.org/jdk.git pull/22044/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22044
View PR using the GUI difftool:
$ git pr show -t 22044
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22044.diff
Using Webrev
Link to Webrev Comment