On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary <[email protected]>
wrote:
> Added missing explicit no-arg constructors to classes in package
> javafx.scene, javafx.css and javafx.stage.
I've completed a partial review. I think that just mechanically adding a
constructor with the same doc everywhere is
not a good approach. The classes should be inspected to see if one is needed
and if its doc is suitable. See my
comments on some of the classes I've looked at.
modules/javafx.controls/src/main/java/javafx/scene/control/TableFocusModel.java
line 51:
> 50:
> 51: /**
> 52: * Causes the item at the given index to receive the focus.
Please add a missing `)` for the class docs on line 30.
modules/javafx.controls/src/main/java/javafx/scene/control/TableSelectionModel.java
line 46:
> 45:
> 46: /**
> 47: * Convenience function which tests whether the given row and column
> index
Same missing `)`
modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 83:
> 82:
> 83: /**
> 84: * There is only one PseudoClass instance for a given pseudoClass.
1. Is having a public constructor for this class a good idea? Instances are
created by a factory method.
2. Both methods in this class need documentation update. `getPseudoClass` has a
poor method description and the
formatting of the `@return` tag is wrong (lowercase and no period).
`getPseudoClassName` is missing a description.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 51:
> 50:
> 51: private static class UniversalSelector {
> 52: private static final Selector INSTANCE =
Is a public constructor suitable in this class? `createSelector(String)` is the
factory. There are public abstract
methods here, on the other hand, so I don't know what the design idea is. The
class docs should be updated too to say
how to use this class.
modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 95:
> 94:
> 95: /**
> 96: * Convert from the parsed CSS value to the target property type.
Looks like a constructor is fine here if the predefined factories are not
suitable, but I'm not familiar with this.
modules/javafx.graphics/src/main/java/javafx/css/converter/ShapeConverter.java
line 51:
> 50: }
> 51:
> 52: @Override public Shape convert(ParsedValue<String, Shape> value, Font
> font) {
Looks like a singleton class.
modules/javafx.graphics/src/main/java/javafx/scene/input/ClipboardContent.java
line 45:
> 44: */
> 45: public ClipboardContent() {
> 46: }
Not sure that "default" means anything here. I don't see any configuration.
modules/javafx.graphics/src/main/java/javafx/application/Preloader.java line
121:
> 120: public Preloader() {
> 121: }
> 122:
Not sure that "default" means anything here. I don't see any configuration.
-------------
Changes requested by nlisker (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/283
