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

2021-10-25 Thread Ajit Ghaisas
On Fri, 22 Oct 2021 13:37:38 GMT, Kevin Rushforth  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  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


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

2021-10-22 Thread Kevin Rushforth
On Thu, 21 Oct 2021 08:58:37 GMT, Ajit Ghaisas  wrote:

>> This PR fixes javadoc warnings in javafx.controls and javafx.web modules.
>> Note : 
>> - The javadoc needs to be generated with the JDK 18 EA build.
>> - There are still 20 javadoc warnings remaining in javafx.controls module 
>> and 3 warnings remaining in javafx.web module. The root cause is different 
>> and they will be addressed under 
>> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   javadoc minor corrections

See comments below.

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

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

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.

modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java line 
47:

> 45: /**
> 46:  * Gets the default singleton {@code SortEvent}.
> 47:  * @param  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.

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

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

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.

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.

modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableColumn.java 
line 625:

> 623:  * Gets the {@code CssMetaData} associated with this class, which 
> may include the
> 624:  * {@code CssMetaData} of its superclasses.
> 625:  * @return empty list is returned as of now

Minor: I recommend adding a second sentence to the Description to the effect 
that it is currently an empty list, and then have the return tag be simply 
`@return an empty list`

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextInputControlSkin.java
 line 118:

> 116: /** Line unit */  LINE,
> 117: /** Paragraph unit */ PARAGRAPH,
> 118: /** Page unit */  PAGE };

I would prefer that the javadoc tag and the enum be on separate lines.


/** 

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

2021-10-21 Thread Ajit Ghaisas
> This PR fixes javadoc warnings in javafx.controls and javafx.web modules.
> Note : 
> - The javadoc needs to be generated with the JDK 18 EA build.
> - There are still 20 javadoc warnings remaining in javafx.controls module and 
> 3 warnings remaining in javafx.web module. The root cause is different and 
> they will be addressed under 
> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996)

Ajit Ghaisas has updated the pull request incrementally with one additional 
commit since the last revision:

  javadoc minor corrections

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/646/files
  - new: https://git.openjdk.java.net/jfx/pull/646/files/6f802004..38563835

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=646=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=646=00-01

  Stats: 55 lines in 47 files changed: 0 ins; 0 del; 55 mod
  Patch: https://git.openjdk.java.net/jfx/pull/646.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646

PR: https://git.openjdk.java.net/jfx/pull/646