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