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.htmlAlso 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 usesetFrame(getBounds2D()) which will attempt to preserve the dimensions of empty clips. So, the "preserve the size of an empty clip" supportis new.I then mentioned that we don't need to go out of our way to preservethe dimensions of an empty clip, but you seem to be saying that wedon't want to change this behavior - but your new code represents thebreak 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 () willreturn 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.
