[11] JDK-8196031: FX Robot mouseMove fails on Windows 10 1709 with HiDPI

[Kevin Rushforth]

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; openjfx-dev at openjdk.java.net
> 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
>>
>>