On Tue, 28 Jul 2020 18:42:46 GMT, Nir Lisker <nlis...@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
