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

[Sergey Bylokhov]

Thanks, Jim!
The second reviewer still needed.

On 02.07.2013 6:40, Jim Graham wrote:
> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>


-- 
Best regards, Sergey.