8345261: Refactor the Dimension2D classes

[nlisker]

A small refactoring of the Dimension classes.

• com.sun.javafx.geom.Dimension was removed and its uses were replaced by com.sun.javafx.geom.Dimension2D.
• com.sun.javafx.geom.Dimension2D became a record.
• javafx.geometry.Dimension2D: fields became final.

I'm not sure we need the implementation class at all considering we are free to use the public one.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8345261: Refactor the Dimension2D classes (Enhancement - P4)

Reviewers

• John Hendrikx (@hjohn - Reviewer) 🔄 Re-review required (review applies to e1ce604a)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1653/head:pull/1653
$ git checkout pull/1653

Update a local copy of the PR:
$ git checkout pull/1653
$ git pull https://git.openjdk.org/jfx.git pull/1653/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1653

View PR using the GUI difftool:
$ git pr show -t 1653

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1653.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@nlisker This change is no longer ready for integration - check the PR body for details.

[mlbridge]

Webrevs

• 01: Full - Incremental (2a72347f)
• 00: Full (e1ce604a)

[hjohn]

Looks good

[nlisker]

Thanks John. Because it's the weekend I'll wait until Monday evening with this.

[kevinrushforth]

@azvegint Can you take a quick look at this since you added the Dimension class as part of your initial Wayland Robot fix?

[azvegint]

These 3 classes are all different from each other. They use different types of variables to store data (double, float, int).

We have com.sun.javafx.geom.Rectangle which uses int, and the com.sun.javafx.geom.Dimension was specifically introduced as its pair which also uses int.
(just as we have a pair of javafx.geometry.Rectangle2D and javafx.geometry.Dimension2D.java).

Switching from int to float may have some side effects on stream processing due to the imprecise comparison.

So I would prefer to keep the com.sun.javafx.geom.Dimension.

[nlisker]

Thanks @azvegint. If the int-based dimensions are coupled with com.sun.javafx.geom.Rectangle (and from the code, that's the only place they are used at), would you object to making Dimension an inner class of Rectangle? The float-based dimensions are used in other 2D shapes, so are more general purpose.

[nlisker]

I also looked at the uses of com.sun.javafx.geom.Rectangle and in almost all cases I've looked at, its values are widened to floats later on. I'm not going to change anything there, but there is an indication that it's not the right tool for the job.

[Maran23]

Not sure if it helps, but when working on #1462, I also found that inconsistencies.
Sometimes double is used and later 'degraded' to a float, sometimes a float is promoted to a double. And as we found here, int is used but later changed to float.

I'm not sure why this happens, but maybe worth to take a deeper look.

[azvegint]

would you object to making Dimension an inner class of Rectangle?

I am fine with it.

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[nlisker]

You reviewed the PR before the requested changes were made :)
I'm going to move the int one into an inner class anyway.

[bridgekeeper]

@nlisker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[nlisker]

@azvegint Please confirm that the change is acceptable.

[andy-goryachev-oracle]

is this class really needed?

Looks like it's only referenced from Arc2D::setArc() and RectanglularShape::setFrame(), as far as I can tell these two methods are never called.

Should all three be removed?

[nlisker]

I'm not familiar with these methods, but I did wonder why we need extra implementations in the initial comment.

[andy-goryachev-oracle]

looks like dead code left over from the original implementation.

I removed the class along with the two methods and jfx builds (you need to merge the latest master though) and runs (i.e. shows arcs and rectangles) just fine. Eclipse IDE also has no issues. I think we can remove these.

[nlisker]

I don't mind removing them in this PR if others agree.

[mstr2]

Thanks @azvegint. If the int-based dimensions are coupled with com.sun.javafx.geom.Rectangle (and from the code, that's the only place they are used at), would you object to making Dimension an inner class of Rectangle? The float-based dimensions are used in other 2D shapes, so are more general purpose.

In my opinion, inner classes are justified when the implementations are coupled, or when one thing only makes sense in the context of another thing. That doesn't seem to be the case here: Dimension is neither used in Rectangle, nor are their implementations coupled.

What this suggests to me is that either Dimension should remain its own top-level class, or if it is not useful there, be moved to where it is used instead.