Re: .classpath files for Eclipse
There is an open bug report [1] that I created before the transition to GitHub that fixes BuildSrc and some apps, I will update it for the latest changes. Right now I am waiting to see which folders are still relevant. [1] https://bugs.openjdk.java.net/browse/JDK-8221708 On Thu, May 20, 2021 at 1:52 PM Jeanette Winzenburg wrote: > > Hi Tom, > > can't say anything to your questions: being on win only and just > having Eclipse projects for base, graphics, controls (everything else > is handled by gradle from the commandline) > > -- Jeanette > > Zitat von Tom Schindl : > > > Hi Nir/Jeannette, > > > > As you are both using Eclipse for development I'd like to get your > > take on the following things. > > > > Dealing with OS specific > > > > The graphics module has to have different source folders depending > > on the OS you are developing on: > > * Windows: build/hlsl/Decora and build/hslPrism > > * Linux/OS-X: build/gensrc/jsl-decora build/gensrc/jsl-prism > > > > So when importing the projects Eclipse does not even build anything > > until you fix the .classpath file - I'd like to propose to mark > > those Source-Folders optional. > > > > Swing-Project > > - > > There the .classpath has a none existing "src/test/java" I wonder > > where that source is and if not there anymore remove the offending > > entry from .classpath-File > > > > BuildSrc > > > > What's the purpose of that project? The classpath is incorrect and > > does it have to be a Java-Project? > > > > > > Tom > > > >
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Thu, 15 Apr 2021 02:21:50 GMT, Nir Lisker wrote: >> Added a SpotLight only to the D3D pipeline currently. >> >> ### API discussion points >> >> - [X] Added `SpotLight` as a subclass of `LightBase`. However, it could >> also be a subclass of `PointLight` as it's a point light with direction and >> extra factors. I saw that `scenario.effect.light.SpotLight` extends its >> respective `PointLight`, but it's not a perfect analogy. In the end, I think >> it's a questions of whether `PointLight` will be expanded in a way which >> doesn't not suit `SpotLight`, and I tend to think that the answer is no. >> >> - [X] The inner and outer angles are the "diameter angles" as shown >> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types). >> I, personally, find it more intuitive that these are the "radius angles", >> so half these angles, as used in the spotlight factor formula. Do you think >> I can change this or do you prefer the current definition of the angles? >> >> - [ ] The current implementation uses an ad-hoc direction property (using a >> `Point3D`). It crossed my mind that we could use the rotation transforms of >> the node to control the direction instead, just like we use the >> translation/layout of the node to get the position (there is an internal >> Affine3D transform for lights, not sure why `AmbientLight` needs it). >> Wouldn't that make more sense? When I rotate the light I would expect to see >> a change in direction. >> >> ### Implementation discussion points >> >> - [ ] I've gotten advice from a graphics engineer to treat point lights as >> spot lights with a 360 degrees coverage, which simplifies a few places. We >> can still try to optimize for a point light by looking at the light >> parameters: `falloff = 0` and `outerAngle = 180`. These possible >> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the >> pixel/fragment shaders in the form of 3 different ways to compute the >> spotlight factor (the `computeLightN` methods). We need to check which of >> these give the best results. >> >> ## Performance >> >> Testing 3 point lights and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Combined rotation and direction Did you test the different `computeSpotlightFactor` methods in the shader? I got some 4 fps difference between the best and worst cases. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8267551: Support loading images from inline data-URIs [v5]
> This PR adds support for loading images from [inline data > URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely > supported by web browsers. This enables developers to package small images in > CSS files, rather than separately deploying the images alongside the CSS file. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Moved test - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/07ff1aa4..3a77ac0b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=03-04 Stats: 44 lines in 3 files changed: 24 ins; 18 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Thu, 15 Apr 2021 02:21:50 GMT, Nir Lisker wrote: >> Added a SpotLight only to the D3D pipeline currently. >> >> ### API discussion points >> >> - [X] Added `SpotLight` as a subclass of `LightBase`. However, it could >> also be a subclass of `PointLight` as it's a point light with direction and >> extra factors. I saw that `scenario.effect.light.SpotLight` extends its >> respective `PointLight`, but it's not a perfect analogy. In the end, I think >> it's a questions of whether `PointLight` will be expanded in a way which >> doesn't not suit `SpotLight`, and I tend to think that the answer is no. >> >> - [X] The inner and outer angles are the "diameter angles" as shown >> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types). >> I, personally, find it more intuitive that these are the "radius angles", >> so half these angles, as used in the spotlight factor formula. Do you think >> I can change this or do you prefer the current definition of the angles? >> >> - [ ] The current implementation uses an ad-hoc direction property (using a >> `Point3D`). It crossed my mind that we could use the rotation transforms of >> the node to control the direction instead, just like we use the >> translation/layout of the node to get the position (there is an internal >> Affine3D transform for lights, not sure why `AmbientLight` needs it). >> Wouldn't that make more sense? When I rotate the light I would expect to see >> a change in direction. >> >> ### Implementation discussion points >> >> - [ ] I've gotten advice from a graphics engineer to treat point lights as >> spot lights with a 360 degrees coverage, which simplifies a few places. We >> can still try to optimize for a point light by looking at the light >> parameters: `falloff = 0` and `outerAngle = 180`. These possible >> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the >> pixel/fragment shaders in the form of 3 different ways to compute the >> spotlight factor (the `computeLightN` methods). We need to check which of >> these give the best results. >> >> ## Performance >> >> Testing 3 point lights and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Combined rotation and direction Here is the performance of mesh on my macOS system with a discrete graphics chip using the LightingSample test program with a mesh of 1000 quads rendered with 3 point lights: | Run | fps | | - | | | Baseline (FX 15) | 25.8 | | After Attenuation added (FX 16) | 15.4 | | SpotLight support (this PR) | 10.6 | This was the same program in all three cases, so no SpotLight and no attenuation. This is the same "worst case" stress test that we used when evaluating attenuation. Hopefully we can optimize and get some of the performance back with optimization. In any case, I think this adds additional motivation for a future
Re: RFR: 8267551: Support loading images from inline data-URIs [v4]
> This PR adds support for loading images from [inline data > URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely > supported by web browsers. This enables developers to package small images in > CSS files, rather than separately deploying the images alongside the CSS file. Michael Strauß has updated the pull request incrementally with two additional commits since the last revision: - Added javadoc - Added javadoc - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/71eb2d20..07ff1aa4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=02-03 Stats: 49 lines in 1 file changed: 16 ins; 11 del; 22 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Fri, 14 May 2021 22:29:25 GMT, Kevin Rushforth wrote: > You may be right about the scale problem being preexisting. I'll double check > when I test on Mac next week (both Retina and external screen). I can confirm that the scaling problem on macOS Retina a preexisting bug (not related to either this or the earlier addition of attenuation). So I'll file a separate issue for this. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8267551: Support loading images from inline data-URIs [v3]
> This PR adds support for loading images from [inline data > URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely > supported by web browsers. This enables developers to package small images in > CSS files, rather than separately deploying the images alongside the CSS file. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Added javadoc - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/e23ab085..71eb2d20 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=01-02 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v2]
> This PR adds support for loading images from [inline data > URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely > supported by web browsers. This enables developers to package small images in > CSS files, rather than separately deploying the images alongside the CSS file. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Added javadoc - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/4f273d0f..e23ab085 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=00-01 Stats: 65 lines in 3 files changed: 40 ins; 8 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Thu, 15 Apr 2021 02:21:50 GMT, Nir Lisker wrote: >> Added a SpotLight only to the D3D pipeline currently. >> >> ### API discussion points >> >> - [X] Added `SpotLight` as a subclass of `LightBase`. However, it could >> also be a subclass of `PointLight` as it's a point light with direction and >> extra factors. I saw that `scenario.effect.light.SpotLight` extends its >> respective `PointLight`, but it's not a perfect analogy. In the end, I think >> it's a questions of whether `PointLight` will be expanded in a way which >> doesn't not suit `SpotLight`, and I tend to think that the answer is no. >> >> - [X] The inner and outer angles are the "diameter angles" as shown >> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types). >> I, personally, find it more intuitive that these are the "radius angles", >> so half these angles, as used in the spotlight factor formula. Do you think >> I can change this or do you prefer the current definition of the angles? >> >> - [ ] The current implementation uses an ad-hoc direction property (using a >> `Point3D`). It crossed my mind that we could use the rotation transforms of >> the node to control the direction instead, just like we use the >> translation/layout of the node to get the position (there is an internal >> Affine3D transform for lights, not sure why `AmbientLight` needs it). >> Wouldn't that make more sense? When I rotate the light I would expect to see >> a change in direction. >> >> ### Implementation discussion points >> >> - [ ] I've gotten advice from a graphics engineer to treat point lights as >> spot lights with a 360 degrees coverage, which simplifies a few places. We >> can still try to optimize for a point light by looking at the light >> parameters: `falloff = 0` and `outerAngle = 180`. These possible >> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the >> pixel/fragment shaders in the form of 3 different ways to compute the >> spotlight factor (the `computeLightN` methods). We need to check which of >> these give the best results. >> >> ## Performance >> >> Testing 3 point lights and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Combined rotation and direction I think the API as currently defined is what we want. Having an explicit direction vector, and then transforming that by the transform of the light is quite flexible in a natural and expected way. I took the existing [`LightMotion`](https://github.com/openjdk/jfx/blob/master/apps/toys/FX8-3DFeatures/src/fx83dfeatures/LightMotion.java) example program, and created a new `SpotLightMotion` program (attached). It's easy to programmatically point the light to the object you wish to illuminate (the sphere or cylinder, in this example). And once you do, rotation works naturally. Here is the code that computes the spot light direction:
Re: RFR: 8267314: Loading some animated GIFs fails with ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096 [v2]
On Fri, 21 May 2021 13:10:50 GMT, Kevin Rushforth wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Modify test to avoid online resources
>
> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
> line 91:
>
>> 89: @Test
>> 90: public void testLoadAllFramesAnimatedGIF() throws
>> ImageStorageException {
>> 91: String path =
>> "https://upload.wikimedia.org/wikipedia/commons/2/2c/Rotating_earth_%28large%29.gif;;
>
> Except for specific network tests we don't want to load anything from the
> internet during a run. For one thing this will fail behind a firewall
> (without setting a proxy, which we don't want to require). For another, the
> content could change out from under us.
>
> The best solution would be to generate a synthetic image with properties that
> will cause it to fail (we can't use a preexisting image due to copyright
> issues).
Okay, I've replaced the test with an encoded image of a single frame. It fails
with IAOOB without the fix, passes with it.
-
PR: https://git.openjdk.java.net/jfx/pull/513
Re: RFR: 8267314: Loading some animated GIFs fails with ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096 [v2]
> This PR limits the `tableIndex` value, used by the LZWDecoder algorithm in > `GIFImageLoader2`, to avoid a potential AIOOB exception that happens on some > animated GIFs, to the maximum size of the tables used (4096). > > In some occasions loading an animated GIF like the one used in the included > test, doesn't throw such exception, because we `allow partially loaded > animated images` in `ImageStorage`, but only a few frames are loaded. > > In theory, greater values of such index would operate over completely full > tables, so there is no need to add new values in this case, and therefore, > there is no risk in limiting the value to 4096. > > This PR will prevent the exception and all the frames should load. The > included test passes now (and fails loading only 10 frames out of 44 without > the proposed fix). Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Modify test to avoid online resources - Changes: - all: https://git.openjdk.java.net/jfx/pull/513/files - new: https://git.openjdk.java.net/jfx/pull/513/files/d6748b89..03897b70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=513=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=513=00-01 Stats: 113 lines in 1 file changed: 108 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/513.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/513/head:pull/513 PR: https://git.openjdk.java.net/jfx/pull/513
Re: RFR: 8267551: Support loading images from inline data-URIs
On Wed, 19 May 2021 20:24:31 GMT, Michael Strauß wrote: > This PR adds support for loading images from [inline data > URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely > supported by web browsers. This enables developers to package small images in > CSS files, rather than separately deploying the images alongside the CSS file. I'd like to see this documented somewhere in the `Image` class docs. - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267425: Intermittent failure of HonorDeveloperSettingsTest unit test
On Thu, 20 May 2021 07:18:28 GMT, Ambarish Rapte wrote: > The same failure was earlier fixed at PR #496, which seems insufficient. > The changes in this PR were also tried ealier in the PR #496 but later > removed, as they seemed not necessary. > Fix is to clean up the test, by removing all children added to root and > hiding the stage. > The test only fails on the GHA run, if it continues to fail after this fix > then we shall skip it till next test sprint. I tried to instrument the code earlier this morning to see what is going on, but I couldn't make it fail, even running it in a loop via GitHub actions on Linux. One thing to note is that the failure suggests some leftover state that has been partially, but not fully cleaned up. Given where the NPE is, there must be a node in the `cacheContainerMap` of `StyleManager` with a non-null scene where the root node of that scene is null. However, unless I'm missing something, this should not be possible since a scene with a null root node is an error. Scene::setRoot throws NPE if you attempt to construct a Scene with a null root or if you later set it to null. Very odd. - PR: https://git.openjdk.java.net/jfx/pull/509
Re: Support loading images from inline data-URIs
This seems like a very reasonable proposal. I'd like to see it documented in the Image class, so I'll mark it as needing a CSR. -- Kevin On 5/19/2021 1:36 PM, Michael Strauß wrote: I would like to propose adding support for loading images from inline data-URIs, as commonly supported by web browsers. This is a low-impact addition to the image loading infrastructure as seen in this draft PR: https://github.com/openjdk/jfx/pull/508 As a follow-on feature, I'd also like to support loading stylesheets from data-URIs. With both features implemented, developers have more flexibility in working with stylesheets. For example, it'd be possible to dynamically generate and add stylesheets to applications without deploying temporary files on disk. Another scenario would be to include small theme-specific icons in their respective stylesheets, which can simplify deployment. Any thoughts?
RFR: 8267551: Support loading images from inline data-URIs
This PR adds support for loading images from [inline data URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely supported by web browsers. This enables developers to package small images in CSS files, rather than separately deploying the images alongside the CSS file. - Commit messages: - Added data-URI loading support for images Changes: https://git.openjdk.java.net/jfx/pull/508/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=508=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267551 Stats: 437 lines in 8 files changed: 414 ins; 9 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: Make themes a first-class concept in JavaFX
Good point about the STYLESHEET_CASPIAN and STYLESHEET_MODENA values. Here are a few more aspects for discussion: Any sufficiently sophisticated theme will probably be comprised of several stylesheets. In fact, the built-in themes are a good example for that. Since the current API doesn't allow to specify more than a single user-agent stylesheet, this is quite limiting and requires some workarounds, for example by manually adding auxiliary stylesheets to every Scene. A thing that is quite common for real-world apps is to keep the built-in theme, and then manually add stylesheets that change aspects of the built-in theme. The downside of this is that this creates a dependency on a particular version of the JavaFX built-in theme. A more reliable solution would be to fork the built-in theme and create a custom theme from that. Since the proposed API has a very thin surface and treats themes simply as dynamic collections of stylesheets, forking an existing theme requires duplicating the logic code that might be contained. In order to keep maintenance cost for the OpenJFX project down, interpreting platform theme properties is the sole responsibility of theme developers. This may incur a significant effort, since theme developers may need to understand the meaning behind various optional properties of different platforms. As it stands, platform properties are only reported for Windows platforms. It will require additional development effort to add support for this to other Glass backends. Am Fr., 21. Mai 2021 um 18:08 Uhr schrieb Kevin Rushforth : > > Please file an Enhancement request first, and from that enhancement > create a new CSR. Ideally JBS would prevent direct creation of an issue > with an issuetype of CSR. > > As for the feature itself, it seems useful, but will need discussion as > to the value proposition (i.e., cost / benefit), and then agreement on > the API itself. One thing that would help in this is some discussion > around how an application developer would use it. > > I have one initial comment on the currently proposed API. You'll likely > need to rethink your proposed change to the STYLESHEET_CASPIAN and > STYLESHEET_MODENA constants. It seems attractive to change it for > consistency, but it is an incompatible change. It is likely there are > applications or third-party controls that rely on the current values. > Also, it leaks the name of the internal com.sun.javafx.* package where > it happens to live today, which is an implementation detail. I think you > should stick with the existing constants and translate it on use in the > implementation as needed. > > -- Kevin
Re: RFR: 8252783: Remove the css Selector and ShapeConverter constructors
On Fri, 21 May 2021 11:48:21 GMT, Ajit Ghaisas wrote: > The javafx.css.Selector and javafx.css.converter.ShapeConverter constructors > were deprecated for removal in openjfx16. > This PR removes these constructors (targeted for openjfx17). Marked as reviewed by pbansal (Committer). - PR: https://git.openjdk.java.net/jfx/pull/512
Re: Make themes a first-class concept in JavaFX
Please file an Enhancement request first, and from that enhancement create a new CSR. Ideally JBS would prevent direct creation of an issue with an issuetype of CSR. As for the feature itself, it seems useful, but will need discussion as to the value proposition (i.e., cost / benefit), and then agreement on the API itself. One thing that would help in this is some discussion around how an application developer would use it. I have one initial comment on the currently proposed API. You'll likely need to rethink your proposed change to the STYLESHEET_CASPIAN and STYLESHEET_MODENA constants. It seems attractive to change it for consistency, but it is an incompatible change. It is likely there are applications or third-party controls that rely on the current values. Also, it leaks the name of the internal com.sun.javafx.* package where it happens to live today, which is an implementation detail. I think you should stick with the existing constants and translate it on use in the implementation as needed. -- Kevin On 5/21/2021 8:39 AM, Michael Strauß wrote: Here's a CSR that explains the details of the proposed public API: https://bugs.openjdk.java.net/browse/JDK-8267540
Re: Make themes a first-class concept in JavaFX
Here's a CSR that explains the details of the proposed public API: https://bugs.openjdk.java.net/browse/JDK-8267540
Re: RFR: 8267314: Loading some animated GIFs fails with ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096
On Fri, 21 May 2021 13:02:14 GMT, Jose Pereda wrote:
> This PR limits the `tableIndex` value, used by the LZWDecoder algorithm in
> `GIFImageLoader2`, to avoid a potential AIOOB exception that happens on some
> animated GIFs, to the maximum size of the tables used (4096).
>
> In some occasions loading an animated GIF like the one used in the included
> test, doesn't throw such exception, because we `allow partially loaded
> animated images` in `ImageStorage`, but only a few frames are loaded.
>
> In theory, greater values of such index would operate over completely full
> tables, so there is no need to add new values in this case, and therefore,
> there is no risk in limiting the value to 4096.
>
> This PR will prevent the exception and all the frames should load. The
> included test passes now (and fails loading only 10 frames out of 44 without
> the proposed fix).
The fix seems safe enough, although should have a second reviewer. The test
will need changes.
modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
line 91:
> 89: @Test
> 90: public void testLoadAllFramesAnimatedGIF() throws
> ImageStorageException {
> 91: String path =
> "https://upload.wikimedia.org/wikipedia/commons/2/2c/Rotating_earth_%28large%29.gif;;
Except for specific network tests we don't want to load anything from the
internet during a run. For one thing this will fail behind a firewall (without
setting a proxy, which we don't want to require). For another, the content
could change out from under us.
The best solution would be to generate a synthetic image with properties that
will cause it to fail (we can't use a preexisting image due to copyright
issues).
-
Changes requested by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/513
Re: RFR: 8252783: Remove the css Selector and ShapeConverter constructors
On Fri, 21 May 2021 11:48:21 GMT, Ajit Ghaisas wrote: > The javafx.css.Selector and javafx.css.converter.ShapeConverter constructors > were deprecated for removal in openjfx16. > This PR removes these constructors (targeted for openjfx17). Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/512
RFR: 8267314: Loading some animated GIFs fails with ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096
This PR limits the `tableIndex` value, used by the LZWDecoder algorithm in `GIFImageLoader2`, to avoid a potential AIOOB exception that happens on some animated GIFs, to the maximum size of the tables used (4096). In some occasions loading an animated GIF like the one used in the included test, doesn't throw such exception, because we `allow partially loaded animated images` in `ImageStorage`, but only a few frames are loaded. In theory, greater values of such index would operate over completely full tables, so there is no need to add new values in this case, and therefore, there is no risk in limiting the value to 4096. This PR will prevent the exception and all the frames should load. The included test passes now (and fails loading only 10 frames out of 44 without the proposed fix). - Commit messages: - Limit tableIndex value to avoid AIOOB exception Changes: https://git.openjdk.java.net/jfx/pull/513/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=513=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267314 Stats: 23 lines in 2 files changed: 11 ins; 1 del; 11 mod Patch: https://git.openjdk.java.net/jfx/pull/513.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/513/head:pull/513 PR: https://git.openjdk.java.net/jfx/pull/513
Integrated: 8267392: ENTER key press on editable TableView throws NPE
On Wed, 19 May 2021 00:48:18 GMT, Marius Hanl wrote: > ~~Note: I reported the bug already, waiting for approval. Internal tracking > id: 9070318. I will update the title as soon as the ticket is created.~~ > EDIT: Changed the title. :) > > This PR is fixing a NP, which is thrown when you press ENTER on an editbale > table, after it is initially shown. > > When pressing ENTER, **TableViewBehaviorBase#activate** is retrieving the > current focused row (**getFocusedCell()**) and from there the corresponding > table column. > This is null, when a table is initially shown. It can also be null, when the > items from the underlying table are changed (e.g. **setItems()**) or when > **getFocusModel().focus(row)** is used. > Therefore, null is a valid value and we should guard against it. This pull request has now been integrated. Changeset: 58439103 Author:Marius Hanl Committer: Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/5843910333ebdbc4ea14033461e980b0d0692ca8 Stats: 39 lines in 3 files changed: 33 ins; 0 del; 6 mod 8267392: ENTER key press on editable TableView throws NPE Reviewed-by: fastegal, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/505
RFR: 8252783: Remove the css Selector and ShapeConverter constructors
The javafx.css.Selector and javafx.css.converter.ShapeConverter constructors were deprecated for removal in openjfx16. This PR removes these constructors (targeted for openjfx17). - Commit messages: - 8252783 fix Changes: https://git.openjdk.java.net/jfx/pull/512/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=512=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252783 Stats: 8 lines in 2 files changed: 0 ins; 5 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/512.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/512/head:pull/512 PR: https://git.openjdk.java.net/jfx/pull/512
Re: RFR: 8150709: Mac OSX and German Keyboard Layout (Y/Z) [v4]
On Thu, 25 Mar 2021 17:41:40 GMT, Martin Fox
wrote:
>> This PR adds code to ensure that KeyCodeCombinations match KeyEvents as
>> expected by more accurately mapping from a Mac key code to a Java key code
>> based on the user’s active keyboard layout (the existing code assumes a US
>> QWERTY layout). The new code first identifies a set of Mac keys which can
>> produce different characters based on the user’s keyboard layout. A Mac key
>> code outside that area is processed exactly as before. For a key inside the
>> layout-sensitive area the code calls UCKeyTranslate to translate the key to
>> an unshifted ASCII character based on the active keyboard and uses that to
>> determine the Java key code.
>>
>> When performing the reverse mapping for the Robot the code first uses the
>> old QWERTY mapping to find a candidate key. If it lies in the
>> layout-sensitive area the code then scans the entire area calling
>> UCKeyTranslate until it finds a match. If the key lies outside the
>> layout-sensitive area it’s processed exactly as before.
>>
>> There are multiple duplicates of these bugs logged against Mac applications
>> built with JavaFX.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8090257 Mac: Inconsistent KeyEvents
>> with alternative keyboard layouts
>> https://bugs.openjdk.java.net/browse/JDK-8088120 [Accelerator, Mac] CMD + Z
>> accelerator is not working with French keyboard
>> https://bugs.openjdk.java.net/browse/JDK-8087915 Mac: accelerator doesn't
>> take into account azerty keyboard layout
>> https://bugs.openjdk.java.net/browse/JDK-8150709 Mac OSX and German Keyboard
>> Layout (Y/Z)
>
> Martin Fox has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed whitespace error.
For reference this was the code I had around from my first iteration working on
that problem
diff --git a/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m
b/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m
index 38a828a41e..8657519f13 100644
--- a/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m
+++ b/modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m
@@ -247,6 +247,24 @@ jint GetJavaKeyCodeFor(unsigned short keyCode)
jint GetJavaKeyCode(NSEvent *event)
{
+ NSString *chars = [event charactersIgnoringModifiers];
+
+ if ([chars length] > 0) {
+ NSInteger offset;
+
+ unichar ch = [chars characterAtIndex:0];
+ if ([[NSCharacterSet letterCharacterSet] characterIsMember:ch])
{
+ NSLog(@"Letter char");
+ unichar lower;
+ lower = tolower(ch);
+ offset = lower - 'a';
+ if (offset >= 0 && offset <= 25) {
+ NSLog(@"Hello Char it is");
+ return com_sun_glass_events_KeyEvent_VK_A +
offset;
+ }
+ }
+ }
+
return GetJavaKeyCodeFor([event keyCode]);
}
-
PR: https://git.openjdk.java.net/jfx/pull/425
Re: RFR: 8267392: ENTER key press on editable TableView throws NPE [v2]
On Wed, 19 May 2021 14:15:00 GMT, Marius Hanl wrote: >> ~~Note: I reported the bug already, waiting for approval. Internal tracking >> id: 9070318. I will update the title as soon as the ticket is created.~~ >> EDIT: Changed the title. :) >> >> This PR is fixing a NP, which is thrown when you press ENTER on an editbale >> table, after it is initially shown. >> >> When pressing ENTER, **TableViewBehaviorBase#activate** is retrieving the >> current focused row (**getFocusedCell()**) and from there the corresponding >> table column. >> This is null, when a table is initially shown. It can also be null, when the >> items from the underlying table are changed (e.g. **setItems()**) or when >> **getFocusModel().focus(row)** is used. >> Therefore, null is a valid value and we should guard against it. > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comment (changed from NP to NPE) Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/505
Re: RFR: 8264127: ListCell editing status is true, when index changes while editing [v15]
On Thu, 20 May 2021 11:12:09 GMT, Florian Kirmaier wrote: >> Fixing ListCell editing status is true, when index changes while editing. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8264127 > small changes based on CR Marked as reviewed by fastegal (Committer). - PR: https://git.openjdk.java.net/jfx/pull/441
Re: Make themes a first-class concept in JavaFX
I've been a proponent of a theme implementation for a long time, even before
JavaFX was separated out (had quite a few discussions with Jonathan about it).
So this proposal is great!
On 2021-05-21 06:36, Michael Strauß wrote:
Currently, the two themes shipped with JavaFX (Caspian and Modena) are
implemented as a set of stylesheets and some internal logic, mostly in
`PlatformImpl`.
Much of this logic deals with optional features or platform-specific
theme modifications. Another piece of logic deals with accessibility
themes: on Windows platforms, both Caspian and Modena have
high-contrast stylesheets that are added or removed in response to OS
theme changes.
Apart from the fact that high-contrast detection only works on English
locales (and there's no simple fix, see JDK-8185447), a major downside
of this implementation is that it special-cases the built-in themes
and doesn't offer enough extension points to make it easy for
developers to create new rich themes.
I think this can be very much improved by making themes a first-class
citizen in JavaFX, and removing the special handling of the two
built-in themes. Here's my draft PR of this feature:
https://github.com/openjdk/jfx/pull/511
This PR adds a new interface in the javafx.application package:
public interface Theme {
ObservableList getStylesheets();
void platformThemeChanged(Map properties);
}
An implementation of this interface populates the list returned by
`Theme.getStylesheets()` with its collection of stylesheets.
Optionally, it can listen to the platform theme properties reported by
`Theme.platformThemeChanged(...)` and modify its list of stylesheets
accordingly.
Platform properties are platform-specific key-value pairs, and theme
implementations are responsible for interpreting the values. For
example, on the Windows platform, a theme implementation might listen
to the "Windows.SPI_HighContrastOn" property in order to detect
whether or not a high-contrast stylesheet should be used. The `Theme`
class documents all currently supported platform properties.
A theme is activated by setting it as the user-agent stylesheet of the
application, where "theme:" is a custom URI scheme:
Application.setUserAgentStylesheet("theme:com.example.CustomTheme");
The existing constants `Application.STYLESHEET_CASPIAN` and
`Application.STYLESHEET_MODENA` are simply redefined to their new
`Theme` implementations
("theme:com.sun.javafx.application.theme.Caspian" and
"theme:com.sun.javafx.application.theme.Modena").
Because theme-specific code is now located in its corresponding
`Theme` class, I've been able to clean up `PlatformImpl` so that it
doesn't special-case the built-in themes anymore.
Going forward: If we add support for loading stylesheets from
data-URIs (similar to https://github.com/openjdk/jfx/pull/508), we can
then fix the high-contrast themes by reading the system colors
reported back by Windows, and creating an in-memory stylesheet for
these colors within the `Modena` implementation.
This feature will also allow developers to create themes that
dynamically adjust to OS-level accent colors or dark mode settings.
For example, a `Theme` implementation can detect dark mode on Windows
10 by listening to the
"Windows.UI.ViewManagement.UISettings.ColorValue_Background" and
"Windows.UI.ViewManagement.UISettings.ColorValue_Foreground"
properties.
I'm looking forward to any opinions on this.
