Re: Proposal For Inclusion of Robot and ParametersImpl in the Public API

[Michael Ennen]

Okay, thanks for that review, Kevin.

I will take into account all of the your suggestions (and if any others
come in as well)
and report back soon.

Thanks,
Michael

On Tue, Mar 20, 2018 at 4:28 PM, Kevin Rushforth  wrote:

> Hi Michael,
>
> Here is some quick feedback.
>
> I think what you have is heading in the right direction as far as the
> public API goes. I'd like to get some feedback from other developers as
> well. I would want to make sure that the API meets the needs of multiple
> developers.
>
> I took a look at the public class and as I may have mentioned earlier, it
> will help to split the API and the implementation even further, by creating
> a peer object as we do for Scene, Stage, etc., rather than having the glass
> platform implementation directly subclass the public Robot class.
>
> Then you can easily delegate to the Glass Robot peer without having any
> implementation leak into the public API (e.g., the overridden create and
> dispose methods).
>
> So what you would have in that case is something more like this:
>
> public final class Robot {
>
> private final GlassRobot peer;
>
> public Robot() {
> // Ensure we have proper permission for creating a robot.
> final SecurityManager sm = System.getSecurityManager();
> if (sm != null) {
> sm.checkPermission(CREATE_ROBOT_PERMISSION);
> }
>
> Application.checkEventThread();
>
> peer = Toolkit.createRobot();
> }
>
> // NOTE: for the rest, the peer can do the thread check
>
> public void keyPress(KeyCode keyCode) {
> peer.keyPress(keyCode);
> }
>
> public void keyRelease(KeyCode keyCode) {
> peer.keyRelease(keyCode);
> }
>
> public final void keyType(KeyCode keyCode) {
> keyPress(keyCode);
> keyRelease(keyCode);
> }
>
> ...
>
>
> And so on. The Toolkit class can return a glass robot "peer" which should
> then only need minor changes  (e.g., to the signatures of methods that have
> slightly different types) from the current one. The QuantumToolkit
> implementation of createPeer can construct, initialize, and return the
> GlassRobot instance. The StubToolkit and DummyToolkit implementations can
> throw an UnsuppportedOperationException.
>
> As for the public API, the set of methods you have seem mostly what we
> would want. Here are a few quick thoughts:
>
> void keyPress(KeyCode keyCode);
> void keyRelease(KeyCode keyCode);
> void keyType(KeyCode keyCode);
>
> int getMouseX(); // maybe should return double?
> int getMouseY();
>
> Point2D getMousePosition();
>
> void mouseMove(int x, int y);   // maybe params should be double?
>
> void mouseMove(Point2D location);
>
> void mousePress(MouseButton button);// This one seems
> redundant...covered by the next method
> void mousePress(MouseButton... buttons);
>
> void mouseRelease(MouseButton button);  // This one seems
> redundant...covered by the next method
> void mouseRelease(MouseButton... buttons);
>
> // I don't see the need for this method
> void mouseWheel(int wheelAmt, VerticalDirection direction);
>
> void mouseWheel(int wheelAmt);
>
> Color getPixelColor(int x, int y);  // maybe params should be double?
>
> Color getPixelColor(Point2D location);
>
> // Probably the following should return WritableImage to match Snapshot.
> maybe also have a variable that takes a WritableImage to allow caller to
> allocate and reuse (that might overkill in this case)?
>
> Image getScreenCapture(int x, int y, int width, int height); // maybe
> params should be double?
> Image getScreenCapture(int x, int y, int width, int height, boolean
> scaleToFit); // maybe params should be double?
> Image getScreenCapture(Rectangle2D region);
> Image getScreenCapture(Rectangle2D region, boolean scaleToFit);
>
> void getScreenCapture(int x, int y, int width, int height, int[] data); //
> Not sure we need / want this one
>
> The biggest question I have is whether we should follow the pattern of the
> other related public APIs in Screen and Stage and use doubles everywhere
> rather than ints. Also, we will need to see whether there are any
> implications for multiple screens (probably not, but the docs will need to
> specify what coordinates the x, and y values are in).
>
> Hopefully this will spark a discussion.
>
>
> -- Kevin
>
>
> Michael Ennen wrote:
>
> Ping :)
>
> On Mon, Jan 8, 2018 at 4:28 PM, Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> I'll take a look some time after RDP2 of JDK 10.
>>
>>
>> -- Kevin
>>
>>
>> Michael Ennen wrote:
>>
>> Hey Kevin,
>>
>> Hope you had a good holiday. Hopefully you will get some time in the
>> coming weeks
>> to review my work.
>>
>> Thanks!
>>
>> On Wed, Dec 20, 2017 at 3:05 PM, Kevin Rushforth <
>> kevin.rushfo...@oracle.com> wrote:
>>
>>> Sure, no problem. One quick comment is that a common way to solve this
>>> is by delegating to an implementation class, which would then be
>>> 

Dependencies on java.desktop

[Tom Schindl]

Hi,

I always thought the JavaFX-Codebase should be able to run with just the
java.base module but I was browsing the codebase a bit and was suprised
(or rather shocked) that even the base-module requires java.desktop.

If I get it correct this because of the java.beans (provided by the
adapters) stuff is found in there. Why hasn't the requires there not
defined as:

requires static java.desktop;

Tom

Re: [11] Review request: JDK-8196617: FX print tests fail with NPE in some environments

[Johan Vos]

I checked this as well, and even without checking you can see why this
fixes the tests (on Windows, with the PseudoPrinter).
This is a very important commit, as it fixes the automated build on
Windows, which makes it much easier to make progress without regression.

- Johan

On Wed, Mar 21, 2018 at 6:32 PM Kevin Rushforth 
wrote:

> I'll defer to Phil on this review.
>
> I have no concerns over the fix, and can confirm that the failing tests
> now pass on my system.
>
> -- Kevin
>
>
> Laurent Bourgès wrote:
> > Kevin,
> >
> > Please review this webrev that fixes the NPE in getMediaName():
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8196617
> > webrev: http://cr.openjdk.java.net/~lbourges/fx/fx-8196617.0/
> > 
> >
> > It was tested OK on the appveyor CI that failed before the patch.
> >
> > Thanks,
> > Laurent
>

Re: [11] Review request: JDK-8196617: FX print tests fail with NPE in some environments

[Kevin Rushforth]

I'll defer to Phil on this review.

I have no concerns over the fix, and can confirm that the failing tests 
now pass on my system.


-- Kevin


Laurent Bourgès wrote:

Kevin,

Please review this webrev that fixes the NPE in getMediaName():
JBS: https://bugs.openjdk.java.net/browse/JDK-8196617
webrev: http://cr.openjdk.java.net/~lbourges/fx/fx-8196617.0/ 



It was tested OK on the appveyor CI that failed before the patch.

Thanks,
Laurent

[11] Review request: JDK-8196617: FX print tests fail with NPE in some environments

[Laurent Bourgès]

Kevin,

Please review this webrev that fixes the NPE in getMediaName():
JBS: https://bugs.openjdk.java.net/browse/JDK-8196617
webrev: http://cr.openjdk.java.net/~lbourges/fx/fx-8196617.0/

It was tested OK on the appveyor CI that failed before the patch.

Thanks,
Laurent