Review request: 8194871: Fix mistakes in FX API docs
Kevin Rushforth
kevin.rushforth at oracle.com
Thu Jan 11 00:27:53 UTC 2018
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.rushforth at oracle.com <mailto:kevin.rushforth at 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.rushforth at oracle.com <mailto:kevin.rushforth at 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.
>>>
>>>
>
More information about the openjfx-dev
mailing list