[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [8] Request for review: 8004859 Graphics.getClipBounds/getClip return differenc
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2013-06-26 12:07:01
Message-ID: 51CAD965.4050402 () oracle ! com
[Download RAW message or body]

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.


[prev in list] [next in list] [prev in thread] [next in thread]