Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
@@ -42,6 +42,12 @@
* *
**********************************************************************/

/**
* Constructor for subclasses to call.
*/
public TableFocusModel() {
}

/**
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

Please add a missing ) for the class docs on line 30.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

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.

This comment has been minimized.

@nlisker

nlisker Aug 25, 2020
Collaborator

Alright, we could add them to this version's doc fixes issue.

* Causes the item at the given index to receive the focus.
*
@@ -37,6 +37,12 @@
*/
public abstract class TableSelectionModel<T> extends MultipleSelectionModelBase<T> {

/**
* Constructor for subclasses to call.
*/
public TableSelectionModel() {
}

/**
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

Same missing )

* Convenience function which tests whether the given row and column index
* is currently selected in this table instance. If the table control is in its
@@ -114,6 +114,12 @@
lineSeparator = prop != null ? prop : "\n";
}

/**
* Constructor for subclasses to call.
*/
public Preloader() {
}
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

Not sure that "default" means anything here. I don't see any configuration.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

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.

This comment has been minimized.

@nlisker

nlisker Aug 25, 2020
Collaborator

I didn't look closely at whether "default" was suitable in the previous cases. I like the JDK's wording, but in cases where there are constructors that allow configuration, the empty constructor could use "default", though specifying what the default values are is more beneficial anyway.


/**
* Indicates download progress.
* This method is called by the FX runtime to indicate progress while
@@ -144,6 +144,13 @@
* @since JavaFX 8.0
*/
public abstract class ScheduledService<V> extends Service<V> {

/**
* Constructor for subclasses to call.
*/
public ScheduledService() {
}

/**
* A Callback implementation for the <code>backoffStrategy</code> property which
* will exponentially backoff the period between re-executions in the case of
@@ -74,6 +74,12 @@
*/
public abstract class PseudoClass {

/**
* Constructor for subclasses to call.
*/
public PseudoClass() {
}

/**
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

  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.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

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.

This comment has been minimized.

@nlisker

nlisker Aug 25, 2020
Collaborator

Maybe we can add the method docs fixes to JDK-8250590?

* There is only one PseudoClass instance for a given pseudoClass.
* @param pseudoClass the pseudo-class
@@ -86,6 +86,12 @@
*/
public class StyleConverter<F, T> {

/**
* Creates a {@code StyleConverter}.
*/
public StyleConverter() {
}

/**
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

Looks like a constructor is fine here if the predefined factories are not suitable, but I'm not familiar with this.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

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

* Convert from the parsed CSS value to the target property type.
*
@@ -532,6 +538,12 @@ protected void cacheValue(ParsedValue key, Object value) {
private final Map<String,Integer> stringMap = new HashMap<String,Integer>();
public final List<String> strings = new ArrayList<String>();

/**
* Creates a {@code StringStore}.
*/
public StringStore() {

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 26, 2020
Member

You missed updating the wording for this one.

}

public int addString(String s) {
Integer index = stringMap.get(s);
if (index == null) {
@@ -38,6 +38,13 @@
* @since JavaFX 2.0
*/
public class ClipboardContent extends HashMap<DataFormat, Object> {

/**
* Creates a {@code ClipboardContent}.
*/
public ClipboardContent() {
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

Not sure that "default" means anything here. I don't see any configuration.

}

/**
* Gets whether a plain text String ({@code DataFormat.PLAIN_TEXT})
* has been put to this {@code ClipboardContent}.
@@ -71,6 +71,12 @@ public void setHelper(PathElement pathElement, PathElementHelper pathElementHelp
*/
WeakReferenceQueue nodes = new WeakReferenceQueue();

/**
* Constructor for subclasses to call.
*/
public PathElement() {
}

void addNode(final Node n) {
nodes.add(n);
}
@@ -127,6 +127,12 @@ public Transform createImmutableTransform(Transform transform,
});
}

/**
* Constructor for subclasses to call.
*/
public Transform() {
}

/* *************************************************************************
* *
* Factories *
@@ -49,6 +49,12 @@
*/
private StringProperty title;

/**
* Creates a {@code DirectoryChooser}.
*/
public DirectoryChooser() {
}

public final void setTitle(final String value) {
titleProperty().set(value);
}
@@ -204,6 +204,12 @@ private static void validateArgs(final String description,
*/
private StringProperty title;

/**
* Creates a {@code FileChooser}.
*/
public FileChooser() {
}

public final void setTitle(final String value) {
titleProperty().set(value);
}
@@ -38,6 +38,12 @@
*/
public class Popup extends PopupWindow {

/**
* Creates a {@code Popup}.
*/
public Popup() {
}

/**
* The ObservableList of {@code Node}s to be rendered on this
* {@code Popup}. The content forms the complete visual representation of
ProTip! Use n and p to navigate between commits in a pull request.