[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [11] JDK-8196031: FX Robot mouseMove fails on Windows 10 1709 with HiDPI
From:       Kevin Rushforth <kevin.rushforth () oracle ! com>
Date:       2018-06-11 23:26:46
Message-ID: f40ebe69-9bba-e234-d608-b31db068bbfd () oracle ! com
[Download RAW message or body]

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@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
> > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]