Glass Robot and getSCreenCapture

[Stephen F Northover]

Hi David,

Sorry to be getting to this so late.  An uninitialized pixel is normally 
considered to be black.  If you throw an exception, clients will need to 
either catch the exception or perform the same test that you are 
performing before they call the "API".  Since this is not pubic API, no 
client will be affected so even if we make the change to throw the 
exception and then decide not to do this later, we can change it.

What is happening now?  Who is being affected by this bug?  Is it easy 
to change the implementations to return black?  This seems better to me 
than throwing the exception.

As I say, if we throw the exception, then we will only break ourselves, 
not clients of FX API.  Have we ensured that the exception will not 
break SQE tests.

Again, sorry to be getting to this so late and apologies if all of this 
has been discussed in another thread that I missed,
Steve

On 2014-03-25 2:46 PM, David Hill wrote:
> 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);
>
>