On Tue, 27 Jul 2021 12:44:58 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
>> This PR corrects/adds missing documentation for classes in javafx.css
>> package.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8250590 - add missing @since tag
I did a pretty complete review of the docs, and left what are mostly minor (and
in some cases picky) comments.
modules/javafx.graphics/src/main/java/javafx/css/CssParser.java line 110:
> 108:
> 109: /**
> 110: * A parser for CSS document string.
I suggest either `document strings.` or `a document string`.
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java line 36:
> 34:
> 35: /**
> 36: * This class serves as a container of CSS property and its value.
`a CSS property`
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java line 91:
> 89: /**
> 90: * Gets the importance of this {@code Declaration}.
> 91: * @return important
Maybe `@return the important flag` or `@return whether this {@code Declaration}
is important`?
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java line 113:
> 111: * the {@code Declaration} belongs. Only the property, value and
> importance are
> 112: * considered.
> 113: * </p>
No need for a `</p>`. Also there is a missing `@param obj` tag.
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java line 114:
> 112: * considered.
> 113: * </p>
> 114: * @return true if this object is the same as the obj argument;
> false otherwise.
I recommend code style for `true`, `obj`, and `false`.
modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 87:
> 85: * <p>
> 86: * Note: There is only one {@code PseudoClass} instance for a given
> pseudo class name.
> 87: * </p>
No need for `</p>`.
modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 99:
> 97: /**
> 98: * Gets the name of the {@code PseudoClass}.
> 99: * @return the name of the {@code PseudoClass}.
You can remove the `.` here.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 101:
> 99:
> 100: /**
> 101: * Same as the matches method expect return true/false rather than a
> match.
Perhaps add an initial sentence like `Gets whether this {@code Selector}
applies to the given {@code Styleable}.`? This (now second) sentence could use
a little rewording to make it a complete sentence and `matches` could maybe be
a link.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 103:
> 101: * Same as the matches method expect return true/false rather than a
> match.
> 102: * @param styleable styleable to match.
> 103: * @return {@code true} if this {@code Selector} applies to the
> given {@code Styleable}.
No need for periods for these two tags.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 108:
> 106:
> 107: /**
> 108: * Same as applies, but will return pseudoclass state that it finds
> along the way.
Same comment as above.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 113:
> 111: * @param depth depth of the {@code Node} heirarchy to look for
> 112: * @return {@code true} if this {@code Selector} and a set of {@code
> PseudoClass}
> 113: * applies to the given {@code Styleable}.
No need for period here.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 133:
> 131: * Writes {@code Selector} data in binary form to given {@code
> DataOutputStream}.
> 132: * @param os {@code DataOutputStream} to write {@code Selector} data
> to
> 133: * @param stringStore unsused
Typo: `unsused` --> `unused`
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 163:
> 161: /**
> 162: * Creates a {@code Selector} object.
> 163: * @param cssSelector css selector string
`CSS` should be upper case (not the parameter name, of course)
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 164:
> 162: * Creates a {@code Selector} object.
> 163: * @param cssSelector css selector string
> 164: * @return a Selector
`selector` or `{@code Selector}`
modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 74:
> 72: /**
> 73: * Gets Immutable List of style-classes of the selector.
> 74: * @return Immutable List<String> of style-classes of the
> selector
Maybe better in this case to just say `list` (lower case) for both the
description and `@return`? Otherwise, use code style. Also, "immutable" should
not be capitalized.
modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 90:
> 88: /**
> 89: * Gets the {@code Set} of {@code StyleClass} of the selector.
> 90: * @return set of style class
style classes (plural)
modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 151:
> 149: // Used in Match. If nodeOrientation is ltr or rtl,
> 150: // then count it as a pseudoclass
> 151: /** Gets {@code NodeOrientation} of this Selector.
Minor: move the text to the next line so the `/**` is on a line by itself.
modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 152:
> 150: // then count it as a pseudoclass
> 151: /** Gets {@code NodeOrientation} of this Selector.
> 152: * @return nodeOrientation
either `node orientation` or `{@code NodeOrientation}`
modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 567:
> 565:
> 566: /**
> 567: * Writes the StringStore strings to a given {@code
> DataOutputStream}.
`{@code StringStore}`
modules/javafx.graphics/src/main/java/javafx/css/converter/EffectConverter.java
line 41:
> 39:
> 40: /**
> 41: * Converter to convert representation of an {@code Effect} to an {@code
> Effect}.
`a representation` or maybe `a string representation`?
modules/javafx.graphics/src/main/java/javafx/css/converter/EffectConverter.java
line 82:
> 80:
> 81: /**
> 82: * Converter to convert DropShadow {@code Effect}.
Maybe `Converter to convert a {@code DropShadow}.` or `Converter to convert a
{@code DropShadow} effect.`
modules/javafx.graphics/src/main/java/javafx/css/converter/EnumConverter.java
line 127:
> 125: */
> 126: // package for unit testing
> 127: static public StyleConverter<?,?> getInstance(final String ename) {
Independent of the doc change, the inline comment is wrong: This is a public,
not a package-scope method. Maybe remove the comment?
modules/javafx.graphics/src/main/java/javafx/css/converter/StringConverter.java
line 47:
> 45: /**
> 46: * Get the {@code StringConverter} instance.
> 47: * @return the {@code StringConverter} instance.
You can remove the period.
modules/javafx.graphics/src/main/java/javafx/css/converter/StringConverter.java
line 72:
> 70:
> 71: /**
> 72: * Converter to convert a sequence of {@code String}s to a String[].
I recommend code style for the `String[]` or else say `an array of {@code
String}s`.
modules/javafx.graphics/src/main/java/javafx/css/converter/StringConverter.java
line 79:
> 77: /**
> 78: * Get the {@code SequenceConverter} instance.
> 79: * @return the {@code SequenceConverter} instance.
Extra period.
modules/javafx.graphics/src/main/java/javafx/css/converter/URLConverter.java
line 254:
> 252:
> 253: /**
> 254: * Converter to convert a sequence of URLs to a String[].
Same comment as above about using code style for `String[]`
-------------
PR: https://git.openjdk.java.net/jfx/pull/589
