Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v3]
On Wed, 9 Feb 2022 18:27:55 GMT, Ajit Ghaisas wrote: >> This is a Javadoc cleanup and correction fix for the TabPane as described in >> the JBS. >> >> Changes done for all the Properties of the TabPane - >> - Moved the property description to be over the property field. >> - Removed the unnecessary docs on property setter/getter and Property method. > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > Address additional review comments Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v3]
On Wed, 9 Feb 2022 18:27:55 GMT, Ajit Ghaisas wrote: >> This is a Javadoc cleanup and correction fix for the TabPane as described in >> the JBS. >> >> Changes done for all the Properties of the TabPane - >> - Moved the property description to be over the property field. >> - Removed the unnecessary docs on property setter/getter and Property method. > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > Address additional review comments Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 13:34:13 GMT, Nir Lisker wrote: >> Good suggestion. >> >> Also optional, you can remove the `` since they are not needed for the >> initial paragraph. > > There are many redundant `` tags in the property docs too if you want to > deal with them as well for consistency (not important). Fixed the description. Also, removed many redundant `` tags. - PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v3]
> This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: Address additional review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/728/files - new: https://git.openjdk.java.net/jfx/pull/728/files/66630a39..537dcac1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=01-02 Stats: 25 lines in 1 file changed: 0 ins; 4 del; 21 mod Patch: https://git.openjdk.java.net/jfx/pull/728.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/728/head:pull/728 PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 13:17:14 GMT, Kevin Rushforth wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
>> 738:
>>
>>> 736:
>>> 737: /**
>>> 738: * This specifies how the {@code TabPane} handles tab closing
>>> from an end-user's
>>
>> Since you are fixing the description here, can you remove the "This" in the
>> beginning?
>
> Good suggestion.
>
> Also optional, you can remove the `` since they are not needed for the
> initial paragraph.
There are many redundant `` tags in the property docs too if you want to
deal with them as well for consistency (not important).
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 13:06:58 GMT, Nir Lisker wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review comments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 738:
>
>> 736:
>> 737: /**
>> 738: * This specifies how the {@code TabPane} handles tab closing
>> from an end-user's
>
> Since you are fixing the description here, can you remove the "This" in the
> beginning?
Good suggestion.
Also optional, you can remove the `` since they are not needed for the
initial paragraph.
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 07:32:20 GMT, Ajit Ghaisas wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
>> 295:
>>
>>> 293: *
>>> 294: * This value can also be set via CSS using {@code
>>> -fx-tab-min-width}.
>>> 295: *
>>
>> I would write
>>
>> The minimum width of a tab in the TabPane. This can be used to limit
>> the length of text in tabs to prevent truncation. Setting the same minimum
>> and maximum widths will fix the width of the tab.
>>
>> It makes it clear that it applies to each tab individually (and not the
>> total width of the tabs). The same applies for the other min/max
>> height/width properties.
>>
>> The sentence "By default the min equals to the max." seems wrong. The
>> `DEFAULT_TAB_MIN_WIDTH` constant is set to 0. It should also be in a
>> `@defaultValue`.
>>
>> The CSS mention is good, but I never saw it mentioned for properties before.
>> Makes me wonder if we should add the CSS property as part of a property's
>> description in general via the automated javadoc tool.
>
> Good suggestion. Fixed.
>
> Regarding CSS mention for properties, I am not sure whether that can be
> automated. If there is a way to achieve it, I can file a separate bug to
> address it.
The CSS remark was me thinking out loud. I'll write about it in the mailing
list since it's a whole new capability of the doc tool.
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 09:39:46 GMT, Ajit Ghaisas wrote:
>> This is a Javadoc cleanup and correction fix for the TabPane as described in
>> the JBS.
>>
>> Changes done for all the Properties of the TabPane -
>> - Moved the property description to be over the property field.
>> - Removed the unnecessary docs on property setter/getter and Property method.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
Looks good to me. I added 1 optional comment.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
738:
> 736:
> 737: /**
> 738: * This specifies how the {@code TabPane} handles tab closing
> from an end-user's
Since you are fixing the description here, can you remove the "This" in the
beginning?
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
> This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/728/files - new: https://git.openjdk.java.net/jfx/pull/728/files/31ae34d7..66630a39 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=00-01 Stats: 59 lines in 1 file changed: 11 ins; 14 del; 34 mod Patch: https://git.openjdk.java.net/jfx/pull/728.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/728/head:pull/728 PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 16:17:32 GMT, Nir Lisker wrote:
>> This is a Javadoc cleanup and correction fix for the TabPane as described in
>> the JBS.
>>
>> Changes done for all the Properties of the TabPane -
>> - Moved the property description to be over the property field.
>> - Removed the unnecessary docs on property setter/getter and Property method.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 174:
>
>> 172: /**
>> 173: * Sets the model used for tab selection. By changing the model
>> you can alter
>> 174: * how the tabs are selected and which tabs are first or last.
>
> This description is phrased like it's a setter. Probably
>
> The selection model used for selecting tabs. Changing the model alters
> how the tabs are selected and which tabs are first or last.
>
> (there's no need to the `` either)
Fixed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 188:
>
>> 186: * the TabPane will immediately update the location of the tabs to
>> reflect
>> 187: * this.
>> 188: * The default position for the tabs is {@code Side.Top}.
>
> I would rephrase a bit:
>
> The position of the tabs in the {@code TabPane}. Changes to the value of this
> property
> immediately update the location of the tabs.
>
> @defaultValue {@code Side.Top}
>
> The 2nd sentence seems redundant anyway and I suggest to remove it. Unless
> otherwise specified, all value changes are applied immediately (only
> `Animation` properties come to mind that specify that the animation has to be
> stopped for the changes to take effect).
I like the rephrased version. I prefer keeping the 2nd sentence in this case as
it indicates a non-trivial visual update.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 244:
>
>> 242: * Refer to the {@link TabClosingPolicy} enumeration for further
>> details.
>> 243: *
>> 244: * The default closing policy is TabClosingPolicy.SELECTED_TAB
>
> * The default value should be in an `@defaultValue` annotation.
> * Missing `{@code }`s
> * The line "Refer to the {@link TabClosingPolicy}" is redundant I think since
> it's linked in the signature/definition, and if we want to keep it then an
> `@see` might be preferable.
> * "end-user's" (missing apostrophe)
Fixed points 1, 2 and 4.
Regarding point 3 -
I have removed the description regarding possible options and have kept only
the line-
`Refer to the {@link TabClosingPolicy} enumeration for further details.`
Mentioning all the options here seems to be a duplication of Javadoc for
enumeration TabClosingPolicy.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 270:
>
>> 268: * the graphic isn't rotated, resulting in it always appearing
>> upright. If
>> 269: * rotateGraphic is set to {@code true}, the graphic will rotate
>> such that it
>> 270: * rotates with the tab text.
>
> * Missing `@code`s
> * I would rephrase the 2nd paragraph to be simpler:
> ```
> If the value is {@code false}, the graphic isn't rotated, resulting in it
> always appearing upright.
> If it is {@code true}, the graphic is rotate with the tab text.
> ```
> * `@defaultValue {@code false}`
Fixed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 295:
>
>> 293: *
>> 294: * This value can also be set via CSS using {@code
>> -fx-tab-min-width}.
>> 295: *
>
> I would write
>
> The minimum width of a tab in the TabPane. This can be used to limit
> the length of text in tabs to prevent truncation. Setting the same minimum
> and maximum widths will fix the width of the tab.
>
> It makes it clear that it applies to each tab individually (and not the total
> width of the tabs). The same applies for the other min/max height/width
> properties.
>
> The sentence "By default the min equals to the max." seems wrong. The
> `DEFAULT_TAB_MIN_WIDTH` constant is set to 0. It should also be in a
> `@defaultValue`.
>
> The CSS mention is good, bit I never saw it mentioned for properties before.
> Makes me wonder if we should add the CSS property as part of a property's
> description in general via the automated javadoc tool.
Good suggestion. Fixed.
Regarding CSS mention for properties, I am not sure whether that can be
automated. If there is a way to achieve it, I can file a separate bug to
address it.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 337:
>
>> 335: *
>> 336: * This value can also be set via CSS using {@code
>> -fx-tab-max-width}.
>> 337: *
>
> * "By default the min equals to the max." The default is `Double.MAX_VALUE`.
> * Comma before "the text will be truncated".
Fixed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 378:
>
>> 376: *
>> 377: * This value can also be set via CSS using {@code
>> -fx-tab-min-h
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 16:24:46 GMT, Kevin Rushforth wrote:
>> This is a Javadoc cleanup and correction fix for the TabPane as described in
>> the JBS.
>>
>> Changes done for all the Properties of the TabPane -
>> - Moved the property description to be over the property field.
>> - Removed the unnecessary docs on property setter/getter and Property method.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 188:
>
>> 186: * the TabPane will immediately update the location of the tabs to
>> reflect
>> 187: * this.
>> 188: * The default position for the tabs is {@code Side.Top}.
>
> Ordinarily we would use a `@defaultValue` tag here. In this case, you are
> just moving existing docs from one method to another, so it could be done in
> a follow-up bug. Can you file a follow-up bug?
I have fixed it as part of this PR itself.
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 10:53:00 GMT, Ajit Ghaisas wrote:
> This is a Javadoc cleanup and correction fix for the TabPane as described in
> the JBS.
>
> Changes done for all the Properties of the TabPane -
> - Moved the property description to be over the property field.
> - Removed the unnecessary docs on property setter/getter and Property method.
Moving the test to the property field and removing it from the methods looks
good. I added some suggestions to touch up the docs a bit and correct some
mistakes.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
174:
> 172: /**
> 173: * Sets the model used for tab selection. By changing the model
> you can alter
> 174: * how the tabs are selected and which tabs are first or last.
This description is phrased like it's a setter. Probably
The selection model used for selecting tabs. Changing the model alters
how the tabs are selected and which tabs are first or last.
(there's no need to the `` either)
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
188:
> 186: * the TabPane will immediately update the location of the tabs to
> reflect
> 187: * this.
> 188: * The default position for the tabs is {@code Side.Top}.
I would rephrase a bit:
The position of the tabs in the {@code TabPane}. Changes to the value of this
property
immediately update the location of the tabs.
@defaultValue {@code Side.Top}
The 2nd sentence seems redundant anyway and I suggest to remove it. Unless
otherwise specified, all value changes are applied immediately (only
`Animation` properties come to mind that specify that the animation has to be
stopped for the changes to take effect).
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
244:
> 242: * Refer to the {@link TabClosingPolicy} enumeration for further
> details.
> 243: *
> 244: * The default closing policy is TabClosingPolicy.SELECTED_TAB
* The default value should be in an `@defaultValue` annotation.
* Missing `{@code }`s
* The line "Refer to the {@link TabClosingPolicy}" is redundant I think since
it's linked in the signature/definition, and if we want to keep it then an
`@see` might be preferable.
* "end-user's" (missing apostrophe)
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
270:
> 268: * the graphic isn't rotated, resulting in it always appearing
> upright. If
> 269: * rotateGraphic is set to {@code true}, the graphic will rotate
> such that it
> 270: * rotates with the tab text.
* Missing `@code`s
* I would rephrase the 2nd paragraph to be simpler:
```
If the value is {@code false}, the graphic isn't rotated, resulting in it
always appearing upright.
If it is {@code true}, the graphic is rotate with the tab text.
```
* `@defaultValue {@code false}`
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
295:
> 293: *
> 294: * This value can also be set via CSS using {@code
> -fx-tab-min-width}.
> 295: *
I would write
The minimum width of a tab in the TabPane. This can be used to limit
the length of text in tabs to prevent truncation. Setting the same minimum
and maximum widths will fix the width of the tab.
It makes it clear that it applies to each tab individually (and not the total
width of the tabs). The same applies for the other min/max height/width
properties.
The sentence "By default the min equals to the max." seems wrong. The
`DEFAULT_TAB_MIN_WIDTH` constant is set to 0. It should also be in a
`@defaultValue`.
The CSS mention is good, bit I never saw it mentioned for properties before.
Makes me wonder if we should add the CSS property as part of a property's
description in general via the automated javadoc tool.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
337:
> 335: *
> 336: * This value can also be set via CSS using {@code
> -fx-tab-max-width}.
> 337: *
* "By default the min equals to the max." The default is `Double.MAX_VALUE`.
* Comma before "the text will be truncated".
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
378:
> 376: *
> 377: * This value can also be set via CSS using {@code
> -fx-tab-min-height}.
> 378: *
Same comments as for the width properties.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
419:
> 417: *
> 418: * This value can also be set via CSS using {@code
> -fx-tab-max-height}.
> 419: *
Same comments as for the width properties.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
779:
> 777: * The drag policy for the tabs. The policy can be changed
> dynamically.
> 778: *
> 779: * @defaultValue TabDragPolicy.FIXED
I think that the 2nd sentence is redundant again. I would add an explanation
like "Specifies if tabs can be reordered or no
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 10:53:00 GMT, Ajit Ghaisas wrote:
> This is a Javadoc cleanup and correction fix for the TabPane as described in
> the JBS.
>
> Changes done for all the Properties of the TabPane -
> - Moved the property description to be over the property field.
> - Removed the unnecessary docs on property setter/getter and Property method.
I looked at the first two properties, and they look fine. I'll do a detailed
review of the rest, but I wanted to point out one thing first. See below.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
188:
> 186: * the TabPane will immediately update the location of the tabs to
> reflect
> 187: * this.
> 188: * The default position for the tabs is {@code Side.Top}.
Ordinarily we would use a `@defaultValue` tag here. In this case, you are just
moving existing docs from one method to another, so it could be done in a
follow-up bug. Can you file a follow-up bug?
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 10:53:00 GMT, Ajit Ghaisas wrote: > This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. I'm reviewing this right now. - PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 10:53:00 GMT, Ajit Ghaisas wrote: > This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. Since we are in rampdown (RDP2) for JavaFX 18, I'd like @arapte to review this also. - PR: https://git.openjdk.java.net/jfx/pull/728
[jfx18] RFR: 8271085: TabPane: Redundant API docs
This is a Javadoc cleanup and correction fix for the TabPane as described in the JBS. Changes done for all the Properties of the TabPane - - Moved the property description to be over the property field. - Removed the unnecessary docs on property setter/getter and Property method. - Commit messages: - Fix Property javadocs Changes: https://git.openjdk.java.net/jfx/pull/728/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271085 Stats: 122 lines in 1 file changed: 17 ins; 97 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/728.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/728/head:pull/728 PR: https://git.openjdk.java.net/jfx/pull/728
