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 <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> 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
>>> 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>
>>> 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> 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/j
>>>>> avafx/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> 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>
>>>>>> 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> 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>
>>>>>>>> <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>
>>>>>>>> <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
>>>>>>>>
>>>>>>>> All suggested changes can be found by using Github Compare View:
>>>>>>>> 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
>>>>>>>> /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/
>>>>>>>> 10-dev/rt/file/tip/modules/javafx.graphics/src/main/java/com
>>>>>>>> /sun/glass/ui/Robot.java)
>>>>>>>> [com.sun.javafx.application.ParametersImpl](http://hg.openjdk.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
>>>>>>>> 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
>>>>>>>> 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
>
>
