8150564: Migrate useful ExtendedRobot methods into awt.Robot #26969
Conversation
|
👋 Welcome back dnguyen! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
Since you are handling the tests in a separate PR, should the JBS issue be labeled with |
| public void click(int buttons) { | ||
| mousePress(buttons); | ||
| waitForIdle(20); | ||
| mouseRelease(buttons); |
There was a problem hiding this comment.
It would be good to call ***release methods in the finally block.
There was a problem hiding this comment.
Is this necessary? Would I have to account for accidentally releasing any buttons that should still be pressed by calling mouseRelease(buttons) in a finally block if some exception is thrown during mousePress?
|
|
||
| /** | ||
| * A convenience method that simulates clicking a mouse button by calling {@code mousePress}, {@code mouseRelease}, | ||
| * and {@code waitForIdle}. Invokes {@code waitForIdle} with a default delay of 20 milliseconds after |
There was a problem hiding this comment.
| * and {@code waitForIdle}. Invokes {@code waitForIdle} with a default delay of 20 milliseconds after | |
| * and {@code waitForIdle}. Invokes {@code waitForIdle} with a default delay of {@value #DEFAULT_STEP_DELAY} milliseconds after |
This renders 20 as a link that links to the constant that users can use.
| * steps from its current location to the destination coordinates. | ||
| * | ||
| * @implSpec Invokes {@link #mouseMove(int, int) mouseMove} with a default | ||
| * {@link #DEFAULT_STEP_LENGTH step-length} and {@link #DEFAULT_STEP_DELAY step-delay}. |
There was a problem hiding this comment.
| */ | ||
| public synchronized void type(int keycode) { | ||
| keyPress(keycode); | ||
| waitForIdle(20); |
There was a problem hiding this comment.
Should this use DEFAULT_STEP_DELAY too?
There was a problem hiding this comment.
In the code this is based on (ExtendedRobot.java) it was called DEFAULT_SPEED and used by click and type as well as glide
As 'named' now, DEFAULT_STEP_DELAY is only appropriate for glide(..). There's no "stepping" here.
And yet click(..) uses it (internally)
so long as we don't document the name DEFAULT_STEP_DELAY on click() or type() it isn't critical.
I see a number of choices
- Use literal 20 here and in click
- Use DEFAULT_STEP_DELAY here and in click - do not document
- Add a new (private) DEFAULT_DELAY - and use it in both cases
- Add a new public DEFAULT_DELAY - and use it in both cases and document it
- Rename DEFAULT_STEP_DELAY to DEFAULT_DELAY - and use it in click and type as well as glide
I'm inclined to go with the last of these - CSR will need to be revised.
But if we don't do that one now, it will be awkward to do later.
Thoughts ?
PS I can see the potential need for an overload of click() and type() which accepts an alternate delay but I don't propose it here, I just note that for click() it would not be possible for the default click() to just accept a delay since that would clash with click(int buttons).
There was a problem hiding this comment.
I recommend revising the CSR. Since the CSR already has reviewers, once it is updated, it can be directly finalized for re-approval. Just describe the changes in a comment, and it should proceed smoothly.
Some useful methods (click, glide, waitForIdle, type) in ExtendedRobot should be migrated into Robot itself so that ExtendedRobot can be removed in the future. The tests using these ExtendedRobot methods will be handled separately.
Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26969/head:pull/26969$ git checkout pull/26969Update a local copy of the PR:
$ git checkout pull/26969$ git pull https://git.openjdk.org/jdk.git pull/26969/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26969View PR using the GUI difftool:
$ git pr show -t 26969Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26969.diff
Using Webrev
Link to Webrev Comment