Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v5]
On Wed, 26 Aug 2020 15:46:46 GMT, Bhawesh Choudhary wrote: >> Added missing explicit no-arg constructors to classes in package >> javafx.scene, javafx.css and javafx.stage. > > Bhawesh Choudhary has updated the pull request incrementally with one > additional commit since the last revision: > > Updated comments as per JDK Convention Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v5]
On Wed, 26 Aug 2020 17:22:36 GMT, Kevin Rushforth wrote: >> Bhawesh Choudhary has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updated comments as per JDK Convention > > 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` I filed the following two follow-up issues: 1. [JDK-8252387](https://bugs.openjdk.java.net/browse/JDK-8252387): Deprecate for removal css Selector and ShapeConverter constructors 2. [JDK-8252389](https://bugs.openjdk.java.net/browse/JDK-8252389): Fix mistakes in FX API docs I also added the note about the PseudoClass docs to: 3. [JDK-8250590](https://bugs.openjdk.java.net/browse/JDK-8250590): Classes and methods in the javafx.css package are missing documentation - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v5]
On Wed, 26 Aug 2020 15:46:46 GMT, Bhawesh Choudhary wrote: >> Added missing explicit no-arg constructors to classes in package >> javafx.scene, javafx.css and javafx.stage. > > Bhawesh Choudhary has updated the pull request incrementally with one > additional commit since the last revision: > > Updated comments as per JDK Convention 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` - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v5]
> Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: Updated comments as per JDK Convention - Changes: - all: https://git.openjdk.java.net/jfx/pull/283/files - new: https://git.openjdk.java.net/jfx/pull/283/files/69b75d69..5bb07901 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/283/webrev.04 - incr: https://webrevs.openjdk.java.net/jfx/283/webrev.03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/283.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/283/head:pull/283 PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v4]
On Wed, 26 Aug 2020 15:21:48 GMT, Bhawesh Choudhary
wrote:
>> Added missing explicit no-arg constructors to classes in package
>> javafx.scene, javafx.css and javafx.stage.
>
> Bhawesh Choudhary has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Updated comments as per JDK convention
Looks good except for the one you missed to update, `StringStore`.
modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 544:
> 543: */
> 544: public StringStore() {
> 545: }
You missed updating the wording for this one.
-
PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v4]
On Tue, 25 Aug 2020 09:42:16 GMT, Nir Lisker wrote: > 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). - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v4]
> Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: Updated comments as per JDK convention - Changes: - all: https://git.openjdk.java.net/jfx/pull/283/files - new: https://git.openjdk.java.net/jfx/pull/283/files/38a7551d..69b75d69 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/283/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/283/webrev.02-03 Stats: 12 lines in 12 files changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/283.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/283/head:pull/283 PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v3]
> Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: Reverted changes for class Selector and ShapeConverter as per review comment - Changes: - all: https://git.openjdk.java.net/jfx/pull/283/files - new: https://git.openjdk.java.net/jfx/pull/283/files/16d8b9ad..38a7551d Webrevs: - full: https://webrevs.openjdk.java.net/jfx/283/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/283/webrev.01-02 Stats: 14 lines in 2 files changed: 0 ins; 14 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/283.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/283/head:pull/283 PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Wed, 19 Aug 2020 00:12:01 GMT, Kevin Rushforth wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Tue, 18 Aug 2020 23:41:23 GMT, Kevin Rushforth wrote:
>> 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.
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.
-
PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Tue, 18 Aug 2020 23:47:07 GMT, Kevin Rushforth wrote: >> 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. Maybe we can add the method docs fixes to [JDK-8250590](https://bugs.openjdk.java.net/browse/JDK-8250590)? - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Tue, 18 Aug 2020 23:31:42 GMT, Kevin Rushforth wrote: >> 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. Alright, we could add them to this version's doc fixes issue. - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
On Wed, 19 Aug 2020 00:12:01 GMT, Kevin Rushforth wrote: >> Bhawesh Choudhary has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Marked few class constructor as deprecated as per review > > 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 @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. - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]
> Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: Marked few class constructor as deprecated as per review - Changes: - all: https://git.openjdk.java.net/jfx/pull/283/files - new: https://git.openjdk.java.net/jfx/pull/283/files/25c7b37c..16d8b9ad Webrevs: - full: https://webrevs.openjdk.java.net/jfx/283/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/283/webrev.00-01 Stats: 6 lines in 2 files changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/283.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/283/head:pull/283 PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 12:31:14 GMT, Nir Lisker 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 value,
>> Font font) {
>
> Looks like a singleton class.
Agreed. This is another candidate for removal.
-
PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary wrote: > Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. 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 - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Tue, 18 Aug 2020 17:49:06 GMT, Nir Lisker wrote: > 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](https://bugs.openjdk.java.net/browse/JDK-8229472) and [JDK-8240688](https://bugs.openjdk.java.net/browse/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. - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary wrote: > Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. @nlisker raises some good issues, so they should be addressed - Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary
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 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
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary wrote: > Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/283
RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
Added missing explicit no-arg constructors to classes in package javafx.scene, javafx.css and javafx.stage. - Commit messages: - 8251353: Many javafx scenegraph classes have implicit no-arg constructors Changes: https://git.openjdk.java.net/jfx/pull/283/files Webrev: https://webrevs.openjdk.java.net/jfx/283/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251353 Stats: 92 lines in 14 files changed: 92 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/283.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/283/head:pull/283 PR: https://git.openjdk.java.net/jfx/pull/283
