Re: RFR: 8271091: Missing API docs in UI controls classes [v4]
On Tue, 26 Oct 2021 06:24:35 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
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.
modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
line 180:
> 178: * variable contains style properties and values and not the
> 179: * selector portion of a style rule.
> 180: * A value of {@code null} is implicitly converted to an empty
> {@code String}.
Maybe this line should be in a new line/paragraph.
modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
line 183:
> 181: *
> 182: * @return the {@code style} property
> 183: * @defaultValue null
`{@code null}`
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.
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.
-
PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271090: Missing API docs in scenegraph classes [v3]
On Tue, 26 Oct 2021 09:54:43 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
Added a few more comments, otherwise looks fine.
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?
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 231:
> 229:
> 230: /**
> 231: * Creates a {@code Window}.
This is also a constructor for subclasses to call.
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 787:
> 785: * reference before the new one gains it. You may swap {@code
> Scene}s on
> 786: * a {@code Window} at any time, even if it is an instance of {@code
> Stage}
> 787: * and with {@link Stage#fullScreenProperty() fullScreen} set to
> true.
"true" should be in `{@code}`
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 788:
> 786: * a {@code Window} at any time, even if it is an instance of {@code
> Stage}
> 787: * and with {@link Stage#fullScreenProperty() fullScreen} set to
> true.
> 788: * If the width or height of this {@code Window} have never been set
> by the
I would start a new paragraph here since it switches from talking about a scene
to talking about sizes.
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 790:
> 788: * If the width or height of this {@code Window} have never been set
> by the
> 789: * application, setting the scene will cause this {@code Window} to
> take its
> 790: * width or height from that scene. Resizing this window by end
> user does
Extra space before "Resizing"
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`?
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 796:
> 794: * on a thread other than the JavaFX Application Thread.
> 795: *
> 796: * @defaultValue null
`{@code null}`
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v3]
On Wed, 27 Oct 2021 15:58:38 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.graphics/src/main/java/javafx/stage/Window.java line 790:
>
>> 788: * If the width or height of this {@code Window} have never been
>> set by the
>> 789: * application, setting the scene will cause this {@code Window} to
>> take its
>> 790: * width or height from that scene. Resizing this window by end
>> user does
>
> Extra space before "Resizing"
`{@code Window}` for consistency
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Wed, 27 Oct 2021 08:12:05 GMT, Johan Vos wrote: > This looks like a good solution, and I agree it fixes the issue. I have 2 > minor concerns: > > 1. Are we sure there are no platform-specific restrictions when using a `+` > in a filename? > 2. Testing this is probably difficult. We can only test it if a build is > created (and the versionInfo is set). > > I think the second concern can not be handled by our current test battery, > but we do some smoke tests before releasing the maven repository, which > should be able to detect this. If you are confident about the first concern, > I think we're all good on this. I tested the change by manipulating the folder value in `NativeLibLoader` on multiple jfx versions. That worked well. Today I tested the '+' sign for folders on windows (nfts) and different formatted usb sticks (Tried FAT, FAT32, exFat), linux and mac. They all work. If we want to be really safe we can replace a + by - or something else, but not sure if really needed. I agree it's hard to test. I also thought of writing a test but it's not really possible. The libraries are also loaded different (no cache directory is created). - PR: https://git.openjdk.java.net/jfx/pull/654
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports @johanvos Can you comment on `idMap` in `com.sun.java.scene.web.WebViewHelper.WebView`? - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8271091: Missing API docs in UI controls classes [v4]
On Wed, 20 Oct 2021 15:45:52 GMT, Nir Lisker wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix review comments > > Took a quick look at the new docs. I didn't check the resulting HTML or the > corrected of the description, just format and grammar. @nlisker, can you please take a look and approve? - PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271090: Missing API docs in scenegraph classes [v3]
On Tue, 26 Oct 2021 09:54:43 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 @nlisker, can you please take a look and approve? - PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg wrote: >> Cleanup of Tree-/TableRowSkin to support switching skins >> >> The misbehavior/s >> - memory leaks due to manually registered listeners that were not removed >> - side-effects due to listeners still active on old skin (like NPEs) >> >> Fix >> - use skin api for all listener registration (for automatic removal in >> dispose) >> - do not install listeners that are not needed (fixedCellSize, same as in >> fix of ListCellSkin >> [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) >> >> Added tests for each listener involved in the fix to guarantee it's still >> working and does not have unwanted side-effects after switching skins. >> >> Note: there are pecularities in row skins (like not updating themselves on >> property changes of its control, throwing NPEs when not added to a >> VirtualFlow) which are not part of this issue but covered in >> [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > re-added forgotten code comments Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:50:32 GMT, Jeanette Winzenburg wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java >> line 134: >> >>> 132: // that when it changes, we can appropriately add / >>> remove cells that may or may not >>> 133: // be required (because we remove all cells that are >>> not visible). >>> 134: >>> registerChangeListener(getVirtualFlow().widthProperty(), e -> >>> tableView.requestLayout()); >> >> If this listener is removed, then is there a chance of reintroducing >> [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? >> Unfortunately, there is no test added for that bug.. so it is difficult to >> catch regression, if any. > > hmm .. the listener is not removed, its registration is moved to > updateTableViewSkin. There are tests covering that it really is still > registered :) > > Forgot to move the old code comment, though. Re-added. Thanks for the explanation. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v12]
On Tue, 26 Oct 2021 19:48:41 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: testScene variable must be volatile and new line at the end of the > file added Looks good to me. The copyright year in test file needs to be changed. Along with it please fix the suggested minor typo. tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java line 2: > 1: /* > 2: * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights > reserved. It should have only one copyright year: 2021. -> `* Copyright (c) 2021, Oracle and/o` 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 - Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Tue, 26 Oct 2021 12:07:23 GMT, Ajit Ghaisas wrote: >> Jeanette Winzenburg has updated the pull request incrementally with one >> additional commit since the last revision: >> >> re-added forgotten code comments > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java > line 134: > >> 132: // that when it changes, we can appropriately add / >> remove cells that may or may not >> 133: // be required (because we remove all cells that are >> not visible). >> 134: >> registerChangeListener(getVirtualFlow().widthProperty(), e -> >> tableView.requestLayout()); > > If this listener is removed, then is there a chance of reintroducing > [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? > Unfortunately, there is no test added for that bug.. so it is difficult to > catch regression, if any. hmm .. the listener is not removed, its registration is moved to updateTableViewSkin. There are tests covering that it really is still registered :) Forgot to move the old code comment, though. Re-added. > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java > line 154: > >> 152: // that when it changes, we can appropriately add / >> remove cells that may or may not >> 153: // be required (because we remove all cells that are >> not visible). >> 154: >> registerChangeListener(getVirtualFlow().widthProperty(), e -> >> treeTableView.requestLayout()); > > Same comment as in TableRowSkin regarding this listener. same comment as to TableRowSkin :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
> Cleanup of Tree-/TableRowSkin to support switching skins > > The misbehavior/s > - memory leaks due to manually registered listeners that were not removed > - side-effects due to listeners still active on old skin (like NPEs) > > Fix > - use skin api for all listener registration (for automatic removal in > dispose) > - do not install listeners that are not needed (fixedCellSize, same as in fix > of ListCellSkin > [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) > > Added tests for each listener involved in the fix to guarantee it's still > working and does not have unwanted side-effects after switching skins. > > Note: there are pecularities in row skins (like not updating themselves on > property changes of its control, throwing NPEs when not added to a > VirtualFlow) which are not part of this issue but covered in > [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: re-added forgotten code comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/632/files - new: https://git.openjdk.java.net/jfx/pull/632/files/52e18d22..5402e1fb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=632=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=632=00-01 Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker wrote: >> Replacements of more immutable collections. >> >> One thing I found was that the field `idMap` in >> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it >> if that's indeed the case. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unused imports Looks good to me. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Tue, 26 Oct 2021 12:09:19 GMT, Marius Hanl wrote: > This problem can happen when using multiple instances with different jfx > early access (ea) versions. > > Example: > Instance 1 uses 18-ea+4 and Instance 2 uses 18-ea+1. > Instance 1 is started (and will cache and use libraries), then instance 2. > Now instance 2 detects via a hash comparison that the cached libraries are > not the same as the supplied ones. > With this information instance 2 tries to delete the old libraries but fails > while doing so (as instance 1 uses them currently) and will terminate right > after. > > The problem here is that instance 1 and 2 are using the same cache folder: > **18-ea**. This is because the `NativeLibLoader` uses the `javafx.version` > property for determining the folder name, which in case of an ea version will > always be **18-ea** (for all ea versions starting with 18 obviously). > > Fix as also mentioned in the ticket is to use the `javafx.runtime.version` > property instead. > With this the folders will be named 18-ea+1 and 18-ea+4. This looks like a good solution, and I agree it fixes the issue. I have 2 minor concerns: 1. Are we sure there are no platform-specific restrictions when using a `+` in a filename? 2. Testing this is probably difficult. We can only test it if a build is created (and the versionInfo is set). I think the second concern can not be handled by our current test battery, but we do some smoke tests before releasing the maven repository, which should be able to detect this. If you are confident about the first concern, I think we're all good on this. - PR: https://git.openjdk.java.net/jfx/pull/654
