Bugzilla – Full Text Bug Listing
| Summary: | Draw2D Euclidean Geometry (Ray, Straight) should support double precision. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Alexander Nyßen <nyssen> | ||||||||
| Component: | GEF-Legacy Draw2d | Assignee: | Alexander Nyßen <nyssen> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | ahunter.eclipse | ||||||||
| Version: | 3.6 | ||||||||||
| Target Milestone: | 3.6.0 (Helios) RC1 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Alexander Nyßen
Created attachment 166050 [details]
Patch demonstrating a possible refactoring of Ray/Straight
As some new operations were introduced to Ray in 3.6 and as the whole Straight class was newly introduced as well, I think it would be best to address this issue before this gets cemented into API (although we are already past M6).
IMHO the best possibility to deal with this would be to remove all operations, newly introduced to Ray in 3.6 again, to deprecate it, and to offer a new double-based implementation (let's call it Vector) with includes those newly introduced operations. Straight could in turn be based on the newly introduced Vector instead of Ray. I think it would also be wishful to separate both classes from the planar geometry things within the geometry package, so I would propose to introduce a new package for this. I have attached a patch that demonstrates what I have in mind.
Anthony, do you see a chance we could incorporate this into 3.6M7, as it would not lead to API-incompatible changes? Hi Alex, this is added API and as far as I can tell cannot break any existing clients, so we are good to commit. Why do we need a new org.eclipse.draw2d.geometry.euclidean package? I suggest we leave these in the existing org.eclipse.draw2d.geometry package. IMHO there are basically three kinds of classes within the current geometry package, base classes like Point and Dimension (and their precision derivates), Euclidean geometry related classes (Ray/Straight/Vector), and solid geometry related ones (Rectangle, etc). My motivation for the introduction of the euclidean sub-package was simple a better separation of concerns. As there are several issues related to refactoring/enhancing/augmenting the Geometry API, it is likely that additional classes will be added in the post Helios timeframe (e.g. Ellipse or Path) and that we could already start with cleary separating those aspects. Does that make sense? I have done some polishing to the Vector/Straight code (basically renaming of methods to ensure a common naming schema; also make position and direction vector fields of straight visible so both classes share a common "look and feel") and I think they are ready to be committed. Anthony, after I have now stated what my motivation for introducing the package was, may I urge you for a final comment about whether we should leave them in the original package or introduce a new package to separate out Euclidean geometry related API? (In reply to comment #5) > Anthony, after I have now stated what my motivation for introducing the package > was, may I urge you for a final comment about whether we should leave them in > the original package or introduce a new package to separate out Euclidean > geometry related API? Lets keep them in the standard geometry package that has been there for the last N releases. OK. Changes committed to cvs HEAD. Hi Alexander, GEF works with a Java 1.4 JRE. With the changes to Vector: import java.math.MathContext; import java.math.RoundingMode; Both cannot be found in my Sun JRE 1.4 version 10. Can you confirm? Our build is supposed to be using a 1.4 JRE, will need to investigate. Yes, you are right, it seems this has been added in 1.5.0 http://javadiff.sourceforge.net/jdiff/reports/j2se142_j2se150b1/changes.html. Well, I am using a 1.5 JDK with the j2SE-1.4 compliance settings, so I did not run into any problems. Ensuring strict API-compatibility to the 1.4 JDK framework classes could probably only be achieved by using a strictly 1.4 compatible VM (which is hard to get for MacOSX Snow Leopard, as it is two years past its EOL). I think the build machine is probably also using a 1.5 VM in 1.4 compliance mode and thus does not report any issues in this case either. What do you propose? Do I have to re-implement the private precision methods to achieve 1.4. BigDecimal compliance? Yes, we have to support Java 1.4 so we need to make the code compatible. I tried in the past to force move us to Java 1.5, and got push back from the community. This was Bug 204605 . It is now three years later and Java 1.4 support is somewhat insane given that I am pretty sure Java 1.4 support has expired. Next year we need to move to 1.5. OK, I will take a look. Created attachment 168041 [details]
Patch to restore 1.4 backwards compatibility
As far as I could observe, this patch changes the implementation to only use those methods offered by BigDecimal as of J2SE-1.4. Could you please confirm that your Sun 1.4 VM likes it this way?
(In reply to comment #12) > Created an attachment (id=168041) [details] > Patch to restore 1.4 backwards compatibility > > As far as I could observe, this patch changes the implementation to only use > those methods offered by BigDecimal as of J2SE-1.4. Could you please confirm > that your Sun 1.4 VM likes it this way? Not quite right, it now causes two test errors I have overlooked. I will have to reconsider. One last fix: Description Resource Path Location Type The method divide(BigDecimal, int) in the type BigDecimal is not applicable for the arguments (BigDecimal) Vector.java /org.eclipse.draw2d/src/org/eclipse/draw2d/geometry line 344 Java Problem Created attachment 168047 [details]
Revised patch to restore 1.4 backwards compatiblity.
OK, this one should be working now. Can you give it another try?
(In reply to comment #15) > OK, this one should be working now. Can you give it another try? Looks good. (In reply to comment #16) > (In reply to comment #15) > > OK, this one should be working now. Can you give it another try? > > Looks good. Test cases are fine as well. I have committed the changes to HEAD. Closing this one as resolved in 3.6RC1. |