Bugzilla – Full Text Bug Listing
| Summary: | PrecisionRectangle should either override crop or implement 'preciseCrop' | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Chris Lee <eclipse> | ||||
| Component: | GEF-Legacy Draw2d | Assignee: | Steven R. Shaw <steveshaw> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | hudsonr | ||||
| Version: | 3.2 | ||||||
| Target Milestone: | 3.2.0 (Callisto) M6 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Chris Lee
In theory, we should override every method. But, we're lazy and just do it on a case-by-case basis. Do you have a real use case, other than throwing an Assertion Failed Exception? Seems like the very least we could do for 3.2 would be to make non-final and add warning for those brave enough to subclass. The actual use case is similar; the problem is for image rendering where the image is in a different coordinate system than the figure, and we need to render a cropped version of the image. The cropping is stored in the figure's coordinate system, but to crop the image, we need to convert its bounds to the figure's coordinate system. Using a regular Rectangle introduces rounding errors, so we use a PrecisionRectangle; but crop doesn't crop precisely. The assert was just to simplify the expectation... I'd definitely hope to make the class non-final, similar to PrecisionDimension and PrecisionPoint (which we can add methods to via extension) This seems like a defect to me, not an enhancement request. I don't think we should add any methods prefixed with precise. And, adding preciseCrop wouldn't even fix the problem. It looks like the solution is to overide equals in PrecisionRectangle. Why wouldn't the fix be simply overriding the Rectangle#crop(Inset insets) to do an accurate calculation? This wouldn't impact API and solves the problem... Created attachment 35562 [details]
patch for PrecisionRectangle
Attached patch for PrecisionRectangle. Created JUnit test to cover case as well.
Randy - I attached a patch. Please review... The patch will change the existing behavior for equals(). Previously integer equivalence was the test even among two precise rectangles. I would want to see if the new equals test is evaluated in existing code such as the Logic example with non-100% zoom level. But more importantly, what is the point of the change? Are we saying that PrecisionRectangle as a replacement for Rectangle (in which case we should override every method), or is it just a special-case class for translating coordinates? Since Rectangle has public fields, we can never prevent clients from directly manipulating the integer precision fields instead of the FP versions. @see AWT's Rectangle class, and Rectangle2D interface for this exact same problem. I don't remember how they solved it. java.awt.Rectangle always operates on the coordinates that are local to the class. However, I suppose they override all the methods. Speaking to your point about this being a specific case class for transforming coordinates, wouldn't the expectation be that operating on those coordinates after translating that they take the precision into account? Why were resize and union overridden then? For the equals method I would suggest being even more strict and calling super.crop as well in the PrecisionRectangle case. PrecisionRectangle is used by tools that are creating requests. For snapping, we need to union the absolute bounds of all selected editparts, I think. Resizing and translating of PRect is used when dragging a resize handle. In general, you aren't supposed to touch parameters on a request since they are returned by reference, so people shouldn't be modifying prects in those cases. That pretty much sums up the initial usage of the class. It is used for mouse interactions that are done in terms of absolute coordinates. Calling super.crop() shouldn't be necessary. I would just access the double precision fields directly, and then call updateInts() at the end. This is what the other methods are doing. But, your patch would also update the ints (one-by-one) since you called the setters. I'd like to suggest not overriding 'equals' for this fix; it'll likely have unintended side effects in existing code that calls it... When equals does get updated though, a PrecisionRectangle should never 'equal' a Rectangle. I agree with setting the precision members directly and calling updateInts afterwards, similar to how the rest of PrecisionRectangle works; be careful with the '+=' and '+' (and '-=' and '-') used interchangeably within the method as well... Steve has checked the equals method for possible unexpected behavior and didn't find any cases where it would occur. If a client has a PrecisionRectangle typed as a Rectangle, and also has another Rectangle, it makes sense to allow them to be compared. The client doesn't care that one is precise. And not allowing this definitely would affect existing code. Submitted to head. - Overrode crop and equals - Created test cases in PrecisionRectangleTest As is, the attached patch has a few bugs in it, esp in equals:
if (o instanceof PrecisionRectangle) {
+ PrecisionRectangle pr = (PrecisionRectangle)o;
+ return pr.preciseX == preciseX &&
+ pr.preciseY == preciseY &&
+ pr.preciseX == preciseY &&
+ pr.preciseY == preciseY;
+ }
should probably compare preciseWidth and preciseHeight, and not compare preciseX to preciseY...
Also, in crop, += and -= should be changed to + and - (or modify the vars directly and call updateInts afterwards).
I fixed the equals method. Please check latest in HEAD. Sorry... I had caught the issues with the crop method, but missed the equals. The JUnit results blinded me from looking closer at the code. :( Re: Equals: I'm not sure the 0.000000001 tolerance is necessary. Isn't this presuming what level of precision is necessary? Looking at the java.awt.Rectangle2D implementation of equals, they don't have any consideration of tolerance in their comparison. Try scaling a Rectangle by 3f, then by 1/3f. If this works without the .00000001, then sure, take it out. |