Hi Sergey,
This looks good to go...
...jim
On 6/26/13 5:07 AM, Sergey Bylokhov wrote:
Hello.
No volunteers to review the fix?
Thanks.
On 10.06.2013 16:49, Sergey Bylokhov wrote:
Hello.
Additional note is that this fix is targeted to 7u40.
On 06.06.2013 22:38, Sergey Bylokhov wrote:
HI, Jim.
Can you review the updated version of the fix:
http://cr.openjdk.java.net/~serb/8004859/webrev.03
I decided to implement an option, where transformed graphics
(identity, translated, scaled) returns equivalent clip. For this in
the transformShape() we preserve orientation of Rectangle, and in the
getClipBounds() we do not lose this information because we use
getBounds2D() instead of getBounds(). Note that I have simplified
getClipBounds() from the previous version:
http://cr.openjdk.java.net/~serb/8004859/webrev.02/src/share/classes/sun/java2d/SunGraphics2D.java.sdiff.html
Also there is no additional check for empty rectangles.
The original code in getClipBounds would end up returning a "new
Rectangle()" if the clip was an empty rectangle due to the way that
"Rectangle2D/Path2D.getBounds()" works. You now use
setFrame(getBounds2D()) which will attempt to preserve the dimensions
of empty clips. So, the "preserve the size of an empty clip" support
is new.
Yes it is new in case of Rectangle2D, but now it works in the same
way as for Rectangle.
On 23.04.2013 1:11, Jim Graham wrote:
I think if empty (un)transforms to something else that is also
classified as empty naturally without having to special case it or
test it then returning the "transformed empty thing" is a fine
return value.
But, if it comes down to "Eeek, it was empty so I need to calculate
a specifically similar empty thing to return" then I feel that the
test should just result in "return new Rectangle()". No use pulling
hair out to make a "more useful non-answer" unless it falls out of
the calculations for free - as in "unless it avoids even having to
check for the exceptional condition in the first place". In the
cases I reviewed I seem to recall that we were doing the tests
anyway (or we were doing equally complex tests in order to normalize
the answer even if we never explicitly tested for "isEmpty()") so it
would make the code much less problematic to just weed the empty
answers out early.
As far as "are any Developers expecting a usefully non-degenerate
empty answer" then I don't think so...
...jim
On 4/18/13 6:24 AM, Sergey Bylokhov wrote:
Hello Jim.
On 1/17/13 4:56 AM, Jim Graham wrote:
The original code in getClipBounds would end up returning a "new
Rectangle()" if the clip was an empty rectangle due to the way that
"Rectangle2D/Path2D.getBounds()" works. You now use
setFrame(getBounds2D()) which will attempt to preserve the dimensions
of empty clips. So, the "preserve the size of an empty clip" support
is new.
I then mentioned that we don't need to go out of our way to preserve
the dimensions of an empty clip, but you seem to be saying that we
don't want to change this behavior - but your new code represents the
break with existing behaviors, no?
I returned to this problem and studied it a little more deeply.
Description.
Assume we set clip=Rectangle[100, 100,-100,-100] for sg2d with a
different transform.
1 identity/translate (default mode for non-retina): getClip () will
return Rectangle[100, 100,-100,-100].
2 scale (default mode for retina): getClip() will return
Rectangle[0,0,100,100] <- bug and it should be fixed.
3 generic: getClip will return Rectangle[0,0,0,0], because we
convert
Rectangle to the Shape through RectIterator, which ignores the
negative
width and height. Note that x and y were not preserved only if w and
h<0, but if w = h =0, then x and y will be preserved.
In an original fix I made case 2 like case 1, so from the point of
view
of users there was no difference in case of retina display.
But now I have doubts and I think that all cases shall work equally
like
in case 3.
What do you think?
...jim
