Integrated: JDK-8324337: Cherry-pick WebKit 617.1 stabilization fixes
On Thu, 25 Jan 2024 13:32:21 GMT, Hima Bindu Meda wrote: > Cherry-picked changes related to webkit-2.42.4.Verified build on all > platforms. Sanity testing looks fine. This pull request has now been integrated. Changeset: 52840a17 Author:Hima Bindu Meda URL: https://git.openjdk.org/jfx/commit/52840a17dee6a92fb9b1f3ff1b314b0ae228b95f Stats: 1297 lines in 161 files changed: 752 ins; 111 del; 434 mod 8324337: Cherry-pick WebKit 617.1 stabilization fixes Reviewed-by: kcr, sykora - PR: https://git.openjdk.org/jfx/pull/1350
Re: RFR: JDK-8324337: Cherry-pick WebKit 617.1 stabilization fixes [v3]
On Mon, 29 Jan 2024 04:24:54 GMT, Hima Bindu Meda wrote: >> Cherry-picked changes related to webkit-2.42.4.Verified build on all >> platforms. Sanity testing looks fine. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > space corrections Marked as reviewed by sykora (Author). - PR Review: https://git.openjdk.org/jfx/pull/1350#pullrequestreview-1850425295
Integrated: 8260013: Snapshot does not work for nodes in a subscene
On Fri, 12 Jan 2024 14:11:14 GMT, Lukasz Kostyra wrote: > Originally this issue showed the problem of Node being incorrectly rendered > (clipped) when snapshotting, compared to a snapshot of the whole Scene. Later > on there was another problem added - lights not being taken into account if > they are added to a SubScene. > > As it later turned out, the original problem from this bug report is a > problem with ParallelCamera incorrectly estimating near/far clipping planes, > which just happened to reveal itself while snapshotting a Node. During > testing I found out you can make the Node clip regardless of snapshot > mechanism. Clipping issue was moved to a separate bug report and this PR only > fixes the inconsistency in lights being gathered for a snapshot. > > `Scene.doSnapshot()` was expanded to also check if SubScene provided to it is > non-null and to fetch lights assigned to it. Scenario was tested with added > SnapshotLightsTest. > > Rest of the tests were checked and don't produce any noticeable regressions. This pull request has now been integrated. Changeset: d653a31c Author:Lukasz Kostyra URL: https://git.openjdk.org/jfx/commit/d653a31c1c1fa486f5042424c4f8e7f0c3b21a17 Stats: 181 lines in 5 files changed: 167 ins; 1 del; 13 mod 8260013: Snapshot does not work for nodes in a subscene Reviewed-by: kcr, mstrauss - PR: https://git.openjdk.org/jfx/pull/1332
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v3]
On Mon, 29 Jan 2024 21:29:31 GMT, Laurent Bourgès wrote: >> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > fixed copyright years of modified files + fixed comments in > DMarlinPrismUtils I fixed few comments, ready ? - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1916164340
Re: RFR: 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS [v3]
On Wed, 17 Jan 2024 12:57:22 GMT, Kevin Rushforth wrote: >> As noted in the JBS bug, this is a follow-on to >> [JDK-8196011](https://bugs.openjdk.org/browse/JDK-8196011) that I discovered >> while testing the fix for >> [JDK-8221261](https://bugs.openjdk.org/browse/JDK-8221261) (a deadlock in >> the IME code when using WebView in a JFXPanel on macOS). >> >> I have tested this in connection with with the proposed fix for JDK-8221261, >> although it is a valid fix regardless. >> >> This expands the fix done in >> [JDK-8221261](https://bugs.openjdk.org/browse/JDK-8221261) to call all of >> the WebKit methods on the right thread. Additionally, we sometimes see >> spurious exceptions where the committed text is coming back as null, so I >> changed the log level to "fine" rather than "severe" for those exceptions. >> I'll file a follow-up bug to see if any of these are real problems or not. > > Kevin Rushforth 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 four additional > commits since the last revision: > > - Revert change to log message when exception occurs > - Merge branch 'master' into 8322703-webkit-ime-crash > - Merge remote-tracking branch 'upstream/master' into > 8322703-webkit-ime-crash > - 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on > macOS + looks good to me, tested on my machine - Marked as reviewed by jbhaskar (Committer). PR Review: https://git.openjdk.org/jfx/pull/1321#pullrequestreview-1850284081
Re: RFR: 8260013: Snapshot does not work for nodes in a subscene [v4]
On Mon, 29 Jan 2024 09:32:06 GMT, Lukasz Kostyra wrote: >> Originally this issue showed the problem of Node being incorrectly rendered >> (clipped) when snapshotting, compared to a snapshot of the whole Scene. >> Later on there was another problem added - lights not being taken into >> account if they are added to a SubScene. >> >> As it later turned out, the original problem from this bug report is a >> problem with ParallelCamera incorrectly estimating near/far clipping planes, >> which just happened to reveal itself while snapshotting a Node. During >> testing I found out you can make the Node clip regardless of snapshot >> mechanism. Clipping issue was moved to a separate bug report and this PR >> only fixes the inconsistency in lights being gathered for a snapshot. >> >> `Scene.doSnapshot()` was expanded to also check if SubScene provided to it >> is non-null and to fetch lights assigned to it. Scenario was tested with >> added SnapshotLightsTest. >> >> Rest of the tests were checked and don't produce any noticeable regressions. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Minor review adjustments Looks good! - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1332#pullrequestreview-1850271783
Withdrawn: 8324219: Remove incorrect documentation from Animation methods
On Fri, 19 Jan 2024 16:07:31 GMT, Michael Strauß wrote:
> The `Animation` class states in the documentation of various methods that the
> method call would be asynchronous, using language similar to:
>
>
> {@code stop()} is an asynchronous call, the {@code Animation} may not stop
> immediately.
>
>
> This is factually wrong, there are no asynchronous calls. In fact, the
> methods in question can only be called synchronously on the JavaFX
> Application thread, and the state of the animation is changed immediately.
> For example, when `stop()` is called, the animation transitions to the
> stopped state instantly, without waiting for the next animation pulse.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jfx/pull/1342
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl wrote:
> For some reason the `skinProperty` did not allow to set a new skin when it is
> the same class as the previous one.
> This leads to multiple issues:
> 1. When creating a new skin (same class as previous), the skin will likely
> install listener but is then rejected when setting it due to the
> `skinProperty` class constraint
> -> `PopupControl` is in a weird state as the current skin was not disposed
> since it is still set, although we already created and 'set' a new skin
> 2. A skin of the same class can behave differently, so it is not really valid
> to reject a skin just because it is the same class as the previous
> -> Just imagine we have the following skin class
>
> class MyCustomSkin extends Skin {
> public MyCustomSkin(C skinnable, boolean someFlag) { ... }
> }
>
> Now if we would change the skin of the `PopupControl` two times like this:
>
> popup.setSkin(new MyCustomSkin(popup, true));
> popup.setSkin(new MyCustomSkin(popup, false));
>
> The second time the skin will be rejected as it is the same class, although I
> may changed the skin behaviour, in this case demonstrated by a boolean flag
> for showing purposes.
>
> This is the same issue and fix as done for `Control` in
> [JDK-8276056](https://bugs.openjdk.org/browse/JDK-8276056) (PR:
> https://github.com/openjdk/jfx/pull/806)
Looks good, and works as I expect it.
-
Marked as reviewed by mstrauss (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1331#pullrequestreview-1850261757
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v17]
On Tue, 30 Jan 2024 01:24:52 GMT, Nir Lisker wrote: >> Added a utility method to run code on the FX thread if it's not already, and >> changed the animation methods to use it. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update tests The updated test looks good. I verified that it will correctly fail if [JDK-8159048](https://bugs.openjdk.org/browse/JDK-8159048) is reintroduced, and will also fail without the fix for _this_ issue (that is, with the current jfx master). - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1850112597
Re: RFR: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
On Tue, 30 Jan 2024 00:53:51 GMT, Michael Strauß wrote: > `WinApplication` contains mappings of platform-specific preference keys to > platform-independent keys. > These keys are incorrect (for example, `Windows.UIColor.ForegroundColor` > instead of `Windows.UIColor.Foreground`). > > This was not discovered during testing because the manual test application > doesn't show the values of platform-independent properties (it only shows the > platform-specific mappings). I've exended the test application to also show > the platform-independent property values. Looks good. Test works as expected. This would be a good candidate to backport to jfx22 once integrated into master. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1353#pullrequestreview-1850100727
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v17]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Update tests - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/13281050..8880aa7e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=16 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=15-16 Stats: 25 lines in 3 files changed: 10 ins; 4 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8301900: TextArea: Committing text with ENTER in an IME window inserts newline [v2]
On Tue, 30 Jan 2024 00:57:53 GMT, Martin Fox wrote: > Currently it commits a zero-length string (which appears to be a no-op) and > leaves the composition buffer untouched. Correcting myself: it does clear the composition buffer. But the zero-length commit doesn't seem to be doing what it needs to. - PR Comment: https://git.openjdk.org/jfx/pull/1351#issuecomment-1915878911
Re: RFR: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
On Tue, 30 Jan 2024 00:53:51 GMT, Michael Strauß wrote: > `WinApplication` contains mappings of platform-specific preference keys to > platform-independent keys. > These keys are incorrect (for example, `Windows.UIColor.ForegroundColor` > instead of `Windows.UIColor.Foreground`). > > This was not discovered during testing because the manual test application > doesn't show the values of platform-independent properties (it only shows the > platform-specific mappings). I've exended the test application to also show > the platform-independent property values. I'll review this. @andy-goryachev-oracle do you want to be the second reviewer? - PR Comment: https://git.openjdk.org/jfx/pull/1353#issuecomment-1915873179
Re: RFR: 8301900: TextArea: Committing text with ENTER in an IME window inserts newline [v2]
On Mon, 29 Jan 2024 19:04:10 GMT, Andy Goryachev wrote: >> Martin Fox has updated the pull request incrementally with one additional >> commit since the last revision: >> >> When IM enabled state changes we dismiss the IM window. > > ... and scenarios described in JDK-8088172 and JDK-8089803 seem to have been > fixed. @andy-goryachev-oracle > @beldenfox are you going to update this PR with a fix for dismissing the IME > window you alluded in the previous comment? I did that with the most recent commit (Jan 27th). > * click elsewhere. the IME window disappears That indicates that you got my most recent commit, otherwise the IME window would not disappear immediately. I'm really glad you caught this. The old code for disabling the IME just called `[self unmarkText]`. I added the code for dismissing the IME window but didn't notice that `unmarkText` doesn't seem to work. Currently it commits a zero-length string (which appears to be a no-op) and leaves the composition buffer untouched. That combination doesn't make much sense and doesn't match Apple's spec. I need to dig around in the code history to see why it's behaving this way. This also explains some of the bad behavior you noticed in JDK-8320912. - PR Comment: https://git.openjdk.org/jfx/pull/1351#issuecomment-1915858678
RFR: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
`WinApplication` contains mappings of platform-specific preference keys to platform-independent keys. These keys are incorrect (for example, `Windows.UIColor.ForegroundColor` instead of `Windows.UIColor.Foreground`). This was not discovered during testing because the manual test application doesn't show the values of platform-independent properties (it only shows the platform-specific mappings). I've exended the test application to also show the platform-independent property values. - Commit messages: - fix incorrect key mappings for windows Changes: https://git.openjdk.org/jfx/pull/1353/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1353&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324879 Stats: 40 lines in 3 files changed: 33 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jfx/pull/1353.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1353/head:pull/1353 PR: https://git.openjdk.org/jfx/pull/1353
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 21:11:45 GMT, Nir Lisker wrote: >> tests/system/src/test/java/test/com/sun/javafx/animation/SynchronizationTest.java >> line 80: >> >>> 78: >>> 79: private Thread thread; >>> 80: private Throwable throwable; >> >> Since these can be set from any thread, maybe use AtomicReference? > > Alright, but because of the latch I don't think it will ever matter in > practice. Thanks. I agree that in practice it won't matter. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470431930
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v6]
On Mon, 29 Jan 2024 16:35:05 GMT, Andy Goryachev wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Move test that needs QuantumToolkit to system tests
>
> tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line
> 149:
>
>> 147: @Disabled
>> 148: @Test
>> 149: void complexTestsThatAreBrokenSince2013() {
>
> @kevinrushforth
> re: ancient tests
>
> this is the only skipped test - quite possibly due to wrong expectations
> (about Thai)
>
> Due to relative complexity of the change, I think we need all the tests we
> could get in this PR
I added the ticket number in the `@Disabled`. The ticket however is pretty
useless to keep open I think.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1470427170
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v7]
On Tue, 30 Jan 2024 00:12:52 GMT, John Hendrikx wrote: >> There are a number of tickets open related to text rendering: >> >> https://bugs.openjdk.org/browse/JDK-8314215 >> >> https://bugs.openjdk.org/browse/JDK-8145496 >> >> https://bugs.openjdk.org/browse/JDK-8129014 >> >> They have in common that wrapped text is taking the trailing spaces on each >> wrapped line into account when calculating where to wrap. This looks okay >> for text that is left aligned (as the spaces will be trailing the lines and >> generally aren't a problem, but looks weird with CENTER and RIGHT >> alignments. Even with LEFT alignment there are artifacts of this behavior, >> where a line like `AAA BBB CCC` (note the **double** spaces) gets split up >> into `AAA `, `BBB ` and `CCC`, but if space reduces further, it will wrap >> **too** early because the space is taken into account (ie. `AAA` may still >> have fit just fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A >> ` or something). >> >> The fix for this is two fold; first the individual lines of text should not >> include any trailing spaces into their widths; second, the code that is >> taking the trailing space into account when wrapping should ignore all >> trailing spaces (currently it is ignoring all but one trailing space). With >> these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, >> and there is no more early wrapping due to a space being taking into account >> while the actual text still would have fit (this is annoying in tight >> layouts, where a line can be wrapped early even though it looks like it >> would have fit). >> >> If it were that simple, we'd be done, but there may be another issue here >> that needs solving: wrapped aligned TextArea's. >> >> TextArea don't directly support text alignment (via a setTextAlignment >> method like Label) but you can change it via CSS. >> >> For Left alignment + wrapping, TextArea will ignore any spaces typed before >> a line that was wrapped. In other words, you can type spaces as much as you >> want, and they won't show up and the cursor won't move. The spaces are all >> getting appended to the previous line. When you cursor through these >> spaces, the cursor can be rendered out of the control's bounds. To >> illustrate, if you have the text `AAA BBB CCC`, and the text >> gets wrapped to `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not >> show up. If you cursor back, the cursor may be outside the control bounds >> because so many spaces are trailing `AAA`. >> >> The above behavior has NOT changed, is pretty standard for wrapped text >> controls,... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix bug which confused char index with glyph index I think that it may be wise to do some clean-up of the code surrounding `TextRun` in some future PR. It's a mish-mash of things currently: - TextRun seems to be used in two ways. - The "normal" way where multiple runs are constructed from a character array, and it retains references to the start/end in that array (although not a reference to the array itself). Because it has no reference to the chars from which it was created, a lot of functionality which could be encapsulated is externalized, and many internals of `TextRun` need to be exposed to feed those external functions. - For javafx-web, which just wants to render a bunch of glyphs, unrelated to any text (so there is no character array it is based on, and its start/end values are bogus) - There is a lot of code that pretends there is a difference between a `GlyphList` (clearly intended for rendering pure glyphs only) and `TextRun`, but there is only one implementation of `GlyphList` (`TextRun`). The code that does the actual rendering in `NGText` bluntly always casts a `GlyphList` to a `TextRun`. It does this for the sole purpose of finding out the start character position of the original characters the `TextRun` was created from (needed for selection rendering), yet, for `TextRun`s created by javafx-web this is irrelevant (it just always returns `0` for the start character position). - It would have been better to do a conditional cast to `TextRun` in `NGText` so that javafx-web can use a much simpler implementation of `GlyphList` not based on `TextRun`. - In a lot of places in this code and surrounding code, fields have been failed to be marked `private` or `final` even though they can be; this may have implications for what JIT can do. - There are unused fields (`isCanonical` always returns `false`, `TextLine` is only stored to get its `RectBounds`, could just store that directly, it's `final`) A clean-up would entail: - Include a reference to the characters it was created from in `TextRun` - Encapsulate a lot of code into `TextRun` (moved from `PrismTextLayout`) that does calculations tha
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v7]
> There are a number of tickets open related to text rendering: > > https://bugs.openjdk.org/browse/JDK-8314215 > > https://bugs.openjdk.org/browse/JDK-8145496 > > https://bugs.openjdk.org/browse/JDK-8129014 > > They have in common that wrapped text is taking the trailing spaces on each > wrapped line into account when calculating where to wrap. This looks okay > for text that is left aligned (as the spaces will be trailing the lines and > generally aren't a problem, but looks weird with CENTER and RIGHT alignments. > Even with LEFT alignment there are artifacts of this behavior, where a line > like `AAA BBB CCC` (note the **double** spaces) gets split up into `AAA `, > `BBB ` and `CCC`, but if space reduces further, it will wrap **too** early > because the space is taken into account (ie. `AAA` may still have fit just > fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A ` or > something). > > The fix for this is two fold; first the individual lines of text should not > include any trailing spaces into their widths; second, the code that is > taking the trailing space into account when wrapping should ignore all > trailing spaces (currently it is ignoring all but one trailing space). With > these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, > and there is no more early wrapping due to a space being taking into account > while the actual text still would have fit (this is annoying in tight > layouts, where a line can be wrapped early even though it looks like it would > have fit). > > If it were that simple, we'd be done, but there may be another issue here > that needs solving: wrapped aligned TextArea's. > > TextArea don't directly support text alignment (via a setTextAlignment method > like Label) but you can change it via CSS. > > For Left alignment + wrapping, TextArea will ignore any spaces typed before a > line that was wrapped. In other words, you can type spaces as much as you > want, and they won't show up and the cursor won't move. The spaces are all > getting appended to the previous line. When you cursor through these spaces, > the cursor can be rendered out of the control's bounds. To illustrate, if > you have the text `AAA BBB CCC`, and the text gets wrapped to > `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not show up. If you > cursor back, the cursor may be outside the control bounds because so many > spaces are trailing `AAA`. > > The above behavior has NOT changed, is pretty standard for wrapped text > controls, and IMHO does not need further attent... John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Fix bug which confused char index with glyph index - Changes: - all: https://git.openjdk.org/jfx/pull/1236/files - new: https://git.openjdk.org/jfx/pull/1236/files/8193db0d..93be3f48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=06 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=05-06 Stats: 134 lines in 2 files changed: 118 ins; 5 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1236.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1236/head:pull/1236 PR: https://git.openjdk.org/jfx/pull/1236
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 22:13:25 GMT, Kevin Rushforth wrote: >> I don't think the executor service catches the exception because if it did >> then the `AnimationTimer` test would also always pass. I do get the AIOOB >> exceptions from the background thread caught in the handler (which fails the >> test). This test is rather stable for me. The `Animation` one is more >> problematic, sometimes failing and sometimes not when it should fail. > > Based on the testing I did, the executor service does prevent at least some > exceptions that happen on the test thread from reaching the uncaught > exception handler. An explicit try/catch will catch those. Alright, will implement the change. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470335752
Re: RFR: 8260013: Snapshot does not work for nodes in a subscene [v4]
On Mon, 29 Jan 2024 09:32:06 GMT, Lukasz Kostyra wrote: >> Originally this issue showed the problem of Node being incorrectly rendered >> (clipped) when snapshotting, compared to a snapshot of the whole Scene. >> Later on there was another problem added - lights not being taken into >> account if they are added to a SubScene. >> >> As it later turned out, the original problem from this bug report is a >> problem with ParallelCamera incorrectly estimating near/far clipping planes, >> which just happened to reveal itself while snapshotting a Node. During >> testing I found out you can make the Node clip regardless of snapshot >> mechanism. Clipping issue was moved to a separate bug report and this PR >> only fixes the inconsistency in lights being gathered for a snapshot. >> >> `Scene.doSnapshot()` was expanded to also check if SubScene provided to it >> is non-null and to fetch lights assigned to it. Scenario was tested with >> added SnapshotLightsTest. >> >> Rest of the tests were checked and don't produce any noticeable regressions. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Minor review adjustments Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1332#pullrequestreview-1849876151
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 21:35:24 GMT, Nir Lisker wrote:
>> tests/system/src/test/java/test/com/sun/javafx/animation/SynchronizationTest.java
>> line 87:
>>
>>> 85: for (int i = 0; i < 10; i++) {
>>> 86: executor.submit(runnable);
>>> 87: }
>>
>> I did a quick prototype of the idea I mentioned in my last comment, and it
>> looks like it works:
>>
>>
>> Runnable wrappedRunnable = () -> {
>> try {
>> runnable.run();
>> } catch (Throwable e) {
>> thread = Thread.currentThread();
>> throwable = e;
>> failed.set(true);
>> waiter.countDown();
>> }
>> };
>>
>> for (int i = 0; i < 10; i++) {
>> executor.submit(wrappedRunnable);
>> }
>>
>>
>> This obviates the need for the `registerExceptionHandler` calls in the two
>> tests themselves (this one in _this_ method is still needed).
>
> I don't think the executor service catches the exception because if it did
> then the `AnimationTimer` test would also always pass. I do get the AIOOB
> exceptions from the background thread caught in the handler (which fails the
> test). This test is rather stable for me. The `Animation` one is more
> problematic, sometimes failing and sometimes not when it should fail.
Based on the testing I did, the executor service does prevent at least some
exceptions that happen on the test thread from reaching the uncaught exception
handler. An explicit try/catch will catch those.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470290986
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 20:49:48 GMT, Nir Lisker wrote: >> tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java >> line 2: >> >>> 1: /* >>> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. >> >> Copyright header should keep 2023, even if it is almost completely changed > > Even if it wasn't released? Copyrights in source code aren't about whether it is released, so probably best in this case to keep the 2023. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470284783
Re: Platform preferences theme detection
Are you sure it wasn’t change in some version of Windows 10 or between 10 and
11? I vaguely recall testing light/dark mode switch on Windows 11.
-andy
From: openjfx-dev on behalf of Michael Strauß
Date: Monday, January 29, 2024 at 14:09
To:
Cc: openjfx-dev@openjdk.org
Subject: Re: Platform preferences theme detection
I see that the names of the platform mappings defined in
WinApplication::getPlatformKeyMappings() are simply wrong
("Windows.UIColor.ForegroundColor" instead of "Windows.UIColor.Foreground"), so
the platform mappings are not applied to the properties.
That's quite surprising, and it's a change that must have slipped into the
feature at a very late stage during development, so that it went unnoticed by
all reviewers.
I'll file a bug and prepare a fix for this issue.
On Mon, Jan 29, 2024 at 10:45 PM Christopher Schnick
mailto:crschn...@xpipe.io>> wrote:
Hello Michael,
I took a look at the implementation and tried to find the issue. From what I
can see, the mappings returned are correct:
[cid:ii_18d573d4eaed3eee5091]
but it seems like they are somehow not applied the PreferencesProperties:
[cid:ii_18d573d4eaed3fcfc8a2]
Let me know whether I can help with anything to debug this issue.
Re: Platform preferences theme detection
I see that the names of the platform mappings defined in
WinApplication::getPlatformKeyMappings() are simply wrong
("Windows.UIColor.ForegroundColor" instead of
"Windows.UIColor.Foreground"), so the platform mappings are not applied to
the properties.
That's quite surprising, and it's a change that must have slipped into the
feature at a very late stage during development, so that it went unnoticed
by all reviewers.
I'll file a bug and prepare a fix for this issue.
On Mon, Jan 29, 2024 at 10:45 PM Christopher Schnick
wrote:
> Hello Michael,
>
> I took a look at the implementation and tried to find the issue. From what
> I can see, the mappings returned are correct:
>
>
>
> but it seems like they are somehow not applied the PreferencesProperties:
>
> Let me know whether I can help with anything to debug this issue.
>
>
Re: Platform preferences theme detection
Hello Michael, I took a look at the implementation and tried to find the issue. From what I can see, the mappings returned are correct: but it seems like they are somehow not applied the PreferencesProperties: Let me know whether I can help with anything to debug this issue. On 1/29/2024 10:37 PM, Michael Strauß wrote: Hi Christopher! - Should this feature work in that ea version? Yes. - Is Windows 10 supported by the color scheme detection? Color scheme detection should be supported on Windows 10 beginning with build 10240. - The documentation says that LIGHT is returned in case theme the detection is not supported. But I guess there is no way to find out whether querying that property is supported on the current system? It might be useful in some circumstances to adapt the behavior when we know that this is not supported. There is no way to directly query whether any of the platform-independent properties (colorScheme, backgroundColor, foregroundColor, accentColor) are supported by the current OS. You can check whether the platform-dependent mappings (for example, "Windows.UIColor.Background") are contained in the map, which will only be the case if supported by the OS. - The documentation mentions that some preferences might not be available on the current system. Does that mean that if they are available, they are also observable? Or might it be possible that a certain setting is available but does not support dynamically updating at runtime and cannot be observed? If you see a mapping for a preference, it should also be updated automatically if the corresponding OS setting is changed. Do you see the mappings for "Windows.UIColor.Background" and "Windows.UIColor.Foreground" in the map returned by Platform.getPreferences()? The color scheme value is derived from these colors. If the mappings are not present, something unexpected is happening. On Mon, Jan 29, 2024 at 9:23 PM Christopher Schnick wrote: Hello, I just tried out the new 22-ea+27 build to see whether we can utilize some of the new features, particularly the platform preferences API, to replace the libraryhttps://github.com/Dansoftowner/jSystemThemeDetector that we are currently using for theme detection. I'm running this on Windows 10 with the latest updates and the color scheme always reports LIGHT, even when dark mode is enabled in the settings. The observable value also does not update when the value changes in the settings. Obviously I have a few questions here: - Should this feature work in that ea version? - Is Windows 10 supported by the color scheme detection? - The documentation says that LIGHT is returned in case theme the detection is not supported. But I guess there is no way to find out whether querying that property is supported on the current system? It might be useful in some circumstances to adapt the behavior when we know that this is not supported. - The documentation mentions that some preferences might not be available on the current system. Does that mean that if they are available, they are also observable? Or might it be possible that a certain setting is available but does not support dynamically updating at runtime and cannot be observed? Best regards Christopher
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 19:00:46 GMT, Kevin Rushforth wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update tests
>
> tests/system/src/test/java/test/com/sun/javafx/animation/SynchronizationTest.java
> line 87:
>
>> 85: for (int i = 0; i < 10; i++) {
>> 86: executor.submit(runnable);
>> 87: }
>
> I did a quick prototype of the idea I mentioned in my last comment, and it
> looks like it works:
>
>
> Runnable wrappedRunnable = () -> {
> try {
> runnable.run();
> } catch (Throwable e) {
> thread = Thread.currentThread();
> throwable = e;
> failed.set(true);
> waiter.countDown();
> }
> };
>
> for (int i = 0; i < 10; i++) {
> executor.submit(wrappedRunnable);
> }
>
>
> This obviates the need for the `registerExceptionHandler` calls in the two
> tests themselves (this one in _this_ method is still needed).
I don't think the executor service catches the exception because if it did then
the `AnimationTimer` test would also always pass. I do get the AIOOB exceptions
from the background thread caught in the handler (which fails the test). This
test is rather stable for me. The `Animation` one is more problematic,
sometimes failing and sometimes not when it should fail.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470247519
Re: Platform preferences theme detection
Hi Christopher! > - Should this feature work in that ea version? Yes. > - Is Windows 10 supported by the color scheme detection? Color scheme detection should be supported on Windows 10 beginning with build 10240. > - The documentation says that LIGHT is returned in case theme the > detection is not supported. But I guess there is no way to find out > whether querying that property is supported on the current system? It > might be useful in some circumstances to adapt the behavior when we know > that this is not supported. There is no way to directly query whether any of the platform-independent properties (colorScheme, backgroundColor, foregroundColor, accentColor) are supported by the current OS. You can check whether the platform-dependent mappings (for example, "Windows.UIColor.Background") are contained in the map, which will only be the case if supported by the OS. > - The documentation mentions that some preferences might not be > available on the current system. Does that mean that if they are > available, they are also observable? Or might it be possible that a > certain setting is available but does not support dynamically updating > at runtime and cannot be observed? If you see a mapping for a preference, it should also be updated automatically if the corresponding OS setting is changed. Do you see the mappings for "Windows.UIColor.Background" and "Windows.UIColor.Foreground" in the map returned by Platform.getPreferences()? The color scheme value is derived from these colors. If the mappings are not present, something unexpected is happening. On Mon, Jan 29, 2024 at 9:23 PM Christopher Schnick wrote: > > Hello, > > I just tried out the new 22-ea+27 build to see whether we can utilize > some of the new features, particularly the platform preferences API, to > replace the library https://github.com/Dansoftowner/jSystemThemeDetector > that we are currently using for theme detection. > > I'm running this on Windows 10 with the latest updates and the color > scheme always reports LIGHT, even when dark mode is enabled in the > settings. The observable value also does not update when the value > changes in the settings. Obviously I have a few questions here: > - Should this feature work in that ea version? > - Is Windows 10 supported by the color scheme detection? > - The documentation says that LIGHT is returned in case theme the > detection is not supported. But I guess there is no way to find out > whether querying that property is supported on the current system? It > might be useful in some circumstances to adapt the behavior when we know > that this is not supported. > - The documentation mentions that some preferences might not be > available on the current system. Does that mean that if they are > available, they are also observable? Or might it be possible that a > certain setting is available but does not support dynamically updating > at runtime and cannot be observed? > > Best regards > Christopher >
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v3]
> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java Laurent Bourgès has updated the pull request incrementally with one additional commit since the last revision: fixed copyright years of modified files + fixed comments in DMarlinPrismUtils - Changes: - all: https://git.openjdk.org/jfx/pull/1348/files - new: https://git.openjdk.org/jfx/pull/1348/files/251d1d61..19189e07 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1348&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1348&range=01-02 Stats: 16 lines in 10 files changed: 0 ins; 4 del; 12 mod Patch: https://git.openjdk.org/jfx/pull/1348.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1348/head:pull/1348 PR: https://git.openjdk.org/jfx/pull/1348
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v16]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Use AtomicReferences - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/789b0f2f..13281050 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=15 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=14-15 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 19:05:46 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update tests > > tests/system/src/test/java/test/com/sun/javafx/animation/SynchronizationTest.java > line 80: > >> 78: >> 79: private Thread thread; >> 80: private Throwable throwable; > > Since these can be set from any thread, maybe use AtomicReference? Alright, but because of the latch I don't think it will ever matter in practice. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470219938
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v15]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with two additional commits since the last revision: - Fix removed methods - Correct methods in docs - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/b3bd557e..789b0f2f Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=14 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=13-14 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 19:22:17 GMT, Jose Pereda wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update tests > > modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line > 989: > >> 987: * This method must be run on the JavaFX Application Thread. >> 988: * >> 989: * @see #playFromStartImpl() > > Do you mean `playFromStart()`? Yes, there are copy-paste mistakes. > tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. > > Copyright header should keep 2023, even if it is almost completely changed Even if it wasn't released? - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470191833 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470192960
Platform preferences theme detection
Hello, I just tried out the new 22-ea+27 build to see whether we can utilize some of the new features, particularly the platform preferences API, to replace the library https://github.com/Dansoftowner/jSystemThemeDetector that we are currently using for theme detection. I'm running this on Windows 10 with the latest updates and the color scheme always reports LIGHT, even when dark mode is enabled in the settings. The observable value also does not update when the value changes in the settings. Obviously I have a few questions here: - Should this feature work in that ea version? - Is Windows 10 supported by the color scheme detection? - The documentation says that LIGHT is returned in case theme the detection is not supported. But I guess there is no way to find out whether querying that property is supported on the current system? It might be useful in some circumstances to adapt the behavior when we know that this is not supported. - The documentation mentions that some preferences might not be available on the current system. Does that mean that if they are available, they are also observable? Or might it be possible that a certain setting is available but does not support dynamically updating at runtime and cannot be observed? Best regards Christopher
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 16:42:15 GMT, Nir Lisker wrote: >> Added a utility method to run code on the FX thread if it's not already, and >> changed the animation methods to use it. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update tests modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 909: > 907: * This method must be run on the JavaFX Application Thread. > 908: * > 909: * @see #playFromStartImpl(String) there is no such symbol. Do you mean `playFrom(String)`? modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 953: > 951: * This method must be run on the JavaFX Application Thread. > 952: * > 953: * @see #playFromStartImpl(Duration) there is no such symbol. Do you mean `playFrom(Duration)`? modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 989: > 987: * This method must be run on the JavaFX Application Thread. > 988: * > 989: * @see #playFromStartImpl() Do you mean `playFromStart()`? tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. Copyright header should keep 2023, even if it is almost completely changed tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. Same, copyright header should keep 2023, even if it is almost completely changed - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470098558 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470099369 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470090981 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470088051 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470088333
Re: RFR: 8089373: Translation from character to key code is not sufficient
On Fri, 17 Nov 2023 20:05:09 GMT, Martin Fox wrote: > On Windows a common shortcut like Ctrl+'+' could only be invoked from the > main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not > have enough context to know whether it should return a result from the main > keyboard or the keypad. > > This PR alters getKeyCodeForChar to pass in the code of the key the system is > trying to match against. Only the Windows platform actually uses this > additional information. > > On the Mac the numeric keypad has always worked due to the odd way > getKeyCodeForChar is implemented (until PR #1209 the keypad worked more > reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; > neither the main keyboard or the numeric keypad work reliably. I have an > upcoming PR which should make both work correctly. will try to review this this week... - PR Comment: https://git.openjdk.org/jfx/pull/1289#issuecomment-1915382478
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 16:42:15 GMT, Nir Lisker wrote: >> Added a utility method to run code on the FX thread if it's not already, and >> changed the animation methods to use it. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update tests tests/system/src/test/java/test/com/sun/javafx/animation/SynchronizationTest.java line 80: > 78: > 79: private Thread thread; > 80: private Throwable throwable; Since these can be set from any thread, maybe use AtomicReference? - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470071779
Re: RFR: 8301900: TextArea: Committing text with ENTER in an IME window inserts newline [v2]
On Sat, 27 Jan 2024 19:19:48 GMT, Martin Fox wrote:
>> In the Mac glass code the presence of "marked" text (which is tracked in the
>> nsAttrBuffer) signals that an IME is active. In this state the current code
>> assumes that when NSTextInputContext handles a `keyDown:` it will either
>> generate a call to `insertText:replacementRange:` or one of the routines
>> that manipulates the marked (composed) text. But this bug shows that
>> sometimes the IME acts on the event without generating any calls back into
>> glass at all.
>>
>> In this PR the logic is simplified: if the NSTextInputContext handles the
>> `keyDown:` and there's marked (composed) text we don't generate a KeyEvent.
>> Otherwise we do. This PR removes the `shouldProcessKeyEvent` flag since it
>> no longer assumes we can catch callbacks to update it correctly.
>>
>> The existing code also assumes that the composition phase ends when the
>> NSTextInputContext calls `insertText:replacementRange` to commit the text.
>> This is true but if the user tries to use a dead-key sequence to generate a
>> non-existent character (like an accented 'q') the context will call
>> `insertText` twice while handling one key down event. In that case we can't
>> exit the composition mode upon seeing the first `insertText` call since it
>> will cause us to mis-handle the second one. This PR defers exiting
>> composition mode until the end of `keyDown:`.
>>
>> I also updated a few deprecated constants so this file no longer generates
>> compiler warnings.
>
> Martin Fox has updated the pull request incrementally with one additional
> commit since the last revision:
>
> When IM enabled state changes we dismiss the IM window.
... and scenarios described in JDK-8088172 and JDK-8089803 seem to have been
fixed.
modules/javafx.graphics/src/main/native-glass/mac/GlassView3D.m line 522:
> 520: nsAttrBuffer = [nsAttrBuffer initWithString: @""];
> 521: }
> 522: else if (!inputContextHandledEvent || (nsAttrBuffer.length == 0)) {
`} else if {` ?
-
PR Comment: https://git.openjdk.org/jfx/pull/1351#issuecomment-1915376186
PR Review Comment: https://git.openjdk.org/jfx/pull/1351#discussion_r1470070946
Re: RFR: 8301900: TextArea: Committing text with ENTER in an IME window inserts newline
On Sat, 27 Jan 2024 19:15:01 GMT, Martin Fox wrote: >> In the Mac glass code the presence of "marked" text (which is tracked in the >> nsAttrBuffer) signals that an IME is active. In this state the current code >> assumes that when NSTextInputContext handles a `keyDown:` it will either >> generate a call to `insertText:replacementRange:` or one of the routines >> that manipulates the marked (composed) text. But this bug shows that >> sometimes the IME acts on the event without generating any calls back into >> glass at all. >> >> In this PR the logic is simplified: if the NSTextInputContext handles the >> `keyDown:` and there's marked (composed) text we don't generate a KeyEvent. >> Otherwise we do. This PR removes the `shouldProcessKeyEvent` flag since it >> no longer assumes we can catch callbacks to update it correctly. >> >> The existing code also assumes that the composition phase ends when the >> NSTextInputContext calls `insertText:replacementRange` to commit the text. >> This is true but if the user tries to use a dead-key sequence to generate a >> non-existent character (like an accented 'q') the context will call >> `insertText` twice while handling one key down event. In that case we can't >> exit the composition mode upon seeing the first `insertText` call since it >> will cause us to mis-handle the second one. This PR defers exiting >> composition mode until the end of `keyDown:`. >> >> I also updated a few deprecated constants so this file no longer generates >> compiler warnings. > > I need to amend this PR. If the IM is disabled in the middle of composition > we don't dismiss the IM window. In the past that was sort of OK because we > ignored the enabled stated and kept sending events to the NSTextInputContext. > With this PR we honor the state and stop talking to the context so the IM > window just freezes and never goes away. I'll be adding a line of code to > ensure we get rid of the IM window when the enabled state changes (which we > should have been doing all along). > > This condition can arise if the user starts composing and then clicks on a > control that doesn't accept IM input (like a button). It can also happen with > [JDK-8087477](https://bugs.openjdk.org/browse/JDK-8087477) which is a bug I'm > still investigating. @beldenfox are you going to update this PR with a fix for dismissing the IME window you alluded in the previous comment? I don't know whether it's the side effect of that scenario, or a new issue, but try this: - using a simple app with a TextArea with pre-existing text (I am using the MonkeyTester, but any simple app would do) - switch to japanese/romaji input - type in "ariga". you'll see the IME window with some candidates (for some reason, the IME window disappears on "ari" - I guess it's expected since the native TextEdit app exhibits the same behavior) - click elsewhere. the IME window disappears PROBLEM 1: the inline candidate remains underlined - switch to German input - click on some other position in the text - type in + q (from JDK-8088172) PROBLEM 2: the text gets inserted into a different position, replacing that underlined Japanese candidate created earlier  - PR Comment: https://git.openjdk.org/jfx/pull/1351#issuecomment-1915373166
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 16:42:15 GMT, Nir Lisker wrote:
>> Added a utility method to run code on the FX thread if it's not already, and
>> changed the animation methods to use it.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update tests
Changes requested by kcr (Lead).
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronizationTest.java
line 87:
> 85: for (int i = 0; i < 10; i++) {
> 86: executor.submit(runnable);
> 87: }
I did a quick prototype of the idea I mentioned in my last comment, and it
looks like it works:
Runnable wrappedRunnable = () -> {
try {
runnable.run();
} catch (Throwable e) {
thread = Thread.currentThread();
throwable = e;
failed.set(true);
waiter.countDown();
}
};
for (int i = 0; i < 10; i++) {
executor.submit(wrappedRunnable);
}
This obviates the need for the `registerExceptionHandler` calls in the two
tests themselves (this one in _this_ method is still needed).
-
PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1849556169
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470065959
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 16:42:15 GMT, Nir Lisker wrote: >> Added a utility method to run code on the FX thread if it's not already, and >> changed the animation methods to use it. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update tests One more thing I tried: I reverted the changes to the animation implementation, going back to the current JavaFX 22 behavior of throwing an exception if start, stop, etc is called on a background thread, and the tests still pass. It looks like the executor service is catching the exception, which is interfering with using an uncaught exception to flag an error. Instead of registering an uncaught exception handler for the test thread, you might wrap the runnable in another runnable with a try / catch and set the "failed" flag, etc., from there? - PR Comment: https://git.openjdk.org/jfx/pull/1352#issuecomment-1915363091
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 16:42:15 GMT, Nir Lisker wrote:
>> Added a utility method to run code on the FX thread if it's not already, and
>> changed the animation methods to use it.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update tests
The updated test looks fine (I did leave one question, but it's not important).
I modified the `runOnFxAppThread` method to always run on the calling thread
(this reintroducing the original problem), and confirmed that
AnimationTimerTest fails (thus catching the bug), but `AnimationTest` still
passes, which matches what you are seeing. That might be OK, since the original
bug report was on AnimationTimer.
tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java
line 48:
> 46: public void handle(long now) {
> 47: // Simulate intensive processing
> 48: Util.sleep(10);
Would this be more likely to catch a problem if this sleep (which is on the FX
thread) were reduced to 1 msec? Not sure, but since the timer is started and
stopped every 10 msec, it might be worth looking at. On the other hand, it does
detect the bug on my Mac, so it might be fine as is.
-
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1849413107
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469993456
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Thu, 25 Jan 2024 17:16:51 GMT, Laurent Bourgès wrote: >> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > fixed test package Our headful CI test run passed. - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1915252859
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v6]
On Mon, 29 Jan 2024 16:22:56 GMT, Andy Goryachev wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move test that needs QuantumToolkit to system tests > > MonkeyTester won't start: > > > Exception in thread "JavaFX Application Thread" > java.lang.ArrayIndexOutOfBoundsException: Index 44 out of bounds for length 38 > at > javafx.graphics/com.sun.javafx.text.TextRun.getAdvance(TextRun.java:362) > at > javafx.graphics/com.sun.javafx.text.PrismTextLayout.computeTrailingSpaceWidth(PrismTextLayout.java:1049) > at > javafx.graphics/com.sun.javafx.text.PrismTextLayout.createLine(PrismTextLayout.java:1017) > at > javafx.graphics/com.sun.javafx.text.PrismTextLayout.layout(PrismTextLayout.java:1333) > at > javafx.graphics/com.sun.javafx.text.PrismTextLayout.ensureLayout(PrismTextLayout.java:231) > at > javafx.graphics/com.sun.javafx.text.PrismTextLayout.getVisualBounds(PrismTextLayout.java:1461) > at javafx.graphics/javafx.scene.text.Text.getVisualBounds(Text.java:424) > at > javafx.graphics/javafx.scene.text.Text.doComputeGeomBounds(Text.java:1191) > at > javafx.graphics/javafx.scene.text.Text$1.doComputeGeomBounds(Text.java:150) > at > javafx.graphics/com.sun.javafx.scene.shape.TextHelper.computeGeomBoundsImpl(TextHelper.java:90) > at > javafx.graphics/com.sun.javafx.scene.NodeHelper.computeGeomBounds(NodeHelper.java:117) > at javafx.graphics/javafx.scene.Node.updateGeomBounds(Node.java:3845) > at javafx.graphics/javafx.scene.Node.getGeomBounds(Node.java:3807) > at javafx.graphics/javafx.scene.Node.getLocalBounds(Node.java:3755) > at javafx.graphics/javafx.scene.Node.updateTxBounds(Node.java:3909) > at > javafx.graphics/javafx.scene.Node.getTransformedBounds(Node.java:3701) > at > javafx.graphics/javafx.scene.Parent.getChildTransformedBounds(Parent.java:1849) > at > javafx.graphics/javafx.scene.Parent.updateCachedBounds(Parent.java:1710) > at javafx.graphics/javafx.scene.Parent.recomputeBounds(Parent.java:1649) > at > javafx.graphics/javafx.scene.Parent.doComputeGeomBounds(Parent.java:1502) > at > javafx.graphics/javafx.scene.Parent$1.doComputeGeomBounds(Parent.java:115) > at > javafx.graphics/com.sun.javafx.scene.ParentHelper.computeGeomBoundsImpl(ParentHelper.java:84) > at > javafx.graphics/com.sun.javafx.scene.NodeHelper.computeGeomBounds(NodeHelper.java:117) > at javafx.graphics/javafx.scene.Node.updateGeomBounds(Node.java:3845) > at javafx.graphics/javafx.scene.Node.getGeomBounds(Node.java:3807) > at > javafx.graphics/javafx.scene.Node.doComputeLayoutBounds(Node.java:3655) > at > javafx.graphics/javafx.scene.Node$1.doComputeLayoutBounds(Node.java:449) > at > javafx.graphics/com.sun.javafx.scene.NodeHelper.computeLayoutBoundsImpl(NodeHelper.java:168) > at javafx.graphics/com.sun.javafx @andy-goryachev-oracle about MonkeyTester not starting, I've reproduced this in a unit test. The `TextRun` code is more complex than I figured. The trailing space calculation does not take into account that a single char can match to multiple glyphs, and so it is using the wrong offset to call `getAdvance` with. I'll try and have a fix + additional tests ready by tomorrow. For reference: the positions array in `TextRun` when compact is a simple array with advance values per glyph, otherwise it is an array of x/y positions. The advance is then calculated by substracting the next glyph's x position minus the current glyph's x position. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1915247197
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v4]
On Mon, 29 Jan 2024 07:29:06 GMT, Karthik P K wrote: >>> Noticed a problem - on Windows 11 and on macOS 14.2.1 hit into shows >>> different values for Text and TextFlow. This issue can be seen with pretty >>> much every text, even "aaa\nbbb\n". In this screenshot, the line >>> corresponds to the mouse position: >>> >> I see this problem as well. I will check on this issue. > >> Noticed a problem - on Windows 11 and on macOS 14.2.1 hit into shows >> different values for Text and TextFlow. >> > > Fixed this issue. Please check. @karthikpandelu it looks worse than before:  could you please pull the latest MonkeyTester and re-test Text and TextFlow pages please? https://github.com/andy-goryachev-oracle/MonkeyTest - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1915207426
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 16:42:15 GMT, Nir Lisker wrote: >> Added a utility method to run code on the FX thread if it's not already, and >> changed the animation methods to use it. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update tests The tests are a best-effort to reproduce the issue and may pass even before the fix. I found that the `AnimationTimer` test cases can fail in 2 ways: one is the reported NPE on the FX thread, but another is an AIIOB on the background thread that originates in (or close to) the master timer. For this reason, I added exception handlers on the FX and on each background thread. The `Animation` test is more unstable, maybe because it doesn't simulate a load on every tick (like in `AnimationTimer::handle()`). Maybe using a `Timer` implementation instead of a `Transition` will allow to reproduce the issue better. - PR Comment: https://git.openjdk.org/jfx/pull/1352#issuecomment-1915141156
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Update tests - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/75de3861..b3bd557e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=13 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=12-13 Stats: 261 lines in 4 files changed: 159 ins; 93 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v6]
On Mon, 29 Jan 2024 14:38:20 GMT, John Hendrikx wrote:
>> There are a number of tickets open related to text rendering:
>>
>> https://bugs.openjdk.org/browse/JDK-8314215
>>
>> https://bugs.openjdk.org/browse/JDK-8145496
>>
>> https://bugs.openjdk.org/browse/JDK-8129014
>>
>> They have in common that wrapped text is taking the trailing spaces on each
>> wrapped line into account when calculating where to wrap. This looks okay
>> for text that is left aligned (as the spaces will be trailing the lines and
>> generally aren't a problem, but looks weird with CENTER and RIGHT
>> alignments. Even with LEFT alignment there are artifacts of this behavior,
>> where a line like `AAA BBB CCC` (note the **double** spaces) gets split up
>> into `AAA `, `BBB ` and `CCC`, but if space reduces further, it will wrap
>> **too** early because the space is taken into account (ie. `AAA` may still
>> have fit just fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A
>> ` or something).
>>
>> The fix for this is two fold; first the individual lines of text should not
>> include any trailing spaces into their widths; second, the code that is
>> taking the trailing space into account when wrapping should ignore all
>> trailing spaces (currently it is ignoring all but one trailing space). With
>> these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great,
>> and there is no more early wrapping due to a space being taking into account
>> while the actual text still would have fit (this is annoying in tight
>> layouts, where a line can be wrapped early even though it looks like it
>> would have fit).
>>
>> If it were that simple, we'd be done, but there may be another issue here
>> that needs solving: wrapped aligned TextArea's.
>>
>> TextArea don't directly support text alignment (via a setTextAlignment
>> method like Label) but you can change it via CSS.
>>
>> For Left alignment + wrapping, TextArea will ignore any spaces typed before
>> a line that was wrapped. In other words, you can type spaces as much as you
>> want, and they won't show up and the cursor won't move. The spaces are all
>> getting appended to the previous line. When you cursor through these
>> spaces, the cursor can be rendered out of the control's bounds. To
>> illustrate, if you have the text `AAA BBB CCC`, and the text
>> gets wrapped to `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not
>> show up. If you cursor back, the cursor may be outside the control bounds
>> because so many spaces are trailing `AAA`.
>>
>> The above behavior has NOT changed, is pretty standard for wrapped text
>> controls,...
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move test that needs QuantumToolkit to system tests
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line
149:
> 147: @Disabled
> 148: @Test
> 149: void complexTestsThatAreBrokenSince2013() {
@kevinrushforth
re: ancient tests
this is the only skipped test - quite possibly due to wrong expectations (about
Thai)
Due to relative complexity of the change, I think we need all the tests we
could get in this PR
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line
429:
> 427: );
> 428: }
> 429:
extra newline
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1469867739
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1469863998
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v5]
On Mon, 29 Jan 2024 16:10:01 GMT, Andy Goryachev wrote:
> > ancient test case that was disabled (see
> > https://bugs.openjdk.org/browse/JDK-8087615).
>
> should the old ticket be added to this PR?
If this fixes all tests that are `@Ignore`d with this old bug ID, then yes. I
still see a skipped test, though. If this is still being skipped be cause of
JDK-8087615, then it should use `@Ignore("JDK-8087615")` (or
`@Disabled("JDK-8087615")` if that is preferred in JUnit5).
-
PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1915073445
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v6]
On Mon, 29 Jan 2024 14:38:20 GMT, John Hendrikx wrote: >> There are a number of tickets open related to text rendering: >> >> https://bugs.openjdk.org/browse/JDK-8314215 >> >> https://bugs.openjdk.org/browse/JDK-8145496 >> >> https://bugs.openjdk.org/browse/JDK-8129014 >> >> They have in common that wrapped text is taking the trailing spaces on each >> wrapped line into account when calculating where to wrap. This looks okay >> for text that is left aligned (as the spaces will be trailing the lines and >> generally aren't a problem, but looks weird with CENTER and RIGHT >> alignments. Even with LEFT alignment there are artifacts of this behavior, >> where a line like `AAA BBB CCC` (note the **double** spaces) gets split up >> into `AAA `, `BBB ` and `CCC`, but if space reduces further, it will wrap >> **too** early because the space is taken into account (ie. `AAA` may still >> have fit just fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A >> ` or something). >> >> The fix for this is two fold; first the individual lines of text should not >> include any trailing spaces into their widths; second, the code that is >> taking the trailing space into account when wrapping should ignore all >> trailing spaces (currently it is ignoring all but one trailing space). With >> these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, >> and there is no more early wrapping due to a space being taking into account >> while the actual text still would have fit (this is annoying in tight >> layouts, where a line can be wrapped early even though it looks like it >> would have fit). >> >> If it were that simple, we'd be done, but there may be another issue here >> that needs solving: wrapped aligned TextArea's. >> >> TextArea don't directly support text alignment (via a setTextAlignment >> method like Label) but you can change it via CSS. >> >> For Left alignment + wrapping, TextArea will ignore any spaces typed before >> a line that was wrapped. In other words, you can type spaces as much as you >> want, and they won't show up and the cursor won't move. The spaces are all >> getting appended to the previous line. When you cursor through these >> spaces, the cursor can be rendered out of the control's bounds. To >> illustrate, if you have the text `AAA BBB CCC`, and the text >> gets wrapped to `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not >> show up. If you cursor back, the cursor may be outside the control bounds >> because so many spaces are trailing `AAA`. >> >> The above behavior has NOT changed, is pretty standard for wrapped text >> controls,... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move test that needs QuantumToolkit to system tests MonkeyTester won't start: Exception in thread "JavaFX Application Thread" java.lang.ArrayIndexOutOfBoundsException: Index 44 out of bounds for length 38 at javafx.graphics/com.sun.javafx.text.TextRun.getAdvance(TextRun.java:362) at javafx.graphics/com.sun.javafx.text.PrismTextLayout.computeTrailingSpaceWidth(PrismTextLayout.java:1049) at javafx.graphics/com.sun.javafx.text.PrismTextLayout.createLine(PrismTextLayout.java:1017) at javafx.graphics/com.sun.javafx.text.PrismTextLayout.layout(PrismTextLayout.java:1333) at javafx.graphics/com.sun.javafx.text.PrismTextLayout.ensureLayout(PrismTextLayout.java:231) at javafx.graphics/com.sun.javafx.text.PrismTextLayout.getVisualBounds(PrismTextLayout.java:1461) at javafx.graphics/javafx.scene.text.Text.getVisualBounds(Text.java:424) at javafx.graphics/javafx.scene.text.Text.doComputeGeomBounds(Text.java:1191) at javafx.graphics/javafx.scene.text.Text$1.doComputeGeomBounds(Text.java:150) at javafx.graphics/com.sun.javafx.scene.shape.TextHelper.computeGeomBoundsImpl(TextHelper.java:90) at javafx.graphics/com.sun.javafx.scene.NodeHelper.computeGeomBounds(NodeHelper.java:117) at javafx.graphics/javafx.scene.Node.updateGeomBounds(Node.java:3845) at javafx.graphics/javafx.scene.Node.getGeomBounds(Node.java:3807) at javafx.graphics/javafx.scene.Node.getLocalBounds(Node.java:3755) at javafx.graphics/javafx.scene.Node.updateTxBounds(Node.java:3909) at javafx.graphics/javafx.scene.Node.getTransformedBounds(Node.java:3701) at javafx.graphics/javafx.scene.Parent.getChildTransformedBounds(Parent.java:1849) at javafx.graphics/javafx.scene.Parent.updateCachedBounds(Parent.java:1710) at javafx.graphics/javafx.scene.Parent.recomputeBounds(Parent.java:1649) at javafx.graphics/javafx.scene.Parent.doComputeGeomBounds(Parent.java:1502) at javafx.graphics/javafx.scene.Parent$1.doComputeGeomBounds(Parent.java:115) at javafx.graphics/com.sun.javafx.scene.ParentHelper.computeGeomBoundsIm
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v13]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Rename more Animation methods - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/e2ff72ab..75de3861 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=12 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=11-12 Stats: 21 lines in 1 file changed: 15 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v5]
On Sun, 28 Jan 2024 23:30:56 GMT, John Hendrikx wrote: > ancient test case that was disabled (see > https://bugs.openjdk.org/browse/JDK-8087615). should the old ticket be added to this PR? - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1915034433
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v12]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Update Timeline method - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/92716631..e2ff72ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=11 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=10-11 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v11]
On Mon, 29 Jan 2024 14:26:01 GMT, Nir Lisker wrote:
>> Added a utility method to run code on the FX thread if it's not already, and
>> changed the animation methods to use it.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add null parent checks
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 976:
> 974: }
> 975:
> 976: private void playFromStartOnFxThread() {
Maybe change this (and the other `playFrom* methods`) to `xxxImpl` as well?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469816588
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v9]
On Mon, 29 Jan 2024 15:39:32 GMT, Nir Lisker wrote: >> tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java >> line 65: >> >>> 63: } >>> 64: >>> 65: waiter.await(GRACE_PERIOD, TimeUnit.SECONDS); >> >> I don't think this will do what you want. This will return almost >> immediately, before any of the animation has run (as soon as the runnable >> that sets the uncaught exception handler finished). What I think you need is >> something more like this: >> >> >> Platform.runLater(() -> registerExceptionHandler()); >> assertTrue(waiter.await(GRACE_PERIOD, TimeUnit.SECONDS)); >> >> // start the 10 threads... >> >> Util.sleep(GRACE_PERIOD); >> >> >> The await ensures that the uncaught exception handler has been registered >> before starting the threads. The sleep then runs the threads for a fixed >> amount of time. > >> I don't think this will do what you want. This will return almost >> immediately, before any of the animation has run (as soon as the runnable >> that sets the uncaught exception handler finished). > > I observed that this setup works correctly for the `AnimationTimer` test. The > test does wait for `GRACE_PERIOD` seconds when there is no exception and then > succeeds, or catches and fails immediately when there onw is thrown. The > `Animation` test was more finicky, but I think that is because I'm not > simulating a processing load properly. > >> The await ensures that the uncaught exception handler has been registered >> before starting the threads. > > While technically true, the handler register in a negligible amount of time > compared to the grace period. Because exceptions are thrown continuously > )it's not a one-time event), the handler will never miss a failure. > > I will change it for correctness sake. I misread it. I see now that you only countdown when there is a failure. In that case, it seems fine as you have it (unless you also want to have a second latch that ensures that the uncaught event handler is in place prior to submitting the runnables). You might want to add a comment as to the purpose of the latch, since it wasn't initially obvious to me. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469812959
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v6]
On Mon, 29 Jan 2024 14:38:20 GMT, John Hendrikx wrote: >> There are a number of tickets open related to text rendering: >> >> https://bugs.openjdk.org/browse/JDK-8314215 >> >> https://bugs.openjdk.org/browse/JDK-8145496 >> >> https://bugs.openjdk.org/browse/JDK-8129014 >> >> They have in common that wrapped text is taking the trailing spaces on each >> wrapped line into account when calculating where to wrap. This looks okay >> for text that is left aligned (as the spaces will be trailing the lines and >> generally aren't a problem, but looks weird with CENTER and RIGHT >> alignments. Even with LEFT alignment there are artifacts of this behavior, >> where a line like `AAA BBB CCC` (note the **double** spaces) gets split up >> into `AAA `, `BBB ` and `CCC`, but if space reduces further, it will wrap >> **too** early because the space is taken into account (ie. `AAA` may still >> have fit just fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A >> ` or something). >> >> The fix for this is two fold; first the individual lines of text should not >> include any trailing spaces into their widths; second, the code that is >> taking the trailing space into account when wrapping should ignore all >> trailing spaces (currently it is ignoring all but one trailing space). With >> these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, >> and there is no more early wrapping due to a space being taking into account >> while the actual text still would have fit (this is annoying in tight >> layouts, where a line can be wrapped early even though it looks like it >> would have fit). >> >> If it were that simple, we'd be done, but there may be another issue here >> that needs solving: wrapped aligned TextArea's. >> >> TextArea don't directly support text alignment (via a setTextAlignment >> method like Label) but you can change it via CSS. >> >> For Left alignment + wrapping, TextArea will ignore any spaces typed before >> a line that was wrapped. In other words, you can type spaces as much as you >> want, and they won't show up and the cursor won't move. The spaces are all >> getting appended to the previous line. When you cursor through these >> spaces, the cursor can be rendered out of the control's bounds. To >> illustrate, if you have the text `AAA BBB CCC`, and the text >> gets wrapped to `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not >> show up. If you cursor back, the cursor may be outside the control bounds >> because so many spaces are trailing `AAA`. >> >> The above behavior has NOT changed, is pretty standard for wrapped text >> controls,... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move test that needs QuantumToolkit to system tests modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 1015: > 1013: /* > 1014: * Calculate the width of trailing spaces for the new TextLine > so they're > 1015: * can be excluded when doing later alignment calculations: It reads not correct. Maybe like this? Suggestion: * Calculate the width of trailing spaces for the new TextLine so they * can be excluded when doing later alignment calculations: - PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1469813724
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v5]
On Mon, 29 Jan 2024 14:16:00 GMT, Nir Lisker wrote: >> I was also going to suggest `xxImpl` with a short javadoc comment. That is a >> pattern we use elsewhere. I don't dislike the current name, but I can see >> Jose's point. > > Changed to `xxImpl` with a doc comment. Thanks. You missed the one in Timeline (which is why there is a compilation error). - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469806371
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Mon, 29 Jan 2024 15:40:28 GMT, Andy Goryachev wrote: > > The justify calculation only looks at regular white spaces (0x20), but I > > think it should really be looking at all types... > > maybe extract that into a separate issue (and link this one?) > > Are there any other places where we should be using Character.isWhitespace() > instead of `(== ' ')` ? No, only the justify code still has it. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1914964711
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Sat, 27 Jan 2024 08:33:47 GMT, John Hendrikx wrote: > The justify calculation only looks at regular white spaces (0x20), but I > think it should really be looking at all types... maybe extract that into a separate issue (and link this one?) Are there any other places where we should be using Character.isWhitespace() instead of `(== ' ')` ? - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1914961281
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v9]
On Mon, 29 Jan 2024 13:07:21 GMT, Kevin Rushforth wrote: > I don't think this will do what you want. This will return almost > immediately, before any of the animation has run (as soon as the runnable > that sets the uncaught exception handler finished). I observed that this setup works correctly for the `AnimationTimer` test. The test does wait for `GRACE_PERIOD` seconds when there is no exception and then succeeds, or catches and fails immediately when there onw is thrown. The `Animation` test was more finicky, but I think that is because I'm not simulating a processing load properly. > The await ensures that the uncaught exception handler has been registered > before starting the threads. While technically true, the handler register in a negligible amount of time compared to the grace period. Because exceptions are thrown continuously )it's not a one-time event), the handler will never miss a failure. I will change it for correctness sake. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469783805
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v6]
> There are a number of tickets open related to text rendering: > > https://bugs.openjdk.org/browse/JDK-8314215 > > https://bugs.openjdk.org/browse/JDK-8145496 > > https://bugs.openjdk.org/browse/JDK-8129014 > > They have in common that wrapped text is taking the trailing spaces on each > wrapped line into account when calculating where to wrap. This looks okay > for text that is left aligned (as the spaces will be trailing the lines and > generally aren't a problem, but looks weird with CENTER and RIGHT alignments. > Even with LEFT alignment there are artifacts of this behavior, where a line > like `AAA BBB CCC` (note the **double** spaces) gets split up into `AAA `, > `BBB ` and `CCC`, but if space reduces further, it will wrap **too** early > because the space is taken into account (ie. `AAA` may still have fit just > fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A ` or > something). > > The fix for this is two fold; first the individual lines of text should not > include any trailing spaces into their widths; second, the code that is > taking the trailing space into account when wrapping should ignore all > trailing spaces (currently it is ignoring all but one trailing space). With > these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, > and there is no more early wrapping due to a space being taking into account > while the actual text still would have fit (this is annoying in tight > layouts, where a line can be wrapped early even though it looks like it would > have fit). > > If it were that simple, we'd be done, but there may be another issue here > that needs solving: wrapped aligned TextArea's. > > TextArea don't directly support text alignment (via a setTextAlignment method > like Label) but you can change it via CSS. > > For Left alignment + wrapping, TextArea will ignore any spaces typed before a > line that was wrapped. In other words, you can type spaces as much as you > want, and they won't show up and the cursor won't move. The spaces are all > getting appended to the previous line. When you cursor through these spaces, > the cursor can be rendered out of the control's bounds. To illustrate, if > you have the text `AAA BBB CCC`, and the text gets wrapped to > `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not show up. If you > cursor back, the cursor may be outside the control bounds because so many > spaces are trailing `AAA`. > > The above behavior has NOT changed, is pretty standard for wrapped text > controls, and IMHO does not need further attent... John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Move test that needs QuantumToolkit to system tests - Changes: - all: https://git.openjdk.org/jfx/pull/1236/files - new: https://git.openjdk.org/jfx/pull/1236/files/cb837343..8193db0d Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=05 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=04-05 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1236.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1236/head:pull/1236 PR: https://git.openjdk.org/jfx/pull/1236
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v11]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Add null parent checks - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/e488ae69..92716631 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=10 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=09-10 Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v5]
On Mon, 29 Jan 2024 12:44:07 GMT, Kevin Rushforth wrote: >> Probably you won't like `xxImpl` either? >> I'm not sure we should start adding `runMustBeOnFxThread` to every method >> name out there that should run on the FX thread. In this case, maybe we >> could simply add a small javadoc with a comment, and that's it. Neither will >> enforce a real check about that. > > I was also going to suggest `xxImpl` with a short javadoc comment. That is a > pattern we use elsewhere. I don't dislike the current name, but I can see > Jose's point. Changed to `xxImpl` with a doc comment. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469655778
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v10]
> Added a utility method to run code on the FX thread if it's not already, and > changed the animation methods to use it. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Change method names - Changes: - all: https://git.openjdk.org/jfx/pull/1352/files - new: https://git.openjdk.org/jfx/pull/1352/files/70bb360d..e488ae69 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=09 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1352&range=08-09 Stats: 35 lines in 2 files changed: 25 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jfx/pull/1352.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1352/head:pull/1352 PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Fri, 26 Jan 2024 06:49:42 GMT, Laurent Bourgès wrote: > I fixed new test so system test run properly. How do you compare test runs in > CI ? GHA jobs only run headless tests. In order to do run system test we need physical test systems. We have the ability to run them internally (in our lab), so I'll do a test run and post results. - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1914777538
Re: RFR: JDK-8324337: Cherry-pick WebKit 617.1 stabilization fixes [v3]
On Mon, 29 Jan 2024 04:24:54 GMT, Hima Bindu Meda wrote: >> Cherry-picked changes related to webkit-2.42.4.Verified build on all >> platforms. Sanity testing looks fine. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > space corrections Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1350#pullrequestreview-1848696004
Re: RFR: JDK-8324337: Cherry-pick WebKit 617.1 stabilization fixes [v2]
On Fri, 26 Jan 2024 19:36:23 GMT, Joeri Sykora wrote: >> Hima Bindu Meda has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Minor corrections as per review comments > > Everything built fine. @tiainen Can you rereview? (only whitespace changed from last time) - PR Comment: https://git.openjdk.org/jfx/pull/1350#issuecomment-1914714891
Re: StubToolkit breaks test
Tests in the javafx.graphics module always use StubToolkit. If you want to use the real toolkit (QuantumToolkit), you need to put the tests in the systemTests project, under "tests/system". -- Kevin On 1/28/2024 3:47 PM, John Hendrikx wrote: I found an old disabled test that is testing PrismTextLayout for which I have a PR outstanding (https://github.com/openjdk/jfx/pull/1236). The test I think is useful, but is loading real fonts (Monaco and Tahoma). Locally this works when I don't use the StubToolkit to run the test, and (I presume) may also work on the various build environments if those fonts are part of those environments (otherwise the test can be disabled with a JUnit 5 Assumption for platforms that lack these fonts -- the test would be useful even if it ran only in one environment as aside from the required fonts it is not OS specific). However, in the build system, the test is run with the StubToolkit, which can't load real fonts (which I do really need for a proper test). The test therefore fails. Is there a way to mark tests to not use the StubToolkit? --John
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v9]
On Mon, 29 Jan 2024 09:33:35 GMT, Jose Pereda wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update tests
>
> tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
> line 20:
>
>> 18:
>> 19: // Based on https://bugs.openjdk.org/browse/JDK-8159048
>> 20: public class SynchronisityTest extends Application {
>
> shouldn't it be `SynchronicityTest`?
Yes, or maybe `SynchronizationTest`.
More importantly, the main test class should not extend `Application`. See
[JDK-8234876](https://bugs.openjdk.org/browse/JDK-8234876).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469546192
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v5]
On Mon, 29 Jan 2024 09:20:27 GMT, Jose Pereda wrote: >>> However, I'm not sure about the method naming `OnFxThread`, as it might >>> imply that what it does is already done on the FX thread, therefore not >>> needing to be wrapped up by `runOnFxThread`. >> >> I understand what you mean, but the way I see it is that they do already run >> on the FX thread because that's the only reason they exist. I could use >> something like `runMustBeOnFxThread` or `runOnlyFromFxThread` if that's >> better. >> Note that there are already `do___` methods (bad names IMO), and adding >> `run___` might be confusing. > > Probably you won't like `xxImpl` either? > I'm not sure we should start adding `runMustBeOnFxThread` to every method > name out there that should run on the FX thread. In this case, maybe we could > simply add a small javadoc with a comment, and that's it. Neither will > enforce a real check about that. I was also going to suggest `xxImpl` with a short javadoc comment. That is a pattern we use elsewhere. I don't dislike the current name, but I can see Jose's point. - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469540887
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v5]
On Sat, 27 Jan 2024 21:31:32 GMT, Nir Lisker wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line
>> 1008:
>>
>>> 1006: * @param runnable a {@code Runnable} encapsulating the code
>>> 1007: */
>>> 1008: public static void runOnFxThread(Runnable runnable) {
>>
>> I understand that this is a private package, and not public API, but do we
>> need a null check of `runnable`? (Not needed for the current uses added from
>> this PR, but possibly for future uses?)
>
> I thought about it when I wrote this method, but I don't see much value in it
> for an internal utility method as it doesn't serve as a boundary. In
> addition, there was a discussion about future work that will eliminate the
> need for this method by dealing with the threading issue at a deeper level,
> so it is somewhat temporary.
I wouldn't worry about it. We have many methods that take a Runnable and don't
do a null check.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469538109
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v9]
On Mon, 29 Jan 2024 03:28:51 GMT, Nir Lisker wrote:
>> Added a utility method to run code on the FX thread if it's not already, and
>> changed the animation methods to use it.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update tests
I left a few more inline comments, mostly on the test, but I also noted some
missing `parent != null` checks.
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 900:
> 898: */
> 899: public void playFrom(String cuePoint) {
> 900: Utils.runOnFxThread(() -> playFromOnFxThread(cuePoint));
I think you need to add the check for `parent != null`.
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 936:
> 934: */
> 935: public void playFrom(Duration time) {
> 936: Utils.runOnFxThread(() -> playFromOnFxThread(time));
I think you need to add the check for `parent != null`.
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 964:
> 962: */
> 963: public void playFromStart() {
> 964: Utils.runOnFxThread(this::playFromStartOnFxThread);
I think you need to add the check for `parent != null`.
tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java
line 22:
> 20: anim.setCycleCount(Animation.INDEFINITE);
> 21:
> 22: while (true) {
Maybe add a comment indicating that the runnable will run until stopped by the
test harness?
tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java
line 30:
> 28: }
> 29: }
> 30: }
Minor: add newline at end of file.
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 24:
> 22: public static void main(String[] args) {
> 23: Application.launch(args);
> 24: }
The main method is not needed.
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 65:
> 63: }
> 64:
> 65: waiter.await(GRACE_PERIOD, TimeUnit.SECONDS);
I don't think this will do what you want. This will return almost immediately,
before any of the animation has run (as soon as the runnable that sets the
uncaught exception handler finished). What I think you need is something more
like this:
Platform.runLater(() -> registerExceptionHandler());
assertTrue(waiter.await(GRACE_PERIOD, TimeUnit.SECONDS));
// start the 10 threads...
Util.sleep(GRACE_PERIOD);
The await ensures that the uncaught exception handler has been registered
before starting the threads. The sleep then runs the threads for a fixed amount
of time.
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 89:
> 87: } catch (InterruptedException e) {
> 88: }
> 89: }
You can use the existing `Util.sleep` method, which already does this.
-
PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1848586078
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469551343
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469550259
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469551543
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469544786
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469560856
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469564886
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469567412
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469546961
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Thu, 25 Jan 2024 17:16:51 GMT, Laurent Bourgès wrote:
>> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java
>
> Laurent Bourgès has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fixed test package
Tested the changes and it fixes the issue as expected.
Left some minor comments inline. I will finish the review soon.
modules/javafx.graphics/src/main/java/com/sun/marlin/ArrayCacheDouble.java line
168:
> 166: if (DO_CLEAN_DIRTY && (toIndex != 0)) {
> 167: // clean-up array of dirty part[fromIndex; toIndex[
> 168: fill(array, fromIndex, toIndex, /*(double)*/ 0.0);
Do you think /*(double)*/ will be helpful? In my opinion, it is not required.
modules/javafx.graphics/src/main/java/com/sun/marlin/ArrayCacheInt.java line
168:
> 166: if (DO_CLEAN_DIRTY && (toIndex != 0)) {
> 167: // clean-up array of dirty part[fromIndex; toIndex[
> 168: fill(array, fromIndex, toIndex, /*(int)*/ 0);
Same as previous comment, /*(int)*/ comment is not required here.
modules/javafx.graphics/src/main/java/com/sun/marlin/ArrayCacheIntClean.java
line 171:
> 169: if (toIndex != 0) {
> 170: // clean-up array of dirty part[fromIndex; toIndex[
> 171: fill(array, fromIndex, toIndex, /*(int)*/ 0);
Same as previous comment, /*(int)*/ comment is not required here.
modules/javafx.graphics/src/main/java/com/sun/marlin/DPathConsumer2D.java line
2:
> 1: /*
> 2: * Copyright (c) 2007, 2022, Oracle and/or its affiliates. All rights
> reserved.
Shouldn't it be 2007, 2024?
modules/javafx.graphics/src/main/java/com/sun/prism/impl/shape/DMarlinPrismUtils.java
line 111:
> 109:
> 110: if (Math.abs(det) <= (2.0d * Double.MIN_VALUE)) {
> 111: // this rendering engine takes one dimensional curves
> and turns
Minor: this -> This
modules/javafx.graphics/src/main/java/com/sun/prism/impl/shape/DMarlinPrismUtils.java
line 118:
> 116:
> 117: // Every path needs an initial moveTo and a pathDone. If
> these
> 118: // are not there this causes a SIGSEGV in libawt.so (at
> the time
Minor: Add space before so and S can be made capital.
Also wanted to check if libawt usage is correct here?
-
PR Review: https://git.openjdk.org/jfx/pull/1348#pullrequestreview-1848155818
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469276629
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469277966
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469279357
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469322152
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469352123
PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1469351817
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v9]
On Mon, 29 Jan 2024 03:28:51 GMT, Nir Lisker wrote:
>> Added a utility method to run code on the FX thread if it's not already, and
>> changed the animation methods to use it.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update tests
tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java
line 1:
> 1: /*
Don't remove license
tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java
line 1:
> 1: /*
Don't remove license
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 1:
> 1: package test.com.sun.javafx.animation;
Add license
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 20:
> 18:
> 19: // Based on https://bugs.openjdk.org/browse/JDK-8159048
> 20: public class SynchronisityTest extends Application {
shouldn't it be `SynchronicityTest`?
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 53:
> 51: private AtomicBoolean failed = new AtomicBoolean(false);
> 52: private CountDownLatch waiter = new CountDownLatch(1);
> 53: private ExecutorService executor = Executors.newCachedThreadPool();
These can be `final`
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469281637
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469281296
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469278053
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469307640
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469295387
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v5]
On Sat, 27 Jan 2024 21:56:59 GMT, Nir Lisker wrote:
>> modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line
>> 903:
>>
>>> 901: }
>>> 902:
>>> 903: private void playFromOnFxThread(String cuePoint) {
>>
>> I take that adding these private `xxOnFxThread` methods is convenient so
>> they can be passed easily to the runnable in `Utils::runOnFxThread`, or
>> accessed from other subclasses.
>>
>> However, I'm not sure about the method naming `OnFxThread`, as it might
>> imply that what it does is already done on the FX thread, therefore not
>> needing to be wrapped up by `runOnFxThread`.
>>
>> Does `runXX` (i.e. `runPlayFrom`) make more sense? In any case, I'd be fine
>> with the current proposal as is.
>
>> However, I'm not sure about the method naming `OnFxThread`, as it might
>> imply that what it does is already done on the FX thread, therefore not
>> needing to be wrapped up by `runOnFxThread`.
>
> I understand what you mean, but the way I see it is that they do already run
> on the FX thread because that's the only reason they exist. I could use
> something like `runMustBeOnFxThread` or `runOnlyFromFxThread` if that's
> better.
> Note that there are already `do___` methods (bad names IMO), and adding
> `run___` might be confusing.
Probably you won't like `xxImpl` either?
I'm not sure we should start adding `runMustBeOnFxThread` to every method name
out there that should run on the FX thread. In this case, maybe we could simply
add a small javadoc with a comment, and that's it. Neither will enforce a real
check about that.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469290172
Re: RFR: 8260013: Snapshot does not work for nodes in a subscene [v3]
On Fri, 26 Jan 2024 13:55:59 GMT, Lukasz Kostyra wrote: >> Originally this issue showed the problem of Node being incorrectly rendered >> (clipped) when snapshotting, compared to a snapshot of the whole Scene. >> Later on there was another problem added - lights not being taken into >> account if they are added to a SubScene. >> >> As it later turned out, the original problem from this bug report is a >> problem with ParallelCamera incorrectly estimating near/far clipping planes, >> which just happened to reveal itself while snapshotting a Node. During >> testing I found out you can make the Node clip regardless of snapshot >> mechanism. Clipping issue was moved to a separate bug report and this PR >> only fixes the inconsistency in lights being gathered for a snapshot. >> >> `Scene.doSnapshot()` was expanded to also check if SubScene provided to it >> is non-null and to fetch lights assigned to it. Scenario was tested with >> added SnapshotLightsTest. >> >> Rest of the tests were checked and don't produce any noticeable regressions. > > Lukasz Kostyra has updated the pull request incrementally with two additional > commits since the last revision: > > - doSnapshot: Adjust light collection > >Snapshot now includes lights only from subScene if it is not null, which >matches how SubScenes are rendered > - SnapshotLightsTest: Add test for a snapshot with an extra light in scene Cleanup done, should be all OK now - PR Comment: https://git.openjdk.org/jfx/pull/1332#issuecomment-1914297543
Re: RFR: 8260013: Snapshot does not work for nodes in a subscene [v4]
> Originally this issue showed the problem of Node being incorrectly rendered > (clipped) when snapshotting, compared to a snapshot of the whole Scene. Later > on there was another problem added - lights not being taken into account if > they are added to a SubScene. > > As it later turned out, the original problem from this bug report is a > problem with ParallelCamera incorrectly estimating near/far clipping planes, > which just happened to reveal itself while snapshotting a Node. During > testing I found out you can make the Node clip regardless of snapshot > mechanism. Clipping issue was moved to a separate bug report and this PR only > fixes the inconsistency in lights being gathered for a snapshot. > > `Scene.doSnapshot()` was expanded to also check if SubScene provided to it is > non-null and to fetch lights assigned to it. Scenario was tested with added > SnapshotLightsTest. > > Rest of the tests were checked and don't produce any noticeable regressions. Lukasz Kostyra has updated the pull request incrementally with one additional commit since the last revision: Minor review adjustments - Changes: - all: https://git.openjdk.org/jfx/pull/1332/files - new: https://git.openjdk.org/jfx/pull/1332/files/5259be87..f05e4415 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1332&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1332&range=02-03 Stats: 20 lines in 2 files changed: 0 ins; 7 del; 13 mod Patch: https://git.openjdk.org/jfx/pull/1332.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1332/head:pull/1332 PR: https://git.openjdk.org/jfx/pull/1332
