RFR: 8271091: Missing API docs in UI controls classes [v2]

[Ajit Ghaisas]

On Fri, 22 Oct 2021 13:37:38 GMT, Kevin Rushforth <kcr at 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