Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Mon, 10 Aug 2020 10:26:50 GMT, Nir Lisker wrote: >> In reading the existing doc carefully, it is both wrong and confusing. It >> has the wrong ordering of transform, scale >> and rotate. It is also confusing given that no distinction is made between >> matrix multiplication order and the observed >> effect on the geometry of the node. You have fixed the part about it being >> wrong, but the confusion remains, especially >> since you made a point of saying that the transforms list is applied in the >> reverse order. The matrices are multiplied >> in the following order: 1. layoutX/Y + translateX/Y/Z >> 2. rotate >> 3. scale >> 4. transforms[0] >> 5. transforms[1] >> ... >> >> We need to both list the actual matrix multiplication order, and then >> describe that the object is transformed from >> object coordinates to local coordinates of the node, to parent coordinated, >> etc., by first applying the transforms in >> the list in reverse order, then scale, rotate, translate+layout. I don't >> think that the `transforms` list is the place >> to do that (it belongs in the Transformations section). This doesn't seem >> like something we can sort out for JavaFX >> 15, so I would split this out and defer it. > > So should I revert all the changes about the transforms and leave it for 16, > or do part of it now and part later? > The changes are in the docs for the `Node` class, the `getTransforms()` > method and the `boundsInParentProperty()` > method. It seems best to revert all of the transform changes in this PR and deal with it in 16. - PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Mon, 10 Aug 2020 06:34:57 GMT, Nir Lisker wrote: >> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 868: >> >>> 867: * clearing the map or overriding its values. These entries are >>> not removed automatically if the node is >>> removed from the layout 868: * manager, so unused entries can exist >>> throughout the life of the node. >>> 869: * >> >> If we were in the habit of using `@apiNote` then that's probably where the >> note about layout managers using the node >> properties would go. Since we aren't, this seems fine. > > I think that `@aipNote` is indeed better. Does it require a CSR? In this case, I think it would be fine without a CSR, since it is just a clarifying note for developers. - PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Fri, 7 Aug 2020 14:37:48 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Corrected javadoc generation errors > > I left a few formatting comments and a substantive one on the clarification > of the transform order, which I recommend > to defer to 16. @kevinrushforth @arapte Can you give your input on the changes to the methods with an extractor in `FXCollections` that I mentioned in the 3rd bullet point of this PR? - PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Fri, 7 Aug 2020 14:18:31 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 5523:
>>
>>> 5522: * Defines the {@code ObservableList} of {@link Transform}
>>> objects to be applied to this {@code Node}. The
>>> transforms in this list 5523: * are applied in the reverse
>>> order of which they are specified as per matrix
>>> multiplication rules. This list of transforms 5524: * is applied
>>> before scaling ({@link #scaleXProperty scaleX},
>>> {@link #scaleYProperty scaleY} and {@link #scaleZProperty scaleZ}),
>>
>> `The transforms in this list are applied in the reverse order of
>> which they are specified as per matrix
>> multiplication rules`
>> To apply transformations in a specific order their respective matrices
>> should be multiplied in reverse order. So just
>> the order of multiplication is reverse but the transformation are applied in
>> same order as they are added into the
>> `getTransforms()`. I think phrasing it as 'the transforms are applied in
>> reverse order...' would not be accurate and
>> confusing to reader.
>
> In reading the existing doc carefully, it is both wrong and confusing. It has
> the wrong ordering of transform, scale
> and rotate. It is also confusing given that no distinction is made between
> matrix multiplication order and the observed
> effect on the geometry of the node. You have fixed the part about it being
> wrong, but the confusion remains, especially
> since you made a point of saying that the transforms list is applied in the
> reverse order. The matrices are multiplied
> in the following order: 1. layoutX/Y + translateX/Y/Z
> 2. rotate
> 3. scale
> 4. transforms[0]
> 5. transforms[1]
> ...
>
> We need to both list the actual matrix multiplication order, and then
> describe that the object is transformed from
> object coordinates to local coordinates of the node, to parent coordinated,
> etc., by first applying the transforms in
> the list in reverse order, then scale, rotate, translate+layout. I don't
> think that the `transforms` list is the place
> to do that (it belongs in the Transformations section). This doesn't seem
> like something we can sort out for JavaFX
> 15, so I would split this out and defer it.
So should I revert all the changes about the transforms and leave it for 16, or
do part of it now and part later?
The changes are in the docs for the `Node` class, the `getTransforms()` method
and the `boundsInParentProperty()`
method.
-
PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Fri, 7 Aug 2020 13:37:28 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Corrected javadoc generation errors > > modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 868: > >> 867: * clearing the map or overriding its values. These entries are not >> removed automatically if the node is >> removed from the layout 868: * manager, so unused entries can exist >> throughout the life of the node. >> 869: * > > If we were in the habit of using `@apiNote` then that's probably where the > note about layout managers using the node > properties would go. Since we aren't, this seems fine. I think that `@aipNote` is indeed better. Does it require a CSR? - PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Tue, 28 Jul 2020 18:42:46 GMT, Nir Lisker wrote:
>> Adds clarifications to the documentation in various places. Some notes:
>>
>> * Point 6 should probably be deferred until it is verified that the
>> tutorials are correct enough, seeing as they were
>> updated to Java 8 only.
>> * Point 8 has been deferred until all the animation bugs have been resolevd.
>> * Point 5: I wrote new documentation about the `extractor` for the
>> `observableArrayList(Callback
>> extractor)` method. Later I found that `observableList(List list,
>> Callback extractor)` already
>> talks about it (I updated it too). I'm not sure which of them we want to
>> keep, or maybe merge them.
>> * Point 1: I think that it's necessary to mention the internal
>> implementation behavior even if it requires a caveat that
>> this is only the current behavior and may change in the future. What
>> constitutes a "change" is extremely important and
>> there is no way for the user to know it. I've tripped on this hard when
>> using ReactFX which uses object equality
>> instead, so when the JavaFX observables are wrapped by ReactFX
>> observables, the behavior changes silently.
>> I think that in the future we will want to let the user define what a change
>> is (for example, by creating an
>> overridable method with the current behavior as the default, or using object
>> equality and letting the user override
>> that, although that's more risky). Even a `HashMap` that uses object
>> equality has the sister implementation
>> `IndentityHashMap` to deal with this ambiguity.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Corrected javadoc generation errors
I left a few formatting comments and a substantive one on the clarification of
the transform order, which I recommend
to defer to 16.
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
48:
> 47: * Current implementing classes in JavaFX check for a change using
> reference
> 48: * equality (and not object equality, {@code Object#equals(Object)}) of
> the value. An
> 49: * invalidation event is generated if the current value is not valid
> anymore.
I think this is fine given the way it is stated. It might be good to move it to
an `@implSpec` section, along with the
previous paragraph regarding lazy evaluation, but that would require a CSR and
can be considered for a future release.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
114:
> 113: * Observable objects returned by the extractor (applied to each
> list element) are listened for changes
> 114: * and transformed into {@link
> ListChangeListener.Change#wasUpdated() "update"} changes of a {@code
> ListChangeListener}. 115: *
I recommend `@linkplain` here since "update" is not the name of a method or
property.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
804:
> 803: *
> 804: * @defaultValue {@code false}; {@code true} for some Controls.
> 805: */
Either `controls` (lower case) or `{@code Control}s` seems better.
modules/javafx.graphics/src/main/java/javafx/animation/Timeline.java line 146:
> 145: /**
> 146: * Creates a {@code Timeline} with no key frames and a {@link
> Animation#getTargetFramerate() target framerate}.
> 147: *
`@linkplain`
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 868:
> 867: * clearing the map or overriding its values. These entries are not
> removed automatically if the node is
> removed from the layout 868: * manager, so unused entries can exist
> throughout the life of the node.
> 869: *
If we were in the habit of using `@apiNote` then that's probably where the note
about layout managers using the node
properties would go. Since we aren't, this seems fine.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 328:
> 327: * {@link #translateXProperty translateX}, {@link #translateYProperty
> translateY} and {@link #translateZProperty
> translateZ} 328: * properties.
> 329: *
See my comment on the transform property. This section seems like the right
place to expand the documentation to
specify the order of matrix multiplication and the order in which they affect
the node's geometry.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
269:
> 268: * @since JavaFX 2.2
> 269: * @defaultValue {@code "..."}
> 270: */
This should go before the `@since` (which is usually the last tag)
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
613:
> 612: * @since JavaFX 8.0
> 613: * @defaultValue 0
> 614: */
Before `@since`
-
PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Wed, 29 Jul 2020 19:02:16 GMT, Ambarish Rapte wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Corrected javadoc generation errors
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 5523:
>
>> 5522: * Defines the {@code ObservableList} of {@link Transform} objects
>> to be applied to this {@code Node}. The
>> transforms in this list 5523: * are applied in the reverse order
>> of which they are specified as per matrix
>> multiplication rules. This list of transforms 5524: * is applied before
>> scaling ({@link #scaleXProperty scaleX},
>> {@link #scaleYProperty scaleY} and {@link #scaleZProperty scaleZ}),
>
> `The transforms in this list are applied in the reverse order of which
> they are specified as per matrix
> multiplication rules`
> To apply transformations in a specific order their respective matrices should
> be multiplied in reverse order. So just
> the order of multiplication is reverse but the transformation are applied in
> same order as they are added into the
> `getTransforms()`. I think phrasing it as 'the transforms are applied in
> reverse order...' would not be accurate and
> confusing to reader.
In reading the existing doc carefully, it is both wrong and confusing. It has
the wrong ordering of transform, scale
and rotate. It is also confusing given that no distinction is made between
matrix multiplication order and the observed
effect on the geometry of the node. You have fixed the part about it being
wrong, but the confusion remains, especially
since you made a point of saying that the transforms list is applied in the
reverse order.
The matrices are multiplied in the following order:
1. layoutX/Y + translateX/Y/Z
2. rotate
3. scale
4. transforms[0]
5. transforms[1]
...
We need to both list the actual matrix multiplication order, and then describe
that the object is transformed from
object coordinates to local coordinates of the node, to parent coordinated,
etc., by first applying the transforms in
the list in reverse order, then scale, rotate, translate+layout. I don't think
that the `transforms` list is the place
to do that (it belongs in the Transformations section).
This doesn't seem like something we can sort out for JavaFX 15, so I would
split this out and defer it.
-
PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Wed, 29 Jul 2020 17:38:27 GMT, Ambarish Rapte wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Corrected javadoc generation errors
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 3401:
>
>> 3400: * {@link #layoutXProperty layoutX}, {@link #layoutYProperty
>> layoutY} and
>> 3401: * {@link #translateXProperty translateX}, {@link
>> #translateYProperty translateY}, {@link #translateZProperty
>> translateZ} 3402: *
>
> I think the translate properties should still remain in a separate ``.
> layout and translate, they together determine the final translation but are
> different properties.
I don't want to imply that they are performed separately in the order
previously written. I think that the difference
in properties is evident by the links.
-
PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Tue, 28 Jul 2020 18:42:46 GMT, Nir Lisker wrote:
>> Adds clarifications to the documentation in various places. Some notes:
>>
>> * Point 6 should probably be deferred until it is verified that the
>> tutorials are correct enough, seeing as they were
>> updated to Java 8 only.
>> * Point 8 has been deferred until all the animation bugs have been resolevd.
>> * Point 5: I wrote new documentation about the `extractor` for the
>> `observableArrayList(Callback
>> extractor)` method. Later I found that `observableList(List list,
>> Callback extractor)` already
>> talks about it (I updated it too). I'm not sure which of them we want to
>> keep, or maybe merge them.
>> * Point 1: I think that it's necessary to mention the internal
>> implementation behavior even if it requires a caveat that
>> this is only the current behavior and may change in the future. What
>> constitutes a "change" is extremely important and
>> there is no way for the user to know it. I've tripped on this hard when
>> using ReactFX which uses object equality
>> instead, so when the JavaFX observables are wrapped by ReactFX
>> observables, the behavior changes silently.
>> I think that in the future we will want to let the user define what a change
>> is (for example, by creating an
>> overridable method with the current behavior as the default, or using object
>> equality and letting the user override
>> that, although that's more risky). Even a `HashMap` that uses object
>> equality has the sister implementation
>> `IndentityHashMap` to deal with this ambiguity.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Corrected javadoc generation errors
Changes requested by arapte (Reviewer).
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
310:
> 309: /**
> 310: * Creates a new empty observable list that is backed by an array
> list.
> 311: * @see #observableList(java.util.List)
Can make the similar change on line number 372 as well.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
321:
> 320: /**
> 321: * Creates a new empty observable list backed by an array list that
> listens to changes in observables of its
> items. 322: * The {@code extractor} parameter specifies observables
> (usually properties) of the objects in the
> created list. When there is
observable list -> `{@code ObservableList}`
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
138:
> 137: * @return the text to display in the label
> 138: * @defaultValue {@code ""} (empty string}
> 139: */
Some other classes use a format like `@defaultValue empty string`. May be the
similar pattern should be followed.
modules/javafx.graphics/src/main/java/javafx/animation/Timeline.java line 59:
> 58: * of Timeline's keyFrames.
> 59: *
> 60: * It is not possible to change the {@code keyFrames} of a running {@code
> Timeline}.
Guessing that this `` is moved down here so that the keyFrame related
explanation is continuous. Even the next ``
talks about `keyFrames`, so may be it can be moved further down after next
`` on line 64 ?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 3401:
> 3400: * {@link #layoutXProperty layoutX}, {@link #layoutYProperty
> layoutY} and
> 3401: * {@link #translateXProperty translateX}, {@link
> #translateYProperty translateY}, {@link #translateZProperty
> translateZ} 3402: *
I think the translate properties should still remain in a separate ``.
layout and translate, they together determine the final translation but are
different properties.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 5523:
> 5522: * Defines the {@code ObservableList} of {@link Transform} objects
> to be applied to this {@code Node}. The
> transforms in this list 5523: * are applied in the reverse order
> of which they are specified as per matrix
> multiplication rules. This list of transforms 5524: * is applied before
> scaling ({@link #scaleXProperty scaleX},
> {@link #scaleYProperty scaleY} and {@link #scaleZProperty scaleZ}),
`The transforms in this list are applied in the reverse order of which
they are specified as per matrix
multiplication rules`
To apply transformations in a specific order their respective matrices should
be multiplied in reverse order. So just
the order of multiplication is reverse but the transformation are applied in
same order as they are added into the
`getTransforms()`. I think phrasing it as 'the transforms are applied in
reverse order...' would not be accurate and
confusing to reader.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 326:
> 325: * list. Predefined transforms are applied afterwards in this order:
> scale, rotation and translation. These are
> applied using the 326: * {@link #scaleXProperty scaleX}, {@li
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
> Adds clarifications to the documentation in various places. Some notes: > > * Point 6 should probably be deferred until it is verified that the tutorials > are correct enough, seeing as they were > updated to Java 8 only. > * Point 8 has been deferred until all the animation bugs have been resolevd. > * Point 5: I wrote new documentation about the `extractor` for the > `observableArrayList(Callback > extractor)` method. Later I found that `observableList(List list, > Callback extractor)` already > talks about it (I updated it too). I'm not sure which of them we want to > keep, or maybe merge them. > * Point 1: I think that it's necessary to mention the internal implementation > behavior even if it requires a caveat that > this is only the current behavior and may change in the future. What > constitutes a "change" is extremely important and > there is no way for the user to know it. I've tripped on this hard when > using ReactFX which uses object equality > instead, so when the JavaFX observables are wrapped by ReactFX observables, > the behavior changes silently. > I think that in the future we will want to let the user define what a change > is (for example, by creating an > overridable method with the current behavior as the default, or using object > equality and letting the user override > that, although that's more risky). Even a `HashMap` that uses object equality > has the sister implementation > `IndentityHashMap` to deal with this ambiguity. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Corrected javadoc generation errors - Changes: - all: https://git.openjdk.java.net/jfx/pull/276/files - new: https://git.openjdk.java.net/jfx/pull/276/files/851c64ca..a44bfb18 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/276/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/276/webrev.00-01 Stats: 22 lines in 3 files changed: 2 ins; 3 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/276.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/276/head:pull/276 PR: https://git.openjdk.java.net/jfx/pull/276
