Re: RFR: 8217472: Add attenuation for PointLight [v11]
> CSR: https://bugs.openjdk.java.net/browse/JDK-8218264 Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/43/files - new: https://git.openjdk.java.net/jfx/pull/43/files/3ab051d2..2c9af767 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/43/webrev.10 - incr: https://webrevs.openjdk.java.net/jfx/43/webrev.09-10 Stats: 134 lines in 10 files changed: 125 ins; 1 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/43.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/43/head:pull/43 PR: https://git.openjdk.java.net/jfx/pull/43
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v4]
> Adds clarifications to the documentation in various places. Some notes: > > * Point 6 should probably be deferred until it is verified that the tutorials > are correct enough, seeing as they were > updated to Java 8 only. > * Point 8 has been deferred until all the animation bugs have been resolevd. > * Point 5: I wrote new documentation about the `extractor` for the > `observableArrayList(Callback > extractor)` method. Later I found that `observableList(List list, > Callback extractor)` already > talks about it (I updated it too). I'm not sure which of them we want to > keep, or maybe merge them. > * Point 1: I think that it's necessary to mention the internal implementation > behavior even if it requires a caveat that > this is only the current behavior and may change in the future. What > constitutes a "change" is extremely important and > there is no way for the user to know it. I've tripped on this hard when > using ReactFX which uses object equality > instead, so when the JavaFX observables are wrapped by ReactFX observables, > the behavior changes silently. > I think that in the future we will want to let the user define what a change > is (for example, by creating an > overridable method with the current behavior as the default, or using object > equality and letting the user override > that, although that's more risky). Even a `HashMap` that uses object equality > has the sister implementation > `IndentityHashMap` to deal with this ambiguity. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Reverted transforms-related changes and added API note for getProperties - Changes: - all: https://git.openjdk.java.net/jfx/pull/276/files - new: https://git.openjdk.java.net/jfx/pull/276/files/6c0a3eb9..3bb17c7e Webrevs: - full: https://webrevs.openjdk.java.net/jfx/276/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/276/webrev.02-03 Stats: 26 lines in 1 file changed: 5 ins; 7 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/276.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/276/head:pull/276 PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Mon, 10 Aug 2020 10:26:50 GMT, Nir Lisker wrote: >> In reading the existing doc carefully, it is both wrong and confusing. It >> has the wrong ordering of transform, scale >> and rotate. It is also confusing given that no distinction is made between >> matrix multiplication order and the observed >> effect on the geometry of the node. You have fixed the part about it being >> wrong, but the confusion remains, especially >> since you made a point of saying that the transforms list is applied in the >> reverse order. The matrices are multiplied >> in the following order: 1. layoutX/Y + translateX/Y/Z >> 2. rotate >> 3. scale >> 4. transforms[0] >> 5. transforms[1] >> ... >> >> We need to both list the actual matrix multiplication order, and then >> describe that the object is transformed from >> object coordinates to local coordinates of the node, to parent coordinated, >> etc., by first applying the transforms in >> the list in reverse order, then scale, rotate, translate+layout. I don't >> think that the `transforms` list is the place >> to do that (it belongs in the Transformations section). This doesn't seem >> like something we can sort out for JavaFX >> 15, so I would split this out and defer it. > > So should I revert all the changes about the transforms and leave it for 16, > or do part of it now and part later? > The changes are in the docs for the `Node` class, the `getTransforms()` > method and the `boundsInParentProperty()` > method. It seems best to revert all of the transform changes in this PR and deal with it in 16. - PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Mon, 10 Aug 2020 06:34:57 GMT, Nir Lisker wrote: >> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 868: >> >>> 867: * clearing the map or overriding its values. These entries are >>> not removed automatically if the node is >>> removed from the layout 868: * manager, so unused entries can exist >>> throughout the life of the node. >>> 869: * >> >> If we were in the habit of using `@apiNote` then that's probably where the >> note about layout managers using the node >> properties would go. Since we aren't, this seems fine. > > I think that `@aipNote` is indeed better. Does it require a CSR? In this case, I think it would be fine without a CSR, since it is just a clarifying note for developers. - PR: https://git.openjdk.java.net/jfx/pull/276
[jfx15] Integrated: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors
On Thu, 30 Jul 2020 00:27:18 GMT, Nir Lisker wrote: > This was missed the in Javadoc fixes for 15. > > Added documentation for all the constructors, did a bit of formatting, and > removed redundant comments. This pull request has now been integrated. Changeset: 7a8708b0 Author:Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/7a8708b0 Stats: 87 lines in 3 files changed: 10 ins; 66 del; 11 mod 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors Reviewed-by: jvos, kcr - PR: https://git.openjdk.java.net/jfx/pull/277
Re: [jfx15] RFR: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors [v4]
On Mon, 10 Aug 2020 05:39:05 GMT, Nir Lisker wrote: >> This was missed the in Javadoc fixes for 15. >> >> Added documentation for all the constructors, did a bit of formatting, and >> removed redundant comments. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Added @param tags Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/277
Re: [jfx15] RFR: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors [v4]
On Mon, 10 Aug 2020 05:39:05 GMT, Nir Lisker wrote: >> This was missed the in Javadoc fixes for 15. >> >> Added documentation for all the constructors, did a bit of formatting, and >> removed redundant comments. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Added @param tags Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/277
Re: [jfx15] RFR: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors [v3]
On Fri, 7 Aug 2020 13:33:22 GMT, Kevin Rushforth wrote: >> True, but I just looked at >> [Border](https://openjfx.io/javadoc/14/javafx.graphics/javafx/scene/layout/Border.html) >> which states immutability, so I did the same. I don't mind changing it. > > I don't mind either way. I don't think it's bad to stating this explicitly here. - PR: https://git.openjdk.java.net/jfx/pull/277
Re: [jfx15] RFR: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors [v3]
On Mon, 10 Aug 2020 12:45:07 GMT, Johan Vos wrote: >> I don't mind either way. > > I don't think it's bad to stating this explicitly here. Agreed. - PR: https://git.openjdk.java.net/jfx/pull/277
Integrated: 8249777: build.gradle: project.version should not contain time stamps
On Wed, 22 Jul 2020 17:22:25 GMT, Kevin Rushforth wrote: > The addMavenPublication method sets the `project.version` to > `$MAVEN_VERSION`, where project is base, graphics, > controls, etc. When doing an ordinary "developer" build, this property > contains the time stamp . This is causing > problems with the NetBeans gradle plugin not being able to determine the > dependencies between the projects (modules). > We haven't noticed this bug before now, because we suppress the creation of > the individual project jar files, but if we > were creating them, we would have the problem that each time we ran gradle, > it would generate a new > `project-$version.jar` file with a different time stamp each time the build > was run. The setting of > `project.version=$MAVEN_VERSION` is only used by the maven "publish" tasks, > which isn't done for developer builds nor > for most productions builds. The proposed solution is to add a new boolean > `MAVEN_PUBLISH` flag, which will default to > `false`. This flag will qualify the setting of the `MAVEN_VERSION` property > and the configuration of the maven > publishing tasks. After this change, it will be necessary to run gradle with > `-PMAVEN_PUBLISH=true` in order to run the > maven publishing tasks. This pull request has now been integrated. Changeset: aefed810 Author:Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/aefed810 Stats: 11 lines in 1 file changed: 1 ins; 9 del; 1 mod 8249777: build.gradle: project.version should not contain time stamps Reviewed-by: sykora, jvos - PR: https://git.openjdk.java.net/jfx/pull/270
Integrated: 8181775: JavaFX WebView does not calculate border-radius properly
On Thu, 14 May 2020 08:15:19 GMT, Bhawesh Choudhary wrote: > root cause of issue is prism's fillRoundedRect() API doesn't allow rendering > of rounded corner rectangle if four > corners have different radii. but same can be achieved via Path. to fix the > issue, in GraphicsContextJava.cpp while > rendering fillRoundedRect, check if all four corners have same radii. if yes, > use FILL_ROUNDED_RECT to draw it > otherwise construct a path from given rounded rect and draw it. This pull request has now been integrated. Changeset: f216c5fe Author:Bhawesh Choudhary Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/f216c5fe Stats: 104 lines in 2 files changed: 0 ins; 95 del; 9 mod 8181775: JavaFX WebView does not calculate border-radius properly Reviewed-by: kcr, ajoseph - PR: https://git.openjdk.java.net/jfx/pull/218
Integrated: 8196079: Remove obsolete Pisces rasterizer
On Sat, 18 Jul 2020 14:57:08 GMT, Kevin Rushforth wrote: > This removes the obsolete OpenPiscesRasterizer (Java-based) and > NativePiscesRasterizer implementations. The Marlin > rasterizer was added in FX 9 and was made the default in FX 10. Marlin both > outperforms Pisces and is more robust. > There is no reason to keep the Pisces rasterizer(s) any more. Note that the > SW pipeline still has a Pisces-based > renderer for the actual rendering of primitives. This is separate from the > rasterizer and is not affected by this > proposed fix. I have tested this on Mac, Windows, and Linux. This pull request has now been integrated. Changeset: 5c596b14 Author:Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/5c596b14 Stats: 10224 lines in 30 files changed: 10217 ins; 1 del; 6 mod 8196079: Remove obsolete Pisces rasterizer Reviewed-by: prr, lbourges - PR: https://git.openjdk.java.net/jfx/pull/268
Re: RFR: 8249777: build.gradle: project.version should not contain time stamps [v2]
On Thu, 23 Jul 2020 13:45:33 GMT, Kevin Rushforth wrote: >> The addMavenPublication method sets the `project.version` to >> `$MAVEN_VERSION`, where project is base, graphics, >> controls, etc. When doing an ordinary "developer" build, this property >> contains the time stamp . This is causing >> problems with the NetBeans gradle plugin not being able to determine the >> dependencies between the projects (modules). >> We haven't noticed this bug before now, because we suppress the creation of >> the individual project jar files, but if we >> were creating them, we would have the problem that each time we ran gradle, >> it would generate a new >> `project-$version.jar` file with a different time stamp each time the build >> was run. The setting of >> `project.version=$MAVEN_VERSION` is only used by the maven "publish" tasks, >> which isn't done for developer builds nor >> for most productions builds. The proposed solution is to add a new boolean >> `MAVEN_PUBLISH` flag, which will default to >> `false`. This flag will qualify the setting of the `MAVEN_VERSION` property >> and the configuration of the maven >> publishing tasks. After this change, it will be necessary to run gradle with >> `-PMAVEN_PUBLISH=true` in order to run the >> maven publishing tasks. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unised mavenPublish property from graphics project Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/270
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Fri, 7 Aug 2020 14:37:48 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Corrected javadoc generation errors > > I left a few formatting comments and a substantive one on the clarification > of the transform order, which I recommend > to defer to 16. @kevinrushforth @arapte Can you give your input on the changes to the methods with an extractor in `FXCollections` that I mentioned in the 3rd bullet point of this PR? - PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Fri, 7 Aug 2020 14:18:31 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 5523:
>>
>>> 5522: * Defines the {@code ObservableList} of {@link Transform}
>>> objects to be applied to this {@code Node}. The
>>> transforms in this list 5523: * are applied in the reverse
>>> order of which they are specified as per matrix
>>> multiplication rules. This list of transforms 5524: * is applied
>>> before scaling ({@link #scaleXProperty scaleX},
>>> {@link #scaleYProperty scaleY} and {@link #scaleZProperty scaleZ}),
>>
>> `The transforms in this list are applied in the reverse order of
>> which they are specified as per matrix
>> multiplication rules`
>> To apply transformations in a specific order their respective matrices
>> should be multiplied in reverse order. So just
>> the order of multiplication is reverse but the transformation are applied in
>> same order as they are added into the
>> `getTransforms()`. I think phrasing it as 'the transforms are applied in
>> reverse order...' would not be accurate and
>> confusing to reader.
>
> In reading the existing doc carefully, it is both wrong and confusing. It has
> the wrong ordering of transform, scale
> and rotate. It is also confusing given that no distinction is made between
> matrix multiplication order and the observed
> effect on the geometry of the node. You have fixed the part about it being
> wrong, but the confusion remains, especially
> since you made a point of saying that the transforms list is applied in the
> reverse order. The matrices are multiplied
> in the following order: 1. layoutX/Y + translateX/Y/Z
> 2. rotate
> 3. scale
> 4. transforms[0]
> 5. transforms[1]
> ...
>
> We need to both list the actual matrix multiplication order, and then
> describe that the object is transformed from
> object coordinates to local coordinates of the node, to parent coordinated,
> etc., by first applying the transforms in
> the list in reverse order, then scale, rotate, translate+layout. I don't
> think that the `transforms` list is the place
> to do that (it belongs in the Transformations section). This doesn't seem
> like something we can sort out for JavaFX
> 15, so I would split this out and defer it.
So should I revert all the changes about the transforms and leave it for 16, or
do part of it now and part later?
The changes are in the docs for the `Node` class, the `getTransforms()` method
and the `boundsInParentProperty()`
method.
-
PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v3]
> Adds clarifications to the documentation in various places. Some notes: > > * Point 6 should probably be deferred until it is verified that the tutorials > are correct enough, seeing as they were > updated to Java 8 only. > * Point 8 has been deferred until all the animation bugs have been resolevd. > * Point 5: I wrote new documentation about the `extractor` for the > `observableArrayList(Callback > extractor)` method. Later I found that `observableList(List list, > Callback extractor)` already > talks about it (I updated it too). I'm not sure which of them we want to > keep, or maybe merge them. > * Point 1: I think that it's necessary to mention the internal implementation > behavior even if it requires a caveat that > this is only the current behavior and may change in the future. What > constitutes a "change" is extremely important and > there is no way for the user to know it. I've tripped on this hard when > using ReactFX which uses object equality > instead, so when the JavaFX observables are wrapped by ReactFX observables, > the behavior changes silently. > I think that in the future we will want to let the user define what a change > is (for example, by creating an > overridable method with the current behavior as the default, or using object > equality and letting the user override > that, although that's more risky). Even a `HashMap` that uses object equality > has the sister implementation > `IndentityHashMap` to deal with this ambiguity. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed parts of the review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/276/files - new: https://git.openjdk.java.net/jfx/pull/276/files/a44bfb18..6c0a3eb9 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/276/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/276/webrev.01-02 Stats: 24 lines in 3 files changed: 7 ins; 7 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/276.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/276/head:pull/276 PR: https://git.openjdk.java.net/jfx/pull/276
Re: [jfx15] RFR: 8228570: Add various documentation clarifications [v2]
On Fri, 7 Aug 2020 13:37:28 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Corrected javadoc generation errors > > modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 868: > >> 867: * clearing the map or overriding its values. These entries are not >> removed automatically if the node is >> removed from the layout 868: * manager, so unused entries can exist >> throughout the life of the node. >> 869: * > > If we were in the habit of using `@apiNote` then that's probably where the > note about layout managers using the node > properties would go. Since we aren't, this seems fine. I think that `@aipNote` is indeed better. Does it require a CSR? - PR: https://git.openjdk.java.net/jfx/pull/276
