Review request: 8194871: Fix mistakes in FX API docs
Kevin Rushforth
kevin.rushforth at oracle.com
Wed Jan 10 23:17:22 UTC 2018
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