Re: JDK-8091393: Observable collections for ObservableMap views
Then maybe a solution would be around adding new methods like
observableKeySet(). These will need to be default methods, and the
implementation could test if keySet() already returns an ObservableSet, in
which case it returns it, and if not it wraps the Set in an
ObservableSetWrapper or something like that.
On Mon, May 30, 2022 at 11:50 AM John Hendrikx
wrote:
> Sorry, I misunderstood, I missed that the methods weren't already
> defined in ObservableMap, so no existing signature is changed.
>
> --John
>
> On 30/05/2022 09:39, Tom Schindl wrote:
> > Hi,
> >
> > Well the binary compat IMHO is not a problem. If your subtype
> > overwrites the return type of a method the compiler will inserts a
> > bridge method:
> >
> > Take this example
> >
> > package bla;
> >
> > import java.util.ArrayList;
> > import java.util.Collection;
> > import java.util.List;
> >
> > public class Test {
> > public interface IB {
> > public Collection get();
> > }
> >
> > public interface I extends IB {
> > public List get();
> > }
> >
> > public class C implements I {
> > public ArrayList get() {
> > return new ArrayList();
> > }
> > }
> > }
> >
> > if you look at C with javap you'll notice
> >
> > Compiled from "Test.java"
> > public class bla.Test$C implements bla.Test$I {
> > final bla.Test this$0;
> > public bla.Test$C(bla.Test);
> > public java.util.ArrayList get();
> > public java.util.Collection get();
> > public java.util.List get();
> > }
> >
> >
> > The problem is more that if someone directly implemented ObservableMap
> > him/her self that it won't compile anymore. So it is a source
> > incompatible change.
> >
> > Tom
> >
> > Am 30.05.22 um 08:58 schrieb John Hendrikx:
> >> It's not binary compatible, as changing the return type results in a
> >> new method that compiled code won't be able to find.
> >>
> >> See also "change result type (including void)" here:
> >>
> https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods
> >>
> >>
> >> --John
> >>
> >> On 30/05/2022 03:22, Nir Lisker wrote:
> >>> Hi,
> >>>
> >>> Picking up an old issue, JDK-8091393 [1], I went ahead and looked at
> >>> the
> >>> work needed to implement it.
> >>>
> >>> keySet() and entrySet() can both be made to return ObservableSet rather
> >>> easily. values() will probably require an ObservableCollection type.
> >>>
> >>> Before discussing these details, my question is: is it backwards
> >>> compatible
> >>> to require that these methods now return a more refined type? I
> >>> think that
> >>> it will break implementations of ObservableMap out in the wild if it
> >>> overrides these methods in Map. What is the assessment here?
> >>>
> >>> https://bugs.openjdk.java.net/browse/JDK-8091393
>
JDK-8091393: Observable collections for ObservableMap views
Hi, Picking up an old issue, JDK-8091393 [1], I went ahead and looked at the work needed to implement it. keySet() and entrySet() can both be made to return ObservableSet rather easily. values() will probably require an ObservableCollection type. Before discussing these details, my question is: is it backwards compatible to require that these methods now return a more refined type? I think that it will break implementations of ObservableMap out in the wild if it overrides these methods in Map. What is the assessment here? https://bugs.openjdk.java.net/browse/JDK-8091393
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 6 May 2022 14:13:55 GMT, Michael Strauß wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unused comments, clean constructor
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 149:
>
>> 147: float spotLightsFactors[MAX_NUM_LIGHTS * 4]; // 2 angles + 1
>> falloff + 1 padding
>> 148: for (int i = 0, d = 0, p = 0, c = 0, a = 0, r = 0, s = 0; i <
>> MAX_NUM_LIGHTS; i++) {
>> 149: D3DLight light = lights[i];
>
> You're invoking the auto-generated copy constructor of `D3DLight` here, where
> the original code didn't do that. Just making sure that that's what you
> intended.
I will change to `D3DLight& light = lights[i];`.
-
PR: https://git.openjdk.java.net/jfx/pull/789
Integrated: 8285534: Update the 3D lighting test sample
On Mon, 25 Apr 2022 11:47:45 GMT, Nir Lisker wrote: > Update the the test utility. Includes: > * Refactoring since there is no more need the split pre- and post-attenuation > and light types. > * Added customizable material to the `Boxes` to test the interaction between > lights and materials.. > * Light colors can now be changed. > * Added ambient lights, > > Note that GitHub decided to count the removal of the `AttenLightingSample` > and addition of the `Controls` file as renaming. The sample is now run now > only through `LightingSample`. This pull request has now been integrated. Changeset: e24eeceb Author:Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/e24eeceb28741f4a044ea2cb0cb23a1174b27c66 Stats: 551 lines in 5 files changed: 316 ins; 198 del; 37 mod 8285534: Update the 3D lighting test sample Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/787
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 6 May 2022 14:09:13 GMT, Michael Strauß wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused comments, clean constructor > > modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h line 34: > >> 32: public: >> 33: D3DLight() = default; >> 34: virtual ~D3DLight() = default; > > It doesn't seem like this class is supposed to be subclassable. I would > suggest removing the constructor and destructor declarations and marking the > class `final`. It might be subclassed in the future. One of the next changes will be a performance upgrade attempt, and we might need to separate the light types instead of bundling them into one that simulates the others. In theory, this class can be removed even, and instead the Java code could call the rendering pipeline directly with all the needed parameters. It's a more intrusive change though, and might as well wait for Panama with this one. - PR: https://git.openjdk.java.net/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 6 May 2022 14:21:58 GMT, Michael Strauß wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused comments, clean constructor > > modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.h line 61: > >> 59: bool lightsDirty = TRUE; >> 60: int cullMode = D3DCULL_NONE; >> 61: bool wireframe = FALSE; > > It seems like you're using `false` and `FALSE` interchangably (see > `D3DPhongMaterial.h` L58). I would suggest using the `false` keyword with the > builtin type `bool`, and the `FALSE` constant with the Win32 type `BOOL`. The original code used the Win-only type. I thought it should be changed too. I should probably do that, - PR: https://git.openjdk.java.net/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
> Refactoring and renaming changes to some of the D3D pipeline files and a few > changes on the Java side. These are various "leftovers" from previous issues > that we didn't want to touch at the time in order to confine the scope of the > changes. They will make future work easier. > > Since there are many small changes, I'm giving a full list here: > > **Java** > > * `NGShape3D.java` > * Extracted methods to help with the cumbersome lighting loop: one method > per light type + empty light (reset light) + default point light. This > section of the code would benefit from the upcoming pattern matching on > `switch`. > * Normalized the direction here instead of in the native code. > * Ambient light is now only set when it exists (and is not black). > * `NGPointLight,java` - removed unneeded methods that were used by > `NGShape3D` before the per-lighting methods were extracted (point light > doesn't need spotlight-specific methods since they each have their own "add" > method). > * `NGSpotLight.java` - removed `@Override` annotations as a result of the > above change. > > **Native C++** > > * Initialized the class members of `D3DLight`, `D3DMeshView` and > `D3DPhongMaterial` in the header file instead of a more cumbersome > initialization in the constructor (this is allowed since C++ 11). > * `D3DLight` > * Commented out unused methods. Were they supposed to be used at some point? > * Renamed the `w` component to `lightOn` since it controls the number of > lights for the special pixel shader variant and having it in the 4th position > of the color array was confusing. > * `D3DPhongShader.h` > * Renamed some of the register constants for more clarity. > * Moved the ambient light color constant from the vertex shader to the > pixel shader (see the shader section below). I don't understand the > calculation of the number of registers in the comment there: "8 ambient > points + 2 coords = 10". There is 1 ambient light, what are the ambient > points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it > is unused. > * Reduced the number of assigned vertex register for the `VSR_LIGHTS` > constant since it included both position and color, but color was unused > there (it was used directly in the pixel shader), so now it's only the > position. > * `D3DMeshView.cc` > * Unified the lighting loop that prepares the lights' 4-vetors that are > passed to the shaders. > * Removed the direction normalization as stated in the change for > `NGShape3D.java`. > * Reordered the shader constant assignment to be the same order as in > `D3DPhongShader.h`. > > **Native shaders** > * Renamed many of the variables to what I think are more descriptive names. > This includes noting in which space they exist as some calculations are done > in model space, some in world space, and we might need to do some in view > space. For vectors, also noted if the vector is to or from (`eye` doesn't > tell me if it's from or to the camera). > * Commented out many unused functions. I don't know what they are for, so I > didn't remove them. > * `vsConstants` > * Removed the light color from here since it's unused, only the position is. > * Removed the ambient light color constant from here since it's unused, and > added it to `psConstants` instead. > * `vs2ps` > * Removed the ambient color interpolation, which frees a register (no > change in performance). > * Simplified the structure (what is `LocalBumpOut` and why is it called > `light` and contains `LocalBump?). > * `Mtl1PS` and `psMath` > * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are > used for better clarity. > * Moved the lights loop to `Mtl1PS`. The calculation itself remains in > `psMath`. > > No changes in performance were measured and the behavior stayed the same. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Remove unused comments, clean constructor - Changes: - all: https://git.openjdk.java.net/jfx/pull/789/files - new: https://git.openjdk.java.net/jfx/pull/789/files/8db9c8ba..05281ba3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=789=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=789=00-01 Stats: 21 lines in 2 files changed: 0 ins; 18 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/789.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/789/head:pull/789 PR: https://git.openjdk.java.net/jfx/pull/789
RFR: 8217853: Cleanup in the D3D native pipeline
Refactoring and renaming changes to some of the D3D pipeline files and a few changes on the Java side. These are various "leftovers" from previous issues that we didn't want to touch at the time in order to confine the scope of the changes. They will make future work easier. Since there are many small changes, I'm giving a full list here: **Java** * `NGShape3D.java` * Extracted methods to help with the cumbersome lighting loop: one method per light type + empty light (reset light) + default point light. This section of the code would benefit from the upcoming pattern matching on `switch`. * Normalized the direction here instead of in the native code. * Ambient light is now only set when it exists (and is not black). * `NGPointLight,java` - removed unneeded methods that were used by `NGShape3D` before the per-lighting methods were extracted (point light doesn't need spotlight-specific methods since they each have their own "add" method). * `NGSpotLight.java` - removed `@Override` annotations as a result of the above change. **Native C++** * Initialized the class members of `D3DLight`, `D3DMeshView` and `D3DPhongMaterial` in the header file instead of a more cumbersome initialization in the constructor (this is allowed since C++ 11). * `D3DLight` * Commented out unused methods. Were they supposed to be used at some point? * Renamed the `w` component to `lightOn` since it controls the number of lights for the special pixel shader variant and having it in the 4th position of the color array was confusing. * `D3DPhongShader.h` * Renamed some of the register constants for more clarity. * Moved the ambient light color constant from the vertex shader to the pixel shader (see the shader section below). I don't understand the calculation of the number of registers in the comment there: "8 ambient points + 2 coords = 10". There is 1 ambient light, what are the ambient points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it is unused. * Reduced the number of assigned vertex register for the `VSR_LIGHTS` constant since it included both position and color, but color was unused there (it was used directly in the pixel shader), so now it's only the position. * `D3DMeshView.cc` * Unified the lighting loop that prepares the lights' 4-vetors that are passed to the shaders. * Removed the direction normalization as stated in the change for `NGShape3D.java`. * Reordered the shader constant assignment to be the same order as in `D3DPhongShader.h`. **Native shaders** * Renamed many of the variables to what I think are more descriptive names. This includes noting in which space they exist as some calculations are done in model space, some in world space, and we might need to do some in view space. For vectors, also noted if the vector is to or from (`eye` doesn't tell me if it's from or to the camera). * Commented out many unused functions. I don't know what they are for, so I didn't remove them. * `vsConstants` * Removed the light color from here since it's unused, only the position is. * Removed the ambient light color constant from here since it's unused, and added it to `psConstants` instead. * `vs2ps` * Removed the ambient color interpolation, which frees a register (no change in performance). * Simplified the structure (what is `LocalBumpOut` and why is it called `light` and contains `LocalBump?). * `Mtl1PS` and `psMath` * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are used for better clarity. * Moved the lights loop to `Mtl1PS`. The calculation itself remains in `psMath`. No changes in performance were measured and the behavior stayed the same. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jfx/pull/789/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=789=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8217853 Stats: 624 lines in 18 files changed: 232 ins; 202 del; 190 mod Patch: https://git.openjdk.java.net/jfx/pull/789.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/789/head:pull/789 PR: https://git.openjdk.java.net/jfx/pull/789
Re: D3D pipeline possible inconsistencies
> > It's possible (although > I don't know for sure) that the image is being treated as a non- > premultiplied color format, and is subsequently converted to a > premultiplied format; if so, this could explain the color darkening. The image is constructed with var image = new WritableImage(1, 1); I called image.getPixelWriter().getPixelFormat().getType(); and got BYTE_BGRA_PRE so it's premultiplied. I filed a placeholder JBS issue [1] until it's determined what exactly needs to be fixed. Do all of the above issues happen with the OpenGL shaders, too? I haven't tested yet, but I will. [1] https://bugs.openjdk.java.net/browse/JDK-8285862 On Wed, Apr 27, 2022 at 3:02 AM Kevin Rushforth wrote: > As you note, there are a few different issues here. To answer your > questions as best I can: > > 1 & 3. We should document that self-illum maps and specular only use the > rgb components and that alpha should be ignored. It's possible (although > I don't know for sure) that the image is being treated as a non- > premultiplied color format, and is subsequently converted to a > premultiplied format; if so, this could explain the color darkening. > > 2. This also needs to be documented. The diffuse component should have > an alpha that applies whether from the diffuse color or from a diffuse > map. I agree with you that the pixel fragment should not be discarded > just because the diffuse component is transparent. A specular highlight > should be possible on a fully transparent object. > > 4. It does seem that the result should be the same regardless of whether > the color comes from a specular map or color, but I'd need to dig further. > > Do all of the above issues happen with the OpenGL shaders, too? > > -- Kevin > > > On 4/26/2022 11:41 AM, Nir Lisker wrote: > > I found a comment [1] on JBS stating that specular and self-Illumination > > alphas should be ignored, so it seems like there's at least 2 bugs here > > already. > > > > > https://bugs.openjdk.java.net/browse/JDK-8090548?focusedCommentId=13771150=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13771150 > > > > On Tue, Apr 26, 2022 at 4:25 AM Nir Lisker wrote: > > > >> Hi, > >> > >> Using the updated lighting test sample [1], I found some odd behavior > with > >> regards to PhongMaterial: > >> > >> 1. The effect of the opacity (alpha channel) of a self-illumination map > is > >> not documented, but lowering its value makes the object darker. I > looked at > >> the pixel shader [2] and only the rgb components are sampled, so I'm a > bit > >> confused here. What is the intended behavior? > >> > >> 2. The opacity of the object is controlled in the shader by both the > >> diffuse color and diffuse map. This is also not documented (although it > >> might be obvious for some). In the shader, the pixel (fragment) is > >> discarded only if the map is fully transparent (line 55), but not the > >> color. This leads to a situation where the object completely disappears > >> when the map is transparent, but not when the color is. In the shader, > the > >> pixel should be transparent because of the multiplication of the alpha, > but > >> it's not, so this is also confusing. Should they both have the same > >> contribution? Shouldn't it be valid to have a transparent diffuse but > still > >> have specular reflections? > >> > >> 3. The specular map and color behave differently in regards to the > >> opacity. There is no documented behavior here. The alpha on the color is > >> ignored (it's used for the specular power), but not on the map - it > >> controls the reflection's strength, probably by making its color > darker. In > >> the shader, lines 76-84 indeed ignore the alpha of the color, but take > the > >> alpha of the map, although later in line 93 it's not used, so again I'm > >> confused. What's the intended behavior? > >> > >> 4. The specular map and color also behave differently in regards to the > >> reflection's strength. In the shader, this happens in line 78: the > specular > >> power is corrected with NTSC_Gray if there is a map (with or without > >> color), but not if there's only a color. Shouldn't the contributions be > the > >> same? Is the NTSC_Gray correction correct in this case? > >> > >> Thanks, > >> Nir > >> > >> [1] https://github.com/openjdk/jfx/pull/787 > >> [2] > >> > https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-prism-d3d/hlsl/Mtl1PS.hlsl > >> > >
Re: RFR: 8285534: Update the 3D lighting test sample [v3]
> Update the the test utility. Includes: > * Refactoring since there is no more need the split pre- and post-attenuation > and light types. > * Added customizable material to the `Boxes` to test the interaction between > lights and materials.. > * Light colors can now be changed. > > Note that GitHub decided to count the removal of the `AttenLightingSample` > and addition of the `Controls` file as renaming. The sample is now run now > only through `LightingSample`. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Added ambient lights - Changes: - all: https://git.openjdk.java.net/jfx/pull/787/files - new: https://git.openjdk.java.net/jfx/pull/787/files/dae9c1d7..cbca5f8f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=787=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=787=01-02 Stats: 23 lines in 3 files changed: 18 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/787.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/787/head:pull/787 PR: https://git.openjdk.java.net/jfx/pull/787
Re: RFR: 8285534: Update the 3D lighting test sample [v2]
> Update the the test utility. Includes: > * Refactoring since there is no more need the split pre- and post-attenuation > and light types. > * Added customizable material to the `Boxes` to test the interaction between > lights and materials.. > * Light colors can now be changed. > > Note that GitHub decided to count the removal of the `AttenLightingSample` > and addition of the `Controls` file as renaming. The sample is now run now > only through `LightingSample`. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Fix for directional lights not added to scene - Changes: - all: https://git.openjdk.java.net/jfx/pull/787/files - new: https://git.openjdk.java.net/jfx/pull/787/files/2f8fdbef..dae9c1d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=787=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=787=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/787.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/787/head:pull/787 PR: https://git.openjdk.java.net/jfx/pull/787
Integrated: 8285725: Wrong link to JBS in README.md
On Wed, 27 Apr 2022 14:01:06 GMT, Nir Lisker wrote: > Updated the README link to match the CONTRIBUTING link. This pull request has now been integrated. Changeset: d69a498c Author: Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/d69a498c2cde73339bc99e6c02c0d47fe4b1b650 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8285725: Wrong link to JBS in README.md Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/788
RFR: 8285725: Wrong link to JBS in README.md
Updated the README link to match the CONTRIBUTING link. - Commit messages: - Fixed link Changes: https://git.openjdk.java.net/jfx/pull/788/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=788=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285725 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/788.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/788/head:pull/788 PR: https://git.openjdk.java.net/jfx/pull/788
Re: Wrong link in README.md?
I created https://github.com/openjdk/jfx/pull/788. By the way, 'openjfx18' version is listed in JBS as unreleased. On Wed, Apr 27, 2022 at 3:16 PM Kevin Rushforth wrote: > Yes, this seems like a bug. I agree that it would be better for the > "issues list" link to use the same filtered list of issues that > CONTRIBUTING.md links to. > > -- Kevin > > > On 4/26/2022 8:54 PM, Nir Lisker wrote: > > In the README.md, under Issue Tracking, the link to "issues list" leads > to > > the JBS homepage. In CONTRIBUTING.md under Bug Report, the (almost) same > > paragraph links to the JavaFX filter in JBS, which is a lot more helpful. > > Shouldn't the link in README also link to the filtered issues list? > > > > - Nir > >
Wrong link in README.md?
In the README.md, under Issue Tracking, the link to "issues list" leads to the JBS homepage. In CONTRIBUTING.md under Bug Report, the (almost) same paragraph links to the JavaFX filter in JBS, which is a lot more helpful. Shouldn't the link in README also link to the filtered issues list? - Nir
Re: D3D pipeline possible inconsistencies
I found a comment [1] on JBS stating that specular and self-Illumination alphas should be ignored, so it seems like there's at least 2 bugs here already. https://bugs.openjdk.java.net/browse/JDK-8090548?focusedCommentId=13771150=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13771150 On Tue, Apr 26, 2022 at 4:25 AM Nir Lisker wrote: > Hi, > > Using the updated lighting test sample [1], I found some odd behavior with > regards to PhongMaterial: > > 1. The effect of the opacity (alpha channel) of a self-illumination map is > not documented, but lowering its value makes the object darker. I looked at > the pixel shader [2] and only the rgb components are sampled, so I'm a bit > confused here. What is the intended behavior? > > 2. The opacity of the object is controlled in the shader by both the > diffuse color and diffuse map. This is also not documented (although it > might be obvious for some). In the shader, the pixel (fragment) is > discarded only if the map is fully transparent (line 55), but not the > color. This leads to a situation where the object completely disappears > when the map is transparent, but not when the color is. In the shader, the > pixel should be transparent because of the multiplication of the alpha, but > it's not, so this is also confusing. Should they both have the same > contribution? Shouldn't it be valid to have a transparent diffuse but still > have specular reflections? > > 3. The specular map and color behave differently in regards to the > opacity. There is no documented behavior here. The alpha on the color is > ignored (it's used for the specular power), but not on the map - it > controls the reflection's strength, probably by making its color darker. In > the shader, lines 76-84 indeed ignore the alpha of the color, but take the > alpha of the map, although later in line 93 it's not used, so again I'm > confused. What's the intended behavior? > > 4. The specular map and color also behave differently in regards to the > reflection's strength. In the shader, this happens in line 78: the specular > power is corrected with NTSC_Gray if there is a map (with or without > color), but not if there's only a color. Shouldn't the contributions be the > same? Is the NTSC_Gray correction correct in this case? > > Thanks, > Nir > > [1] https://github.com/openjdk/jfx/pull/787 > [2] > https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-prism-d3d/hlsl/Mtl1PS.hlsl >
D3D pipeline possible inconsistencies
Hi, Using the updated lighting test sample [1], I found some odd behavior with regards to PhongMaterial: 1. The effect of the opacity (alpha channel) of a self-illumination map is not documented, but lowering its value makes the object darker. I looked at the pixel shader [2] and only the rgb components are sampled, so I'm a bit confused here. What is the intended behavior? 2. The opacity of the object is controlled in the shader by both the diffuse color and diffuse map. This is also not documented (although it might be obvious for some). In the shader, the pixel (fragment) is discarded only if the map is fully transparent (line 55), but not the color. This leads to a situation where the object completely disappears when the map is transparent, but not when the color is. In the shader, the pixel should be transparent because of the multiplication of the alpha, but it's not, so this is also confusing. Should they both have the same contribution? Shouldn't it be valid to have a transparent diffuse but still have specular reflections? 3. The specular map and color behave differently in regards to the opacity. There is no documented behavior here. The alpha on the color is ignored (it's used for the specular power), but not on the map - it controls the reflection's strength, probably by making its color darker. In the shader, lines 76-84 indeed ignore the alpha of the color, but take the alpha of the map, although later in line 93 it's not used, so again I'm confused. What's the intended behavior? 4. The specular map and color also behave differently in regards to the reflection's strength. In the shader, this happens in line 78: the specular power is corrected with NTSC_Gray if there is a map (with or without color), but not if there's only a color. Shouldn't the contributions be the same? Is the NTSC_Gray correction correct in this case? Thanks, Nir [1] https://github.com/openjdk/jfx/pull/787 [2] https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-prism-d3d/hlsl/Mtl1PS.hlsl
RFR: 8285534: Update the 3D lighting test sample
Update the the test utility. Includes: * Refactoring since there is no more need the split pre- and post-attenuation and light types. * Added customizable material to the `Boxes` to test the interaction between lights and materials.. * Light colors can now be changed. Note that GitHub decided to count the removal of the `AttenLightingSample` and addition of the `Controls` file as renaming. The sample is now run now only through `LightingSample`. - Commit messages: - Restore - del - Initial commit Changes: https://git.openjdk.java.net/jfx/pull/787/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=787=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285534 Stats: 541 lines in 5 files changed: 304 ins; 203 del; 34 mod Patch: https://git.openjdk.java.net/jfx/pull/787.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/787/head:pull/787 PR: https://git.openjdk.java.net/jfx/pull/787
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue
On Thu, 18 Nov 2021 21:38:28 GMT, Kevin Rushforth wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> This will need an API review followed by an implementation review.
@kevinrushforth This needs one more reviewer, it can be anyone, but I assume
you want to review it.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v14]
On Tue, 22 Mar 2022 07:46:40 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix wording
Marked as reviewed by nlisker (Reviewer).
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v13]
On Mon, 21 Mar 2022 08:59:34 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Small wording change in API of ObservableValue after proof reading
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line
164:
> 162: * @param mapper the mapping function to apply to a value, cannot be
> {@code null}
> 163: * @return an {@code ObservableValue} that holds the result of
> applying the given
> 164: * mapping function on its value, or {@code null} when it
I think "on this value", not "on its value", no?
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 09:55:30 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
>> line 146:
>>
>>> 144: * Creates an {@code ObservableValue} that holds the result of
>>> applying a
>>> 145: * mapping on this {@code ObservableValue}'s value. The result is
>>> updated
>>> 146: * when this {@code ObservableValue}'s value changes. If this
>>> value is
>>
>> I think a lot of the new documentation in this class sacrifices
>> understandability for precision in trying to deal with the difference
>> between "this ObservableValue" and "this ObservableValue's value".
>>
>> However, my feeling is that that's not helping users who are trying to
>> understand the purpose of the new APIs.
>> What do you think about a simplified version like this:
>> `Creates a new {@ObservableValue} that applies a mapping function to this
>> {@code ObservableValue}. The result is updated when this {@code
>> ObservableValue} changes.`
>>
>> Sure, it's not literally mapping _this ObservableValue instance_, but would
>> this language really confuse readers more that the precise language?
>>
>> Another option might be to combine both:
>> `Creates a new {@ObservableValue} that applies a mapping function to this
>> {@code ObservableValue}. More precisely, it creates a new {@code
>> ObservableValue} that holds the result of applying a mapping function to the
>> value of this {@code ObservableValue}.`
>
> Yeah, agreed, it is a bit annoying to have to deal with the fact that these
> classes are wrappers around an actual value and having to refer to them as
> such to be "precise". I'm willing to make another pass at all of these to
> change the wording. What do you think @nlisker ?
I read this comment after what I wrote about `flatMap`, so mstr2 also had the
idea of "More precisely", which is good :)
I would suggested something similar to what I did there:
Creates a new {@code ObservableValue} that holds the value supplied by the
given mapping function. The result
is updated when this {@code ObservableValue} changes.
If this value is {@code null}...
More precisely, the created {@code ObservableValue} holds the result of
applying a mapping on this
{@code ObservableValue}'s value.
Same comments about `@return` and `@throws` NPE as I had for `flatMap`.
`orElse` will also becomes something like
Creates a new {@code ObservableValue} that holds this value, or the given value
if it is {@code null}. The
result is updated when this {@code ObservableValue} changes.
More precisely, the created {@code ObservableValue} holds this {@code
ObservableValue}'s value, or
the given value if it is {@code null}.
Also not sure if the "More precisely" description is needed here.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 23:55:36 GMT, Michael Strauß wrote:
>> I've changed this to use your wording as I think it does read much better.
>>
>> Perhaps also possible:
>>
>> Creates a new {@code ObservableValue} that holds the value of a nested
>> {@code ObservableValue} supplied
>> by the given mapping function.
>>
>> ?
>
> Both seem fine, I don't have any preference over one or the other.
I struggled with finding a good description here
[previously](https://github.com/openjdk/jfx/pull/675#discussion_r777801130). I
think that mstr2 gave a good approach. What we can do if we want to have "the
best of both worlds" is to write something in this form:
. More precisely,
I would offer something like this based on your suggestions:
Creates a new {@code ObservableValue} that holds the value of a nested {@code
ObservableValue} supplied by the
given mapping function. The result is updated when either this or the nested
{@code ObservableValue} changes.
If either this or the nested value is {@code null}, the resulting value is
{@code null} (no mapping is applied if
this value is {@code null}).
More precisely, the created {@code ObservableValue} holds the value of an
{@code ObservableValue} resulting
from applying a mapping on this {@code ObservableValue}'s value.
I'm honestly not sure the "More precisely" part is even needed at his point. Up
to you.
The `@return` description can be changed accordingly with the simplified
explanation if you think it's clearer.
You can also specify a `@throws` NPE if the mapping function parameter is
`null` instead of writing "cannot be null", like mstr2 suggested in another
place if you like this pattern.
By the way, if we change "Creates an..." to "Creates a new..." we should change
it in the other methods. I don't think there's a difference.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 09:32:18 GMT, John Hendrikx wrote: >> modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java >> line 68: >> >>> 66: }; >>> 67: } >>> 68: } >> >> Several files are missing newlines after the last closing brace. Do we >> enforce this? >> >> Also, if there's a newline after the first line of a class declaration, >> shouldn't there also be a newline before the last closing brace? > > Let me add those new lines at the end of files (everywhere) as Github is also > flagging it with an ugly red marker. I tend to unconsciously remove them > myself on longer files as it looks weird in editors to have an unused line at > the bottom. > > As for the newline before the last closing brace, that doesn't seem to be > done a lot in the current code base. I've added those newlines at the top as > it seems fairly consistent in the code base, although I'm not a fan as I use > empty lines only to separate things when there is no clear separation already > (like an opening brace). I don't think jcheck checks for newlines anywhere. Usually the style that I see is a newline after the definition of the class and at the end of the file (sometimes), but not before the last closing brace. - PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Fri, 18 Mar 2022 09:48:39 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java
>> line 67:
>>
>>> 65: */
>>> 66: default Subscription and(Subscription other) {
>>> 67: Objects.requireNonNull(other);
>>
>> This exception could be documented with `@throws NullPointerException if
>> {@code other} is null`
>
> I've updated the docs a bit -- it hasn't received much attention because this
> is not going to be API for now
Yes, in "phase 2" when this class is made public there will be a proper docs
review.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]
On Thu, 10 Mar 2022 17:49:38 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Process review comments (2)
Re-approving
-
Marked as reviewed by nlisker (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v9]
On Thu, 10 Mar 2022 05:44:35 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Process review comments
Looks very good. I left a few minor optional comments after doing a quick
re-review of everything. You can wait until the other reviews with these if you
w
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v8]
On Tue, 8 Mar 2022 21:03:12 GMT, Nir Lisker wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix wrong test values > > modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java > line 648: > >> 646: >> 647: /** >> 648: * Ensures nothing has been observed. > > "Ensures nothing has been observed since the last check" or something like > that because the values get cleared. Do you think it's not necessary? - PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v8]
On Thu, 27 Jan 2022 21:49:07 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix wrong test values
The tests look good. I'm happy with the coverage and added one comment about a
missing case. My own sanity checks also work as I expect.
Will approve when thes
Re: Mention of the CSS properties in JavaDocs
How does it handle JavaFX-specific properties? I thought that JavaFX uses a modified javadoc tool. On Sat, Feb 12, 2022 at 4:52 PM Kevin Rushforth wrote: > While something like this could be handy, I doubt that adding this much > knowledge of JavaFX into the javadoc tool would gain any traction. > > -- Kevin > > > On 2/9/2022 7:11 AM, Nir Lisker wrote: > > Hi, > > > > When reviewing the docs changes to TabPane, I saw that some properties > > mention the CSS that is related to them. I was wondering if we could > > standardize it through something like a @css tag that is given the css > > string constant, or read automatically through the CssMetaData. > > > > As an example: > > > > /** > > * Specifies the maximum width of a tab. > > * ... > > * @css -fx-tab-max-width > > * @defaultValue 10 > > */ > > > > If the javadoc tool has access to these during its runtime, it can read > the > > string by looking in the getCssMetaData() override of the property and > then > > read the first argument of the CssMetaData constructor. > > > > Thoughts? > >
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v3]
On Wed, 9 Feb 2022 18:27:55 GMT, Ajit Ghaisas wrote: >> This is a Javadoc cleanup and correction fix for the TabPane as described in >> the JBS. >> >> Changes done for all the Properties of the TabPane - >> - Moved the property description to be over the property field. >> - Removed the unnecessary docs on property setter/getter and Property method. > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > Address additional review comments Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/728
Mention of the CSS properties in JavaDocs
Hi, When reviewing the docs changes to TabPane, I saw that some properties mention the CSS that is related to them. I was wondering if we could standardize it through something like a @css tag that is given the css string constant, or read automatically through the CssMetaData. As an example: /** * Specifies the maximum width of a tab. * ... * @css -fx-tab-max-width * @defaultValue 10 */ If the javadoc tool has access to these during its runtime, it can read the string by looking in the getCssMetaData() override of the property and then read the first argument of the CssMetaData constructor. Thoughts?
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 13:17:14 GMT, Kevin Rushforth wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
>> 738:
>>
>>> 736:
>>> 737: /**
>>> 738: * This specifies how the {@code TabPane} handles tab closing
>>> from an end-user's
>>
>> Since you are fixing the description here, can you remove the "This" in the
>> beginning?
>
> Good suggestion.
>
> Also optional, you can remove the `` since they are not needed for the
> initial paragraph.
There are many redundant `` tags in the property docs too if you want to
deal with them as well for consistency (not important).
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 07:32:20 GMT, Ajit Ghaisas wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
>> 295:
>>
>>> 293: *
>>> 294: * This value can also be set via CSS using {@code
>>> -fx-tab-min-width}.
>>> 295: *
>>
>> I would write
>>
>> The minimum width of a tab in the TabPane. This can be used to limit
>> the length of text in tabs to prevent truncation. Setting the same minimum
>> and maximum widths will fix the width of the tab.
>>
>> It makes it clear that it applies to each tab individually (and not the
>> total width of the tabs). The same applies for the other min/max
>> height/width properties.
>>
>> The sentence "By default the min equals to the max." seems wrong. The
>> `DEFAULT_TAB_MIN_WIDTH` constant is set to 0. It should also be in a
>> `@defaultValue`.
>>
>> The CSS mention is good, but I never saw it mentioned for properties before.
>> Makes me wonder if we should add the CSS property as part of a property's
>> description in general via the automated javadoc tool.
>
> Good suggestion. Fixed.
>
> Regarding CSS mention for properties, I am not sure whether that can be
> automated. If there is a way to achieve it, I can file a separate bug to
> address it.
The CSS remark was me thinking out loud. I'll write about it in the mailing
list since it's a whole new capability of the doc tool.
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 09:39:46 GMT, Ajit Ghaisas wrote:
>> This is a Javadoc cleanup and correction fix for the TabPane as described in
>> the JBS.
>>
>> Changes done for all the Properties of the TabPane -
>> - Moved the property description to be over the property field.
>> - Removed the unnecessary docs on property setter/getter and Property method.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
Looks good to me. I added 1 optional comment.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
738:
> 736:
> 737: /**
> 738: * This specifies how the {@code TabPane} handles tab closing
> from an end-user's
Since you are fixing the description here, can you remove the "This" in the
beginning?
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 10:53:00 GMT, Ajit Ghaisas wrote:
> This is a Javadoc cleanup and correction fix for the TabPane as described in
> the JBS.
>
> Changes done for all the Properties of the TabPane -
> - Moved the property description to be over the property field.
> - Removed the unnecessary docs on property setter/getter and Property method.
Moving the test to the property field and removing it from the methods looks
good. I added some suggestions to touch up the docs a bit and correct some
mistakes.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
174:
> 172: /**
> 173: * Sets the model used for tab selection. By changing the model
> you can alter
> 174: * how the tabs are selected and which tabs are first or last.
This description is phrased like it's a setter. Probably
The selection model used for selecting tabs. Changing the model alters
how the tabs are selected and which tabs are first or last.
(there's no need to the `` either)
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
188:
> 186: * the TabPane will immediately update the location of the tabs to
> reflect
> 187: * this.
> 188: * The default position for the tabs is {@code Side.Top}.
I would rephrase a bit:
The position of the tabs in the {@code TabPane}. Changes to the value of this
property
immediately update the location of the tabs.
@defaultValue {@code Side.Top}
The 2nd sentence seems redundant anyway and I suggest to remove it. Unless
otherwise specified, all value changes are applied immediately (only
`Animation` properties come to mind that specify that the animation has to be
stopped for the changes to take effect).
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
244:
> 242: * Refer to the {@link TabClosingPolicy} enumeration for further
> details.
> 243: *
> 244: * The default closing policy is TabClosingPolicy.SELECTED_TAB
* The default value should be in an `@defaultValue` annotation.
* Missing `{@code }`s
* The line "Refer to the {@link TabClosingPolicy}" is redundant I think since
it's linked in the signature/definition, and if we want to keep it then an
`@see` might be preferable.
* "end-user's" (missing apostrophe)
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
270:
> 268: * the graphic isn't rotated, resulting in it always appearing
> upright. If
> 269: * rotateGraphic is set to {@code true}, the graphic will rotate
> such that it
> 270: * rotates with the tab text.
* Missing `@code`s
* I would rephrase the 2nd paragraph to be simpler:
```
If the value is {@code false}, the graphic isn't rotated, resulting in it
always appearing upright.
If it is {@code true}, the graphic is rotate with the tab text.
```
* `@defaultValue {@code false}`
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
295:
> 293: *
> 294: * This value can also be set via CSS using {@code
> -fx-tab-min-width}.
> 295: *
I would write
The minimum width of a tab in the TabPane. This can be used to limit
the length of text in tabs to prevent truncation. Setting the same minimum
and maximum widths will fix the width of the tab.
It makes it clear that it applies to each tab individually (and not the total
width of the tabs). The same applies for the other min/max height/width
properties.
The sentence "By default the min equals to the max." seems wrong. The
`DEFAULT_TAB_MIN_WIDTH` constant is set to 0. It should also be in a
`@defaultValue`.
The CSS mention is good, bit I never saw it mentioned for properties before.
Makes me wonder if we should add the CSS property as part of a property's
description in general via the automated javadoc tool.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
337:
> 335: *
> 336: * This value can also be set via CSS using {@code
> -fx-tab-max-width}.
> 337: *
* "By default the min equals to the max." The default is `Double.MAX_VALUE`.
* Comma before "the text will be truncated".
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
378:
> 376: *
> 377: * This value can also be set via CSS using {@code
> -fx-tab-min-height}.
> 378: *
Same comments as for the width properties.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
419:
> 417: *
> 418: * This value can also be set via CSS using {@code
> -fx-tab-max-height}.
> 419: *
Same comments as for the width properties.
modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
779:
> 777: * The drag policy for the tabs. The policy can be changed
> dynamically.
> 778: *
> 779: * @defaultValue TabDragPolicy.FIXED
I think that the 2nd sentence is redundant again. I would add an explanation
like "Specifies if tabs can be reordered or
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 10:53:00 GMT, Ajit Ghaisas wrote: > This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. I'm reviewing this right now. - PR: https://git.openjdk.java.net/jfx/pull/728
[jfx18] Integrated: 8279345: Realign class docs of LightBase and subclasses
On Sun, 16 Jan 2022 22:54:22 GMT, Nir Lisker wrote: > Now that the standard concrete light types were added, there is an > opportunity to rearrange and rewrite some of the class docs. Here is a > summary of the changes: > > * Moved the explanations of attenuation and direction up to `LightBase` since > different light types share characteristics. `LightBase` now contains a > summary of its subtypes and all the explanations of the > properties/characteristics of the lights divided into sections: Color, Scope, > Direction, Attenuation. > * Each light type links to the relevant section in `LightBase` when it > mentioned the properties it has. > * Added examples of real-world applications for each light type. > * Consolidated the writing style for all the subclasses. This pull request has now been integrated. Changeset: ed399825 Author:Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/ed39982515e1f4399841ffd75849988ff651396d Stats: 152 lines in 5 files changed: 96 ins; 26 del; 30 mod 8279345: Realign class docs of LightBase and subclasses Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses [v5]
On Fri, 4 Feb 2022 18:26:57 GMT, Nir Lisker wrote: >> Now that the standard concrete light types were added, there is an >> opportunity to rearrange and rewrite some of the class docs. Here is a >> summary of the changes: >> >> * Moved the explanations of attenuation and direction up to `LightBase` >> since different light types share characteristics. `LightBase` now contains >> a summary of its subtypes and all the explanations of the >> properties/characteristics of the lights divided into sections: Color, >> Scope, Direction, Attenuation. >> * Each light type links to the relevant section in `LightBase` when it >> mentioned the properties it has. >> * Added examples of real-world applications for each light type. >> * Consolidated the writing style for all the subclasses. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Changed description of ambient light Thanks for the reviews everyone! - PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses [v5]
On Thu, 27 Jan 2022 19:01:32 GMT, Kevin Rushforth wrote: >> I think the description should focus on the meaning of the respective term >> in the lighting equation, and not on a non-technical analogy. In this case, >> the analogy is a bit misleading on several aspects, including the fact that >> ambient lighting is not dependent on an area being enclosed. Here's a >> suggestion: >> >> Ambient lights add a constant term to the amount of light reflected by each >> point on >> the surface of an object, thereby increasing the brightness of the object >> uniformly and >> independently of the orientation of its surfaces. It is often used to >> represent the base >> amount of illumination in a scene. > > I think the key aspects of an Ambient light are that: the light seems to come > from all directions (and thus has no position or direction), and that it > illuminates objects independently of the position or orientation of the > object. > > I don't have a problem with also giving a real-world analogy if one can be > found that makes it easier to understand without causing confusion. I tried to unify some of the points that were brought up. See if you would like more changes. - PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses [v5]
> Now that the standard concrete light types were added, there is an > opportunity to rearrange and rewrite some of the class docs. Here is a > summary of the changes: > > * Moved the explanations of attenuation and direction up to `LightBase` since > different light types share characteristics. `LightBase` now contains a > summary of its subtypes and all the explanations of the > properties/characteristics of the lights divided into sections: Color, Scope, > Direction, Attenuation. > * Each light type links to the relevant section in `LightBase` when it > mentioned the properties it has. > * Added examples of real-world applications for each light type. > * Consolidated the writing style for all the subclasses. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Changed description of ambient light - Changes: - all: https://git.openjdk.java.net/jfx/pull/717/files - new: https://git.openjdk.java.net/jfx/pull/717/files/cae874d6..987a6165 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=717=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=717=03-04 Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/717.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/717/head:pull/717 PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses [v4]
> Now that the standard concrete light types were added, there is an > opportunity to rearrange and rewrite some of the class docs. Here is a > summary of the changes: > > * Moved the explanations of attenuation and direction up to `LightBase` since > different light types share characteristics. `LightBase` now contains a > summary of its subtypes and all the explanations of the > properties/characteristics of the lights divided into sections: Color, Scope, > Direction, Attenuation. > * Each light type links to the relevant section in `LightBase` when it > mentioned the properties it has. > * Added examples of real-world applications for each light type. > * Consolidated the writing style for all the subclasses. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Fixed table accessibility - Changes: - all: https://git.openjdk.java.net/jfx/pull/717/files - new: https://git.openjdk.java.net/jfx/pull/717/files/fd12a2ea..cae874d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=717=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=717=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/717.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/717/head:pull/717 PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses [v3]
> Now that the standard concrete light types were added, there is an > opportunity to rearrange and rewrite some of the class docs. Here is a > summary of the changes: > > * Moved the explanations of attenuation and direction up to `LightBase` since > different light types share characteristics. `LightBase` now contains a > summary of its subtypes and all the explanations of the > properties/characteristics of the lights divided into sections: Color, Scope, > Direction, Attenuation. > * Each light type links to the relevant section in `LightBase` when it > mentioned the properties it has. > * Added examples of real-world applications for each light type. > * Consolidated the writing style for all the subclasses. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Added implNote - Changes: - all: https://git.openjdk.java.net/jfx/pull/717/files - new: https://git.openjdk.java.net/jfx/pull/717/files/3f62718b..fd12a2ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=717=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=717=01-02 Stats: 15 lines in 1 file changed: 8 ins; 5 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/717.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/717/head:pull/717 PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses [v2]
> Now that the standard concrete light types were added, there is an > opportunity to rearrange and rewrite some of the class docs. Here is a > summary of the changes: > > * Moved the explanations of attenuation and direction up to `LightBase` since > different light types share characteristics. `LightBase` now contains a > summary of its subtypes and all the explanations of the > properties/characteristics of the lights divided into sections: Color, Scope, > Direction, Attenuation. > * Each light type links to the relevant section in `LightBase` when it > mentioned the properties it has. > * Added examples of real-world applications for each light type. > * Consolidated the writing style for all the subclasses. 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/717/files - new: https://git.openjdk.java.net/jfx/pull/717/files/dd2c99f5..3f62718b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=717=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=717=00-01 Stats: 10 lines in 3 files changed: 0 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/717.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/717/head:pull/717 PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses
On Tue, 25 Jan 2022 01:17:51 GMT, Kevin Rushforth wrote: >> Now that the standard concrete light types were added, there is an >> opportunity to rearrange and rewrite some of the class docs. Here is a >> summary of the changes: >> >> * Moved the explanations of attenuation and direction up to `LightBase` >> since different light types share characteristics. `LightBase` now contains >> a summary of its subtypes and all the explanations of the >> properties/characteristics of the lights divided into sections: Color, >> Scope, Direction, Attenuation. >> * Each light type links to the relevant section in `LightBase` when it >> mentioned the properties it has. >> * Added examples of real-world applications for each light type. >> * Consolidated the writing style for all the subclasses. > > modules/javafx.graphics/src/main/java/javafx/scene/LightBase.java line 119: > >> 117: *The transparency (alpha) component of a light is ignored. >> 118: * >> 119: * There are no guarantee that these behaviors will not change. > > This is a new specification of behavior rather than a doc reorganization or > clarification of an existing note. As such, I think it should be done > separately and with a CSR. Even if this just clarifies the current behavior and doesn't make a guarantee? - PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses
On Tue, 25 Jan 2022 06:21:37 GMT, Ambarish Rapte wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/AmbientLight.java line 37:
>>
>>> 35: *
>>> 36: * {@code AmbientLight}s can represent strong light sources in an
>>> enclosed area where the lights bounces from many
>>> 37: * objects, causing them to be illuminated from many directions. A
>>> strong light in a room and moonlight are common light
>>
>>> A strong light in a room ...
>>
>> I think this is OK, as long as a reader doesn't think of an overhead light
>> in a room, which also has the properties of a point light.
>
> Ambient light is a light that comes from all directions, scattered from
> different surfaces and it does not cast shadow. So I think the example of
> strong light and moon light should be avoided. Moon light is more like the
> Sun light, a directional light.
What examples would you use?
If a light in a room is strong it will barely cast shadows since its
reflections from all directions eliminate them.
Maybe I should mention that Ambient light can be used with a dark color to
provide a base weak lighting of the environment, and on top of it use other
lights.
-
PR: https://git.openjdk.java.net/jfx/pull/717
Re: Unused members in JMemoryBuddy
Got it, thanks. On Fri, Jan 21, 2022 at 11:08 AM Florian Kirmaier < florian.kirma...@gmail.com> wrote: > Hi Nir Lisker, > > Yes, these are required. > Btw, here is the link to the project, which can be used independently of > OpenJFX: https://github.com/Sandec/JMemoryBuddy > > > * line 89: AssertCollectable assertCollectable = new > AssertCollectable(weakReference); > This is useful to find the reference, which was not collectible. You can > just search the heap for the class "AssertCollectable" and find the > troublemaker. This really speeds up debugging failing tests. > > > * line 237: the method setMxBeanProxyName > This was provided by someone to the original project. This makes it > possible to configure the "createHeapDump" method to work with different > JVMs. > > > * line 309: setAsReferenced > You are right, this is only used to make sure a certain reference will not > be collected. > This way, it's possible to define a set of objects which are referenced, > and then check whether certain objects are still collectible. > > Florian Kirmaier > > On Fri, 21 Jan 2022 at 08:07, Nir Lisker wrote: > >> Hi, >> >> Looking at JMemoryBuddy, there are unused fields and methods. It's not >> clear if they were left in by mistake or are part of future work. >> >> * line 89: AssertCollectable assertCollectable = new >> AssertCollectable(weakReference); >> * line 237: the method setMxBeanProxyName >> * line 309: setAsReferenced >> >> The last one is part of the class SetAsReferenced, which is used in the >> memoryTest method's setAsReferenced, but the list it is added to is never >> read from. Could be that it just holds references so that they won't be >> collected. >> >> Are these needed? >> >> - Nir >> >
Unused members in JMemoryBuddy
Hi, Looking at JMemoryBuddy, there are unused fields and methods. It's not clear if they were left in by mistake or are part of future work. * line 89: AssertCollectable assertCollectable = new AssertCollectable(weakReference); * line 237: the method setMxBeanProxyName * line 309: setAsReferenced The last one is part of the class SetAsReferenced, which is used in the memoryTest method's setAsReferenced, but the list it is added to is never read from. Could be that it just holds references so that they won't be collected. Are these needed? - Nir
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v5]
On Mon, 10 Jan 2022 21:09:15 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix grammar mistakes and did some small rephrases
The `LazyObjectBindingTest` tests look good. I was thinking about adding a test
that checks the `compyteValue` calls when a listener is at
Re: JavaFX Launch Failure on Ubuntu from JNI
>
> 1. If your main class extends Application, and you try to run it like:
> java -jar MyApplication.jar
> It will fail immediately with:
> Error: JavaFX runtime components are missing, and are required to run this
> application
> 2. If you "trick" it, by making your Application class a separate class
> that you call from your main class, it will run fine using:
> java -jar MyApplication.jar
This is documented behavior, although I admit it was hard to find it.
If you go to the getting started documentation at
https://openjfx.io/openjfx-docs/ and the go to Runtime images > Non-Modular
project it will tell you that:
"As explained here, in order to create a runnable jar with all the required
JavaFX dependencies, you will need to use a launcher class that doesn't
extend from Application." with a link to the explanation.
Another option is to add the vm argument -Djava.library.path and point to
the missing runtime components (which I believe is just the bin directory
of the JavaFX runtime where the .dll files are in the case of Windows).
If we could explain this in the error message somehow it will save a lot of
trouble for a lot of people. Something like:
"Error: JavaFX runtime components are missing, and are required to run this
application. Either start the application from a class that does not extend
Application or specify the -Djava.library.path VM argument pointing to the
bin directory".
I don't know if this specific case is detectable. Maybe Johan can comment.
On Thu, Jan 20, 2022 at 2:08 AM Steve Hannah wrote:
> I've reduced the problem down to something minimal and have found that:
>
> 1. If your main class extends Application, and you try to run it like:
> java -jar MyApplication.jar
>
> It will fail immediately with:
> Error: JavaFX runtime components are missing, and are required to run this
> application
>
> 2. If you "trick" it, by making your Application class a separate class
> that you call from your main class, it will run fine using:
> java -jar MyApplication.jar
>
> 3. It will also run fine in this scenario using
> -Djava.class.path=MyApplication.jar instead of -jar:
> java -Djava.class.path=MyApplication.jar Main
>
> 3. If I try to simulate the exact same thing with my own launcher, it will
> hang somewhere in the JavaFX initialization:
>
> with javafx.verbose=true, the output is:
>
> System.loadLibrary(prism_es2) succeeded
> JavaFX: using com.sun.javafx.tk.quantum.QuantumToolkit
> System.loadLibrary(glass) succeeded
>
> But it hangs there, and never displays the screen.
>
> The C code for this launcher is:
>
> char *mainClass;
>
> JavaVM *vm;
> JNIEnv *env;
> JavaVMInitArgs vm_args;
> JavaVMOption options[2];
> mainClass = "Main";
> options[0].optionString = "-Djava.class.path=MyApplication.jar";
> options[1].optionString = "-Djavafx.verbose=true";
> vm_args.version = JNI_VERSION_1_2;
> vm_args.options = options;
> vm_args.nOptions = 2;
> vm_args.ignoreUnrecognized = 0;
>
> jobjectArray args;
> jint res = JNI_CreateJavaVM(, (void **), _args);
> if (res < 0) {
> printf("Can't create Java VM\n");
> exit(1);
> }
> jclass cls = (*env)->FindClass(env, mainClass);
> if (cls == 0) {
>
> printf("Main class %s class not found\n", mainClass);
> exit(1);
> }
> jmethodID mid =
> (*env)->GetStaticMethodID(env, cls, "main", "([Ljava/lang/String;)V");
> if (mid == 0) {
> printf("main() method not found\n");
> exit(1);
> }
> //jstring argString = (*env)->NewStringUTF(env, ""); //empty arg list
> args =
> (*env)->NewObjectArray(env, 0, (*env)->FindClass(env,
> "java/lang/String"), NULL);
> if (args == 0) {
> printf("Out of memory\n");
> exit(1);
> }
>
> (*env)->CallStaticVoidMethod(env, cls, mid, args);
>
>
>
> Can anyone spot any differences between that and running with the "java"
> binary:?
> java -Djava.class.path=MyApplication.jar Main
>
> I have experimented both with JDKs that include JavaFX (e.g. Zulu) and ones
> that do not (e.g. AdoptOpenJDK). Both exhibit the same behaviour (except
> with AdoptOpenJDK, I also add the javafx jars to the classpath).
>
> For this test I'm using JDK 11 on Mac OS Mojave, but it is consistent with
> my earlier troubles on Ubuntu (also JDK11).
>
>
> Best regards
>
> Steve
>
>
> On Wed, Jan 19, 2022 at 3:06 PM John Neffenger wrote:
>
> > On 1/19/22 2:12 PM, Steve Hannah wrote:
> > > I have been resisting using modules for a long time because it just
> makes
> > > things more complicated, ...
> >
> > It also makes some things easier, though, and certainly smaller. It's
> > easier to use an old-school Makefile with modules, and using 'jlink' can
> > get a simple Hello World JavaFX application and Java runtime down to
> > just 31 megabytes.
> >
> > Here's my minimal, no-magic example:
> >
> > https://github.com/jgneff/hello-javafx
> >
> > with a simple Makefile:
> >
> > https://github.com/jgneff/hello-javafx/blob/main/Makefile
> >
> > and a Maven POM for use online with Maven Central or offline with a
> > local Debian- or
[jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses
Now that the standard concrete light types were added, there is an opportunity to rearrange and rewrite some of the class docs. Here is a summary of the changes: * Moved the explanations of attenuation and direction up to `LightBase` since different light types share characteristics. `LightBase` now contains a summary of its subtypes and all the explanations of the properties/characteristics of the lights divided into sections: Color, Scope, Direction, Attenuation. * Each light type links to the relevant section in `LightBase` when it mentioned the properties it has. * Added examples of real-world applications for each light type. * Consolidated the writing style for all the subclasses. - Commit messages: - Updated copyright years - Initial commit Changes: https://git.openjdk.java.net/jfx/pull/717/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=717=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279345 Stats: 149 lines in 5 files changed: 93 ins; 26 del; 30 mod Patch: https://git.openjdk.java.net/jfx/pull/717.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/717/head:pull/717 PR: https://git.openjdk.java.net/jfx/pull/717
Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses
On Sun, 16 Jan 2022 22:54:22 GMT, Nir Lisker wrote: > Now that the standard concrete light types were added, there is an > opportunity to rearrange and rewrite some of the class docs. Here is a > summary of the changes: > > * Moved the explanations of attenuation and direction up to `LightBase` since > different light types share characteristics. `LightBase` now contains a > summary of its subtypes and all the explanations of the > properties/characteristics of the lights divided into sections: Color, Scope, > Direction, Attenuation. > * Each light type links to the relevant section in `LightBase` when it > mentioned the properties it has. > * Added examples of real-world applications for each light type. > * Consolidated the writing style for all the subclasses. @kevinrushforth @arapte Please review this docs-only change for JavaFX 18. - PR: https://git.openjdk.java.net/jfx/pull/717
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v5]
On Mon, 10 Jan 2022 21:09:15 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix grammar mistakes and did some small rephrases
The API looks good to me. After you get the other reviewers to look at it you
can update the CSR.
As for the fluent binding tests:
* The `private`
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v4]
On Wed, 5 Jan 2022 12:29:01 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply changes suggested in review and updated copyright years to 2022
The changes look good. I added some minor grammar comments. I think that the
API is in a good spot.
modules/javafx.base/src/main/
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 5 Jan 2022 10:51:28 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
>> line 152:
>>
>>> 150: * @return an {@link ObservableValue} which provides a mapping of
>>> the value
>>> 151: * held by this {@code ObservableValue}, and provides {@code
>>> null} when
>>> 152: * this {@code ObservableValue} holds {@code null}, never null
>>
>> No need for `@link`.
>>
>> Since we already explained how the mapping works, maybe we can be more brief
>> here:
>>
>> an {@code ObservableValue} that holds the result of the mapping of the value
>> held by this {@code ObservableValue}; never {@code null} itself
>
> I think that `@return` should mention that the returned observable can hold
> `null`, how about:
>
> an {@code ObservableValue} that holds a mapping of this {@code
> ObservableValue}'s value
> or holds {@code null} when the value is {@code null}; never returns
> {@code null}
Good idea to mention that it can hold `null`.
I slightly prefer to say that the returned `ObservableValue` holds the result
of the mapping rather than holds the mapping. I don't really mind it, but it's
the phrasing used in the method description "holds the result of applying a
mapping". "The mapping" itself could be mistaken for the mapping `Function` in
my opinion. If you think it's clear, you can change it to that phrasing, it's
also fine.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v4]
On Wed, 5 Jan 2022 12:29:01 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply changes suggested in review and updated copyright years to 2022
modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java line
194:
> 192: /**
> 193: * Checks i
Integrated: 8234921: Add DirectionalLight to the selection of 3D light types
On Mon, 28 Jun 2021 12:13:44 GMT, Nir Lisker wrote: > Adds a directional light as a subclass of `LightBase`. I think that this is > the correct hierarchy for it. > > I tried to simulate a directional light by putting a point light far away, > but I got artifacts when the distance was large. Instead, I added an on/off > attenuation flag as the 4th component of the attenuation 4-vector. When it is > 0, a simpler computation is used in the pixel/fragment shader that calculates > the illumination based on the light direction only (making the position > variables meaningless). When it is 1, the point/spot light computation is > used. It's possible that the vertex shader can also be simplified in this > case since it does not need to transform the position vectors, but I left > this optimization avenue for another time. > > I noticed a drop of ~1 fps in the stress test of 5000 meshes. > > I added a system test that verifies the correct color result from a few > directions. I also updated the lighting sample application to include 3 > directional lights and tested them on all the models visually. The lights > seem to behave the way I would expect. This pull request has now been integrated. Changeset: 32f21ffd Author:Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/32f21ffda9ca21285aac7119458efa35e9b44418 Stats: 543 lines in 29 files changed: 482 ins; 11 del; 50 mod 8234921: Add DirectionalLight to the selection of 3D light types Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v6]
On Mon, 20 Dec 2021 13:13:09 GMT, Nir Lisker wrote: >> Adds a directional light as a subclass of `LightBase`. I think that this is >> the correct hierarchy for it. >> >> I tried to simulate a directional light by putting a point light far away, >> but I got artifacts when the distance was large. Instead, I added an on/off >> attenuation flag as the 4th component of the attenuation 4-vector. When it >> is 0, a simpler computation is used in the pixel/fragment shader that >> calculates the illumination based on the light direction only (making the >> position variables meaningless). When it is 1, the point/spot light >> computation is used. It's possible that the vertex shader can also be >> simplified in this case since it does not need to transform the position >> vectors, but I left this optimization avenue for another time. >> >> I noticed a drop of ~1 fps in the stress test of 5000 meshes. >> >> I added a system test that verifies the correct color result from a few >> directions. I also updated the lighting sample application to include 3 >> directional lights and tested them on all the models visually. The lights >> seem to behave the way I would expect. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Added import The CSR has been approved, but I don't see it being registered here. - PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 5 Jan 2022 09:52:29 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java
>> line 204:
>>
>>> 202: *
>>> 203: * @return {@code true} if this binding is allowed to become
>>> valid, otherwise
>>> 204: * {@code false}
>>
>> Typo: overriden -> overridden
>>
>> I would add a a first-sentence summary and an explanation as to why a
>> binding would not allow it. I would write something like
>>
>> Checks if the binding is allowed to become valid. Overriding classes can
>> prevent a binding from becoming
>> valid. This is useful when .
>>
>> The default implementation always allows bindings to become valid.
>
> I've made your suggested changes and added this explanation: "This is useful
> in subclasses which do not always listen for invalidations of their
> dependencies and prefer to recompute the current value instead. This can also
> be useful if caching of the current computed value is not desirable."
>
> Furthermore, I noticed I forgot to make the code changes that prevent caching
> of the value when the binding is invalid -- bindings currently cache their
> value even when invalid, which could lead to situations where something is
> still being referenced in an invalid binding that should have been GC'd.
Something like that was previously discussed in
https://github.com/javafxports/openjdk-jfx/pull/110.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 15 Dec 2021 11:43:36 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Upgrade tests to JUnit 5
modules/javafx.base/src/main/java/javafx/beans/value/MappedBinding.java line 34:
> 32:
> 33: class MappedBinding extends LazyObjectBinding {
> 34: private final ObservableValue source;
We usually have an empty line below the class declaration. Not sure if this is
enforced. Same for the other classes.
modules/javafx.base/src/main/java/javafx/beans/value/MappedBinding.java line 48:
> 46:
> 47: @Override
> 48: protected T computeValue() {
Maybe move this method above `observeInputs` like in `FlatMapBinding`. Same for
`OrElseBinding`.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 15 Dec 2021 11:43:36 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Upgrade tests to JUnit 5
This is the review of the API. I suggested also adding the examples in the POC
or similar to the relevant methods.
I have already commented on the implementation whi
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 15 Dec 2021 11:43:36 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Upgrade tests to JUnit 5
I will start my review this week.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v6]
> Adds a directional light as a subclass of `LightBase`. I think that this is > the correct hierarchy for it. > > I tried to simulate a directional light by putting a point light far away, > but I got artifacts when the distance was large. Instead, I added an on/off > attenuation flag as the 4th component of the attenuation 4-vector. When it is > 0, a simpler computation is used in the pixel/fragment shader that calculates > the illumination based on the light direction only (making the position > variables meaningless). When it is 1, the point/spot light computation is > used. It's possible that the vertex shader can also be simplified in this > case since it does not need to transform the position vectors, but I left > this optimization avenue for another time. > > I noticed a drop of ~1 fps in the stress test of 5000 meshes. > > I added a system test that verifies the correct color result from a few > directions. I also updated the lighting sample application to include 3 > directional lights and tested them on all the models visually. The lights > seem to behave the way I would expect. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Added import - Changes: - all: https://git.openjdk.java.net/jfx/pull/548/files - new: https://git.openjdk.java.net/jfx/pull/548/files/7b3709d7..c3fa4b49 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=548=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=548=04-05 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/548.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/548/head:pull/548 PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v3]
On Mon, 20 Dec 2021 12:57:51 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixes for review comments > > tests/performance/3DLighting/attenuation/Environment.java line 87: > >> 85: >> 86: directionalLight1.setDirection(new Point3D(-LIGHT_X_DIST, 0, >> LIGHT_Z_DIST)); >> 87: directionalLight2.setDirection(new Point3D(LIGHT_X_DIST, 0, >> LIGHT_Z_DIST)); > > You need to add an import for `Point3D`. Weird that the IDE didn't warn. It seems like it did not identify the file as part of the source code. - PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v2]
On Sat, 18 Dec 2021 16:34:21 GMT, Kevin Rushforth wrote: >> Btw, you don't need the "abs" for the gLightAttenuation check (you don't >> have it now when checking against 0.5) so you can just change it to >> `gLightAttenuation[i].w < EPSILON` which seems cleaner than testing against >> 0.5. > > I see you kept it as 0.5 and documented it. That's fine, too. I just didn't want to mess with the math and need to retest everything just for this. If I add the EPSILON checks for falloff later I will use it there too. - PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v5]
> Adds a directional light as a subclass of `LightBase`. I think that this is > the correct hierarchy for it. > > I tried to simulate a directional light by putting a point light far away, > but I got artifacts when the distance was large. Instead, I added an on/off > attenuation flag as the 4th component of the attenuation 4-vector. When it is > 0, a simpler computation is used in the pixel/fragment shader that calculates > the illumination based on the light direction only (making the position > variables meaningless). When it is 1, the point/spot light computation is > used. It's possible that the vertex shader can also be simplified in this > case since it does not need to transform the position vectors, but I left > this optimization avenue for another time. > > I noticed a drop of ~1 fps in the stress test of 5000 meshes. > > I added a system test that verifies the correct color result from a few > directions. I also updated the lighting sample application to include 3 > directional lights and tested them on all the models visually. The lights > seem to behave the way I would expect. Nir Lisker has updated the pull request incrementally with two additional commits since the last revision: - Fixed indentation - Added comments about floating point comparison - Changes: - all: https://git.openjdk.java.net/jfx/pull/548/files - new: https://git.openjdk.java.net/jfx/pull/548/files/452e5672..7b3709d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=548=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=548=03-04 Stats: 6 lines in 6 files changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/548.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/548/head:pull/548 PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v2]
On Tue, 7 Dec 2021 16:38:03 GMT, Kevin Rushforth wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update copyright year
>
> modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h line 106:
>
>> 104:
>> 105: void computeLight(float i, float3 n, float3 refl, float specPower,
>> float3 L, float3 lightDir, in out float3 d, in out float3 s) {
>> 106: if (gLightAttenuation[i].w < 0.5) {
>
> Is there any performance difference between `< 0.5` and `!= 0` ? I suspect
> not, but in any case, you might consider the latter (as I also mentioned in
> the java files). The latter is more clear, so if you choose to stick with
> what you have, I'd like to see a comment added.
Initially I went with the equality test, but it did not work well, probably
since we are comparing floating points. This just seemed safer. If you want to
try the equality test on you machine(s) it would be interesting, but I don't
know if I would trust it. Same with the ES shaders. On the Java side I would
trust it, but at this point it's a matter of consistency.
I suspect that this also messed up with the computation approaches of spot
light that check `falloff != 0` where we should be checking with a small delta:
`abs(falloff) < EPSILON` instead.
Performance should be revisited anyway I think at a later stage after the
emissive color is added (where we need to figure out splitting shaders, which
also has to do with the removal of the 3 lights restriction).
-
PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v4]
> Adds a directional light as a subclass of `LightBase`. I think that this is > the correct hierarchy for it. > > I tried to simulate a directional light by putting a point light far away, > but I got artifacts when the distance was large. Instead, I added an on/off > attenuation flag as the 4th component of the attenuation 4-vector. When it is > 0, a simpler computation is used in the pixel/fragment shader that calculates > the illumination based on the light direction only (making the position > variables meaningless). When it is 1, the point/spot light computation is > used. It's possible that the vertex shader can also be simplified in this > case since it does not need to transform the position vectors, but I left > this optimization avenue for another time. > > I noticed a drop of ~1 fps in the stress test of 5000 meshes. > > I added a system test that verifies the correct color result from a few > directions. I also updated the lighting sample application to include 3 > directional lights and tested them on all the models visually. The lights > seem to behave the way I would expect. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Updated copyright year - Changes: - all: https://git.openjdk.java.net/jfx/pull/548/files - new: https://git.openjdk.java.net/jfx/pull/548/files/e95db983..452e5672 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=548=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=548=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/548.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/548/head:pull/548 PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v3]
> Adds a directional light as a subclass of `LightBase`. I think that this is > the correct hierarchy for it. > > I tried to simulate a directional light by putting a point light far away, > but I got artifacts when the distance was large. Instead, I added an on/off > attenuation flag as the 4th component of the attenuation 4-vector. When it is > 0, a simpler computation is used in the pixel/fragment shader that calculates > the illumination based on the light direction only (making the position > variables meaningless). When it is 1, the point/spot light computation is > used. It's possible that the vertex shader can also be simplified in this > case since it does not need to transform the position vectors, but I left > this optimization avenue for another time. > > I noticed a drop of ~1 fps in the stress test of 5000 meshes. > > I added a system test that verifies the correct color result from a few > directions. I also updated the lighting sample application to include 3 > directional lights and tested them on all the models visually. The lights > seem to behave the way I would expect. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Fixes for review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/548/files - new: https://git.openjdk.java.net/jfx/pull/548/files/adcf1fce..e95db983 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=548=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=548=01-02 Stats: 8 lines in 4 files changed: 3 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/548.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/548/head:pull/548 PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v2]
On Tue, 7 Dec 2021 16:22:25 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright year > > modules/javafx.graphics/src/main/java/com/sun/javafx/scene/DirectionalLightHelper.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved. > > 2021 Can the script do all these corrections? - PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes [v9]
On Fri, 17 Dec 2021 15:18:51 GMT, Kevin Rushforth wrote: >> This PR updates the `CONTRIBUTING.md` guide to address the following: >> >> 1. Clarify the process for adding new features / API changes, specifically >> that they must be discussed on the mailing list as the first step. >> 2. Add a link to the mailing list in the section regarding contributing bug >> fixes. >> 3. Remove the text about cross-linking the PR and JBS issue, and add a note >> that the Skara tooling takes care of this >> 4. Remove the section about manually resolving the JBS issue, and add a note >> that the Skara bot automatically does this when the PR is integrated. >> 5. Suggest the use of the "/reviewers 2" and "/csr" commands when appropriate >> 6. Update the note regarding which JDK(s) to use. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/303
Re: RFR: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes [v8]
On Fri, 17 Dec 2021 14:08:23 GMT, Michael Strauß wrote: >> Kevin Rushforth has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Minor updates > > CONTRIBUTING.md line 18: > >> 16: >> 17: >> 18: If you find yourself wishing for a feature that doesn't exist in >> OpenJFX, you are probably not alone. There are bound to be others out there >> with similar needs. Many of the features that OpenJFX has today have been >> added because our users saw the need. > > I think you should restore this sentence. It feels uplifting and encouraging, > while the other added sections feel pretty discouraging at first sight for > newcomers to the project. Fair point. I don't mind restoring it. - PR: https://git.openjdk.java.net/jfx/pull/303
Re: RFR: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes [v8]
On Fri, 17 Dec 2021 13:33:57 GMT, Kevin Rushforth wrote: >> This PR updates the `CONTRIBUTING.md` guide to address the following: >> >> 1. Clarify the process for adding new features / API changes, specifically >> that they must be discussed on the mailing list as the first step. >> 2. Add a link to the mailing list in the section regarding contributing bug >> fixes. >> 3. Remove the text about cross-linking the PR and JBS issue, and add a note >> that the Skara tooling takes care of this >> 4. Remove the section about manually resolving the JBS issue, and add a note >> that the Skara bot automatically does this when the PR is integrated. >> 5. Suggest the use of the "/reviewers 2" and "/csr" commands when appropriate >> 6. Update the note regarding which JDK(s) to use. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Minor updates I added 1 suggestion. CONTRIBUTING.md line 204: > 202: section of the Code Review Policies doc, we also need a > [CSR](https://wiki.openjdk.java.net/display/csr/Main), which documents the > API change and its approval. > 203: The CSR can be reviewed in parallel. Changes in the API that arise > during the review need to be reflected in the CSR, meaning > 204: that the final review / approval of the CSR usually happens late in the > review cycle. I would advise contributors to not deal with the CSR until the review is almost done. It just duplicates the places that you need to update. I'm saying this from personal experience :) - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/303
Re: RFR: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes [v8]
On Fri, 17 Dec 2021 13:33:57 GMT, Kevin Rushforth wrote: >> This PR updates the `CONTRIBUTING.md` guide to address the following: >> >> 1. Clarify the process for adding new features / API changes, specifically >> that they must be discussed on the mailing list as the first step. >> 2. Add a link to the mailing list in the section regarding contributing bug >> fixes. >> 3. Remove the text about cross-linking the PR and JBS issue, and add a note >> that the Skara tooling takes care of this >> 4. Remove the section about manually resolving the JBS issue, and add a note >> that the Skara bot automatically does this when the PR is integrated. >> 5. Suggest the use of the "/reviewers 2" and "/csr" commands when appropriate >> 6. Update the note regarding which JDK(s) to use. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Minor updates I will review this shortly. - PR: https://git.openjdk.java.net/jfx/pull/303
Re: RFR: 8197991: Selecting many items in a TableView is very slow
On Wed, 17 Nov 2021 05:34:46 GMT, Abhinay Agarwal wrote: > This work improves the performance of `MultipleSelectionModel` over large > data sets by caching some values and avoiding unnecessary calls to > `SelectedIndicesList#size`. It further improves the performance by reducing > the number of iterations required to find the index of an element in the > BitSet. > > The work is based on [an abandoned > patch](https://github.com/openjdk/jfx/pull/127) submitted by @yososs > > There are currently 2 manual tests for this fix. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ReadOnlyUnbackedObservableList.java line 119: > 117: Object obj = get(i); > 118: if (o.equals(obj)) return i; > 119: } An alternative is return IntStream.range(0, size()) .filter(i -> o.equals(get(i))) .findFirst() .orElse(-1); I don't know if it helps. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ReadOnlyUnbackedObservableList.java line 193: > 191: arr[i] = get(i); > 192: } > 193: return arr; Have you tried `return stream().toArray();`? - PR: https://git.openjdk.java.net/jfx/pull/673
Integrated: 8272808: Update constant collections to use the new immutable collections - leftovers
On Tue, 21 Sep 2021 11:13:06 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. This pull request has now been integrated. Changeset: 5fc047ba Author:Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/5fc047ba2300b5a285a58cd76451e41fefe6e462 Stats: 168 lines in 8 files changed: 0 ins; 34 del; 134 mod 8272808: Update constant collections to use the new immutable collections - leftovers Reviewed-by: kcr, jvos - PR: https://git.openjdk.java.net/jfx/pull/627
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 I removed the field. Needs a quick re-review. - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v3]
> 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 field - Changes: - all: https://git.openjdk.java.net/jfx/pull/627/files - new: https://git.openjdk.java.net/jfx/pull/627/files/79606579..fc72e5dd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=627=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=627=01-02 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/627.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/627/head:pull/627 PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8275848: Deprecate for removal mistakenly exposed field from class javafx.scene.shape.Box [v2]
On Fri, 29 Oct 2021 11:42:38 GMT, Ajit Ghaisas wrote: >> This PR deprecates mistakenly exposed field from class >> javafx.scene.shape.Box. > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > review fix Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/655
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 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: 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: 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: 8255015: Inconsistent illumination of 3D shape by PointLight
On Sun, 24 Oct 2021 16:04:40 GMT, Andreas Heger wrote: > I tried to use the Node.snapshot method, but in this case the taken snapshot > always had the correct illumination, no matter what display was used and even > though the scene on the display showed the wrong illumination. I guess the > snapshot method renders the node directly into an image and does not use the > physical screen content and so the pixel scale factor does not play any role > in this case. Maybe this is why the [lighting system tests](https://github.com/openjdk/jfx/tree/master/tests/system/src/test/java/test/javafx/scene/lighting3D) pass on Retina screens. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8275815: OCA link in README.md and CONTRIBUTING.md is broken
On Fri, 22 Oct 2021 17:53:27 GMT, Kevin Rushforth wrote: > The PR fixes the broken links as described in the JBS issue. The correct link > for the Oracle Contributor Agreement (OCA) is: > > https://oca.opensource.oracle.com/ Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/652
Re: RFR: 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)
modules/javafx.graphics/src/main/java/javafx/scene/Camera.java line 164:
> 162: /**
> 163: * Creates a {@code Camera}.
> 164: */
Sine the constructor is a `protected` for an `abstract` class, it doesn't
create a `Camera` in the normal sense of constructors. I would write something
like "Shared constructor for subclasses of `Camera`".
modules/javafx.graphics/src/main/java/javafx/scene/paint/Material.java line 81:
> 79: /**
> 80: * Creates a {@code Material}.
> 81: */
Same comment as in `Camera`.
modules/javafx.graphics/src/main/java/javafx/scene/shape/Box.java line 91:
> 89: * Default size of the {@code Box}.
> 90: */
> 91: public static final double DEFAULT_SIZE = 2;
This field was exposed by mistake probably. The other shapes don't expose
theirs. I recommend to deprecate for removal.
modules/javafx.graphics/src/main/java/javafx/scene/shape/Shape3D.java line 102:
> 100: /**
> 101: * Creates a {@code Shape3D}.
> 102: */
Same comment as in `Camera`,
modules/javafx.media/src/main/java/javafx/scene/media/Track.java line 85:
> 83: /**
> 84: * Gets the Map containing all known metadata for this
> Track.
> 85: * @return the Map containing all known metadata for
> this Track
We tend to use `{@code }` over ` `, but I don't think it matter.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 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.
> - 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)
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.
modules/javafx.controls/src/main/java/javafx/scene/chart/AreaChart.java line
582:
> 580:
> 581: /**
> 582: * Get the {@code CssMetaData} associated with this class, which may
> include the
Should be "Gets the..."
Same for the other methods.
modules/javafx.controls/src/main/java/javafx/scene/control/CheckMenuItem.java
line 89:
> 87:
> **/
> 88: /**
> 89: * Constructs a default CheckMenuItem.
`{@code CheckMenuItem}`
modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line
874:
> 872:
> 873: /**
> 874: * Get the unmodifiable list of the controls css styleable
> properties.
Maybe "Gets the unmodifiable list of the control's css styleable properties."?
I'm not sure what this method really does, but if it relates to a single
control then that apostrophe is needed.
modules/javafx.controls/src/main/java/javafx/scene/control/DateCell.java line
46:
> 44:
> 45: /**
> 46: * Creates a default DateCell.
`{@code DateCell}`
modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
line 159:
> 157:
> 158: /**
> 159: * Focus the item at the given index.
Focuses
modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
line 160:
> 158: /**
> 159: * Focus the item at the given index.
> 160: * @param index the index of the item that needs to be focused.
No need for a period at the end.
modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
line 1147:
> 1145:
> 1146: /**
> 1147: * Constructs a default CSSBridge
`{@code CSSBridge}`
modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java line
47:
> 45: /**
> 46: * Get the default singleton {@code SortEvent}.
> 47: * @param type of control
"the type of control"
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableViewSkinBase.java
line 252:
> 250:
> 251: /**
> 252: * Constructs a {@code TableViewSkinBase} for given control.
"for the given"
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextInputControlSkin.java
line 731:
> 729:
> 730: /**
> 731: * Handle input method event.
"Handles an input method event"
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualContainerBase.java
line 67:
> 65:
> 66: /**
> 67: * Constructs a VirtualContainerBase
`{@code VirtualContainerBase}`
-
PR: https://git.openjdk.java.net/jfx/pull/646
Re: [External] : Re: Eager Evaluation of Invalidation Listeners
> > As mentioned already, if we make any change in this area, it will need > to be very well-tested, and even then we risk breaking applications. > I can make the change and can fix some of the tests (which will probably mean deleting some of them since they explicitly rely on this behavior). Some tests could be in areas of the code I'm not familiar with. As for breaking applications, I'm not sure what we're supposed to do. Ask people if they misused InvalidationListener? On Fri, Oct 15, 2021 at 2:12 AM Kevin Rushforth wrote: > That's a good idea to dig into the history further. I looked through the > closed-source repo, and the behavior of calling get to validate the > observable when adding an InvalidationListener for bindings has been > there pretty much from the beginning, even before the creation of the > ExpressionHelper class (in July 2011). > > I looked at when InvaldationListeners were added to the Bindings classes > (in March 2011), and the initial implementation of that method calls > get() from the InvalidationListener. That was the commit that also added > the `testAddingListenerWillAlwaysReceiveInvalidationEvent` test methods. > > So this was clearly a deliberate design choice from the beginning. I can > see the rationale for it, but in retrospect it seems an unfortunate choice. > > As mentioned already, if we make any change in this area, it will need > to be very well-tested, and even then we risk breaking applications. > > -- Kevin > > > On 10/6/2021 12:39 AM, John Hendrikx wrote: > > Is it possible to dig in the history of ExpressionHelper a bit > > further? In git it seems limited to 9 years ago, but in JIRA I found > > this bug report: > > > > https://bugs.openjdk.java.net/browse/JDK-8119521 > > > > It describes an issue where an InvalidationListener is not working > > correctly (according to the reporter) and where #get must be called to > > make it behave as expected. But it turns out this was already fixed > > -- this specific fix might have been the one that introduced the #get > > call in ExpressionHelper. > > > > On 06/10/2021 04:38, Nir Lisker wrote: > >> I would also answer "no" to both points. This is also what the docs say. > >> > >> So the question is: how likely do we think that changing this > >> behavior will > >>> break existing applications? > >>> > >> > >> That's the main question. I tested the JavaFX code with the new behavior > >> and some tests break immediately, though a few I've looked at seemed > >> to be > >> testing the invalidation listener behavior itself (in their own > >> context). I > >> don't know what would break outside of the tests. If we go this > >> route, we > >> might want to create PRs to fix the tests before we actually change > >> the behavior (in contrast to doing it all in the same PR). I think that > >> tests fail in several modules and it might require several people to fix > >> these tests depending on their areas of expertise. Then we would need to > >> test runtime behavior to see what breaks outside of the tests. > >> > >> In my own codebase nothing breaks, but it's relatively small. > >> > >> On the related question, I like the idea of nulling out the current > >> value > >>> when a property is bound. > >>> > >> > >> I can pick up from where the older PR stopped. It's a simple change. > >> > >> On Wed, Oct 6, 2021 at 3:15 AM Kevin Rushforth > >> > >> wrote: > >> > >>> Given that the intention of InvalidationListener was to be a > >>> light-weight > >>> way to listen to properties without causing a binding chain to be > >>> revalidated, it does seem reasonable to me that the listener is only > >>> fired > >>> on the valid --> invalid transition, which is the specified > >>> behavior, even > >>> if the implementation doesn't currently do that in all cases. > >>> > >>> The two related questions then are: > >>> > >>> 1. Should adding an invalidation listener to property do an immediate > >>> get(), which will ensure that the property is then valid? This will > >>> force > >>> an eager evaluation of the property and "arm" the property to fire an > >>> invalidation even the next time it is invalidated. > >>> > >>> 2. Should adding an invalidation listener to a currently invalid > >>> property > >>> cause the lis
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 Do you want to do something about `idMap` in `com.sun.java.scene.web.WebViewHelper.WebView` that I pointed out in the PR? - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/627/files - new: https://git.openjdk.java.net/jfx/pull/627/files/255a314d..79606579 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=627=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=627=00-01 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/627.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/627/head:pull/627 PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Thu, 14 Oct 2021 23:50:10 GMT, Kevin Rushforth 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. > > modules/javafx.web/src/main/java/com/sun/webkit/Utilities.java line 91: > >> 89: >> 90: // List of packages to reject >> 91: private static final List PACKAGES_REJECT_LIST = List.of( > > After this change, I think the import of `java.util.Arrays` is unused. Even more than that one :) - PR: https://git.openjdk.java.net/jfx/pull/627
Re: [External] : Re: Eager Evaluation of Invalidation Listeners
I disagree with this interpretation. Observable says > Implementations in this library mark themselves as invalid when the first > invalidation event occurs. They do not generate anymore invalidation events > until their value is recomputed and valid again. And ObservableValue says An ObservableValue generates two types of events: ... An invalidation event > is generated if the current value is not valid anymore. Implementations in this library mark themselves as invalid when the first > invalidation event occurs. They do not generate any more invalidation > events until their value is recomputed and valid again. It's clear that the validity is with respect to the value, not the listener. There is 1 value and it is either valid or invalid. If we want to define validity on a per-listener basis, the docs would need to be changed too. I don't know how much sense it makes practically because I don't think anyone used them with this intention in mind. It could be a middleground to bridge the current "negligence" with the stricter docs, but it's a more fundamental change conceptually. On Wed, Oct 6, 2021 at 8:02 PM Michael Strauß wrote: > The documentation of `Observable` states that "an implementation [...] > may support lazy evaluation" and that "implementations [...] should > strive to generate as few events as possible". > This seems to me like it would be within spec to fire an invalidation > event for every single change. It would also be within spec to fire > redundant invalidation events only in certain scenarios (like adding a > listener). > > The problem could also be approached from a different angle: what does > it mean for a property to be "valid"? > As implemented, the "valid" state is an opaque and unknowable > implementation detail. We could re-define "valid" to mean: valid from > the perspective of an InvalidationListener. > A newly-added InvalidationListener wouldn't know that the property is > invalid (and has no way of knowing), and can therefore reasonably > assume that, from its perspective, the property is valid. It would > receive an invalidation event when the property value is changed. > From the perspective of pre-existing listeners, however, the property > could well have been invalid all the time, so they won't receive an > invalidation event. > > On Wed, Oct 6, 2021 at 2:16 AM Kevin Rushforth > wrote: > > > > Given that the intention of InvalidationListener was to be a > > light-weight way to listen to properties without causing a binding chain > > to be revalidated, it does seem reasonable to me that the listener is > > only fired on the valid --> invalid transition, which is the specified > > behavior, even if the implementation doesn't currently do that in all > cases. > > > > The two related questions then are: > > > > 1. Should adding an invalidation listener to property do an immediate > > get(), which will ensure that the property is then valid? This will > > force an eager evaluation of the property and "arm" the property to fire > > an invalidation even the next time it is invalidated. > > > > 2. Should adding an invalidation listener to a currently invalid > > property cause the listener to be called (either immediately or the next > > time the object is invalidated)? > > > > I think the ideal answer to both questions is "no", although I share > > John's concern that changing this behavior for InvalidationListeners > > could break applications. So the question is: how likely do we think > > that changing this behavior will break existing applications? > > > > I think it's something we can and should consider changing. Unless there > > are serious concerns, I would support changing this behavior as part of > > avoiding eager evaluation when using invalidation listeners. It would > > need to be well tested (of course), and would need a CSR describing the > > compatibility risk, and should probably get a release note. > > > > Any concerns in doing this? > > > > On the related question, I like the idea of nulling out the current > > value when a property is bound. > > > > -- Kevin > > > > > > On 9/11/2021 9:41 AM, Nir Lisker wrote: > > > I think that we need your input on this to move it forward. > > > > > > On Fri, Sep 3, 2021 at 7:49 AM Nir Lisker > > <mailto:nlis...@gmail.com>> wrote: > > > > > > so the value field should perhaps be nulled out when bound. > > > > > > > > > There was a PR for something like that in the old repo: > > > https://github.c
Integrated: 8272870: Add convenience factory methods for border and background
On Tue, 24 Aug 2021 16:29:11 GMT, Nir Lisker wrote: > Added convenience factory factory methods for Background and Border. This pull request has now been integrated. Changeset: bb73d43b Author: Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/bb73d43b57c4da9713fb0213e156d61aab873b9a Stats: 59 lines in 4 files changed: 59 ins; 0 del; 0 mod 8272870: Add convenience factory methods for border and background Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/610
Re: [External] : Re: Eager Evaluation of Invalidation Listeners
I would also answer "no" to both points. This is also what the docs say. So the question is: how likely do we think that changing this behavior will > break existing applications? > That's the main question. I tested the JavaFX code with the new behavior and some tests break immediately, though a few I've looked at seemed to be testing the invalidation listener behavior itself (in their own context). I don't know what would break outside of the tests. If we go this route, we might want to create PRs to fix the tests before we actually change the behavior (in contrast to doing it all in the same PR). I think that tests fail in several modules and it might require several people to fix these tests depending on their areas of expertise. Then we would need to test runtime behavior to see what breaks outside of the tests. In my own codebase nothing breaks, but it's relatively small. On the related question, I like the idea of nulling out the current value > when a property is bound. > I can pick up from where the older PR stopped. It's a simple change. On Wed, Oct 6, 2021 at 3:15 AM Kevin Rushforth wrote: > Given that the intention of InvalidationListener was to be a light-weight > way to listen to properties without causing a binding chain to be > revalidated, it does seem reasonable to me that the listener is only fired > on the valid --> invalid transition, which is the specified behavior, even > if the implementation doesn't currently do that in all cases. > > The two related questions then are: > > 1. Should adding an invalidation listener to property do an immediate > get(), which will ensure that the property is then valid? This will force > an eager evaluation of the property and "arm" the property to fire an > invalidation even the next time it is invalidated. > > 2. Should adding an invalidation listener to a currently invalid property > cause the listener to be called (either immediately or the next time the > object is invalidated)? > > I think the ideal answer to both questions is "no", although I share > John's concern that changing this behavior for InvalidationListeners could > break applications. So the question is: how likely do we think that > changing this behavior will break existing applications? > > I think it's something we can and should consider changing. Unless there > are serious concerns, I would support changing this behavior as part of > avoiding eager evaluation when using invalidation listeners. It would need > to be well tested (of course), and would need a CSR describing the > compatibility risk, and should probably get a release note. > > Any concerns in doing this? > > On the related question, I like the idea of nulling out the current value > when a property is bound. > > -- Kevin > > > On 9/11/2021 9:41 AM, Nir Lisker wrote: > > I think that we need your input on this to move it forward. > > On Fri, Sep 3, 2021 at 7:49 AM Nir Lisker wrote: > >> so the value field should perhaps be nulled out when bound. >> >> >> There was a PR for something like that in the old repo: >> https://github.com/javafxports/openjdk-jfx/pull/110 >> <https://urldefense.com/v3/__https://github.com/javafxports/openjdk-jfx/pull/110__;!!ACWV5N9M2RV99hQ!bIbtLsC0tg02c9a_lgKnM1Xta2USX8QkKRL4imOUSw8xshJsVquOeulplJR7dfEzQUf6$> >> >> On Fri, Sep 3, 2021 at 5:35 AM John Hendrikx wrote: >> >>> >>> >>> On 02/09/2021 11:57, Nir Lisker wrote: >>> > So in order >>> > to make sure that a new interested invalidation listener does not >>> miss >>> > the fact that a property was *already* invalid, the easiest >>> solution >>> > might have been to revalidate it upon registration of a new >>> listener >>> > >>> > >>> > But why does an invalidation listener need to know the past state of >>> the >>> > property? It is only interested in the valid -> invalid transition. If >>> > the property was invalid then the listener (in theory) shouldn't >>> receive >>> > any events anyway on subsequent invalidations. (I understand that you >>> > don't justify this, I'm posing it as a general question.) >>> >>> Strictly speaking, no, if users are using InvalidationListener correctly >>> then this is definitely correct behavior. I'm not really advocating a >>> change, and I'd even prefer that it be brought in line with the >>> documentation. >>> >>> I think however that many users are not using it correctly and expect an >>> invalidation event always the next time the value changes (and their >>>
Re: Proof of concept for fluent bindings for ObservableValue
Looks good. I assume we will add more bindings in the future like
conditionOn or anything else that I would put under "extras", so maybe the
title could be more specific since there will be more fluent bindings in
the future?
On Tue, Oct 5, 2021 at 1:34 PM John Hendrikx wrote:
> I've created https://bugs.openjdk.java.net/browse/JDK-8274771
>
> Please have a look.
>
> --John
>
> On 04/10/2021 17:51, Nir Lisker wrote:
> > I think that a PR can be created. The only point we need to decide is
> about
> > the subscription models we talked about above. ReactFX uses something
> > different for each, you use the same. That can determine if we need
> > different classes for each binding type.
> >
> > I can create the JBS issue for this one and a draft CSR if you want.
> >
> > On Tue, Sep 21, 2021 at 1:36 PM Nir Lisker wrote:
> >
> >> The OrElseBinding is incorrect
> >>>
> >>
> >> Yes, that was a typo with the order of the arguments in the ternary
> >> statement.
> >>
> >> I can rewrite the tests for JUnit 4 but I'd need a Nested runner (I
> >>> think the tests would become pretty unreadable and less useful /
> >>> thourough otherwise).
> >>>
> >>> What are the options?
> >>>
> >>
> >> This is a bigger question. Kevin will have to answer that.
> >>
> >> As for the subscription model: flat map has a more complicated one, but
> >>> orElse, orElseGet and map all have the same one.
> >>>
> >>
> >> From what I saw, ReactFX uses a different subscription model for these.
> I
> >> could have misread it.
> >>
> >> The messages will need to be written from the perspective of the Binding
> >>> class then IMHO.
> >>>
> >>
> >> That's fine.
> >>
> >> As for the Optional methods, I'll have a look in my code base and see if
> >> the places I would like to use them at will become irrelevant anyway
> with
> >> the fluent bindings. I'm fine with proceeding with the current names of
> the
> >> proposed methods.
> >>
> >> On Sun, Sep 19, 2021 at 6:12 PM John Hendrikx wrote:
> >>
> >>> I need to get you the tests I wrote, unfortunately, they're JUnit 5,
> >>> please see the tests here:
> >>>
> >>>
> >>>
> https://github.com/hjohn/MediaSystem-v2/tree/master/mediasystem-jfx/src/test/java/javafx/beans/value
> >>>
> >>> The OrElseBinding is incorrect, the compute value should read:
> >>>
> >>> protected T computeValue() {
> >>>T value = source.getValue();
> >>>return value == null ? constant : value;
> >>> }
> >>>
> >>> Similar for OrElseGetBinding.
> >>>
> >>> I can rewrite the tests for JUnit 4 but I'd need a Nested runner (I
> >>> think the tests would become pretty unreadable and less useful /
> >>> thourough otherwise).
> >>>
> >>> What are the options?
> >>>
> >>> - Integrate a nested runner (there is an Apache 2.0 licensed one
> >>> available)
> >>> - Create our own nested runner (about 200 lines of code)
> >>> - Can we introduce JUnit 5?
> >>> - Rewrite to plain JUnit 4?
> >>>
> >>> Let me know, and I can integrate them.
> >>>
> >>> --John
> >>>
> >>> On 19/09/2021 02:12, Nir Lisker wrote:
> >>>> I've played around with the implementation and pushed a modified
> >>>> prototype to the sandbox [1]. I ended up with something similar to
> >>> ReactFX:
> >>>> the orElse and orElseGet methods have their own binding classes that
> >>>> extend LazyObjectBinding, just like MapBinding and FlatMapBinding. The
> >>>> reason being that both their compute value and their subscription
> models
> >>>> are slightly different. While they can be matched to MapBinding like
> you
> >>>> did, it entails a bit of a roundabout way in my opinion. Creating a
> >>>> supplier for the constant value of orElse somewhat defeats the
> purpose I
> >>>> think.
> >>>
> >>> I have no strong opinion here, just wanted to keep the MR small. The
> >>> supplier should be an inline candidate, but creating a separate class
> is
> >>> fine too.
> >>>
> >>> As for the subscription model: flat map has a m
Re: Proof of concept for fluent bindings for ObservableValue
I think that a PR can be created. The only point we need to decide is about
the subscription models we talked about above. ReactFX uses something
different for each, you use the same. That can determine if we need
different classes for each binding type.
I can create the JBS issue for this one and a draft CSR if you want.
On Tue, Sep 21, 2021 at 1:36 PM Nir Lisker wrote:
> The OrElseBinding is incorrect
>>
>
> Yes, that was a typo with the order of the arguments in the ternary
> statement.
>
> I can rewrite the tests for JUnit 4 but I'd need a Nested runner (I
>> think the tests would become pretty unreadable and less useful /
>> thourough otherwise).
>>
>> What are the options?
>>
>
> This is a bigger question. Kevin will have to answer that.
>
> As for the subscription model: flat map has a more complicated one, but
>> orElse, orElseGet and map all have the same one.
>>
>
> From what I saw, ReactFX uses a different subscription model for these. I
> could have misread it.
>
> The messages will need to be written from the perspective of the Binding
>> class then IMHO.
>>
>
> That's fine.
>
> As for the Optional methods, I'll have a look in my code base and see if
> the places I would like to use them at will become irrelevant anyway with
> the fluent bindings. I'm fine with proceeding with the current names of the
> proposed methods.
>
> On Sun, Sep 19, 2021 at 6:12 PM John Hendrikx wrote:
>
>> I need to get you the tests I wrote, unfortunately, they're JUnit 5,
>> please see the tests here:
>>
>>
>> https://github.com/hjohn/MediaSystem-v2/tree/master/mediasystem-jfx/src/test/java/javafx/beans/value
>>
>> The OrElseBinding is incorrect, the compute value should read:
>>
>> protected T computeValue() {
>>T value = source.getValue();
>>return value == null ? constant : value;
>> }
>>
>> Similar for OrElseGetBinding.
>>
>> I can rewrite the tests for JUnit 4 but I'd need a Nested runner (I
>> think the tests would become pretty unreadable and less useful /
>> thourough otherwise).
>>
>> What are the options?
>>
>> - Integrate a nested runner (there is an Apache 2.0 licensed one
>> available)
>> - Create our own nested runner (about 200 lines of code)
>> - Can we introduce JUnit 5?
>> - Rewrite to plain JUnit 4?
>>
>> Let me know, and I can integrate them.
>>
>> --John
>>
>> On 19/09/2021 02:12, Nir Lisker wrote:
>> > I've played around with the implementation and pushed a modified
>> > prototype to the sandbox [1]. I ended up with something similar to
>> ReactFX:
>> > the orElse and orElseGet methods have their own binding classes that
>> > extend LazyObjectBinding, just like MapBinding and FlatMapBinding. The
>> > reason being that both their compute value and their subscription models
>> > are slightly different. While they can be matched to MapBinding like you
>> > did, it entails a bit of a roundabout way in my opinion. Creating a
>> > supplier for the constant value of orElse somewhat defeats the purpose I
>> > think.
>>
>> I have no strong opinion here, just wanted to keep the MR small. The
>> supplier should be an inline candidate, but creating a separate class is
>> fine too.
>>
>> As for the subscription model: flat map has a more complicated one, but
>> orElse, orElseGet and map all have the same one.
>>
>> > In addition, I think that it's fine to move the arguments' null checks
>> to
>> > the binding classes themselves, even if it's a couple of levels deeper
>> on
>> > the stack, while adding a message in the 2nd argument of
>> > Objects.requireNonNull for clarity. These classes are "self-contained"
>> so
>> > it makes sense for them to check their arguments. They might be used in
>> > other places, perhaps in the public Bindings class.
>>
>> The messages will need to be written from the perspective of the Binding
>> class then IMHO.
>>
>> > I also moved the subscription-related methods from ObservableValue to
>> > static methods in Subscription, at least for now, to defer the API
>> related
>> > to Subscription.
>>
>> Sounds good.
>>
>> > The branch doesn't compile because I didn't finish working on the
>> > visibility issue, but it's close enough to what I envision it like for
>> the
>> > first PR.
>>
>> I've ported the changes over to my branch and ran the tests on it, they
>> all pass apart from the above
Re: [External] : Re: Convenience factories for Border and Background
Then the PR is ready for a final review. I'll create the CSR shortly.
On Wed, Sep 29, 2021 at 3:07 PM Kevin Rushforth
wrote:
> Absent a strong motivation for adding this, I recommend to go with the
> current proposal, which does not include this newly requested method. It
> can always be added later if there is a compelling need.
>
> -- Kevin
>
> On 9/24/2021 4:50 AM, Nir Lisker wrote:
>
> I don't have a strong opinion on this addition.
>
> On Fri, Sep 24, 2021 at 2:47 PM Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> I don't have an objection to adding this one additional convenience
>> method if it is generally useful. If there aren't a lot of applications
>> that would use it, it seems better to go with just the two identified so
>> far and consider this one later.
>>
>> So: would this be a generally useful addition?
>>
>> -- Kevin
>>
>>
>> On 9/21/2021 2:43 AM, Marius Hanl wrote:
>> > As also written in a comment
>> > here: https://github.com/openjdk/jfx/pull/610
>> <https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/610__;!!ACWV5N9M2RV99hQ!Z57Y3Q7VQc4ZpD__IA299Q56jo8wF4MJh1J9wWOm6uFYnJfouoapPE0p9pPWIry9Tyav$>
>> > I would like to propose one more convenience method which should be
>> > added to JavaFX:
>> >
>> > public static Border stroke(Paint stroke, double width) {
>> > return new Border(new BorderStroke(stroke,
>> BorderStrokeStyle.SOLID, null, ne
>> > w BorderWidths(width)));
>> > }
>> >
>> > I think it's quite common that you want to create a solid border
>> with
>> > another width then the default of 1 (for every side).
>> >
>> > Note: This is also the last use case I think makes sense to add as a
>> > convenience method.
>> > Any other use case is likely to be so complex that it makes sense to
>> > use the normal existing constructors.
>> >
>> > Feel free to share you opinion.
>> >
>> > - Marius
>> >
>> > Gesendet: Dienstag, 08. Juni 2021 um 03:19 Uhr
>> > Von: "Nir Lisker"
>> > An: "Kevin Rushforth"
>> > Cc: "openjfx-dev@openjdk.java.net Mailing"
>> >
>> > Betreff: Re: [External] : Re: Convenience factories for Border and
>> > Background
>> > The new API:
>> > 1. `Border.of(Paint stroke)` or `Border.stroke(Paint stroke)` that
>> does
>> > `new Border(new BorderStroke(Paint stroke , BorderStrokeStyle.SOLID,
>> > null,
>> > null));`
>> > 2. `Background.of((Paint fill)` or `Background.fill(Paint fill)`
>> that
>> > does
>> > `new Background(new BackgroundFill(Paint fill, null, null));`
>> > I don't mind either name choice.
>> > On Tue, Jun 8, 2021 at 2:50 AM Kevin Rushforth
>> >
>> > wrote:
>> > > If I recall, there were a few developers that chimed in. It's a
>> > simple
>> > > enough addition -- at least your original proposal (not the
>> > suggestion of
>> > > mirroring the Color API, which I don't like) -- that it seems OK
>> to
>> > me.
>> > >
>> > > Can you repost your currently proposed API and see if those who
>> might
>> > like
>> > > to use it are satisfied with it?
>> > >
>> > > -- Kevin
>> > >
>> > >
>> > > On 6/7/2021 4:41 PM, Nir Lisker wrote:
>> > >
>> > > Does this constitute sufficient interest in the enhancement?
>> > >
>> > > On Thu, May 13, 2021 at 6:41 PM Michael Strau�
>> >
>> > > wrote:
>> > >
>> > >> Another option could be to mirror the `Color` API in both
>> `Border`
>> > and
>> > >> `Background`, like in the following examples:
>> > >>
>> > >> Color.rgb(125, 100, 75)
>> > >> Border.rgb(125, 100, 75)
>> > >> Background.rgb(125, 100, 75)
>> > >>
>> > >> Color.gray(127)
>> > >> Border.gray(127)
>> > >> Background.gray(127)
>> > >>
>> > >> Color.web("orange", 0.5)
>> > >> Border.web("orange", 0.5)
>> > >> Background.web("orange", 0.5)
>> > >>
>> > >> We could also mirror the named color constants, which would
>> enable a
>> > >> very compact syntax:
>> > >>
>> > >> StackPane pane = new StackPane();
>> > >> pane.setBorder(Border.RED);
>> > >> pane.setBackground(Background.BLUE);
>> > >>
>> > >> This is very similar to how "red" or "blue" are valid values for
>> > >> "-fx-border" or "-fx-background" in CSS.
>> > >>
>> > >
>> > >
>>
>>
>
Re: Gauging interest in updating the JavaFX test framework with JUnit 5
I much prefer JUnit 5 to 4, so I'm in favor. On Sat, Sep 25, 2021 at 12:40 AM John Hendrikx wrote: > Posting this to gauge the interest in adding JUnit 5 as a test > dependency to JavaFX, enabling writing tests with this new version of > JUnit while still supporting all JUnit 4 tests. > > A draft PR has been submitted here: > https://github.com/openjdk/jfx/pull/633 > > And an issue has been filed here: > https://bugs.openjdk.java.net/browse/JDK-8274274 > > Although very personally motivated to be able to write nested unit tests > in future JavaFX pull requests, I think this would be a great addition > to have in our testing arsenal. > > The main benefits of using JUnit 5: > > Better integration with Java 8, specifically use of Lambda functions > where this would make sense. For example `assertThrows` is really useful > and can replace the `expected` parameter in the Test annotation > (although this has also been backported to recent JUnit 4 versions). > > Support for better organization of tests (using Nested) and better > naming. See this image to see Nested and naming in action: > > https://user-images.githubusercontent.com/995917/111858133-d6ce0f80-8936-11eb-9724-be2c15150590.png > > Better extension system allowing to combine extensions whereas JUnit 4 > only allowed one extension at a time. > > Native support for parameterized tests, repeated tests, nested tests, > conditional test execution (by platform or environment for example), > assumptions, test generation and timeouts. > > Please let us know if you'd like to see a newer version of JUnit > included in JavaFX to further ease testing :) > > --John > > > >
