[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: Review request: 8194871: Fix mistakes in FX API docs
From: Kevin Rushforth <kevin.rushforth () oracle ! com>
Date: 2018-01-11 0:59:24
Message-ID: 5A56B6EC.2060208 () oracle ! com
[Download RAW message or body]
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.rushforth@oracle.com <mailto:kevin.rushforth@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.rushforth@oracle.com <mailto:kevin.rushforth@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@oracle.com
>>>> <mailto:kevin.rushforth@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.
>>>>
>>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic