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

[Jim Graham]

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