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
> 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> 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
>> for easy browsing:
>>
>> http://cr.openjdk.java.net/~kcr/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.
>>
>>
>
