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

Conversation

@bhaweshkc
Copy link
Collaborator

@bhaweshkc bhaweshkc commented Aug 17, 2020

Added missing explicit no-arg constructors to classes in package javafx.scene, javafx.css and javafx.stage.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Nir Lisker (nlisker - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/283/head:pull/283
$ git checkout pull/283

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 17, 2020

👋 Welcome back bchoudhary! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Aug 17, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 17, 2020

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Aug 17, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Aug 17, 2020

@nlisker
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth requested a review from aghaisas Aug 18, 2020
Copy link
Collaborator

@nlisker nlisker left a comment

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.

*/
public Selector() {
}

private static class UniversalSelector {

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

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 comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

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

*/
public ShapeConverter() {
}

This comment has been minimized.

@nlisker

nlisker Aug 18, 2020
Collaborator

Looks like a singleton class.

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

Agreed. This is another candidate for removal.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

@nlisker raises some good issues, so they should be addressed

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 18, 2020

The classes should be inspected to see if one is needed and if its doc is suitable.

This is an excellent point. One of the main reasons for not relying on implicit, no-arg constructors is that you might get a public constructor where one isn't intended. See JDK-8229472 and JDK-8240688 for what is needed if we determine that there should not be a public constructor.

To that end, I'll go through it more carefully and comment on this specific question.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I think that two of the classes have implicit constructors that are there by accident. Once we get agreement, I'll file a follow-on bug for those, and those changes should be reverted.

As for the other comments, I would prefer a follow-up bug for any doc cleanup that isn't part of adding in an explicit constructor. Tempting as it might be to fix it, it seems out of scope.

That leaves the question about the wording. The more I think about it the more I like what the JDK did as opposed to what we did. The question then becomes: if we agree that it's a better pattern, do we adopt it here and then clean it up for the other two batches or just leave it as is for now, and file one cleanup bug for later. I'd like to hear from @aghaisas and @nlisker

*/
public Selector() {
}

private static class UniversalSelector {

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

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

*/
public ShapeConverter() {
}

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 19, 2020
Member

Agreed. This is another candidate for removal.

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Aug 25, 2020

@bhaweshkc You can't deprecate the constructors in this PR since it will need a separate CSR. You should revert the changes on these classes completely and file a new bug (and CSR) dealing with deprecations.

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Aug 25, 2020

I think that two of the classes have implicit constructors that are there by accident. Once we get agreement, I'll file a follow-on bug for those, and those changes should be reverted.

I finished reviewing the rest of the classes and I had no additional comments about them, so I agree about deprecating for removal those 2 classes' constructors.

As for the other comments, I would prefer a follow-up bug for any doc cleanup that isn't part of adding in an explicit constructor. Tempting as it might be to fix it, it seems out of scope.

That's fine. I left inline comments about those.

That leaves the question about the wording. The more I think about it the more I like what the JDK did as opposed to what we did. The question then becomes: if we agree that it's a better pattern, do we adopt it here and then clean it up for the other two batches or just leave it as is for now, and file one cleanup bug for later. I'd like to hear from @aghaisas and @nlisker

Since it will require an additional cleanup issue anyway, I don't think it matters when we do it, but since we're here I see no reason not to start already.

@kevinrushforth kevinrushforth removed the request for review from aghaisas Aug 25, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 26, 2020

Since it will require an additional cleanup issue anyway, I don't think it matters when we do it, but since we're here I see no reason not to start already.

Agreed.

Let's adopt the same language as the JDK. If there are configurable parameters, we can indicate the default by adding an @defaultValue tag (I don't think we need the word default in the description).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good except for the one you missed to update, StringStore.

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

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 26, 2020
Member

You missed updating the wording for this one.

@bhaweshkc bhaweshkc requested review from nlisker and kevinrushforth Aug 26, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good. I also verified that the only two class without an explicit constructor are the two we identified as needing to be deprecated for removal in a follow-up issue: Selector and SshapeConverter

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 26, 2020

I filed the following two follow-up issues:

  1. JDK-8252387: Deprecate for removal css Selector and ShapeConverter constructors
  2. JDK-8252389: Fix mistakes in FX API docs

I also added the note about the PseudoClass docs to:

  1. JDK-8250590: Classes and methods in the javafx.css package are missing documentation
@openjdk
Copy link

@openjdk openjdk bot commented Aug 28, 2020

@bhaweshkc This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

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

Reviewed-by: kcr, nlisker
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 7 commits pushed to the master branch:

  • 7a4bd9b: 8252060: gstreamer fails to build with gcc 10
  • ddf8814: 8251555: Remove unused focusedWindow field in glass Window to avoid leak
  • c3fb321: 8252107: Media pipeline initialization can crash if audio or video bin state change fails
  • eb9886a: Merge
  • 85821ca: 8251241: macOS: iconify property doesn't change after minimize when resizable is false
  • 41ff3fa: 8249537: Update copyright header for files modified in 2020
  • b25ffc7: 8251352: Many javafx.base classes have implicit no-arg constructors

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 7a4bd9b25f2b94d3c8a85f79804f81f113350d3f.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @nlisker) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Aug 28, 2020
@bhaweshkc
Copy link
Collaborator Author

@bhaweshkc bhaweshkc commented Aug 28, 2020

/integrate

@openjdk openjdk bot added the sponsor label Aug 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Aug 28, 2020

@bhaweshkc
Your change (at version 5bb0790) is now ready to be sponsored by a Committer.

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Aug 28, 2020

/sponsor

@openjdk openjdk bot closed this Aug 28, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Aug 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Aug 28, 2020

@nlisker @bhaweshkc The following commits have been pushed to master since your change was applied:

  • 7a4bd9b: 8252060: gstreamer fails to build with gcc 10
  • ddf8814: 8251555: Remove unused focusedWindow field in glass Window to avoid leak
  • c3fb321: 8252107: Media pipeline initialization can crash if audio or video bin state change fails
  • eb9886a: Merge
  • 85821ca: 8251241: macOS: iconify property doesn't change after minimize when resizable is false
  • 41ff3fa: 8249537: Update copyright header for files modified in 2020
  • b25ffc7: 8251352: Many javafx.base classes have implicit no-arg constructors

Your commit was automatically rebased without conflicts.

Pushed as commit 23ad8f4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants