[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
From:       Ambarish Rapte <arapte () openjdk ! java ! net>
Date:       2020-07-29 19:44:39
Message-ID: PDzXBkpRUb9lIJfmJPaHNkXdn9YU-MN1SRVdmatnzxs=.2e296ba9-e834-46f7-86d9-51b729d85dc4 () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]