Uploaded here:
http://cr.openjdk.java.net/~kcr/8194871/webrev.01/
This looks good.
+1
I'll push it tomorrow.
-- Kevin
Nir Lisker wrote:
Attached a new webrev.
On Thu, Jan 11, 2018 at 2:27 AM, Kevin Rushforth
<kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>> wrote:
Since we are talking about the layout bounds of a node, I would
avoid using the term '3D scene' (or subscene) since in this case
it is the node that has the characteristic of 3D (geometry or
transforms) associated with it.
Anyway, let's go with the note at the end somewhere. Since layout
is a 2D concept, I think it's fine if 3D users don't see anything
about 3D until later.
Do you want to send a delta patch for that? Or you can send an
entire updated webrev. Whichever you prefer.
-- Kevin
Nir Lisker wrote:
Yes, I initially had it as a note in the end saying something
like this:
Note that for nodes in a 3D Scene (or SubScene),
layoutBounds is cuboid.
but thought that for someone working with 3D, seeing a 2D
discussion all the way until the end will be confusing. (Also
thought about putting a footnote at the end of the first
sentence, but that's not a recommended style as far as I know.)
My only slight concern is that "3D shapes" might hint at the
Shape3D class, while any node in a 3D environment will have 3D
bounds. This is also a technicality, so I wouldn't mind either
way. Feel free to make a final verdict.
On Thu, Jan 11, 2018 at 1:17 AM, Kevin Rushforth
<kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>>
wrote:
The changes look good to me for the most part. I only have
one comment.
Node.java:
- * The rectangular bounds that should be used for layout ...
+ * The rectangular (cuboid for 3D nodes) bounds that
should be used for layout ...
While technically correct, in that the layout bounds of a
TriangleMesh or Sphere will be a 3D bounds, layout is a 2D
operation, so this seems a bit misleading. If you still feel
that a comment is desired, then I would recommend it not
being in the opening sentence, but rather a note later in the
description...something like:
Note that for 3D shapes, the layout bounds is actually a
rectangular box with X, Y, and Z values, although only X and
Y are used in layout calculations.
-- Kevin
Kevin Rushforth wrote:
I just removed the trailing whitespace (using the handy
tools/scripts/checkWhiteSpace script with the '-F' option).
-- Kevin
Nir Lisker wrote:
Thanks,
modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java:209:
Trailing whitespace
That one is an empty line inside a code block, if it matters.
On Thu, Jan 11, 2018 at 12:14 AM, Kevin Rushforth
<kevin.rushfo...@oracle.com
<mailto:kevin.rushfo...@oracle.com>> wrote:
> I'll review it, and sponsor the change. Since I will
be pushing it, I will need one more reviewer.
Actually, this is incorrect. As long as I list you as
contributor, jcheck is perfectly happy with just me as
reviewer.
If anyone else wants to review it, too, that would be
fine, but not necessary for this type of fix.
-- Kevin
Kevin Rushforth wrote:
Thank you for providing the patch. I uploaded it to
cr.openjdk.java.net <http://cr.openjdk.java.net> for
easy browsing:
http://cr.openjdk.java.net/~kcr/8194871/webrev.00/
<http://cr.openjdk.java.net/%7Ekcr/8194871/webrev.00/>
I'll review it, and sponsor the change. Since I will
be pushing it, I will need one more reviewer.
My quick sanity checking shows trailing whitespace in
two files, which would cause jcheck to fail:
$ hg jcheck
modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java:209:
Trailing whitespace
modules/javafx.graphics/src/main/java/javafx/animation/Transition.java:161:
Trailing whitespace
I can fix this before I push.
-- Kevin
Nir Lisker wrote:
Hi Kevin,
Please review the attached webrev.
I addressed a few fixes I found as I was working, so
they are not listed in the JIRA report.
About Transition#getParentTargetNode:
The current behavior of parent-child relationship is
that an animation can be added to multiple parent
transitions. Each parent transition will see that
animation as its child, however, the child will see
only one of those animations as its parent - the one
to which is was added last. This asymmetry is a
recipe for trouble (and I argue should be addressed
at some point).
For this reason, the doc does not specify the "last
one wins" behavior, so that no contract is created.
This means that it's not clear which parent is going
to be queried on each (recursive) invocation.
Most of the changes could be backported to 8 and 9.
In 9, the methods getRangeShape and getUnderlineShape
of TextAreaSkin are also missing documentation.
