RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors

[Nir Lisker]

On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary <bchoudhary at openjdk.org> 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