Bug ID: JDK-8163384 Fix doclint errors and warnings in javafx.graphics module

JDK 9
9Fixed

Changeset: 1ada4df30f82 Author: ckyang Date: 2017-03-08 15:45 -0800 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/1ada4df30f82
08-03-2017
I only see one remaining problem (minor). modules/javafx.graphics/src/main/java/javafx/scene/Node.java /** * Defines a function to be called when a new touch point is pressed. - * @return the event handler when a new touch point is pressed. + * @return the event handler that is called when a new touch point is pressed * @since JavaFX 2.2 */ public final ObjectProperty<EventHandler<? super TouchEvent>> onTouchPressedProperty() { return getEventHandlerProperties().onTouchPressedProperty(); This looks fine, but is only one of many similar problems in Node.java No need for a new webrev if you want to fix the other event handlers similarly. +1
08-03-2017
Here is the revised webrev.03 that addresses all the feedback I received for 02. I also take this chance to include the fix for JDK-8172160 and some minor cleanup that helps to keep consistency within file I fixed. http://cr.openjdk.java.net/~ckyang/JDK-8163384/webrev.03/ Here is the delta diff between 02 and 03: http://cr.openjdk.java.net/~ckyang/JDK-8163384/webrev.0203.diff/
08-03-2017
Overall looks quite good. Here is my feedback. You can decide whether to do anything about the comments marked as "NIT:" modules/javafx.graphics/src/main/java/javafx/animation/Animation.java NIT: perhaps instead of replacing the following: * must otherwise be > 0. with: * must otherwise be > 0. You could use: * must otherwise be {@literal >} 0. modules/javafx.graphics/src/main/java/javafx/animation/KeyFrame.java NIT: Two comments: 1) We don't generally end @return statememts with "." 2) We don't generally say "of this {@code KeyFrame}" so that could be removed modules/javafx.graphics/src/main/java/javafx/animation/KeyValue.java + * @return the interpolator to be used for calculating the key value along + * the particular interval. NIT: You can remove the "." + * @param <T> the {@code KeyValue} That should be: "the type of the {@code KeyValue}" modules/javafx.graphics/src/main/java/javafx/animation/SequentialTransition.java + * @return a list of Animations that will be played sequentially. NIT: You can remove the "." modules/javafx.graphics/src/main/java/javafx/animation/Timeline.java + * @return the {@link KeyFrame KeyFrames} of this {@code Timeline}. NIT: Remove " of this {@code Timeline}." modules/javafx.graphics/src/main/java/javafx/application/Application.java - * <ul> * <pre> * public static void main(String[] args) { * Application.launch(MyApp.class, args); * } * </pre> - * </ul> NIT: Maybe not important, but you can preserve the intent of the <ul> ... </ul> by indenting the example code 4 spaces. * NOTE: This method is not called on the JavaFX Application Thread. An * application must not construct a Scene or a Stage in this * method. * An application may construct other JavaFX objects in this method. * </p> + * @throws java.lang.Exception if this method is not called on the JavaFX + * Application Thread. / public void init() throws Exception { } This is simply wrong (for two reasons). For one thing, this method is never called on the FX application thread. More to the point, however, this exception is not something that is thrown by the FX runtime, but rather allows the application sub-class to override this method and throw any checked exception without needing to catch it. Is the @throws java.lang.Exception needed? If so, then you would need to look in the JDK for similar examples. Similarly, the @throws description for start() and stop() is wrong. modules/javafx.graphics/src/main/java/javafx/application/HostServices.java NIT: Same comment as in Application about the indentation. Also, as long as you are changing it, you might consider using "{@code ...}" for the example so you can change the < back to <, etc. Up to you. modules/javafx.graphics/src/main/java/javafx/concurrent/Service.java + @param <V> the type of object returned by the Service. NIT: Maybe remove the "." ? modules/javafx.graphics/src/main/java/javafx/concurrent/Task.java - * public final ObservableList<Rectangle> getPartialResults() { return partialResults.get(); } - * public final ReadOnlyObjectProperty<ObservableList<Rectangle>> partialResultsProperty() { + * public final ObservableList<Rectangle> getPartialResults() { return partialResults.get(); } + * {@literal public final ReadOnlyObjectProperty<ObservableList<Rectangle>> partialResultsProperty()} { NIT: You can use @literal for the first of these two lines as well. modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java + * <pre> NIT: This didn't use "<code>" after "<pre>" -- should it? modules/javafx.graphics/src/main/java/javafx/css/CssParser.java + / Parse an in-line style from a Node This needs a "." added to the end. + / + * Convenience method for unit tests + * @param property the property + * @param expr the expression + * @return the parsed value + / public ParsedValue parseExpr(String property, String expr) { if (property == null \|\| expr == null) return null; Not just a doc issue, but this is not a good pattern to have public API that is only there for unit tests. Need to think about this one. modules/javafx.graphics/src/main/java/javafx/css/FontCssMetaData.java 47 The property name is concatenated with "-weight", 48 * "-style", "-family" and "-size" to 49 * create the sub-properties. For example, MINOR: It would be good to add an initial sentence like: * Constructs a FontCSSMetaData object from the specified property and initial Font. modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java - * <code> NIT: For consistency, would it better to keep "<code> ... </code>", but move it inside the pre? modules/javafx.graphics/src/main/java/javafx/css/Rule.java + * @return a observable list of Declaration It would be more grammatically correct to say "declarations" + * @return the converted string of this object NIT: You can remove " of this object" modules/javafx.graphics/src/main/java/javafx/css/Selector.java + * @return the boolean representing the state of the node and its parents + * matches the pseudo-classes defined (if any) for this selector This is not gramatically correct. Do you mean "...representing WHETHER the state of the node ..." ? Might be better to say: * @return {@code true} if the current state of the node and its parents matches * the pseudo-classes defined (if any) for this selector. modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java - * StyleConverter converts {@code ParsedValue&tl;F,T>} from type F to type T. The + * StyleConverter converts <code>{@literal ParsedValue<F,T>}</code> Better to simply use: {@code ParsedValue<F,T>} - * <code>public Color convert(ParsedValueImpl<String,Color> value, Font font)</code> + * <code>{@literal public Color convert(ParsedValueImpl<String,Color> value, Font font)}</code> Ditto. Can just use: {@code public Color convert(ParsedValueImpl<String,Color> value, Font font)} + * @return the converted target property type. NIT: Can remove the "." + * @param <E> A {@code StyleConverter} that converts a String representation + * of an {@code Enum} to an {@code Enum} This is not right. Maybe something like the following? * @param <E> the type of the Enum - * <code><pre> + * <code> NIT: Same comment earlier about moving <code> inside <pre> rather than deleting it (for consistency). + * Write binary data + * @param os the data output stream + * @param sstore the string store + * @throws java.io.IOException the exception * @since 9 / Add "." after the first sentence. Same for other methods and for the StringStore class. modules/javafx.graphics/src/main/java/javafx/css/Styleable.java + @see <a href="../scene/doc-files/cssref.html">CSS Reference Guide</a> COMMENT: I'm surprised this works, but as long as it does, then fine. + * @return the type of this {@code Styleable}. NIT: You can remove the "." + * @return the CssMetaData of this Styleable. NIT: You can remove " of this Styleable." + * <pre><code> + {@literal public ObservableValue<Double> offsetProperty()} { {@literal return (ObservableValue<Double>)offset;} } public Double getOffset() { return offset.getValue().doubleValue(); } public void setOffset(Double value) { offset.setValue(value); } - private final StyleableProperty<Number> offset = FACTORY.createStyleableNumberProperty(this, "offset", "-my-offset", s -> ((MyButton)s).offset); + {@literal private final StyleableProperty<Number> offset = FACTORY.createStyleableNumberProperty(this, "offset", "-my-offset", s -> ((MyButton)s).offset)}; * <br> - * </pre></code> + * </code></pre> NIT: You can use a single {@code ... } and avoid the multiple @literal tags (maybe not worth changing?) + * @param <E> The StyleableProperty created with initial value and inherit flag This should be: * @param <E> The type of StyleableProperty modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java + /** Load a binary stylesheet file from a input stream NIT: Add a "." at the end of the sentence. modules/javafx.graphics/src/main/java/javafx/geometry/Bounds.java NIT: You can remove the " of this {@code Bounds}." from the @return tags modules/javafx.graphics/src/main/java/javafx/geometry/Rectangle2D.java NIT: You can remove " of this {@code Rectangle2D}." from the @return tags. + * @return the value presents the job pages to print as an array of PageRange. NIT: You can remove the "." + * @return an instance of <code>Collation</code> NIT: Maybe better to use {@code Collation} ... modules/javafx.graphics/src/main/java/javafx/scene/Node.java - * <code><pre> + * <pre> * textnode.setLayoutX(finalX - textnode.getLayoutBounds().getMinX()); - * </pre></code> + * </pre> NIT: Maybe use <pre>{@code ... }</pre> pattern? + * @return should this {@code Node} be mirrored Better to say: * @return true if this {@code Node} should be mirrored + * @return the event handler when a new touch point is pressed. That is a little misleading. Better might be: * @return the event handler that is called when a new touch point is pressed modules/javafx.graphics/src/main/java/javafx/scene/Scene.java + * @return the function to be called when the mouse button is released on + * this scene during drag and drop gesture. NIT: You might remove the "." here? modules/javafx.graphics/src/main/java/javafx/scene/effect/DisplacementMap.java NIT: Can you use "{@code ...}" for the code sample? modules/javafx.graphics/src/main/java/javafx/scene/image/PixelFormat.java It would look better if you used {@code ... } for the code samples rather than changing the > into > modules/javafx.graphics/src/main/java/javafx/scene/input/DragEvent.java NIT: You can remove the "." from the @return tags modules/javafx.graphics/src/main/java/javafx/scene/input/KeyCode.java NIT: You can remove the "." from the @return tags modules/javafx.graphics/src/main/java/javafx/scene/input/Mnemonic.java NIT: Maybe use @code rather than @link for the second occurrence? modules/javafx.graphics/src/main/java/javafx/scene/input/ScrollEvent.java NIT: Maybe put <code> inside <pre> rather than removing it (or better still, if you can use {@code ...}) modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java NIT: + * @return The list of BackgroundFills NIT: we generally use lower-case for the first word of @param and @return tags modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundPosition.java + * @return true if horizontalPosition is in percentage, otherwise is a + * literal number That is confusing. Maybe best to just say what the "true" condition is: * @return true if horizontalPosition should be interpreted as a percentage Or you might say this: * @return true if horizontalPosition should be interpreted as a percentage, false if it * should be interpreted as a literal number modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundSize.java + * @return true if width is in percentage, otherwise is a normal value Same comment as in BackgroundPosition.java + * @return if true the image can fit inside the background positioning area That is awkward. You will want to reword it. modules/javafx.graphics/src/main/java/javafx/scene/layout/Border.java + * <a href="../scene/doc-files/cssref.html">JavaFX CSS Reference Guide</a> for a + * complete description of the CSS rules for styling the border of a Region. This is a broken link. You need to lose the "scene/" in the above URL I think. modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderImage.java + * @return if true the center patch should be drawn Awkward. + * @param filled If true the center patch should be drawn Not as bad when it is used in an @param, but still not ideal. Better might be: A flag indicating whether the center patch should be drawn modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderStrokeStyle.java + * @return he limit for the StrokeLineJoin.MITER line join style "the" limit... modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderWidths.java + * @return true if top is as a percentage of the region height otherwise false "is as a" is awkward. Either drop the "as" or change the "is as" to "should be interpreted as". Also, you need a comma before the "otherwise false". modules/javafx.graphics/src/main/java/javafx/scene/layout/CornerRadii.java + * @return if true topLeftHorizontalRadius is in percentage, otherwise a value Awkward. modules/javafx.graphics/src/main/java/javafx/scene/layout/GridPane.java + * @return if true lines are displayed to show the gridpane's rows and columns "true if ..." modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.java + * @return if true resizable children will be resized to fill the full + * height of the hbox or be resized to their preferred height and aligned + * according to the alignment vpos value This is misleading. As with earlier example, either just list the true case, or use something like "true if ....., false if ...." modules/javafx.graphics/src/main/java/javafx/scene/layout/VBox.java Same comment as with HBox. modules/javafx.graphics/src/main/java/javafx/scene/paint/RadialGradient.java + * @return if true the center and radius values are proportional otherwise + * absolute Awkward.
07-03-2017
[~vadim] Thanks for the feedback! I have fixed KeyFrame and Task based on your feedback. I did minimum change to Interpolator as I try to minimize the change in this fix (not touching doc that causes javadoc warning or error). I also decided not to add code tag to Platform as this will make the fix inconsistent within this file (non of the true and false in this file is code tag). And we (including JDK) are quite inconsistent on the use of code tag so I will try to minimize this type of change in my revision. Here is webre.02: http://cr.openjdk.java.net/~ckyang/JDK-8163384/webrev.02/ Here is the delta diff between 01 and 02: http://cr.openjdk.java.net/~ckyang/JDK-8163384/webrev.0102.diff/
07-03-2017
I compared maybe a third of built javadocs and here's what I've found: In the javafx.animation.Interpolator do you want to bring the first interpolate(Object, Object, double) method's paremeters description and return tag to the same style as other interpolate methods? javafx.animation.KeyFrame.equals - I think true and false are usually in the {@code}? javafx.application.Platform - the same in the isSupported javafx.concurrent.Task:295 - @ is missing before Override.
06-03-2017
Here is version 2: http://cr.openjdk.java.net/~ckyang/JDK-8163384/webrev.01/ Include the suggestions from Jonathan Gibbons: 1) No more empty alt="" in image tag. 2) Replace <pre><code>...</code></pre> with <pre>{@code ... }</pre> whenever possible except in cases where code block has special symbols such as <> or @. 3) Fix missing method comments or descriptions at best as I can 4) Reduce the use of < or > with {@literal } in code block but I didn't try to fix all as there are way too many places in existing codebase.
03-03-2017
This is a huge webrev so I hope the various component owners can help to review the proposed fix in his respective component. The focus of the fix here is to resolve all errors and warnings generated by doclint while preserving the look and format of the generated html doc if possible. There is almost zero effort spent to improve the clarity of the javadoc which is beyond the scope of this task. http://cr.openjdk.java.net/~ckyang/JDK-8163384/webrev.00/
28-02-2017

Blocks :	JDK-8090255 - Fix doclint errors and warnings in JavaFX API docs
Duplicate :	JDK-8172160 - time parameter not defined in Keyframe constructor API