Michael Ennen wrote:
Quick question:

Currently a Robot is created by calling `Application.createRobot` which delegates to the underlying platform-specific application class (GtkApplication, WinApplication, etc.)
via `com.sun.glass.ui.Application.GetApplication().createRobot();`

I just meant that the Toolkit class was a convenient place to call com.sun.glass.ui.Application.GetApplication().createRobot() -- something like this:

Toolkit:

public GlassRobot createRobot() {   // or could be abstract
   throw new UnsupportedOperationException("not implemented");
}

QuantumToolkit:

public GlassRobot createRobot() {
   return com.sun.glass.ui.Application.GetApplication().createRobot();
}

The reason for doing it there is because it already has the mechanism for handling QuantumToolkit versus StubToolkit. Otherwise you wouldn't need the extra level of indirection.

-- Kevin

You suggest moving this to the Toolkit class? If GtkRobot, WinRobot, etc. all extend the abstract base class GlassRobot (the peer), how will Toolkit be able to instantiate the correct, platform-specific class? Sorry, I got stuck trying to implement this part
of your review. The rest is easy for me to follow and will be no problem.


On Tue, Mar 20, 2018 at 4:28 PM, Kevin Rushforth <kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>> 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 <mailto: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
        <mailto: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 sub-classes.


            -- Kevin


            Michael Ennen wrote:
            I am not trying to be a burden here. I understand that
            you may not have time to hand-hold
            to this degree. I will try and make progress, sorry for
            the follow up question.

            On Wed, Dec 20, 2017 at 2:08 PM, Michael Ennen
            <mike.en...@gmail.com <mailto:mike.en...@gmail.com>> wrote:

                How can Robot call into the implementation when it
                is a super class of the implementations?

                On Wed, Dec 20, 2017 at 2:04 PM, Kevin Rushforth
                <kevin.rushfo...@oracle.com
                <mailto:kevin.rushfo...@oracle.com>> wrote:



                    Michael Ennen wrote:
                    I have a question about how to proceed with
                    the Robot code.

                    The base abstract Robot class
                    is: 
https://github.com/brcolow/openjfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/robot/Robot.java
                    
<https://github.com/brcolow/openjfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/robot/Robot.java>

                    As you can see for each method, such as
                    "getMouseX()" there is a "_" prefixed method
                    which is abstract and a non-prefixed method:

                    protected abstract int _getMouseX();

                    public int getMouseX() {
                        Application.checkEventThread();
                        return _getMouseX();
                    }

                    I have copied this from the private Robot API.

                    Is there a better way to do this? Would this
                    pass review?

                    Yes there are better ways to do this. No it
                    would not pass review, since this would be
                    leaking implementation into the public API.

                    Rather than copying the public / protected
                    methods from the internal package, it probably
                    makes more sense to start with what a Robot API
                    should look like and then have that call into
                    the implementation (suitably modified so it
                    better matches the public API). For one thing
                    you will then leave the implementation,
                    including the per-platform code, where it
                    belongs -- in glass. The Robot API can be
                    informed by the current implementation, but
                    should not be defined by it.

                    -- Kevin



                    Thanks very much.


                    On Tue, Dec 5, 2017 at 5:29 PM, Kevin
                    Rushforth <kevin.rushfo...@oracle.com
                    <mailto:kevin.rushfo...@oracle.com>> wrote:

                        Glad you got the build working. You can
                        post back on this thread when you are ready.


                        -- Kevin


                        Michael Ennen wrote:
                        Correction:

                        Adding ""--add-exports
                        javafx.graphics/javafx.scene.robot=ALL-UNNAMED"
                        to buildSrc/addExports.

                        For posterity :)

                        On Mon, Dec 4, 2017 at 6:08 PM, Michael
                        Ennen <mike.en...@gmail.com
                        <mailto:mike.en...@gmail.com>> wrote:

                            Ah, indeed, missed adding
                            "--add-opens
                            javafx.graphics/javafx.scene.robot=ALL-UNNAMED"
                            to buildSrc/addExports.
                            Thanks for the guidance on that.

                            I will continue to work on this in
                            the GitHub repo and polish it up (add
                            javadocs, better method signatures,
etc.) and even plan on maybe improving the
                            underlying native Robot
                            implementations (for example
                            fixing/improving the
                            way color profiles are handled for
                            MacRobot).

                            I will also take a look at "fixing"
                            JemmyFX to use the new public API (as
                            well as any other place in the JavaFX
                            code
                            base that does).

                            I was expecting that JDK 11 would be
                            the appropriate time frame,
                            especially because it will be the
                            release where
                            private APIs will be totally
                            inaccessible, correct?

                            After I get it in a reasonable state
                            should I post back on this mailing
                            list thread or what would be the
                            appropriate
                            way?

                            Thanks Kevin.

                            On Mon, Dec 4, 2017 at 5:12 PM, Kevin
                            Rushforth <kevin.rushfo...@oracle.com
                            <mailto:kevin.rushfo...@oracle.com>>
                            wrote:

                                This is a limitation of the the
                                way --patch-modules works. You
                                will need to add an entry in:

                                buildSrc/addExports

                                Btw, as for the proposal itself,
                                this might need to be a JEP
                                depending on the scope. In any
                                case, it could be considered in
                                the JDK 11 time frame, but there
                                are several things that need to
                                be worked out before making Robot
                                a public API, including the fact
                                that the JemmyFX framework in the
                                openjfx/jfx/tests directory uses
                                Robot. Once you get a working
                                prototype, it would be
                                interesting to discuss it in more
                                detail.

                                -- Kevin



                                Michael Ennen wrote:
                                Currently I am stuck with tests not being able 
to see the new
                                "javafx.scene.robot" module:

                                Task :systemTests:compileTestJava
                                
C:\Users\brcolow\dev\openjfx\tests\system\src\test\java\test\robot\com\sun\glass\ui\monocle\ModalDialogTest.java:34:
                                error: package javafx.scene.robot is not visible
                                import javafx.scene.robot.Robot;
                                                   ^
                                  (package javafx.scene.robot is declared in 
module javafx.graphics, which
                                does not export it)
                                
C:\Users\brcolow\dev\openjfx\tests\system\src\test\java\test\robot\com\sun\glass\ui\monocle\RobotTest.java:33:
                                error: package javafx.scene.robot is not visible
                                import javafx.scene.robot.Robot;

                                I have added:

                                    exports javafx.scene.robot;

                                to: 
modules/javafx.graphics/src/main/java/module-info.java

                                But this does not seem to be enough.

                                On Sun, Dec 3, 2017 at 4:48 PM, Michael Ennen 
<mike.en...@gmail.com> <mailto:mike.en...@gmail.com> wrote:

                                I am still working on all the necessary changes 
to actually allow openjfx
                                to compile.
                                Tons to learn in that arena and I know the code 
as it is written won't
                                totally work.
                                For example one can no longer:

                                #include "com_sun_glass_ui_Robot.h"

                                as in 
openjfx\modules\javafx.graphics\src\main\native-glass\win\Robot.cpp

                                But I am not sure how those headers are 
generated and if I can just simply
                                change
                                it to "#include javafx_scene_robot_Robot.h" 
(which I very much doubt).

                                On Sun, Dec 3, 2017 at 2:29 PM, Michael Ennen 
<mike.en...@gmail.com> <mailto:mike.en...@gmail.com>
                                wrote:

                                I have created a (small) proposal (building on 
the work of Benjamin
                                Gudehaus) about moving some classes in to the 
public API so that TestFX (a
                                JavaFX UI testing framework) can continue to 
work with future JDK releases.
                                The somewhat nicely formatted proposal can be 
found as a Github gist:

                                
https://gist.github.com/brcolow/26370db6cab0355186d4a1d13b30fc19 
<https://gist.github.com/brcolow/26370db6cab0355186d4a1d13b30fc19>

                                All suggested changes can be found by using 
Github Compare View:

                                
https://github.com/brcolow/openjfx/compare/4ccdbbbce5234e2c5 
<https://github.com/brcolow/openjfx/compare/4ccdbbbce5234e2c5>
                                e1f4f1cb8f20430feaa53b6...master

                                But I have copied it to this email for 
convenience:

                                ----------------------- PROPOSAL 
-----------------------

                                TestFX, the JavaFX GUI testing framework 
currently requires 4 (four)
                                classes that are part of the JDK's private API. 
They are:

                                
[com.sun.glass.ui.Application](http://hg.openjdk.java.net/op
                                
enjfx/10-dev/rt/file/tip/modules/javafx.graphics/src/main/
                                java/com/sun/glass/ui/Application.java)
                                
[com.sun.glass.ui.Pixels](http://hg.openjdk.java.net/openjfx 
<http://hg.openjdk.java.net/openjfx>
                                
/10-dev/rt/file/tip/modules/javafx.graphics/src/main/java/
                                com/sun/glass/ui/Pixels.java)
                                
[com.sun.glass.ui.Robot](http://hg.openjdk.java.net/openjfx/ 
<http://hg.openjdk.java.net/openjfx/>
                                
10-dev/rt/file/tip/modules/javafx.graphics/src/main/java/com
                                /sun/glass/ui/Robot.java)
                                [com.sun.javafx.application.Pa 
<http://com.sun.javafx.application.Pa>rametersImpl](http://hg.openjd
                                
k.java.net/openjfx/10-dev/rt/file/tip/modules/javafx.
                                
graphics/src/main/java/com/sun/javafx/application/ParametersImpl.java 
<http://k.java.net/openjfx/10-dev/rt/file/tip/modules/javafx.graphics/src/main/java/com/sun/javafx/application/ParametersImpl.java>)

                                In order to compile the project with Java 9, we 
use the following flags:

                                ```sh
                                --add-exports 
javafx.graphics/com.sun.glass.ui=org.testfx
                                --add-exports 
javafx.graphics/com.sun.javafx.application=org.testfx
                                ```

                                If the --add-exports flags are disabled in a 
future Java release TestFX
                                will require these four classes to be moved 
into the public API to
                                continue working.

                                While these classes are probably not very 
useful for applications to use
                                directly, any JavaFX application wanting to 
write UI tests will most
                                likely
                                use TestFX and thus they will indirectly be 
using these classes.

                                JavaFX internal tests also use these classes 
for essentially the same
                                purpose (UI tests).

                                ### Details of Usage For Each Private API Class

                                #### com.sun.javafx.application.ParametersImpl

                                ##### TestFX Usage

                                ```java
                                ParametersImpl parameters = new 
ParametersImpl(applicationArgs);
                                ParametersImpl.registerParameters(application, 
parameters);
                                ```

                                The parameters are set on a constructed 
Application.

                                ##### Suggested Public API Replacement

                                `javafx.application.Application`:

                                ```java
                                /**
                                 * Sets the parameters for this Application.
                                 *
                                 * <p>
                                 * NOTE: this method should not be called from 
the Application
                                constructor,
                                 * as it will return null. It may be called in 
the init() method or any
                                 * time after that.
                                 * </p>
                                 *
                                 * @param parameters the parameters to set for 
this Application
                                 */
                                public final Parameters setParameters(String... 
parameters) {
                                    ParametersImpl parameters = new 
ParametersImpl(parameters);
                                    ParametersImpl.registerParameters(this, 
parameters);
                                }
                                ```

                                #### com.sun.glass.ui.Application

                                ##### TestFX Usage

                                ```java
                                return 
Application.GetApplication().createRobot();
                                ```

                                The Application class is used to instantiate a 
Robot.

                                ##### Suggested Public API Replacement

                                `javafx.application.Application`:

                                
https://github.com/brcolow/openjfx/blob/master/modules/javaf 
<https://github.com/brcolow/openjfx/blob/master/modules/javaf>
                                
x.graphics/src/main/java/javafx/application/Application.java#L527

                                #### com.sun.glass.ui.Pixels

                                ##### TestFX Usage

                                ```java
                                @Override
                                public Image getCaptureRegion(Rectangle2D 
region) {
                                    return 
waitForAsyncFx(RETRIEVAL_TIMEOUT_IN_MILLIS, () -> {
                                        Pixels glassPixels = 
useRobot().getScreenCapture(
                                            (int) region.getMinX(), (int) 
region.getMinY(),
                                            (int) region.getWidth(), (int) 
region.getHeight()
                                        );
                                        return 
convertFromGlassPixels(glassPixels);
                                    });
                                }

                                private Image convertFromGlassPixels(Pixels 
glassPixels) {
                                    int width = glassPixels.getWidth();
                                    int height = glassPixels.getHeight();
                                    WritableImage image = new 
WritableImage(width, height);

                                    int bytesPerComponent = 
glassPixels.getBytesPerComponent();
                                    if (bytesPerComponent == 
INT_BUFFER_BYTES_PER_COMPONENT) {
                                        IntBuffer intBuffer = (IntBuffer) 
glassPixels.getPixels();
                                        writeIntBufferToImage(intBuffer, image);
                                    }

                                    return image;
                                }

                                private void writeIntBufferToImage(IntBuffer 
intBuffer,
                                                                   
WritableImage image) {
                                    PixelWriter pixelWriter = 
image.getPixelWriter();
                                    double width = image.getWidth();
                                    double height = image.getHeight();

                                    for (int y = 0; y < height; y++) {
                                        for (int x = 0; x < width; x++) {
                                            int argb = intBuffer.get();
                                            pixelWriter.setArgb(x, y, argb);
                                        }
                                    }
                                }
                                ```

                                Pixels is used to create a screen capture.

                                ##### Suggested Public API Replacement

                                Bypass needing to expose the Pixels class to 
the public API by
                                changing the getScreenCapture method of Robot - 
that is, changing:

                                `public Pixels getScreenCapture(int x, int y, 
int width, int height)`

                                to:

                                `public Image getScreenCapture(int x, int y, 
int width, int height)`

                                #### com.sun.glass.ui.Robot

                                ##### TestFX Usage

                                Essentially every method of Robot is used:

                                ```
                                public void keyPress(int code)
                                public void keyRelease(int code)
                                public int getMouseX()
                                public int getMouseY()
                                public void mouseMove(int x, int y)
                                public void mousePress(int buttons)
                                public void mouseRelease(int buttons)
                                public void mouseWheel(int wheelAmt)
                                public int getPixelColor(int x, int y)
                                public Pixels getScreenCapture(int x, int y, 
int width, int height)
                                ```

                                ##### Suggested Public API Replacement

                                
https://github.com/brcolow/openjfx/blob/master/modules/javaf 
<https://github.com/brcolow/openjfx/blob/master/modules/javaf>
                                
x.graphics/src/main/java/javafx/scene/robot/Robot.java


                                --
                                Michael Ennen

                                --
                                Michael Ennen





-- Michael Ennen




-- Michael Ennen




-- Michael Ennen




-- Michael Ennen




-- Michael Ennen




-- Michael Ennen




-- Michael Ennen




--
Michael Ennen

Reply via email to