Re: Review request: 8194871: Fix mistakes in FX API docs

[Kevin Rushforth]

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 
> 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
>
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
> 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 

Re: Review request: 8194871: Fix mistakes in FX API docs

[Kevin Rushforth]

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 
> 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
>
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.

Re: Review request: 8194871: Fix mistakes in FX API docs

[Nir Lisker]

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  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.
>>
>>
>

Re: Review request: 8194871: Fix mistakes in FX API docs

[Kevin Rushforth]

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 
> 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.

Re: Review request: 8194871: Fix mistakes in FX API docs

[Kevin Rushforth]

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 
> 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.

Re: Review request: 8194871: Fix mistakes in FX API docs

[Nir Lisker]

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.
>
>

Re: Review request: 8194871: Fix mistakes in FX API docs

[Kevin Rushforth]

> 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.

Re: Review request: 8194871: Fix mistakes in FX API docs

[Kevin Rushforth]

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.