On Mon, 17 Aug 2020 12:31:14 GMT, Nir Lisker <nlis...@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
