Re: RFR: 8266860: [macos] Incorrect duration reported for HLS live streams
On Tue, 11 May 2021 23:43:36 GMT, Alexander Matveev wrote: > For indefinite durations CMTimeGetSeconds was returning NaN (not-a-number > double value) and our code expects -1.0. Based on doc we should be using > CMTIME_IS_INDEFINITE to test if duration is indefinite. Fixed by using > CMTIME_IS_INDEFINITE to test if duration is indefinite and if true -1.0 will > be return to our Java layer. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/495
Re: RFR: 8263760: Update gradle to version 7.0.1 [v2]
On Thu, 13 May 2021 21:55:59 GMT, Kevin Rushforth wrote: >> This PR fixes the gradle deprecation warnings described in >> [JDK-8240336](https://bugs.openjdk.java.net/browse/JDK-8240336) and updates >> the JavaFX build to use gradle 7.0.1 as described in >> [JDK-8263760](https://bugs.openjdk.java.net/browse/JDK-8263760). The minimum >> version of gradle is set to 6.3. >> >> In addition to keeping gradle up to date, updating to gradle 7 will allow >> building and testing JavaFX with JDK 16. >> >> I have done a full build and test on all three platforms, comparing the >> artifacts produced before and after this change. >> >> ### Notes to Reviewers: >> >> The PR branch has two separate commits, one for each fix, in case reviewers >> want to look at them separately. As always, they will be squashed into a >> single commit when integrated. Both bug IDs will be listed in the commit. >> >> The following changes were done for >> [JDK-8240336](https://bugs.openjdk.java.net/browse/JDK-8240336) to eliminate >> the use of deprecated features removed in gradle 7: >> >> 1. Replaced `compile` with either `implementation` or `compileClasspath` as >> needed >> 2. Replaced obsolete `archiveName` and `destinationDir` properties in >> archive tasks with `archiveFileName` and `destinationDirectory` >> 3. Added missing `@Input` annotation to custom Groovy task properties >> 4. Bumped the minimum version of gradle to 6.3 (which we have been using for >> more than 1 year) >> >> >> The following changes were done for >> [JDK-8263760](https://bugs.openjdk.java.net/browse/JDK-8263760) to update to >> gradle 7.0.1: >> >> 1. Ran `bash ./gradlew wrapper --gradle-version=7.0.1` >> 2. Updated the gradle version in `build.properties` to `7.0.1` >> 3. Updated the SHA-256 checksum in `gradle/wrapper/gradle-wrapper.properties` > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "Revert auto-generated change to DEFAULT_JVM_OPTS in gradlew scripts" > > This reverts commit 0b70d096055f5b5c892f052d5eee54da357eab63. Isn't that just the settings for running the gradle wrapper - i.e. it is not setting the heap used by the gradle daemon process that will actually run the build script? I only see it used one spot in the script to run the gradle-wrapper.jar, in which case it makes sense that it is intentionally set low as the wrapper itself isn't doing much other than ensuring the correct version of Gradle is used. - PR: https://git.openjdk.java.net/jfx/pull/498
Re: RFR: 8263760: Update gradle to version 7.0.1 [v2]
On Thu, 13 May 2021 21:55:59 GMT, Kevin Rushforth wrote: >> This PR fixes the gradle deprecation warnings described in >> [JDK-8240336](https://bugs.openjdk.java.net/browse/JDK-8240336) and updates >> the JavaFX build to use gradle 7.0.1 as described in >> [JDK-8263760](https://bugs.openjdk.java.net/browse/JDK-8263760). The minimum >> version of gradle is set to 6.3. >> >> In addition to keeping gradle up to date, updating to gradle 7 will allow >> building and testing JavaFX with JDK 16. >> >> I have done a full build and test on all three platforms, comparing the >> artifacts produced before and after this change. >> >> ### Notes to Reviewers: >> >> The PR branch has two separate commits, one for each fix, in case reviewers >> want to look at them separately. As always, they will be squashed into a >> single commit when integrated. Both bug IDs will be listed in the commit. >> >> The following changes were done for >> [JDK-8240336](https://bugs.openjdk.java.net/browse/JDK-8240336) to eliminate >> the use of deprecated features removed in gradle 7: >> >> 1. Replaced `compile` with either `implementation` or `compileClasspath` as >> needed >> 2. Replaced obsolete `archiveName` and `destinationDir` properties in >> archive tasks with `archiveFileName` and `destinationDirectory` >> 3. Added missing `@Input` annotation to custom Groovy task properties >> 4. Bumped the minimum version of gradle to 6.3 (which we have been using for >> more than 1 year) >> >> >> The following changes were done for >> [JDK-8263760](https://bugs.openjdk.java.net/browse/JDK-8263760) to update to >> gradle 7.0.1: >> >> 1. Ran `bash ./gradlew wrapper --gradle-version=7.0.1` >> 2. Updated the gradle version in `build.properties` to `7.0.1` >> 3. Updated the SHA-256 checksum in `gradle/wrapper/gradle-wrapper.properties` > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "Revert auto-generated change to DEFAULT_JVM_OPTS in gradlew scripts" > > This reverts commit 0b70d096055f5b5c892f052d5eee54da357eab63. We haven't updated the gradle wrapper since gradle 4.3. I did it this time as a better practice, which came up during the review of PR #411 in [this comment](https://github.com/openjdk/jfx/pull/411#issuecomment-784438990). Anyway, I do agree with you that the value is (surprisingly) low, but it's easy to override this by setting the `GRADLE_OPTS` env var. That's what we've always done in our CI build. Let's see what the reviewers think. If others think we should change it, I'd certainly be willing to reconsider setting it to a higher value (which seems better than just removing the setting like I did in the earlier, now-reverted commit). - PR: https://git.openjdk.java.net/jfx/pull/498
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
On Fri, 14 May 2021 22:30:16 GMT, Michael Strauß wrote: >> The internal BidirectionalBinding class implements bidirectional bindings >> for JavaFX properties. The design intent of this class is to provide >> specializations for primitive value types to prevent boxing conversions (cf. >> specializations of the Property class with a similar design intent). >> >> However, the primitive BidirectionalBinding implementations do not meet the >> design goal of preventing boxing conversions, because they implement >> ChangeListener. >> >> ChangeListener is a generic SAM interface, which makes it impossibe to >> invoke an implementation of ChangeListener::changed with a primitive value >> (i.e. any primitive value will be auto-boxed). >> >> The boxing conversion happens, as with all ChangeListeners, at the >> invocation site (for example, in ExpressionHelper). Since the boxing >> conversion has already happened by the time any of the BidirectionalBinding >> implementations is invoked, there's no point in using primitive >> specializations of BidirectionalBinding after the fact. >> >> This issue can be solved by having BidirectionalBinding implement >> InvalidationListener instead, which by itself does not incur a boxing >> conversion. Because bidirectional bindings are eagerly evaluated, the >> observable behavior remains the same. >> >> I've filed a bug report with the same title. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > added missing oldValue assignments Looks good (I note that the test failure on Linux is an unrelated bug that is under evaluation). - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Thu, 15 Apr 2021 02:21:50 GMT, Nir Lisker wrote: >> Added a SpotLight only to the D3D pipeline currently. >> >> ### API discussion points >> >> - [X] Added `SpotLight` as a subclass of `LightBase`. However, it could >> also be a subclass of `PointLight` as it's a point light with direction and >> extra factors. I saw that `scenario.effect.light.SpotLight` extends its >> respective `PointLight`, but it's not a perfect analogy. In the end, I think >> it's a questions of whether `PointLight` will be expanded in a way which >> doesn't not suit `SpotLight`, and I tend to think that the answer is no. >> >> - [X] The inner and outer angles are the "diameter angles" as shown >> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types). >> I, personally, find it more intuitive that these are the "radius angles", >> so half these angles, as used in the spotlight factor formula. Do you think >> I can change this or do you prefer the current definition of the angles? >> >> - [ ] The current implementation uses an ad-hoc direction property (using a >> `Point3D`). It crossed my mind that we could use the rotation transforms of >> the node to control the direction instead, just like we use the >> translation/layout of the node to get the position (there is an internal >> Affine3D transform for lights, not sure why `AmbientLight` needs it). >> Wouldn't that make more sense? When I rotate the light I would expect to see >> a change in direction. >> >> ### Implementation discussion points >> >> - [ ] I've gotten advice from a graphics engineer to treat point lights as >> spot lights with a 360 degrees coverage, which simplifies a few places. We >> can still try to optimize for a point light by looking at the light >> parameters: `falloff = 0` and `outerAngle = 180`. These possible >> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the >> pixel/fragment shaders in the form of 3 different ways to compute the >> spotlight factor (the `computeLightN` methods). We need to check which of >> these give the best results. >> >> ## Performance >> >> Testing 3 point lights and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Combined rotation and direction You may be right about the scale problem being preexisting. I'll double check when I test on Mac next week (both Retina and external screen). - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Sat, 3 Apr 2021 15:20:41 GMT, Michael Strauß wrote: > The internal BidirectionalBinding class implements bidirectional bindings for > JavaFX properties. The design intent of this class is to provide > specializations for primitive value types to prevent boxing conversions (cf. > specializations of the Property class with a similar design intent). > > However, the primitive BidirectionalBinding implementations do not meet the > design goal of preventing boxing conversions, because they implement > ChangeListener. > > ChangeListener is a generic SAM interface, which makes it impossibe to invoke > an implementation of ChangeListener::changed with a primitive value (i.e. any > primitive value will be auto-boxed). > > The boxing conversion happens, as with all ChangeListeners, at the invocation > site (for example, in ExpressionHelper). Since the boxing conversion has > already happened by the time any of the BidirectionalBinding implementations > is invoked, there's no point in using primitive specializations of > BidirectionalBinding after the fact. > > This issue can be solved by having BidirectionalBinding implement > InvalidationListener instead, which by itself does not incur a boxing > conversion. Because bidirectional bindings are eagerly evaluated, the > observable behavior remains the same. > > I've filed a bug report with the same title. I've added the two missing `oldValue` initializations. - PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
On Fri, 14 May 2021 21:40:39 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> added missing oldValue assignments
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
> line 585:
>
>> 583: private TypedGenericBidirectionalBinding(Property property1,
>> Property property2) {
>> 584: super(property1, property2);
>> 585: propertyRef1 = new WeakReference<>(property1);
>
> Don't you need to initialize `oldValue` here?
>
>
> oldValue = property1.get();
Yes, it should be initialized.
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
> The internal BidirectionalBinding class implements bidirectional bindings for > JavaFX properties. The design intent of this class is to provide > specializations for primitive value types to prevent boxing conversions (cf. > specializations of the Property class with a similar design intent). > > However, the primitive BidirectionalBinding implementations do not meet the > design goal of preventing boxing conversions, because they implement > ChangeListener. > > ChangeListener is a generic SAM interface, which makes it impossibe to invoke > an implementation of ChangeListener::changed with a primitive value (i.e. any > primitive value will be auto-boxed). > > The boxing conversion happens, as with all ChangeListeners, at the invocation > site (for example, in ExpressionHelper). Since the boxing conversion has > already happened by the time any of the BidirectionalBinding implementations > is invoked, there's no point in using primitive specializations of > BidirectionalBinding after the fact. > > This issue can be solved by having BidirectionalBinding implement > InvalidationListener instead, which by itself does not incur a boxing > conversion. Because bidirectional bindings are eagerly evaluated, the > observable behavior remains the same. > > I've filed a bug report with the same title. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: added missing oldValue assignments - Changes: - all: https://git.openjdk.java.net/jfx/pull/454/files - new: https://git.openjdk.java.net/jfx/pull/454/files/1142087a..538b9b38 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=454&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=454&range=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/454.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/454/head:pull/454 PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Fri, 14 May 2021 21:28:15 GMT, Kevin Rushforth wrote: > There is also the outstanding issue with a retina display (scale=2) on macOS. Yes, it has to be looked at, but I think it is unrelated to this particular patch. If this is the case, we can file a different issue and continue with this one. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Sat, 3 Apr 2021 15:20:41 GMT, Michael Strauß wrote:
> The internal BidirectionalBinding class implements bidirectional bindings for
> JavaFX properties. The design intent of this class is to provide
> specializations for primitive value types to prevent boxing conversions (cf.
> specializations of the Property class with a similar design intent).
>
> However, the primitive BidirectionalBinding implementations do not meet the
> design goal of preventing boxing conversions, because they implement
> ChangeListener.
>
> ChangeListener is a generic SAM interface, which makes it impossibe to invoke
> an implementation of ChangeListener::changed with a primitive value (i.e. any
> primitive value will be auto-boxed).
>
> The boxing conversion happens, as with all ChangeListeners, at the invocation
> site (for example, in ExpressionHelper). Since the boxing conversion has
> already happened by the time any of the BidirectionalBinding implementations
> is invoked, there's no point in using primitive specializations of
> BidirectionalBinding after the fact.
>
> This issue can be solved by having BidirectionalBinding implement
> InvalidationListener instead, which by itself does not incur a boxing
> conversion. Because bidirectional bindings are eagerly evaluated, the
> observable behavior remains the same.
>
> I've filed a bug report with the same title.
The fix looks good, but I noted two places where I think you need to initialize
`oldValue`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
line 585:
> 583: private TypedGenericBidirectionalBinding(Property property1,
> Property property2) {
> 584: super(property1, property2);
> 585: propertyRef1 = new WeakReference<>(property1);
Don't you need to initialize `oldValue` here?
oldValue = property1.get();
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
line 657:
> 655: private TypedNumberBidirectionalBinding(Property property1,
> Property property2) {
> 656: super(property1, property2);
> 657: propertyRef1 = new WeakReference<>(property1);
Same comment as above. Shouldn't `oldValue` be set?
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Thu, 15 Apr 2021 02:21:50 GMT, Nir Lisker wrote: >> Added a SpotLight only to the D3D pipeline currently. >> >> ### API discussion points >> >> - [X] Added `SpotLight` as a subclass of `LightBase`. However, it could >> also be a subclass of `PointLight` as it's a point light with direction and >> extra factors. I saw that `scenario.effect.light.SpotLight` extends its >> respective `PointLight`, but it's not a perfect analogy. In the end, I think >> it's a questions of whether `PointLight` will be expanded in a way which >> doesn't not suit `SpotLight`, and I tend to think that the answer is no. >> >> - [X] The inner and outer angles are the "diameter angles" as shown >> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types). >> I, personally, find it more intuitive that these are the "radius angles", >> so half these angles, as used in the spotlight factor formula. Do you think >> I can change this or do you prefer the current definition of the angles? >> >> - [ ] The current implementation uses an ad-hoc direction property (using a >> `Point3D`). It crossed my mind that we could use the rotation transforms of >> the node to control the direction instead, just like we use the >> translation/layout of the node to get the position (there is an internal >> Affine3D transform for lights, not sure why `AmbientLight` needs it). >> Wouldn't that make more sense? When I rotate the light I would expect to see >> a change in direction. >> >> ### Implementation discussion points >> >> - [ ] I've gotten advice from a graphics engineer to treat point lights as >> spot lights with a 360 degrees coverage, which simplifies a few places. We >> can still try to optimize for a point light by looking at the light >> parameters: `falloff = 0` and `outerAngle = 180`. These possible >> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the >> pixel/fragment shaders in the form of 3 different ways to compute the >> spotlight factor (the `computeLightN` methods). We need to check which of >> these give the best results. >> >> ## Performance >> >> Testing 3 point lights and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Combined rotation and direction I started to look at this today, and will continue next week. There is also the outstanding issue with a retina display (scale=2) on macOS. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: CFV: New OpenJFX Reviewer: Jeanette Winzenburg
Vote: YES On Fri, May 14, 2021 at 5:23 PM Kevin Rushforth wrote: > I hereby nominate Jeanette Winzenburg [1] to OpenJFX Reviewer. > > Jeanette is an OpenJFX community member, who has contributed 33 commits > [2][3] to OpenJFX. Jeanette consistently participates in code reviews of > various OpenJFX pull requests, specifically in the area of UI Controls. > > Votes are due by May 28, 2021 at 16:00 UTC. > > Only current OpenJFX Reviewers [4] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Three-Vote Consensus voting instructions, see [5]. Nomination to a > project Reviewer is described in [6]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.java.net/census#fastegal > > [2] > > https://github.com/openjdk/jfx/search?q=author-email%3Afastegal%40openjdk.org&s=author-date&type=Commits > > [3] > > https://github.com/openjdk/jfx/search?q=author-email%3Afastegal%40swingempire.de&s=author-date&type=Commits > > > [4] https://openjdk.java.net/census#openjfx > > [5] https://openjdk.java.net/bylaws#three-vote-consensus > > [6] https://openjdk.java.net/projects#project-reviewer > >
Re: CFV: New OpenJFX Reviewer: Arun Joseph
Vote: YES On Fri, May 14, 2021 at 5:23 PM Kevin Rushforth wrote: > I hereby nominate Arun Joseph [1] to OpenJFX Reviewer. > > Arun is a member of JavaFX team at Oracle, who has contributed 41 > commits [2][3][4] to OpenJFX. Arun often participates in code reviews of > various OpenJFX pull requests. > > Votes are due by May 28, 2021 at 16:00 UTC. > > Only current OpenJFX Reviewers [5] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Three-Vote Consensus voting instructions, see [6]. Nomination to a > project Reviewer is described in [7]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.java.net/census#ajoseph > > [2] > > https://github.com/openjdk/jfx/search?q=author-email%3Aajoseph%40openjdk.org&s=author-date&type=Commits > > [3] > > https://github.com/openjdk/jfx/search?q=author-email%3Aarun.aj.joseph%40oracle.com&s=author-date&type=Commits > > [4] > > https://github.com/openjdk/jfx/commit/b2d85645ffc7edc327b28e12eede6505912d7491 > > [5] https://openjdk.java.net/census#openjfx > > [6] https://openjdk.java.net/bylaws#three-vote-consensus > > [7] https://openjdk.java.net/projects#project-reviewer > >
Re: RFR: 8267160: Monocle mouse never get ENTERED state
On Fri, 14 May 2021 16:29:19 GMT, Kevin Rushforth wrote: >> true to both questions. >> The reason for this change is that this is now doing the same as what is >> done in `monocle.TouchState` and `monocle.TouchInput`. >> TBH, I would prefer a different approach, where no fallback is passed, and >> the `if` block is only executed when `recalculateCache` is set to `true`. >> The only case where this `getWindow` method is invoked, is from `MouseInput` >> and it is only used to detect if the "old" window is different from the >> current window. Hence, it all seems a bit overkill to me, but for some >> reason, I thought keeping things consistent (mouseState versus TouchState) >> would make sense. >> If you are ok with it, I can simply rewrite this without looking at the >> TouchState case? > > Either is fine with me. As long as this fixes your problem, go ahead with the > fix. Or if you prefer, go ahead and rewrite it. I'll keep it like this for now, and I created a follow-up issue (JDK-8267175) for dealing with touch/mouse/key state/input handling in general in monocle. - PR: https://git.openjdk.java.net/jfx/pull/502
Integrated: 8267160: Monocle mouse never get ENTERED state
On Fri, 14 May 2021 14:37:04 GMT, Johan Vos wrote: > allow to pass a fallback window in case the existing one is null (or can't be > computed). > > Fix for JDK-8267160 This pull request has now been integrated. Changeset: 8ca7815a Author:Johan Vos URL: https://git.openjdk.java.net/jfx/commit/8ca7815afc2d592fe057ec50328fdc41ca86112e Stats: 13 lines in 2 files changed: 7 ins; 0 del; 6 mod 8267160: Monocle mouse never get ENTERED state Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/502
Re: CFV: New OpenJFX Reviewer: Jeanette Winzenburg
Vote: YES On Fri, May 14, 2021 at 6:25 PM Kevin Rushforth wrote: > Vote: YES > > -- Kevin > > On 5/14/2021 8:23 AM, Kevin Rushforth wrote: > > I hereby nominate Jeanette Winzenburg [1] to OpenJFX Reviewer. > >
Re: CFV: New OpenJFX Reviewer: Arun Joseph
Vote: YES On Fri, May 14, 2021 at 6:25 PM Kevin Rushforth wrote: > Vote: YES > > -- Kevin > > On 5/14/2021 8:22 AM, Kevin Rushforth wrote: > > I hereby nominate Arun Joseph [1] to OpenJFX Reviewer. > >
Re: RFR: 8267160: Monocle mouse never get ENTERED state
On Fri, 14 May 2021 14:37:04 GMT, Johan Vos wrote: > allow to pass a fallback window in case the existing one is null (or can't be > computed). > > Fix for JDK-8267160 Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/502
Re: RFR: 8267160: Monocle mouse never get ENTERED state
On Fri, 14 May 2021 15:55:48 GMT, Johan Vos wrote:
>> modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/MouseState.java
>> line 94:
>>
>>> 92: */
>>> 93: MonocleWindow getWindow(boolean recalculateCache, MonocleWindow
>>> fallback) {
>>> 94: if (recalculateCache) {
>>
>> Two questions:
>>
>> 1. It looks like `fallback` is effectively unused (always null). Did you add
>> this for a future use case?
>> 2. Unless I'm missing something, the only behavioral change is that it no
>> longer executes the `if` block when the window is null if `recalculateCache`
>> is false.
>
> true to both questions.
> The reason for this change is that this is now doing the same as what is done
> in `monocle.TouchState` and `monocle.TouchInput`.
> TBH, I would prefer a different approach, where no fallback is passed, and
> the `if` block is only executed when `recalculateCache` is set to `true`. The
> only case where this `getWindow` method is invoked, is from `MouseInput` and
> it is only used to detect if the "old" window is different from the current
> window. Hence, it all seems a bit overkill to me, but for some reason, I
> thought keeping things consistent (mouseState versus TouchState) would make
> sense.
> If you are ok with it, I can simply rewrite this without looking at the
> TouchState case?
Either is fine with me. As long as this fixes your problem, go ahead with the
fix. Or if you prefer, go ahead and rewrite it.
-
PR: https://git.openjdk.java.net/jfx/pull/502
Re: RFR: 8267160: Monocle mouse never get ENTERED state
On Fri, 14 May 2021 15:37:38 GMT, Kevin Rushforth wrote:
>> allow to pass a fallback window in case the existing one is null (or can't
>> be computed).
>>
>> Fix for JDK-8267160
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/MouseState.java
> line 94:
>
>> 92: */
>> 93: MonocleWindow getWindow(boolean recalculateCache, MonocleWindow
>> fallback) {
>> 94: if (recalculateCache) {
>
> Two questions:
>
> 1. It looks like `fallback` is effectively unused (always null). Did you add
> this for a future use case?
> 2. Unless I'm missing something, the only behavioral change is that it no
> longer executes the `if` block when the window is null if `recalculateCache`
> is false.
true to both questions.
The reason for this change is that this is now doing the same as what is done
in `monocle.TouchState` and `monocle.TouchInput`.
TBH, I would prefer a different approach, where no fallback is passed, and the
`if` block is only executed when `recalculateCache` is set to `true`. The only
case where this `getWindow` method is invoked, is from `MouseInput` and it is
only used to detect if the "old" window is different from the current window.
Hence, it all seems a bit overkill to me, but for some reason, I thought
keeping things consistent (mouseState versus TouchState) would make sense.
If you are ok with it, I can simply rewrite this without looking at the
TouchState case?
-
PR: https://git.openjdk.java.net/jfx/pull/502
Re: RFR: 8267160: Monocle mouse never get ENTERED state
On Fri, 14 May 2021 14:37:04 GMT, Johan Vos wrote:
> allow to pass a fallback window in case the existing one is null (or can't be
> computed).
>
> Fix for JDK-8267160
I'm not sure I understand the fix. Questions inline.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/MouseState.java
line 94:
> 92: */
> 93: MonocleWindow getWindow(boolean recalculateCache, MonocleWindow
> fallback) {
> 94: if (recalculateCache) {
Two questions:
1. It looks like `fallback` is effectively unused (always null). Did you add
this for a future use case?
2. Unless I'm missing something, the only behavioral change is that it no
longer executes the `if` block when the window is null if `recalculateCache` is
false.
-
PR: https://git.openjdk.java.net/jfx/pull/502
Re: CFV: New OpenJFX Reviewer: Jeanette Winzenburg
Vote: YES -- Kevin On 5/14/2021 8:23 AM, Kevin Rushforth wrote: I hereby nominate Jeanette Winzenburg [1] to OpenJFX Reviewer.
Re: CFV: New OpenJFX Reviewer: Arun Joseph
Vote: YES -- Kevin On 5/14/2021 8:22 AM, Kevin Rushforth wrote: I hereby nominate Arun Joseph [1] to OpenJFX Reviewer.
CFV: New OpenJFX Reviewer: Jeanette Winzenburg
I hereby nominate Jeanette Winzenburg [1] to OpenJFX Reviewer. Jeanette is an OpenJFX community member, who has contributed 33 commits [2][3] to OpenJFX. Jeanette consistently participates in code reviews of various OpenJFX pull requests, specifically in the area of UI Controls. Votes are due by May 28, 2021 at 16:00 UTC. Only current OpenJFX Reviewers [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Three-Vote Consensus voting instructions, see [5]. Nomination to a project Reviewer is described in [6]. Thanks. -- Kevin [1] https://openjdk.java.net/census#fastegal [2] https://github.com/openjdk/jfx/search?q=author-email%3Afastegal%40openjdk.org&s=author-date&type=Commits [3] https://github.com/openjdk/jfx/search?q=author-email%3Afastegal%40swingempire.de&s=author-date&type=Commits [4] https://openjdk.java.net/census#openjfx [5] https://openjdk.java.net/bylaws#three-vote-consensus [6] https://openjdk.java.net/projects#project-reviewer
CFV: New OpenJFX Reviewer: Arun Joseph
I hereby nominate Arun Joseph [1] to OpenJFX Reviewer. Arun is a member of JavaFX team at Oracle, who has contributed 41 commits [2][3][4] to OpenJFX. Arun often participates in code reviews of various OpenJFX pull requests. Votes are due by May 28, 2021 at 16:00 UTC. Only current OpenJFX Reviewers [5] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Three-Vote Consensus voting instructions, see [6]. Nomination to a project Reviewer is described in [7]. Thanks. -- Kevin [1] https://openjdk.java.net/census#ajoseph [2] https://github.com/openjdk/jfx/search?q=author-email%3Aajoseph%40openjdk.org&s=author-date&type=Commits [3] https://github.com/openjdk/jfx/search?q=author-email%3Aarun.aj.joseph%40oracle.com&s=author-date&type=Commits [4] https://github.com/openjdk/jfx/commit/b2d85645ffc7edc327b28e12eede6505912d7491 [5] https://openjdk.java.net/census#openjfx [6] https://openjdk.java.net/bylaws#three-vote-consensus [7] https://openjdk.java.net/projects#project-reviewer
RFR: 8267160: Monocle mouse never get ENTERED state
allow to pass a fallback window in case the existing one is null (or can't be computed). Fix for JDK-8267160 - Commit messages: - allow to pass a fallback window in case the existing one is null (or can't be computed). Changes: https://git.openjdk.java.net/jfx/pull/502/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=502&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267160 Stats: 13 lines in 2 files changed: 7 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/502.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/502/head:pull/502 PR: https://git.openjdk.java.net/jfx/pull/502
Re: RFR: 8264127: ListCell editing status is true, when index changes while editing [v12]
On Wed, 12 May 2021 08:13:32 GMT, Florian Kirmaier wrote: >> Fixing ListCell editing status is true, when index changes while editing. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8264127: > we now use a try finally statement, to make sure updateEditingIndex is > reset! I've added a test for the try-catch-finally block, based on the test in the TreeTableCell. (for some reason i can't comment it in the CR) - PR: https://git.openjdk.java.net/jfx/pull/441
Re: RFR: 8264127: ListCell editing status is true, when index changes while editing [v12]
On Thu, 13 May 2021 12:29:46 GMT, Jeanette Winzenburg
wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8264127:
>> we now use a try finally statement, to make sure updateEditingIndex is
>> reset!
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line
> 342:
>
>> 340: }
>> 341: updateEditing();
>> 342: }
>
> forgot yesterday: to keep consistent with TreeTableCell (and TreeCell), the
> updateEditing should be moved into the else-block (as last call) - couldn't
> find any difference (in fact couldn't reproduce the error the if/else is
> supposed to solve) as long as updateEditing behaves, though.
I've moved it to the if-else. Probably doesn't matter.
> modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line
> 546:
>
>> 544:
>> 545: if (editing && (index == -1 || list == null || index !=
>> editIndex)) {
>> 546: // If my index is not the one being edited then I need to
>> cancel
>
> probably just me not being able to see at a glance if the overall logic in
> the method is the exact same as before the fix (modulo the fix itself :),
> but: I would prefer an extra method used in both early return for the
> -1/null-list and the old else-if.
>
> If the second reviewer sees the equivalence at a glance, I'll believe his
> eyes :)
I've simplified the if-else statements. Should now be obvious that it's the
same as in TreeTableCell.
-
PR: https://git.openjdk.java.net/jfx/pull/441
Re: RFR: 8264127: ListCell editing status is true, when index changes while editing [v13]
> Fixing ListCell editing status is true, when index changes while editing. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: 8264127 More changes, simplifications and tests based on CR - Changes: - all: https://git.openjdk.java.net/jfx/pull/441/files - new: https://git.openjdk.java.net/jfx/pull/441/files/f4af1d26..1fa3738a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=441&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=441&range=11-12 Stats: 54 lines in 2 files changed: 46 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/441.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/441/head:pull/441 PR: https://git.openjdk.java.net/jfx/pull/441
Re: RFR: 8263760: Update gradle to version 7.0.1
On Thu, 13 May 2021 21:18:33 GMT, Kevin Rushforth wrote: > On closer inspection, I am reverting my earlier change to `gradlew` (the one > that reverted the setting of `DEFAULT_JVM_OPTS` to a 64Mbyte heap). The > `gradle` launcher in gradle 7.0.1 also sets its `DEFAULT_JVM_OPTS` to a > 64Mbyte heap as you can see in the following, taken from > `gradle-7.0.1/bin/gradle`: > > ``` > # Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to > pass JVM options to this script. > DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' > ``` > > So even though I agree that a 64 Mbyte heap is a bit on the small side, it's > more important that running `gradlew` behave the same as installing gradle > and running `$GRADLE_HOME/bin/gradle`. Plus I'd prefer not to maintain local > modifications of this upstream script. I don't think this is a good idea. The only modification of the wrapper scripts would need to be the removal of the heap setting, nothing else. Setting 64MB heap will definitely cause performance issues for JFX builds (because of Garbage collection, not enough memory for caching, ...). + the previous setting was not to restrict the heap in Wrapper scripts. So someone already removed the heap setting as 64MB is the setting back from at least Gradle 4. - PR: https://git.openjdk.java.net/jfx/pull/498
