Re: [jfx22] RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Sat, 24 Feb 2024 09:53:01 GMT, Nir Lisker wrote: > Backport of commit > [b43c4edf](https://github.com/openjdk/jfx/commit/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. I note that this is a clean backport of a doc-only fix. Approved to go into jfx22. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1380#pullrequestreview-1899492670
[jfx22] RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
Backport of commit [b43c4edf](https://github.com/openjdk/jfx/commit/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. - Commit messages: - Backport b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b Changes: https://git.openjdk.org/jfx/pull/1380/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1380&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325550 Stats: 66 lines in 3 files changed: 19 ins; 25 del; 22 mod Patch: https://git.openjdk.org/jfx/pull/1380.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1380/head:pull/1380 PR: https://git.openjdk.org/jfx/pull/1380
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc [v2]
On Fri, 23 Feb 2024 22:32:23 GMT, Andy Goryachev wrote: >> `@return` statements usually state this condition explicitly, like >> `Map#get`: "the value to which the specified key is mapped, or `null` if >> this map contains no mapping for the key". > > My point is that it should be mentioned in the first paragraph then. It can > be repeated in @return later, if you prefer. Map.get() does indeed mention > this in the first paragraph: > >  Given that the changes are so far limited to spelling and formatting (code style for null), I think it would be fine to limit the changes in this PR to what he has already done. - PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501267342
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc [v2]
On Fri, 23 Feb 2024 22:32:16 GMT, Nir Lisker wrote: >> Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs >> in `Node` and `Scene`. >> >> Note that the default value for a `Scene`'s `NodeOrientation` depends on a >> system property, while for `Node` it isn't (which means `SubScene` will be >> different from `Scene`). Not sure if this is intended. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comments Marked as reviewed by angorya (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1379#pullrequestreview-1899094754
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc [v2]
On Fri, 23 Feb 2024 22:32:16 GMT, Nir Lisker wrote: >> Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs >> in `Node` and `Scene`. >> >> Note that the default value for a `Scene`'s `NodeOrientation` depends on a >> system property, while for `Node` it isn't (which means `SubScene` will be >> different from `Scene`). Not sure if this is intended. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comments Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1379#pullrequestreview-1899088659
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 22:28:59 GMT, Nir Lisker wrote: > Only `Scene` has the RTL system property checked. I suppose because Scene must INHERIT this value from the system? - PR Comment: https://git.openjdk.org/jfx/pull/1379#issuecomment-1962094000
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc [v2]
On Fri, 23 Feb 2024 21:58:30 GMT, Nir Lisker wrote: >> I don't mind either way. > > `@return` statements usually state this condition explicitly, like `Map#get`: > "the value to which the specified key is mapped, or `null` if this map > contains no mapping for the key". My point is that it should be mentioned in the first paragraph then. It can be repeated in @return later, if you prefer. Map.get() does indeed mention this in the first paragraph:  - PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501237620
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 17:46:20 GMT, Nir Lisker wrote: > Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs in > `Node` and `Scene`. > > Note that the default value for a `Scene`'s `NodeOrientation` depends on a > system property, while for `Node` it isn't (which means `SubScene` will be > different from `Scene`). Not sure if this is intended. By the way, is the difference in the default values between `Node` and `Scene` intentional? Only `Scene` has the RTL system property checked. - PR Comment: https://git.openjdk.org/jfx/pull/1379#issuecomment-1962085277
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc [v2]
> Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs in > `Node` and `Scene`. > > Note that the default value for a `Scene`'s `NodeOrientation` depends on a > system property, while for `Node` it isn't (which means `SubScene` will be > different from `Scene`). Not sure if this is intended. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1379/files - new: https://git.openjdk.org/jfx/pull/1379/files/dac77a49..a3c375df Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1379&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1379&range=00-01 Stats: 53 lines in 2 files changed: 21 ins; 25 del; 7 mod Patch: https://git.openjdk.org/jfx/pull/1379.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1379/head:pull/1379 PR: https://git.openjdk.org/jfx/pull/1379
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 19:38:20 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java
>> line 187:
>>
>>> 185: * Returns the child's bottom anchor constraint, if set.
>>> 186: * @param child the child node of an anchor pane
>>> 187: * @return the offset from the bottom of the anchor pane, or
>>> {@code null} if no bottom anchor was set
>>
>> minor suggestion: move explanation of when it returns null to the
>> description, i.e.:
>>
>>
>> Returns the child's bottom anchor constraint, if set, otherwise returns
>> {@code null}.
>>
>> @return the offset from the bottom of the anchor pane, or {@code null}
>
> I don't mind either way.
`@return` statements usually state this condition explicitly, like `Map#get`:
"the value to which the specified key is mapped, or `null` if this map contains
no mapping for the key".
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501214852
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 20:16:55 GMT, Nir Lisker wrote: > I suggest to move the doc to the field. Yeah that's even better. - PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501132703
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 19:44:06 GMT, Andy Goryachev wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 6272:
>>
>>> 6270: * of text in both worlds.
>>> 6271: *
>>> 6272: * @defaultValue if the system property {@code
>>> javafx.scene.nodeOrientation.RTL} is {@code true},
>>
>> Restore the `@return` to avoid a warning.
>
> it seems that placing javadoc at the property declaration instead of the
> method which returns said property eliminates the warning.
> See for example Stage:1329
Yes, this is why I was surprised that new warnings showed up. The `@return`
text is auto-generated for properties, so why would a return tag be needed? I
guess the doc tool doesn't understand that when the doc is placed on the
property getter instead of on the field, which I think is a bug.
I suggest to move the doc to the field.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501126942
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 19:34:41 GMT, Kevin Rushforth wrote:
>> Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs
>> in `Node` and `Scene`.
>>
>> Note that the default value for a `Scene`'s `NodeOrientation` depends on a
>> system property, while for `Node` it isn't (which means `SubScene` will be
>> different from `Scene`). Not sure if this is intended.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 6272:
>
>> 6270: * of text in both worlds.
>> 6271: *
>> 6272: * @defaultValue if the system property {@code
>> javafx.scene.nodeOrientation.RTL} is {@code true},
>
> Restore the `@return` to avoid a warning.
it seems that placing javadoc at the property declaration instead of the method
which returns said property eliminates the warning.
See for example Stage:1329
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501100898
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 18:31:10 GMT, Andy Goryachev wrote:
>> Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs
>> in `Node` and `Scene`.
>>
>> Note that the default value for a `Scene`'s `NodeOrientation` depends on a
>> system property, while for `Node` it isn't (which means `SubScene` will be
>> different from `Scene`). Not sure if this is intended.
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java
> line 187:
>
>> 185: * Returns the child's bottom anchor constraint, if set.
>> 186: * @param child the child node of an anchor pane
>> 187: * @return the offset from the bottom of the anchor pane, or {@code
>> null} if no bottom anchor was set
>
> minor suggestion: move explanation of when it returns null to the
> description, i.e.:
>
>
> Returns the child's bottom anchor constraint, if set, otherwise returns
> {@code null}.
>
> @return the offset from the bottom of the anchor pane, or {@code null}
I don't mind either way.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501096059
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 17:46:20 GMT, Nir Lisker wrote:
> Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs in
> `Node` and `Scene`.
>
> Note that the default value for a `Scene`'s `NodeOrientation` depends on a
> system property, while for `Node` it isn't (which means `SubScene` will be
> different from `Scene`). Not sure if this is intended.
The changes to the docs look fine, although I note the same additional warnings
that Andy saw (restoring the `@return` will fix it; the text after the
`@return` doesn't matter, since it is unused).
Also, I think it would be better to revert all code changes, even though they
look harmless.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 6449:
> 6447: EFFECTIVE_ORIENTATION_LTR | AUTOMATIC_ORIENTATION_LTR;
> 6448:
> 6449: private static final NodeOrientation DEFAULT_NODE_ORIENTATION =
> NodeOrientation.INHERIT;
This is unrelated to the doc change (and the addition of a static final const
makes it not strictly a doc-only change). Presuming that you want to backport
this to jfx22, I recommend to revert this change. Especially since the RC
deadline is next week.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 6465:
> 6463: * of text in both worlds.
> 6464: *
> 6465: * @defaultValue {@code NodeOrientation.INHERIT}
The `@return` is still needed (although unused) to avoid a warning.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 6245:
> 6243:
> 6244: @SuppressWarnings("removal")
> 6245: private static final NodeOrientation DEFAULT_NODE_ORIENTATION =
Same comment as in Node.java -- I recommend reverting this change if you intend
to backport it to jfx22.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 6272:
> 6270: * of text in both worlds.
> 6271: *
> 6272: * @defaultValue if the system property {@code
> javafx.scene.nodeOrientation.RTL} is {@code true},
Restore the `@return` to avoid a warning.
-
PR Review: https://git.openjdk.org/jfx/pull/1379#pullrequestreview-1898671234
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501010198
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501010794
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501011782
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501093086
Re: RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 17:46:20 GMT, Nir Lisker wrote:
> Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs in
> `Node` and `Scene`.
>
> Note that the default value for a `Scene`'s `NodeOrientation` depends on a
> system property, while for `Node` it isn't (which means `SubScene` will be
> different from `Scene`). Not sure if this is intended.
looks good (with minor suggestion)
however, this change adds 2 javadoc warnings which need to be addressed, I
would think .
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 6465:
> 6463: * of text in both worlds.
> 6464: *
> 6465: * @defaultValue {@code NodeOrientation.INHERIT}
adds a javadoc warning:
/Users/angorya/Projects/jfx3/jfx/rt/modules/javafx.graphics/src/main/java/javafx/scene/Node.java:6468:
warning: no @return
public final ObjectProperty nodeOrientationProperty() {
^
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 6273:
> 6271: *
> 6272: * @defaultValue if the system property {@code
> javafx.scene.nodeOrientation.RTL} is {@code true},
> 6273: * {@code NodeOrientation.RIGHT_TO_LEFT}, otherwise {@code
> NodeOrientation.INHERIT}
adds a javadoc warning:
/Users/angorya/Projects/jfx3/jfx/rt/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java:6276:
warning: no @return
public final ObjectProperty nodeOrientationProperty() {
^
modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java line
187:
> 185: * Returns the child's bottom anchor constraint, if set.
> 186: * @param child the child node of an anchor pane
> 187: * @return the offset from the bottom of the anchor pane, or {@code
> null} if no bottom anchor was set
minor suggestion: move explanation of when it returns null to the description,
i.e.:
Returns the child's bottom anchor constraint, if set, otherwise returns {@code
null}.
@return the offset from the bottom of the anchor pane, or {@code null}
modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java line
209:
> 207: * Returns the child's right anchor constraint, if set.
> 208: * @param child the child node of an anchor pane
> 209: * @return the offset from the right of the anchor pane, or {@code
> null} if no right anchor was set
and here
-
Changes requested by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1379#pullrequestreview-1898708803
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501034793
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501035303
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501033228
PR Review Comment: https://git.openjdk.org/jfx/pull/1379#discussion_r1501033642
RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs in `Node` and `Scene`. Note that the default value for a `Scene`'s `NodeOrientation` depends on a system property, while for `Node` it isn't (which means `SubScene` will be different from `Scene`). Not sure if this is intended. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jfx/pull/1379/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1379&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325550 Stats: 41 lines in 3 files changed: 4 ins; 6 del; 31 mod Patch: https://git.openjdk.org/jfx/pull/1379.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1379/head:pull/1379 PR: https://git.openjdk.org/jfx/pull/1379
