Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8228570: Add various documentation clarifications #276

Conversation

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Jul 27, 2020

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.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8228570: Add various documentation clarifications

Reviewers

  • Ambarish Rapte (arapte - Reviewer)
  • Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/276/head:pull/276
$ git checkout pull/276

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 27, 2020

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@nlisker nlisker changed the base branch from master to jfx15 Jul 27, 2020
@nlisker nlisker marked this pull request as ready for review Jul 27, 2020
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Jul 27, 2020

/reviewers 2

@openjdk openjdk bot added the rfr label Jul 27, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Jul 27, 2020

@nlisker
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 27, 2020

@kevinrushforth kevinrushforth self-requested a review Jul 27, 2020
* <li>{@link #translateXProperty translateX}, {@link #translateYProperty translateY},
* {@link #translateZProperty translateZ}</li>
* <li>{@link #layoutXProperty layoutX}, {@link #layoutYProperty layoutY} and
* {@link #translateXProperty translateX}, {@link #translateYProperty translateY}, {@link #translateZProperty translateZ}</li>

This comment has been minimized.

@arapte

arapte Jul 29, 2020
Member

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.

This comment has been minimized.

@nlisker

nlisker Aug 2, 2020
Author Collaborator

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.

* before {@link #translateXProperty translateX}, {@link #translateYProperty translateY}, {@link #scaleXProperty scaleX}, and
* {@link #scaleYProperty scaleY}, {@link #rotateProperty rotate} transforms.
* Defines the {@code ObservableList} of {@link Transform} objects to be applied to this {@code Node}. The transforms in this list
* are applied in the <i>reverse</i> order of which they are specified as per matrix multiplication rules. This list of transforms

This comment has been minimized.

@arapte

arapte Jul 29, 2020
Member

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.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

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.

This comment has been minimized.

@nlisker

nlisker Aug 10, 2020
Author Collaborator

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.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 10, 2020
Member

It seems best to revert all of the transform changes in this PR and deal with it in 16.

* Multiple transformations may be applied to a node by specifying an ordered
* chain of transforms. The order in which the transforms are applied is
* defined by the ObservableList specified in the {@link #getTransforms transforms} variable.
Comment on lines 324 to 326

This comment has been minimized.

@arapte

arapte Jul 29, 2020
Member

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

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I left a few formatting comments and a substantive one on the clarification of the transform order, which I recommend to defer to 16.

* Returns an observable map of properties on this node for use primarily by application developers. Layout managers use this map
* as well to specify layout constraints on the node, such as {@code HBox#setHgrow}, so the developer should be mindful of
* clearing the map or overriding its values. These entries are not removed automatically if the node is removed from the layout
* manager, so unused entries can exist throughout the life of the node.
Comment on lines 865 to 868

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

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.

This comment has been minimized.

@nlisker

nlisker Aug 10, 2020
Author Collaborator

I think that @aipNote is indeed better. Does it require a CSR?

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 10, 2020
Member

In this case, I think it would be fine without a CSR, since it is just a clarifying note for developers.

* before {@link #translateXProperty translateX}, {@link #translateYProperty translateY}, {@link #scaleXProperty scaleX}, and
* {@link #scaleYProperty scaleY}, {@link #rotateProperty rotate} transforms.
* Defines the {@code ObservableList} of {@link Transform} objects to be applied to this {@code Node}. The transforms in this list
* are applied in the <i>reverse</i> order of which they are specified as per matrix multiplication rules. This list of transforms

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

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.

* list. Predefined transforms are applied afterwards in this order: scale, rotation and translation. These are applied using the
* {@link #scaleXProperty scaleX}, {@link #scaleYProperty scaleY}, {@link #scaleZProperty scaleZ}, {@link #rotateProperty rotate},
* {@link #translateXProperty translateX}, {@link #translateYProperty translateY} and {@link #translateZProperty translateZ}
* properties.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020
Member

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.

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Aug 10, 2020

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

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I left a couple inline comments. I think the changes to the two FXCollections methods that take an extractor parameter are fine, other than the one comment I made which would help unify the docs between the two methods.

@arapte
arapte approved these changes Aug 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2020

@nlisker This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8228570: Add various documentation clarifications

Reviewed-by: arapte, kcr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 3 commits pushed to the jfx15 branch:

  • 7a8708b: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors
  • 487854c: 8246343: Fix mistakes in FX API docs
  • fc38ce6: 8249647: Many classes in package javafx.beans.binding in module javafx.base have implicit no-arg constructors

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge jfx15 into your branch, and then specify the current head hash when integrating, like this: /integrate 7a8708b0ad39847243c9f2239fe3d44647ef9291.

➡️ To integrate this PR with the above commit message to the jfx15 branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Aug 13, 2020
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Aug 13, 2020

/integrate

@openjdk openjdk bot closed this Aug 13, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Aug 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2020

@nlisker The following commits have been pushed to jfx15 since your change was applied:

  • 7a8708b: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors
  • 487854c: 8246343: Fix mistakes in FX API docs
  • fc38ce6: 8249647: Many classes in package javafx.beans.binding in module javafx.base have implicit no-arg constructors

Your commit was automatically rebased without conflicts.

Pushed as commit 208d828.

@nlisker nlisker deleted the nlisker:8228570_Add_various_documentation_clarifications branch Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants