Glass Robot and getSCreenCapture

[David Hill]

On 3/21/14, Mar 21, 12:53 PM, David Hill wrote:

Having heard a little feedback, here is my proposal in the form of a review request :-)
>
>
> My recommendation would be modify the JavaDoc contract to specify that the x,y and x+width, y+height must be within the screen bounds, with an IllegalArgumentException if out of bounds. This would be checked in Robot, prior to calling the native impls.
>
> This code is "internal" API, so I expect the real impact would be on SQE.
I really like the idea of adding a bounds restriction - so that the requested bounds must be within the Screen.
It seems the simplest solution to my issue of handling the odd edge case of out of bound pixels, with the least likely impact.

This means that existing code in the implementations are not affected.
I suspect that there will we little if any impact on SQE tests, given that most of us would avoid asking for a screen capture with undefined pixels. I do expect that we will encounter a few exceptions to this when tests are run on smaller displays (like embedded).

I also added bounds checking to Robot.getPixelColor() for consistency, and because Embedded passes this call through to common code for screen capture.

I did a grep through the JavaFX code base, and don't see any JavaFX use cases. I expect any "golden image" test code could be affected.

I separated out this internal API changes from my embedded changes so we have a clear and easy thing to review.

Jira: https://javafx-jira.kenai.com/browse/RT-36382

Patch: is inline in the Jira, but also here:

diff -r bb72bd2fa889 modules/graphics/src/main/java/com/sun/glass/ui/Robot.java
--- a/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue Mar 25 14:21:26 2014 -0400
+++ b/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue Mar 25 14:41:37 2014 -0400
@@ -144,9 +144,20 @@
      protected abstract int _getPixelColor(int x, int y);
      /**
       * Returns pixel color at specified screen coordinates in IntARGB format.
+     *
+     * If the requested pixel is not contained by the actual Screen
+     * bounds an IllegalArgumentException will be thrown.
+     *
+     * @param x The screen X of the requested capture (must be >=0)
+     * @param y The screen Y of the requested capture (must be >=0)
       */
      public int getPixelColor(int x, int y) {
          Application.checkEventThread();
+        Screen s = Screen.getMainScreen();
+        if (x < 0 || y < 0 ||
+            x >= s.getWidth() || y > s.getHeight()) {
+           throw new IllegalArgumentException("Capture out of bounds");
+        }
          return _getPixelColor(x, y);
      }

@@ -162,13 +173,27 @@
       * will result in a Pixels object with dimensions (20x20). Calling code
       * should use the returned objects's getWidth() and getHeight() methods
       * to determine the image size.
-     *
+     *
       * If (@code isHiDPI) is {@code false}, the returned Pixels object is of
       * the requested size. Note that in this case the image may be scaled in
       * order to fit to the requested dimensions if running on a HiDPI display.
+     *
+     * If the requested capture bounds is not contained by the actual Screen
+     * bounds an IllegalArgumentException will be thrown.
+     *
+     * @param x The screen X of the requested capture (must be >=0)
+     * @param y The screen Y of the requested capture (must be >=0)
+     * @param width The of width the requested capture (must be >=1 and fit on the screen)
+     * @param height The of width the requested capture (must be >=1 and fit on the screen)
+     * @param isHiDPI return HiDPI if available
       */
      public Pixels getScreenCapture(int x, int y, int width, int height, boolean isHiDPI) {
          Application.checkEventThread();
+        Screen s = Screen.getMainScreen();
+        if (x < 0 || y < 0 ||
+            x + width > s.getWidth() || y + height > s.getHeight()) {
+           throw new IllegalArgumentException("Capture out of bounds");
+        }
          return _getScreenCapture(x, y, width, height, isHiDPI);
      }

@@ -176,6 +201,14 @@
       * Returns a capture of the specified area of the screen.
       * It is equivalent to calling getScreenCapture(x, y, width, height, false),
       * i.e. this method takes a "LowDPI" screen shot.
+     *
+     * If the requested capture bounds is not contained by the actual Screen
+     * bounds an IllegalArgumentException will be thrown.
+     *
+     * @param x The screen X of the requested capture (must be >=0)
+     * @param y The screen Y of the requested capture (must be >=0)
+     * @param width The of width the requested capture (must be >=1 and fit on the screen)
+     * @param height The of width the requested capture (must be >=1 and fit on the screen)
       */
      public Pixels getScreenCapture(int x, int y, int width, int height) {
          return getScreenCapture(x, y, width, height, false);


-- 
David Hill <David.Hill at Oracle.com>
Java Embedded Development

"On a clear disk, you can seek forever."