Bugzilla – Full Text Bug Listing
| Summary: | org.eclipse.draw2d.geometry.Rectangle.Rectangle(Point,Point) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Holger Oehm <public> | ||||||||
| Component: | GEF-Legacy Draw2d | Assignee: | Steven R. Shaw <steveshaw> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | hudsonr, public | ||||||||
| Version: | 3.2 | ||||||||||
| Target Milestone: | 3.2.0 (Callisto) RC3 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Holger Oehm
Created attachment 40283 [details]
Short main() routine showing strange behavior of Rectangles
probably points to an issue with Rectangle#union union is working ok. The problem is that: new Rectangle().setLocation(point) does not create a rectangle that contains that point. It creates an empty rectangle containing nothing, but at that location. So the first point is not guaranteed to be contained in the result. This is also affecting Point.max(), which is creating a new rectangle for some strange reason. We should just write the code instead of trying to be cute and call other methods. Just in case, here is what I use as a workaround (feel free to use it, if you like):
final int x = Math.min(start.x, end.x);
final int y = Math.min(start.y, end.y);
final int width = Math.abs(end.x - start.x);
final int height = Math.abs(end.y - start.y);
final Rectangle rect = new Rectangle(x, y, width + 1, height + 1);
Created attachment 40982 [details]
proposed patch
- Proposed patch to Rectangle class in constructor.
- New JUnit in org.eclipse.draw2d.test
(In reply to comment #5) Hi Steven, I think one needs to add one to the width and height of the Rectangle to make it contain both Points. I am not completely sure about that, but AFAIU the javadoc of the Rectangle, it says that Points on the right/lower bounds of the Rectangle are not contained in the Rectangle. Best, Holger. PS: BTW, I like the patch and the test case, too. ;-) Yes, need to add 1. I think the test case should hard-code the expected result, rather than just test two results to each other. Correct result is 0,0,11,11. Another case which previously failed: new Rect(br, tl). It looks like most of the code in Rectangle is not consistent with this Java doc comment though -
i.e.
public Point getBottomRight() {
return new Point(x + width, y + height);
}
Created attachment 41136 [details]
new proposed patch
Ok - but I verified the new implementation returns the exact same result as the previous constructor where the width and height don't include the bottom right.
See new attachment for patch...
(In reply to comment #8) > It looks like most of the code in Rectangle is not consistent with this Java > doc comment though - > i.e. > public Point getBottomRight() { > return new Point(x + width, y + height); > } That constructor is the exception to the rule, which is why it is documented. The top and left edges of a rectangle are inclusive, the bottom and right are exclusive. This is how both SWT and AWT work as well. The patch seems fine, but I'm not sure there is a hurry for 3.2. A workaround to the bug is to call: new Rectangle(p1, new Dimension(1,1)).union(p2); I could go either way: +0.5 One could argue that this is data corruption since the contructor parameters are not being translated properly. I think this is a safe fix. +1 Committed change |