On 3/26/14, Mar 26, 10:59 AM, Stephen F Northover wrote:
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.
Why ? Because I happened to have to fix some code and saw that what we had in
ARM was broken, or at least made some poor assumptions around out of bounds
pixels.
I really, really don't like "implied" contract code, and robot screen capture
is certainly implied. My preference in javadoc, even on internal API that outlines the
expected behavior, instead of having to read impl code on platforms I am not familiar
with.
The main item still is:
* what does an out of bounds pixel look like. The current answer is -
depends. Certainly on ARM some of the existing code would return one of,
uninitialized, or 0. Note that 0 is not always the same as black (0xff000000).
I sought to make the impl code easier by denying a call that would be out of
bounds. I am finding that this may be difficult or even impossible, when you
glue two screens of different sizes. Thanks to Anthony educating me on how this
works.
I did glance through the other impls code and I did not see any specific
handling of out of bounds pixels. Anthony says that handling was done by the
the underlying system calls.
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.
Ensured ? Not sure that is possible except by running the SQE tests with a
restriction in place. I have asked for a reading from SQE but have not heard
back yet.
But this discussion should be happening anyway, as we discuss what should or
should not be done.
Dave
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);
--
David Hill <david.h...@oracle.com>
Java Embedded Development
"Machines take me by surprise with great frequency."
-- Alan Turing (1912 - 1954)
