Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v13]
On Thu, 28 Oct 2021 13:50:40 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request incrementally with one additional > commit since the last revision: > > 8255015: Copyright year and wrong spelling in comment corrected Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/531
Integrated: 8271091: Missing API docs in UI controls classes
On Wed, 20 Oct 2021 14:42:44 GMT, Ajit Ghaisas wrote: > This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - > [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) This pull request has now been integrated. Changeset: a9474055 Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/a9474055994d104288aff22fdb70f76ed8519627 Stats: 269 lines in 49 files changed: 147 ins; 15 del; 107 mod 8271091: Missing API docs in UI controls classes Reviewed-by: nlisker, kcr - PR: https://git.openjdk.java.net/jfx/pull/646
Integrated: 8271090: Missing API docs in scenegraph classes
On Fri, 22 Oct 2021 11:23:07 GMT, Ajit Ghaisas wrote: > This PR fixes javadoc warnings primarily in javafx.graphics module along with > a remaining few in javafx.fxml, javafx.base and javafx.media modules. > > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still few remaining warnings in these modules. The root cause is > different and they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) This pull request has now been integrated. Changeset: e7a106fa Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/e7a106faf3c0bb0126080d2f516248195679bf61 Stats: 133 lines in 28 files changed: 89 ins; 4 del; 40 mod 8271090: Missing API docs in scenegraph classes Reviewed-by: kcr, nlisker - PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271091: Missing API docs in UI controls classes [v5]
On Thu, 28 Oct 2021 14:27:52 GMT, Ajit Ghaisas wrote: >> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. >> Note : >> - The javadoc needs to be generated with the JDK 18 EA build. >> - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - >> [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) >> - There are still 20 javadoc warnings remaining in javafx.controls module >> and 3 warnings remaining in javafx.web module. The root cause is different >> and they will be addressed under >> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > fix review comments I didn't check the generated HTML pages or the correctness of the docs with respect to the functionality of their methods, but the docs themselves look good. - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271090: Missing API docs in scenegraph classes [v4]
On Thu, 28 Oct 2021 07:23:53 GMT, Ajit Ghaisas wrote: >> This PR fixes javadoc warnings primarily in javafx.graphics module along >> with a remaining few in javafx.fxml, javafx.base and javafx.media modules. >> >> Note : >> - The javadoc needs to be generated with the JDK 18 EA build. >> - There are still few remaining warnings in these modules. The root cause is >> different and they will be addressed under >> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > fix review comments Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271091: Missing API docs in UI controls classes [v5]
On Thu, 28 Oct 2021 14:27:52 GMT, Ajit Ghaisas wrote: >> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. >> Note : >> - The javadoc needs to be generated with the JDK 18 EA build. >> - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - >> [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) >> - There are still 20 javadoc warnings remaining in javafx.controls module >> and 3 warnings remaining in javafx.web module. The root cause is different >> and they will be addressed under >> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > fix review comments This looks good. I confirmed that the implicit constructor for `PopupControl.CSSBridge` is `protected` in JavaFX 17, so making the explicitly added contructor be `protected` is correct. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v13]
On Thu, 28 Oct 2021 13:50:40 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request incrementally with one additional > commit since the last revision: > > 8255015: Copyright year and wrong spelling in comment corrected Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8271091: Missing API docs in UI controls classes [v5]
On Thu, 28 Oct 2021 23:30:32 GMT, Kevin Rushforth wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix review comments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
> line 1132:
>
>> 1130: * Constructor for subclasses to call.
>> 1131: */
>> 1132: protected CSSBridge() {
>
> Please revert this change. Since the class is protected, it's a compatible
> change, but any change to a method's signature, return type, or modifiers
> needs a CSR.
Oh! never mind. I misread this. I see that you _are_ reverting it back. That's
good then (and a good catch).
-
PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271091: Missing API docs in UI controls classes [v5]
On Thu, 28 Oct 2021 14:27:52 GMT, Ajit Ghaisas wrote:
>> This PR fixes javadoc warnings in javafx.controls and javafx.web modules.
>> Note :
>> - The javadoc needs to be generated with the JDK 18 EA build.
>> - 2 javadoc warnings in javafx.controls TabPane class will be fixed under -
>> [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085)
>> - There are still 20 javadoc warnings remaining in javafx.controls module
>> and 3 warnings remaining in javafx.web module. The root cause is different
>> and they will be addressed under
>> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix review comments
The doc changes look good. Please revert the access modifier change.
modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
line 1132:
> 1130: * Constructor for subclasses to call.
> 1131: */
> 1132: protected CSSBridge() {
Please revert this change. Since the class is protected, it's a compatible
change, but any change to a method's signature, return type, or modifiers needs
a CSR.
-
Changes requested by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Sat, 25 Sep 2021 13:55:15 GMT, John Hendrikx wrote: >> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests >> still work. Also added a single JUnit 5 tests, and confirmed it works. >> >> I've updated the Eclipse project file for the base module only. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Readd junit declaration in allprojects and set junit version to 4.13.2 I was on holidays, I've updated the title. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: Improve property system to facilitate correct usage
I'd like to discuss the API changes surrounding content bindings for this PR: https://github.com/openjdk/jfx/pull/590 Content bindings in the property system are semantically similar to regular bindings: 1. You can only have a single content binding for a collection-type property 2. Unidirectional content bindings and bidirectional content bindings are mutually exclusive For this reason, and in order to support the behavioral changes outlined in the PR, the content binding API (in ReadOnlyListProperty) was changed as follows: void bindContent(ObservableList source); + @Deprecated(since = "18", forRemoval = true) void unbindContent(Object object); + void unbindContent(); + boolean isContentBound(); This is not a breaking change, and allows application developers to phase out the superfluous argument for `unbindContent` over time. What might be more controversial is that this imposes a new implementation requirement for some exotic implementations of collection-type properties: 1. Right now, content bindings are implemented in ReadOnlyListProperty, and every implementation of ROLP inherits the default content binding implementation. 2. With the changed API, the implementation moves from ReadOnlyListProperty to (ReadOnly)ListPropertyBase. This means that a class that implements (RO)LP, but not (RO)LPBase, does not inherit the default content binding implementation and needs to provide its own implementation if it wants to support content bindings. Of course, implementing (RO)LP without using (RO)LPBase is extremely unusual. It requires the implementer to re-implement lots of code like `ListExpressionHelper` and binding listeners. Still, it's an additional requirement for these implementations that also want to support content bindings. If content bindings are not required, then the "old" (RO)LP implementation will continue to compile and work. What we get from this change are powerful correctness guarantees for users of the API. Do you think this is a trade worth making? On Wed, Jul 28, 2021 at 1:23 AM Michael Strauß wrote: > > I propose a set of changes to the JavaFX property system that I've > outlined in this PR: https://github.com/openjdk/jfx/pull/590 > > The changes fall into two categories: enforcement of correct usage > (there are several cases listed in the PR), and deprecating untyped > APIs (for removal in a future version) so as to make the intent of the > API more clear to developers. > > Even though there are breaking changes, the impact on application code > should be minimal. I'd welcome any comments on this proposal.
Re: RFR: 8271091: Missing API docs in UI controls classes [v5]
> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - > [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/646/files - new: https://git.openjdk.java.net/jfx/pull/646/files/b4d5dc4f..1ab36f72 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=03-04 Stats: 10 lines in 3 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/646.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646 PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271091: Missing API docs in UI controls classes [v4]
On Wed, 27 Oct 2021 16:12:11 GMT, Nir Lisker wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix review comments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/CheckMenuItem.java
> line 89:
>
>> 87:
>> **/
>> 88: /**
>> 89: * Constructs a default {@code CheckMenuItem}.
>
> `Label` uses "Creates an empty label" for the default constructor because it
> has no text or graphic. Maybe it's more informative that way.
It does make sense to modify it. I will change it.
> modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
> line 1133:
>
>> 1131: */
>> 1132: public CSSBridge() {
>> 1133: }
>
> Looking at the [docs for
> 17](https://openjfx.io/javadoc/17/javafx.controls/javafx/scene/control/PopupControl.CSSBridge.html),
> the constructor there is `protected`, here it's `public`. Was this changed
> recently? If it's supposed to be `protected`, then the constructor is for
> subclasses.
This is a very good catch. Thanks!
Changing this to public was a mistake this PR was about to make.
In openjfx17, there is no constructor defined - that means - it gets generated
implicitly which is `protected`.
I introduced an explicit constructor in this PR while fixing the "missing
javadoc" comment. Inadvertently I had made it `public` in this PR.
Now, I have made it `protected` in the latest commit.
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualContainerBase.java
> line 67:
>
>> 65:
>> 66: /**
>> 67: * Constructs a {@code VirtualContainerBase}
>
> The class is `abstract`, so possibly the constructor should be `protected`,
> and we might want to say "Constructor for subclasses" anyway.
I have corrected the javadoc to match "Constructor for subclasses to call.".
I would like to avoid changing the access modifier to `protected` as part of
javadoc fix.
-
PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v13]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request incrementally with one additional commit since the last revision: 8255015: Copyright year and wrong spelling in comment corrected - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/1598c604..9c10d967 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=531&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=531&range=11-12 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v12]
On Wed, 27 Oct 2021 09:36:20 GMT, Ambarish Rapte wrote: >> Andreas Heger has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8255015: testScene variable must be volatile and new line at the end of >> the file added > > tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java > line 148: > >> 146: * Creates a new scene with a subscene which contains a perspective >> camera and a sphere >> 147: * Although this test class checks the pointlight illumination, >> there is no explicit pointlight >> 148: * in the scene. For the test, it is sufficient to use the default >> pointlight which is created > > minor: Please include this small correction along with the copyright year > change. > pointlight -> point light Thanks for the review! I corrected the copyright year and the point light spelling. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8271090: Missing API docs in scenegraph classes [v4]
On Thu, 28 Oct 2021 07:23:53 GMT, Ajit Ghaisas wrote: >> This PR fixes javadoc warnings primarily in javafx.graphics module along >> with a remaining few in javafx.fxml, javafx.base and javafx.media modules. >> >> Note : >> - The javadoc needs to be generated with the JDK 18 EA build. >> - There are still few remaining warnings in these modules. The root cause is >> different and they will be addressed under >> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > fix review comments I didn't check the generated HTML pages, and in some cases I'm not familiar enough with the API to evaluate the correctness of the description, but I checked the doc comments by themselves and these look good. - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v4]
> This PR fixes javadoc warnings primarily in javafx.graphics module along with > a remaining few in javafx.fxml, javafx.base and javafx.media modules. > > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still few remaining warnings in these modules. The root cause is > different and they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/650/files - new: https://git.openjdk.java.net/jfx/pull/650/files/e4d337c5..69b6a759 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=02-03 Stats: 16 lines in 2 files changed: 0 ins; 0 del; 16 mod Patch: https://git.openjdk.java.net/jfx/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/650/head:pull/650 PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v3]
On Wed, 27 Oct 2021 16:06:38 GMT, Nir Lisker wrote:
> Added a few more comments, otherwise looks fine.
Thanks for your detailed review.
> modules/javafx.graphics/src/main/java/javafx/stage/PopupWindow.java line 156:
>
>> 154:
>> 155: /**
>> 156: * Creates a {@code PopupWindow}.
>
> The `PopupWIndow` class is abstract. Do we still keep this wording?
I have changed it to - "Constructor for subclasses to call." as in other cases.
> modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 794:
>
>> 792: *
>> 793: * An {@link IllegalStateException} is thrown if this property is
>> set
>> 794: * on a thread other than the JavaFX Application Thread.
>
> Shouldn't this be in a `@throws`?
Yes, only javadoc for `setScene()` method should use `@throws`.
The description for the property does not (and should not) recognize a
`@throws`, hence I have kept the original description intact.
-
PR: https://git.openjdk.java.net/jfx/pull/650
