Re: RFR: 8332539: Update libxml2 to 2.12.7
On Fri, 24 May 2024 18:18:22 GMT, Hima Bindu Meda wrote: > Updated libxml to v2.12.7. Sanity testing looks fine. No issue seen Code changes look good. Tests are green. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1464#pullrequestreview-2077902852
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move getStyleClassNames to location it was introduced to reduce diff I built it and tested on a retail self-checkout app that uses a lot of css. All good. - PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2130271332
RFR: 8332539: Update libxml2 to 2.12.7
Updated libxml to v2.12.7. Sanity testing looks fine. No issue seen - Commit messages: - configure libxml on windows - configure libxml on linux - update libxml to v2.12.7 Changes: https://git.openjdk.org/jfx/pull/1464/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1464=00 Issue: https://bugs.openjdk.org/browse/JDK-8332539 Stats: 67 lines in 9 files changed: 26 ins; 9 del; 32 mod Patch: https://git.openjdk.org/jfx/pull/1464.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1464/head:pull/1464 PR: https://git.openjdk.org/jfx/pull/1464
Re: RFR: 8332539: Update libxml2 to 2.12.7
On Fri, 24 May 2024 18:18:22 GMT, Hima Bindu Meda wrote: > Updated libxml to v2.12.7. Sanity testing looks fine. No issue seen Reviewers: @kevinrushforth @tiainen - PR Comment: https://git.openjdk.org/jfx/pull/1464#issuecomment-2130131350
Re: RFR: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window [v4]
On Thu, 23 May 2024 14:01:40 GMT, Marius Hanl wrote: >> This PR fixes the problem that maximizing/fullscreen a `Stage` or `Dialog` >> is broken when `sizeToScene()` was called before or after. >> >> The approach here is to ignore the `sizeToScene()` request when the `Stage` >> is maximized or set to fullscreen. >> Otherwise the Window Manager of the OS will be confused and you will get >> weird flickering or wrong Window buttons (e.g. on Windows, the 'Maximize' >> button still shows the 'Restore' Icon, while we already resized the `Stage` >> to not be maximized). > > Marius Hanl has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Implement isSizeToSceneAllowed() method to determines whether the > sizeToScene() request is allowed. Implement much more tests > - Merge branch 'master' of https://github.com/openjdk/jfx into > jdk-8326619-maximize-minimize-scene > - improve tests > - JDK-8326619: Improve tests > - JDK-8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the > Window I won't have time to do a detailed review for a while, but the updated approach to the fix looks promising. I note that the change in behavior will need to be documented another way, possibly in the base `Window::sizeToScene` method, since the newly added method is, correctly, not public (nor should it be). When running the new test on macOS, I see a few test failures followed by a crash. The crash is clearly a bug in JavaFX glass code (I'll file a bug for that), but the failures point to a problem with the test. I noticed while running it that there are no delays between various window operations in the tests. This will never work on Mac (and likely is a factor in provoking the crash), and will not be reliable on other platforms. Here are the test failures: SizeToSceneTest > testInitialSizeOnSizeToScene() FAILED org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: but was: at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40) at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210) at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77) at app//test.javafx.stage.SizeToSceneTest.testInitialSizeOnSizeToScene(SizeToSceneTest.java:197) SizeToSceneTest > testInitialSizeSizeToSceneFullscreenOnOff() FAILED org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: but was: at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40) at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210) at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77) at app//test.javafx.stage.SizeToSceneTest.testInitialSizeSizeToSceneFullscreenOnOff(SizeToSceneTest.java:229) SizeToSceneTest > testInitialSizeMaximizedOnOffSizeToScene() FAILED org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: but was: at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40) at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210) at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77) at app//test.javafx.stage.SizeToSceneTest.testInitialSizeMaximizedOnOffSizeToScene(SizeToSceneTest.java:245) SizeToSceneTest > testInitialSizeFullscreenOnOffSizeToScene() FAILED org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: but was: at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40) at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210) at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77) at app//test.javafx.stage.SizeToSceneTest.testInitialSizeFullscreenOnOffSizeToScene(SizeToSceneTest.java:213) SizeToSceneTest > testInitialSizeSizeToSceneMaximizedOnOff() FAILED org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: but was: at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40) at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210) at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77) at
Re: RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling
On Thu, 23 May 2024 21:51:55 GMT, Marius Hanl wrote:
> Alternative PR to https://github.com/openjdk/jfx/pull/1330 which does not
> modify the layout of `VirtualFlow`.
>
> This PR fixes the glitching by removing the code in `NGNode.renderRectClip`,
> which made many calculations leading to floating point errors.
> Interestingly I found out, that `getClippedBounds(..)` is already returning
> the correct bounds that just need to be intersected with the clip of the
> `Graphics` object.
>
> So the following code is effectively doing the same:
>
> Old:
>
> BaseBounds newClip = clipNode.getShape().getBounds();
> if (!clipNode.getTransform().isIdentity()) {
> newClip = clipNode.getTransform().transform(newClip, newClip);
> }
> final BaseTransform curXform = g.getTransformNoClone();
> final Rectangle curClip = g.getClipRectNoClone();
> newClip = curXform.transform(newClip, newClip); // <- The value of newClip
> after the transform is what getClippedBounds(..) is returning
> newClip.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
> Rectangle clipRect = new Rectangle(newClip)
>
>
> New:
>
> BaseTransform curXform = g.getTransformNoClone();
> BaseBounds clipBounds = getClippedBounds(new RectBounds(), curXform);
> Rectangle clipRect = new Rectangle(clipBounds);
> clipRect.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
>
>
> As you can see, there are very similar, but `getClippedBounds` does a much
> better job in calculating the bounds.
> I also wrote a tests proving the bug. I took 100% of the setup and values
> from a debugging session I did when reproducing this bug.
>
> I checked several scenarios and code and could not find any regressions.
> Still, since this is change affects all nodes with rectangular clips, we
> should be careful.
> Performance wise I could not spot any difference, I do not expect any
> difference.
> **So I would like to have at least 2 reviewers.**
> Note that I will do more testing as well soon on all JavaFX applications I
> have access to.
>
> ---
>
> As written in the other PR, I have some interesting findings on this
> particular problem.
>
> Copy from the other PR:
> --
>
> Ok, so I found out the following:
> When a Rectangle is used as clip without any effect or opacity modification,
> the rendering goes another (probably faster) route with rendering the clip.
> That's why setting the `opacity` to `0.99` fixes the issue - another route
> will be used for the rendering.
> This happens at the low level (`NGNode`) side of JavaFX.
> ...
> I could track it down to be a typical floating point problem
> ...
> The bug always appears when I scroll and the clip RectBounds are somethi...
I see a problem on Windows 11 at 125% scale: using the tester app in the
ticket, the table focus rectangle's right edge is not visible at some width
(i.e. appears and disappears when resizing the window width):
When it is visible, there is no jitter described in the ticket. Also, it can
be reproduced at 100% scale.
It may or may not be a separate issue.
modules/javafx.graphics/src/test/java/test/com/sun/javafx/sg/prism/NGNodeTest.java
line 626:
> 624: @Override
> 625: public void setClipNode(NGNode clipNode) {
> 626: System.out.println(clipNode);
should this println be removed?
-
PR Comment: https://git.openjdk.org/jfx/pull/1462#issuecomment-2130095158
PR Review Comment: https://git.openjdk.org/jfx/pull/1462#discussion_r1613777480
Integrated: 8332732: Clean up non-standard use of /// comments in JavaFX
On Thu, 23 May 2024 15:22:38 GMT, Kevin Rushforth wrote: > [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405) added markdown > support to javadoc, using `///` to indicate markdown documentation comments. > As a result, building JavaFX docs with JDK 23 produces new warnings, which > causes the build to fail (since we treat warnings as errors). > > This PR was generated by running a program to automate the modification of > each line that contains three or more slash chars in a row, `///` (except > those in String literals). The replacement algorithm is as follows: > > 1. Replace each occurrence of exactly 3 slash chars `///` with 2 slash chars > `//` > 2. Replace each occurrence of 4 or more slash chars with an equal number of > `-` chars, leaving the first two `//` chars of the first run of `` as is. > > This is similar to the proposed fix in java.base for > [JDK-8331879](https://bugs.openjdk.org/browse/JDK-8331879) / PR > openjdk/jdk#19130 (except for how I propose to handle the case of exactly > three slashes). > > As an alternative approach, I could fix just the 5 javadoc comments causing > the error, and leave the rest alone, but this will prevent problems from > cropping up in the future, and is in keeping with changes going into the JDK > source base. This pull request has now been integrated. Changeset: 31fe622c Author:Kevin Rushforth URL: https://git.openjdk.org/jfx/commit/31fe622c4e540df06ed727343ace16cba3942534 Stats: 1309 lines in 50 files changed: 0 ins; 0 del; 1309 mod 8332732: Clean up non-standard use of /// comments in JavaFX Reviewed-by: angorya - PR: https://git.openjdk.org/jfx/pull/1461
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move getStyleClassNames to location it was introduced to reduce diff The code looks good. I didn't test it, but I'm fine with integrating. - PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2129765316
JavaFX 22.0.2 will be closed for fixes on June 10th
All, If you have backports that you want to get into jfx22u for JavaFX 22.0.2, please do so by Monday, June 10th. Note that approval from one of the project leads is needed as outlined in this message [1]. Thanks. -- Kevin [1] https://mail.openjdk.org/pipermail/openjfx-dev/2024-January/044659.html
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v9]
On Wed, 22 May 2024 15:50:46 GMT, Florian Kirmaier wrote: >> As seen in the unit test of the PR, when we click on the area above/below >> the scrollbar the position jumps - but the jump is now not always consistent. >> In the current version on the last cell - the UI always jumps to the top. In >> the other cases, the assumed default cell height is used. >> >> With this PR, always the default cell height is used, to determine how much >> is scrolled. >> This makes the behavior more consistent. >> >> Especially from the unit-test, it's clear that with this PR the behavior is >> much more consistent. >> >> This is also related to the following PR: >> https://github.com/openjdk/jfx/pull/1194 > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8323511: Adjust javadoc of VirtualFlow.getViewportLength() Marked as reviewed by angorya (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1326#pullrequestreview-2077078270
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Fri, 24 May 2024 12:12:27 GMT, Kevin Rushforth wrote: > > > I wonder if we may want to add some tests for the `FixedCapacitySet`? > > > > > > Yeah, now that it is more likely that this will make it into FX, I will add > > a small set of unit tests for this class. > > Since this PR is ready to integrate, I think it would be fine to file a new > test bug for the additional tests if you like. If you prefer to add the new > tests now, that's fine, too (we can re-review it). I'm fine integrating this as-is and adding a test soon after. I will leave this over the weekend to give others time to review. Also some clarification on the contributing rules: "all Reviewers who have requested the chance to review have done so" -- does the indication at the top right of the PR count towards this or should it be a comment? :) In the first case, @nlisker and @arapte, please indicate if you wish to review this still. - PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2129527739
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v9]
On Wed, 22 May 2024 15:50:46 GMT, Florian Kirmaier wrote: >> As seen in the unit test of the PR, when we click on the area above/below >> the scrollbar the position jumps - but the jump is now not always consistent. >> In the current version on the last cell - the UI always jumps to the top. In >> the other cases, the assumed default cell height is used. >> >> With this PR, always the default cell height is used, to determine how much >> is scrolled. >> This makes the behavior more consistent. >> >> Especially from the unit-test, it's clear that with this PR the behavior is >> much more consistent. >> >> This is also related to the following PR: >> https://github.com/openjdk/jfx/pull/1194 > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8323511: Adjust javadoc of VirtualFlow.getViewportLength() Marked as reviewed by jvos (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1326#pullrequestreview-2076803074
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v9]
On Wed, 22 May 2024 15:50:46 GMT, Florian Kirmaier wrote: >> As seen in the unit test of the PR, when we click on the area above/below >> the scrollbar the position jumps - but the jump is now not always consistent. >> In the current version on the last cell - the UI always jumps to the top. In >> the other cases, the assumed default cell height is used. >> >> With this PR, always the default cell height is used, to determine how much >> is scrolled. >> This makes the behavior more consistent. >> >> Especially from the unit-test, it's clear that with this PR the behavior is >> much more consistent. >> >> This is also related to the following PR: >> https://github.com/openjdk/jfx/pull/1194 > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8323511: Adjust javadoc of VirtualFlow.getViewportLength() The latest version looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1326#pullrequestreview-2076800098
Re: RFR: 8332863: Crash in JPEG decoder if we enable MEM_STATS
On Fri, 24 May 2024 06:48:50 GMT, Jayathirth D V wrote: > In IJG library's jmemmgr.c file we can define MEM_STATS(by default this flag > is not defined and we don't see any issue) to enable printing of memory > statistics log. But if we enable it, we get crash while disposing IJG stored > objects in jmemmgr->free-pool() function. > > > # > # A fatal error has been detected by the Java Runtime Environment: > # > # SIGSEGV (0xb) at pc=0x0001269d5164, pid=47784, tid=259 > # > # JRE version: Java(TM) SE Runtime Environment (21.0+35) (build > 21+35-LTS-2513) > # Java VM: Java HotSpot(TM) 64-Bit Server VM (21+35-LTS-2513, mixed mode, > sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64) > # Problematic frame: > # C [libjavafx_iio.dylib+0x49164] free_pool+0x88 > # > # No core dump will be written. Core dumps have been disabled. To enable core > dumping, try "ulimit -c unlimited" before starting Java again > # > # If you would like to submit a bug report, please visit: > # https://bugreport.java.com/bugreport/crash.jsp > # The crash happened outside the Java Virtual Machine in native code. > # See problematic frame for where to report the bug. > > --- T H R E A D --- > > Current thread (0x000121a42c00): JavaThread "JavaFX Application Thread" > [_thread_in_native, id=259, stack(0x00016d11c000,0x00016d918000) > (8176K)] > > Stack: [0x00016d11c000,0x00016d918000], sp=0x00016d912780, free > space=8153k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libjavafx_iio.dylib+0x49164] free_pool+0x88 > C [libjavafx_iio.dylib+0x49410] self_destruct+0x3c > C [libjavafx_iio.dylib+0xe888] jpeg_destroy+0x3c > C [libjavafx_iio.dylib+0x4bb1c] imageio_dispose+0x98 > C [libjavafx_iio.dylib+0x4b178] disposeIIO+0x2c > C [libjavafx_iio.dylib+0x4b140] > Java_com_sun_javafx_iio_jpeg_JPEGImageLoader_disposeNative+0x2c > > > This is happening because we delete the error handler before we actually > start deleting IJG stored objects and while freeing the IJG objects we try to > access cinfo->err->trace_level of error handler. This early deletion of error > handler is happening in jpegloader.c->imageio_dispose() function. > > I have moved deletion of error handler logic after we destroy IJG stored > objects in jpegloader.c->imageio_dispose(). This resolves this issue. > There is no regression test case because we need to enable MEM_STATS flag to > see this issue. > Ran graphics unit tests also and i don't see any issues with this change. Reviewer: @arapte - PR Comment: https://git.openjdk.org/jfx/pull/1463#issuecomment-2129398988
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Fri, 24 May 2024 08:22:28 GMT, John Hendrikx wrote: > > I wonder if we may want to add some tests for the `FixedCapacitySet`? > > Yeah, now that it is more likely that this will make it into FX, I will add a > small set of unit tests for this class. Since this PR is ready to integrate, I think it would be fine to file a new test bug for the additional tests if you like. If you prefer to add the new tests now, that's fine, too (we can re-review it). - PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2129385052
Re: RFR: 8311895: CSS Transitions [v16]
On Fri, 24 May 2024 10:15:37 GMT, drmarmac wrote: > * Attempting to do background-color transitions doesn't work, I presume > because it's not yet `Interpolatable` in this PR. Do we need to emit a > warning in such a case? Right now the `transition` CSS code just seems to be > ignored. CSS transitions work with styleable properties that are either primitives or `Interpolatable` objects. So if you apply a transition to the sub-property `-fx-background-color`, what really happens is that the CSS subsystem creates a new `Background` instance and applies it to the `Region.background` property. However, since `Background` is not interpolatable, you won't see an animation. This will start to work once all relevant classes are interpolatable, which will come with a subsequent PR. I'm not sure if we really need a warning. The unexpected non-animation is really only due to a few missing interpolatable classes, which will be fixed soon. > * Same with the current limitation of not being able to mix shorthand and > longhand notations, do we need a warning if it is attempted? This is out of scope for this PR, since the problem is not unique to the CSS `transition` property. We should improve the CSS parser to handle shorthand/longhand notations uniformly. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2129318991
Re: RFR: 8311895: CSS Transitions [v16]
On Fri, 24 May 2024 10:02:44 GMT, drmarmac wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 53 commits:
>>
>> - Merge branch 'master' into feature/css-transitions
>> - update 'since' tags
>> - Fix javadoc error
>> - Change javadoc comment
>> - Merge branch 'master' into feature/css-transitions
>> - Discard redundant transitions in StyleableProperty impls
>> - Changes per review
>> - Merge branch 'master' into feature/css-transitions
>> - Merge branch 'master' into feature/css-transitions
>> - Add TransitionMediator
>> - ... and 43 more: https://git.openjdk.org/jfx/compare/aa9907a5...6614abb9
>
> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line
> 291:
>
>> 289: * All rise points are within the open interval (0..1).
>> 290: */
>> 291: BOTH,
>
> Looks like the docs for BOTH and NONE are swapped? (this would also affect
> the CSR)
Good catch! I've also updated the CSR.
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9081:
>
>> 9079: TransitionDefinition transition = get(i);
>> 9080:
>> 9081: boolean selected =
>> "all".equals(transition.propertyName())
>
> Is "all" the same as `TransitionDefinitionCssMetaData.PROPERTY_ALL`, so the
> magic string can be avoided?
I've replaced `"all"` with a named constant in all relevant places.
> modules/javafx.graphics/src/test/java/test/javafx/scene/Node_transition_Test.java
> line 162:
>
>> 160: List transitions =
>> NodeShim.getTransitionDefinitions(node);
>> 161: assertEquals(3, transitions.size());
>> 162: assertTransitionEquals("-fx-background-color",
>> Duration.seconds(1), Duration.seconds(0.5), CSS_EASE, transitions.get(0));
>
> `node` is a `Rectangle`, which doesn't have a `background`property. Doesn't
> hurt this specific test, but maybe it's better to use an existing property.
I've changed the test to use `-fx-fill` instead.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613307416
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613307906
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613308024
Re: RFR: 8311895: CSS Transitions [v17]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Future enhancements
> CSS transitions requires all participating objects to implement the
> `Interpolatable` interface. For example, targeting `-fx-background-color`
> only works if all background-related objects are interpolatable: `Color`,
> `BackgroundFill`, and `Background`.
>
> In a follow-up PR, the following types will implement the `Interpolatable`
> interface:
> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 57 commits:
- Merge branch 'refs/heads/master' into feature/css-transitions
- extract magic string to named constant
- use existing property in test
- fixed documentation
- Merge branch 'master' into feature/css-transitions
- update 'since' tags
- Fix javadoc error
- Change javadoc comment
- Merge branch 'master' into feature/css-transitions
- Discard redundant transitions in StyleableProperty impls
- ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx=870=16
Stats: 4677 lines in 43 files changed: 4634 ins; 4 del; 39 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v16]
On Thu, 2 May 2024 08:40:28 GMT, Michael Strauß wrote:
>> Implementation of [CSS
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>>
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the
>> `Interpolatable` interface. For example, targeting `-fx-background-color`
>> only works if all background-related objects are interpolatable: `Color`,
>> `BackgroundFill`, and `Background`.
>>
>> In a follow-up PR, the following types will implement the `Interpolatable`
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>>
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the
>> `transition` property. However, due to limitations of JavaFX CSS, mixing
>> both notations doesn't work:
>>
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>>
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 53 commits:
>
> - Merge branch 'master' into feature/css-transitions
> - update 'since' tags
> - Fix javadoc error
> - Change javadoc comment
> - Merge branch 'master' into feature/css-transitions
> - Discard redundant transitions in StyleableProperty impls
> - Changes per review
> - Merge branch 'master' into feature/css-transitions
> - Merge branch 'master' into feature/css-transitions
> - Add TransitionMediator
> - ... and 43 more: https://git.openjdk.org/jfx/compare/aa9907a5...6614abb9
I did some testing of this PR and started looking into the implementation (see
code comments).
- Fade in / fade out with delay and various easing functions via the opacity
property works as expected, , scaleX/scaleY also look good
- Attempting to do background-color transitions doesn't work, I presume because
it's not yet `Interpolatable` in this PR. Do we need to emit a warning in such
a case? Right now the `transition` CSS code just seems to be ignored.
- Same with the current limitation of not being able to mix shorthand and
longhand notations, do we need a warning if it is attempted?
- Generally this looks like a high quality PR, filling a long-standing gap.
Currently available alternative solutions (e.g. using code-driven animations)
are quite cumbersome and not easy to get right.
modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line
291:
> 289: * All rise points are within the open interval (0..1).
> 290: */
> 291: BOTH,
Looks like the docs for BOTH and NONE are swapped? (this would also affect the
CSR)
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9081:
> 9079: TransitionDefinition transition = get(i);
> 9080:
> 9081: boolean selected =
> "all".equals(transition.propertyName())
Is "all" the same as `TransitionDefinitionCssMetaData.PROPERTY_ALL`, so the
magic string can be avoided?
modules/javafx.graphics/src/test/java/test/javafx/scene/Node_transition_Test.java
line 162:
> 160: List transitions =
> NodeShim.getTransitionDefinitions(node);
> 161: assertEquals(3, transitions.size());
> 162: assertTransitionEquals("-fx-background-color",
> Duration.seconds(1), Duration.seconds(0.5), CSS_EASE, transitions.get(0));
`node` is a `Rectangle`, which doesn't have a `background`property. Doesn't
hurt this specific test, but maybe it's better to use an existing property.
-
PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2076453636
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613220507
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613227193
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613233729
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v3]
On Thu, 23 May 2024 10:53:36 GMT, Thiago Milczarek Sayao wrote: >> This fixes two bugs appointed on the JBS issue: >> >> 1) Sometimes window was moving to the top left corner - seems to be a bug >> somewhere in `gdk_window_get_origin` when used before map (a X concept when >> the window appears). The change is to ignore the configure events (happens >> when location or size changes) until window is mapped. Before map java is >> notified in the `set_bounds` function. >> >> This seems to happen on newer versions of linux distros. >> >> 2) Specific to KDE, in the case of the example provided, when an MODAL >> window pops, it calls `set_enabled` `false` on the child (or all other >> windows if APPLICATION_MODAL) which causes it to update the window >> constraints. When maximized, the constraints where applied anyways, causing >> the window to still be maximized but not show as maximized. The change is to >> not apply constraints when not floating (meaning floating on the screen - >> not maximized, fullscreen or iconified). > > Thiago Milczarek Sayao has updated the pull request with a new target base > due to a merge or a rebase. The incremental webrev excludes the unrelated > changes brought in by the merge/rebase. The pull request contains seven > additional commits since the last revision: > > - Merge branch 'refs/heads/master' into 833 > - Should still report location > - Fix > - Teste > - Teste > - Teste > - Fix 833 I can confirm that this now fixes the issue in Ubuntu with KDE desktop environment. Both [JDK-833](https://bugs.openjdk.org/browse/JDK-833) and [JDK-8332352](https://bugs.openjdk.org/browse/JDK-8332352) are reporting the same issue. So I will close the latter as duplicate. - PR Comment: https://git.openjdk.org/jfx/pull/1460#issuecomment-2129085714
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Thu, 23 May 2024 22:54:15 GMT, Marius Hanl wrote: > Code looks good to me. As mentioned above, I tested it already and everything > was good, so I'm certain that there is no regression here. > > I'm generally not a big fan of 'reimplementing' Collections, but I can see > why it is needed here and it also makes sense, especially for something like > CSS which needs to be fast. Something I saw as well in Swing (just check the > `ArrayTable` class ). I'm not a fan either, especially because custom collection implementations when referred to by their interface may sneak their way through the various classes and eventually be returned to users. Any deviation from the collection contracts then suddenly is a bug users may encounter. I would have done this with plain `HashSet` (or `Set.of`) were it not for the fact that a lot of performance gains were possible due to the limited way FX is using these sets. > I wonder if we may want to add some tests for the `FixedCapacitySet`? Yeah, now that it is more likely that this will make it into FX, I will add a small set of unit tests for this class. - PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2128894297
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Thu, 23 May 2024 22:44:08 GMT, Marius Hanl wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move getStyleClassNames to location it was introduced to reduce diff > > modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 99: > >> 97: /* >> 98: * Copied from removed StyleClassSet to give StyleClasses a fixed >> index when >> 99: * first encountered. No longer needed once StyleClass is removed. > > Is this a possible idea for the future? Yes, once this class is moved to private API (see https://github.com/openjdk/jfx/pull/1333) then this method can be removed. The `SimpleSelector` class is already deprecated for removal since 23. The method is almost impossible to reach (you'd have to cast to `SimpleSelector`), but removing it as part of this PR would have required a CSR making this harder to integrate. - PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1613039200
RFR: 8332863: Crash in JPEG decoder if we enable MEM_STATS
In IJG library's jmemmgr.c file we can define MEM_STATS(by default this flag is not defined and we don't see any issue) to enable printing of memory statistics log. But if we enable it, we get crash while disposing IJG stored objects in jmemmgr->free-pool() function. # # A fatal error has been detected by the Java Runtime Environment: # # SIGSEGV (0xb) at pc=0x0001269d5164, pid=47784, tid=259 # # JRE version: Java(TM) SE Runtime Environment (21.0+35) (build 21+35-LTS-2513) # Java VM: Java HotSpot(TM) 64-Bit Server VM (21+35-LTS-2513, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64) # Problematic frame: # C [libjavafx_iio.dylib+0x49164] free_pool+0x88 # # No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again # # If you would like to submit a bug report, please visit: # https://bugreport.java.com/bugreport/crash.jsp # The crash happened outside the Java Virtual Machine in native code. # See problematic frame for where to report the bug. --- T H R E A D --- Current thread (0x000121a42c00): JavaThread "JavaFX Application Thread" [_thread_in_native, id=259, stack(0x00016d11c000,0x00016d918000) (8176K)] Stack: [0x00016d11c000,0x00016d918000], sp=0x00016d912780, free space=8153k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) C [libjavafx_iio.dylib+0x49164] free_pool+0x88 C [libjavafx_iio.dylib+0x49410] self_destruct+0x3c C [libjavafx_iio.dylib+0xe888] jpeg_destroy+0x3c C [libjavafx_iio.dylib+0x4bb1c] imageio_dispose+0x98 C [libjavafx_iio.dylib+0x4b178] disposeIIO+0x2c C [libjavafx_iio.dylib+0x4b140] Java_com_sun_javafx_iio_jpeg_JPEGImageLoader_disposeNative+0x2c This is happening because we delete the error handler before we actually start deleting IJG stored objects and while freeing the IJG objects we try to access cinfo->err->trace_level of error handler. This early deletion of error handler is happening in jpegloader.c->imageio_dispose() function. I have moved deletion of error handler logic after we destroy IJG stored objects in jpegloader.c->imageio_dispose(). This resolves this issue. There is no regression test case because we need to enable MEM_STATS flag to see this issue. Ran graphics unit tests also and i don't see any issues with this change. - Commit messages: - 8332863: Crash in JPEG decoder if we enable MEM_STATS Changes: https://git.openjdk.org/jfx/pull/1463/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1463=00 Issue: https://bugs.openjdk.org/browse/JDK-8332863 Stats: 4 lines in 1 file changed: 2 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1463.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1463/head:pull/1463 PR: https://git.openjdk.org/jfx/pull/1463
