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

[Phil Race]

I'm not sure why you didn't just in-line fixRectangleOrientation() but 
the fix looks fine.

-phil.

On 7/2/2013 2:23 PM, Sergey Bylokhov wrote:
> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>
>