Hi Jim,
Just quick reply to some of your questions.
1) TransformUtils.java: My initial take was to delete this class but
after chatting with Kevin, we decided to keep it in the interest of
limiting the scope of this change. Moving those methods will touch much
more files. We have about 2 weeks to finish the whole encapsulation of
all JavaFX impl_* methods.
2) TransformHelper.java: This is the class naming pattern we use to
encapsulate all accessor interface classes. Also only less than half of
TransformUtils.java was moved into TransformHelper.java. A big portion,
ImmutableTransform, has to move into Transform.java.
3) ImmutableTransform: Yes, this is unclean in my opinion when I was
reading it. However this is design/implementation cleanup work is out of
the scope of this JIRA. I would suggest we file a separate JIRA for it
and since you are the code owner of Transform you can have it too! :-)
Lastly, JavaFX code review is done in the JIRA so that discussion of
that issue will stay in that JIRA.
Thanks,
- Chien
On 5/6/16, 8:10 PM, Jim Graham wrote:
TransformUtils.java - can't this class be deleted now since all it
exists to do is to forward to TransformHelper?
Alternatively, why create TransformHelper in the first place when it
is 90% just a copy of what TransformUtils used to be (ignoring the
inner class that got moved to Transform) with an accessor interface
thrown in for good measure? Just to change the name? That could be
done by using a source rename rather than gutting the old file and
creating the new one from scratch...? And we don't need both of them,
do we?
Transform.java, line 2172 & 2187 - is there a reason to forward to the
factory rather than just instantiating directly?
Transform.java, line 2177 & 2189 - I suppose the reason that the
arguments are not declared to be Immutable in the first place is that
some of these are used from other packages that have no visibility to
the Immutable. I was going to say that we should protect against them
not being Immutables, but I guess these uses are hidden so it doesn't
really affect anything by doing the cast - but it seems sloppy when
looking at the code without looking everywhere to see how they are
used (which I suppose is short-hand for "someone in the future might
mess up because it's not obvious that these can't deal with any other
type").
TransformHelper.java, line 108 & 112 - [formatting] perhaps move the
m[xyz],tx to the next line for consistency between the two (like 68 &
76)?
The implementation of ImmutableTransform copies a fair bit of
internals from Affine, but it could just wrap an Affine and save
itself all of that work. Arguably, we could make it a super-class of
Affine and then it would own all of that code and Affine would
subclass only for the modification methods, but we'd either have to
expose it as a public class or have an unexplained inaccessible
superclass on Affine...? (Also, it's not clear if public methods on a
non-public superclass of Affine would be visible through Affine so
Affine would either have to have forwarding methods or we'd have to
make the super class public).
On the other hand, what would be wrong with having a public BaseAffine
that was Affine without its modification methods, and having Affine
subclass it? It would be effectively Immutable, but using the word
Immutable in its name might sound like a promise which would be
violated by its subclass - better to leave that property implied by
its lack of mutating methods and possibly mentioning the intent in the
javadoc...
...jim
On 5/3/2016 11:24 PM, Chien Yang wrote:
Kevin and Jim,
Please review the proposed fix:
Webrev: https://bugs.openjdk.java.net/browse/JDK-8155762
JIRA: http://cr.openjdk.java.net/~ckyang/JDK-8155762/webrev.00/
Thanks,
- Chien
