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