[jfx15] RFR: 8228570: Add various documentation clarifications [v2]

[Ambarish Rapte]

On Tue, 28 Jul 2020 18:42:46 GMT, Nir Lisker <nlisker at openjdk.org> 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<E, Observable[]>
>>   extractor)` method. Later I found that `observableList(List<E> list, Callback<E, Observable[]> 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:  * <p>
> 60:  * It is not possible to change the {@code keyFrames} of a running {@code Timeline}.

Guessing that this `<p>` is moved down here so that the keyFrame related explanation is continuous. Even the next `<p>`
talks about `keyFrames`, so may be it can be moved further down after next `<p>` on line 64 ?

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 3401:

> 3400:      * <li>{@link #layoutXProperty layoutX}, {@link #layoutYProperty layoutY} and
> 3401:      * {@link #translateXProperty translateX}, {@link #translateYProperty translateY}, {@link #translateZProperty
> translateZ}</li> 3402:      * </ol>

I think the translate properties should still remain in a separate `<li>`.
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 <i>reverse</i> 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 <i>reverse</i> 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}, {@link #scaleYProperty scaleY}, {@link #scaleZProperty
> scaleZ}, {@link #rotateProperty rotate}, 327:  * {@link #translateXProperty translateX}, {@link #translateYProperty
> translateY} and {@link #translateZProperty translateZ}

This block of comment sounds useful to me. It does talk very specifically about the order of transforms in the
`getTransforms()` and is correct. How about keeping this block and then continue the new comment from `Custom...`

-------------

PR: https://git.openjdk.java.net/jfx/pull/276