Summary: | Request for extra methods on PrecisionRectangle and a fix on PrecisionPoint | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Tools] GEF | Reporter: | Alex Boyko <aboyko> | ||||||||||||||
Component: | GEF-Legacy Draw2d | Assignee: | Alex Boyko <aboyko> | ||||||||||||||
Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||
Severity: | normal | ||||||||||||||||
Priority: | P3 | CC: | ahunter.eclipse, hudsonr, ppshah | ||||||||||||||
Version: | 3.2.1 | Keywords: | contributed | ||||||||||||||
Target Milestone: | 3.4.0 (Ganymede) M5 | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Windows XP | ||||||||||||||||
Whiteboard: | |||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 194482 | ||||||||||||||||
Attachments: |
|
Description
Alex Boyko
2007-12-10 16:16:59 EST
Created attachment 84900 [details]
Patch for PrecisionPoint and PrecisionRecatngle
Hi Randy and Pratik.
Could you please take a look at the patch and tell if it's ok to commit it?
Thanks in advance.
Created attachment 85011 [details]
patch
Previous patch was not uploaded correctly. Here is the correctly uploaded patch. Sorry for any inconvenience.
Created attachment 85133 [details]
Corrected patch
Corrected the patch:
1. Added the correct @since #
2. In PrecisionRectangle#transpose() after double precise values are transposed it's better to call super to transpose the int values intsead of updateInts().
We have one non-standard signature on PrecisionRectangle already: union(PrecisionRectangle). But maybe we could avoid further confusion if we make some changes to Point, Dimension, and/or Rectangle first. What if we add: float preciseX() float preciseY() to the standard Point class? It could be package visibility, but I don't see any problem in their being public too. Then this new union(point) method would not behave differently based on which method gets called (i.e. union(Point) vs. union(PrecisePoint)), or we could avoid instance of checks, as with: public Rectangle setLocation(Point loc) { + if (loc instanceof PrecisionPoint) { + PrecisionPoint preciseLoc = (PrecisionPoint) loc; + this.preciseX = preciseLoc.preciseX; + this.preciseY = preciseLoc.preciseY; + updateInts(); + return this; + } else { + return super.setLocation(loc); + } +} But with new methods on Point, etc.: public Rectangle setLocation(Point loc) { + this.preciseX = loc.preciseX(); + this.preciseY = loc.preciseY(); + updateInts(); + return this; +} If the client could call union(Point) instead of union(double, double), that could minimize the number of new signatures. Created attachment 85304 [details]
Added methods to Point, Dimension and Rectangle to make cleaner API on precise geomteric objects
I've added double preciseX() type of methods on Point, Dimension and Rectangle and reworked PrecisionPoint, PrecisionDimension and PrecisionRectangle accordingly.
Also I removed "final" for Rectangle#union(Rectangle) and Rectangle#union(Point) methods such that they can be overriden on PrecisionRectangle. PrecisionRectangle#union(PrecisionRectangle) is deprecated therefore.
Please let me know if this looks as you'd expect it.
Thnanks in advance.
is getPreciseCenter still needed, or could PrecisionRectangle just override getCenter() now that x & y are polymorphic, or whatever it's called. Created attachment 85466 [details]
Corrected patch
Indeed, it's not needed at all - Point#getCenter() must be overriden in this case.
The corrected patch is uploaded.
So, if everything else looks ok, perhaps, Anthony could commit that last corrected patch? Hi Anthony, Could you commit the last corrected patch please? Thanks in advance. I am getting a single compile error in public PrecisionPoint getCenter() { return new PrecisionPoint(preciseX + preciseWidth / 2.0, preciseY + preciseHeight / 2.0); } PrecisionRectangle.java: line 300: The return type is incompatible with Rectangle.getCenter() It looks like Pratik added *.prefs to the .cvsignore a while back, which can allow the code to be compiled with the wrong compatibility settings (1.5 instead of 1.4). It makes sense to me to force anyone creating patches to also check-out the proper project-specific compiler settings to avoid this in the future. Created attachment 86005 [details]
Corrected patch
Corrected the compiler error
(In reply to comment #11) > It looks like Pratik added *.prefs to the .cvsignore a while back, which can > allow the code to be compiled with the wrong compatibility settings (1.5 > instead of 1.4). It makes sense to me to force anyone creating patches to also > check-out the proper project-specific compiler settings to avoid this in the > future. > Hmm, This is a problem since the line org.eclipse.jdt.core.compiler.compliance=1.4 is in the GEF plug-in but not Draw2d. I will fix when doing Bug 204605 Committed to HEAD. |