On Fri, 22 Oct 2021 13:37:38 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> javadoc minor corrections
>
> modules/javafx.controls/src/main/java/javafx/scene/control/DatePicker.java
> line 470:
>
>> 468: * {@code CssMetaData} of its superclasses.
>> 469: * @return the {@code CssMetaData}
>> 470: * @since JavaFX 8.0
>
> The class already has an `@since JavaFX 8.0` tag so this is unnecessary. It's
> also unrelated to this fix (and we would need a CSR for any changes to
> `@since` tags).
OK. I will remove this since tag.
> modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
> line 160:
>
>> 158: /**
>> 159: * Focuses the item at the given index.
>> 160: * @param index the index of the item that needs to be focused
>
> Minor: I recommend removing "that needs" from this `@param`.
Done.
> modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
> line 201:
>
>> 199: * this specific {@code PopupControl}.
>> 200: * @return the {@code StringProperty} representing the CSS style
>> 201: */
>
> I recommend taking the information from the setter and copying it here. The
> docs can/should then be removed from both the setter and the getter. The
> first sentence should describe the property so does not need to start with
> "Gets the ...". You can add the information about `null` being turned into
> the empty string as a sentence in the Description. The `@defaultValue empty
> string` means that the information in the `@return` of the getter is
> unnecessary.
I could move the description from the setter to the javadoc of `getStyle()`
method as suggested. A `@return` is still needed for `getStyle()` as not to get
the javadoc warning.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 47:
>
>> 45: /**
>> 46: * Gets the default singleton {@code SortEvent}.
>> 47: * @param <C> the type of control
>
> Can you also add this `@param` tag to the class description? I'm a little
> surprised that javadoc doesn't flag it as missing.
Added.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 59:
>
>> 57: /**
>> 58: * Constructs a new {@code Event} with the specified event source,
>> target
>> 59: * and type. If the source or target is set to {@code null}, it is
>> replaced
>
> That should be "a new `{@code SortEvent}` ...". Also, since there is no type
> parameter, you should remove "and type" (and replace the comma after source
> with "and").
Done.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 63:
>
>> 61: *
>> 62: * @param source the event source which sent the event
>> 63: * @param target the target of the scroll to operation
>
> That should be "the target of the event".
Changed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 405:
>
>> 403: }
>> 404:
>> 405: public final DoubleProperty tabMaxWidthProperty() {
>
> The best practice for properties is to put the description on exactly one of
> the `xxx` private field or the `xxxProperty` method, and not on the setter or
> getter (unless there is a specific need to document something special in the
> setter or getter). There is a separate JBS issue,
> [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085), tracking the
> fix for the `maxWidth` and `maxHeight` properties.
>
> You can either revert this change, in which case we'll need a new PR for that
> issue (it can be done later), or else modify this change to match the best
> practice as noted in JDK-8271085 and add that issue to this PR. The latter
> will require addressing the other methods in this file as noted in that JBS
> bug.
I prefer reverting this and handling it separately under a separate PR.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 501:
>
>> 499: }
>> 500:
>> 501: public final DoubleProperty tabMaxHeightProperty() {
>
> Same comment as above. This needs to be reverted or modified.
I prefer reverting this and handling it separately under a separate PR.
-------------
PR: https://git.openjdk.java.net/jfx/pull/646
