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

[Sergey Bylokhov]

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


-- 
Best regards, Sergey.