Looks good.
+1
-- Kevin
On 6/11/2018 1:48 AM, Pankaj Bansal wrote:
Hello Kevin,
Thanks for the review.
I have incorporated all the review comments. Please have a look.
webrev: http://cr.openjdk.java.net/~pbansal/8196031/webrev.01/
Regards,
Pankaj Bansal
-----Original Message-----
From: Kevin Rushforth
Sent: Saturday, June 9, 2018 1:15 AM
To: Pankaj Bansal; [email protected]
Subject: Re: [11] JDK-8196031: FX Robot mouseMove fails on Windows 10 1709 with
HiDPI
The fix works for me, but I have a couple comment on the implementation and a
couple more on the test:
1. You borrowed the following from the Java2D fix:
+ signum((int)fx);
+ signum((int)fy);
This effectively does a rounding in the normalized absolute coordinate space
use by the Windows mouse move request. Given that we already round the values
in the unscaled (FX) space before the conversion, I don't think this is
necessary or desirable. I recommend removing it (and you can remove the
function entirely, since it was added just for this).
2. One other difference between this and the 2D fix is that the FX fix uses
65535 (which was in my initial prototype), whereas the 2D fix uses 65536. The
latter is probably more accurate, so you might want to change it to 65536.
Test comments:
3. I recommend using Util.runAndWait rather than runLater with your own latch everywhere.
That method also propagates exceptions so you can do an assertEquals right in that block
rather than saving a "mismatch"
flag. You will get a better error message that way since you will see the
actual and expected values in the assertions rather than having to look at the
log (or run with --info) to see them.
4. Since you are using a for loop with increment by 1 in cross and edge, it might be
better to use "int"s for those params after all (sorry for leading you astray
on this one earlier).
5. Minor: we usually use 20 seconds for test timeouts, and this is a
short-running test, so maybe switch to that? (it's up to you)
-- Kevin
On 6/7/2018 12:57 PM, Pankaj Bansal wrote:
Hi Kevin & Murali,
Please review this fix,
Webrev:
http://cr.openjdk.java.net/~pbansal/8196031/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8196031
Regards,
Pankaj
