[OpenJDK 2D-Dev] [8] Request for review: 8004859 Graphics.getClipBounds/getClip return difference nonequivalent bounds, depending from transform.

[Sergey Bylokhov]

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


-- 
Best regards, Sergey.