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

Kevin Rushforth kcr at openjdk.java.net
Wed Aug 19 00:14:21 UTC 2020

On Mon, 17 Aug 2020 12:31:14 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Added missing explicit no-arg constructors to classes in package javafx.scene, javafx.css and javafx.stage.
> 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.

The change to the class header seems unrelated. It would be a good thing to fix, but could be done as part of a
follow-on doc cleanup issue.

> 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.

Right, but isn't that true of most of the classes, including those in the two related bugs that were fixed?

Worth noting is that the JDK chose different language for abstract classes than concrete ones. For abstract classes,
they just use the following language:

* Constructor for subclasses to call.

And for concrete ones:

Constructs a {@code Foo}.

This gets around the problem of whether or not `default` adds any useful information since it is the only constructor.
Not saying we should adopt this now, but just adding another data point.

> 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.

Yes, this constructor is needed. `PseudoClass` is abstract, so it's constructor is just there for subclasses to call
(note that for a constructor of an abstract class, `public` and `protected` mean the same thing).

As for the other methods in the class, I agree that they should be changed...but in a follow-up issue.

> 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.

This one does not look like it should be public. It seems quite by accident, since the only two classes that subclass
`Selector` are in the same package and are constructed by factory methods (their constructors are package-scope).

This seems like a good candidate for removal (via the deprecate-for-removal, remove route).

> 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.

This one needs to be public since it is subclassed in many other classes.

> 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.

Agreed. This is another candidate for removal.


PR: https://git.openjdk.java.net/jfx/pull/283

More information about the openjfx-dev mailing list