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 I fixed new test so system test run properly. How do you compare test runs in CI ? - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1911570665
Re: Thought experiment: constructing Nodes on a different thread...
Thanks, it seems I misunderstood something fundamental here.
I always figured the memory effects are limited to the variables
accessed/mutated within a synchronized block (or *could* be limited to
just those by selectively flushing data by some advanced VM).
Apparently that's not the case, and a full flush is simply done after a
synchronized block completes, which would include all mutated fields,
not just those inside a synchronized block.
--John
On 25/01/2024 15:50, Kevin Rushforth wrote:
Yes, I think you are missing something. Unless you have an animation
that just decides all on it's own to find and attach a newly
discovered node to the scene and start rendering it, there will be a
synchronization as a result of calling runLater. Consider the
following code:
Label label = new Label();
label.setText("hi");
// Attach it to the scene on the FX thread.
Platform.runLater(() -> root.getChildren().add(label));
The "runLater" necessarily synchronizes with the FX Application thread
so that the runnable can be put on its queue and later taken off the
queue and executed on the FX Application thread. As specified in the
Platform::runLater API docs: Actions in a thread prior to submitting a
runnable to this method happen-before actions performed by the
runnable in the JavaFX Application Thread.
So I don't see the problem.
-- Kevin
On 1/25/2024 1:50 AM, John Hendrikx wrote:
All this threading talk has made me wonder something:
Let's say there are two threads, the FX thread and Thread 1. I do
the following:
- On thread 1: create Label
- On thread 1: set Label text to "xyz"
I now attach this label to an active Scene graph. This should be done
on the FX thread as we're going to be manipulating an active Scene
graph, so:
- On FX thread: attach Label to active scene graph
There is no synchronization in place on the Label's text field. What
guarantees do I have that when the label is shown on screen it will
show "xyz" ?
IMHO, there is no such guarantee, and so any creation or manipulation
of Nodes that will later be part of an active scene graph (and thus
accessed by the FX thread) **must** be done on the FX thread.
Involving any other thread in their creation or manipulation runs the
risk of seeing an object in the incorrect state (or even an
"impossible" state when multiple fields are involved that normally
change together).
Effectively, assuming that when you create Nodes you always have the
intention of showing them at some point, you can never construct
Nodes on any other thread than the FX thread...
Am I missing something?
--John
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Thu, 25 Jan 2024 23:44:06 GMT, Andy Goryachev wrote: >>> Regarding other whitespace characters. Using \u2003 em space, pasting the >>> following text >> >> I think we can use `Character.isWhitespace`. For the most part it aligns >> with "is this a breaking space". It's not perfect, but we can do more when >> there are complaints. Here's my test: >> >> >> char isWhiteSpace (breaking space)? >> \u0009 true >> \u0020 true >> \u00a0 false >> \u1680 true >> \u180e false <- unexpected >> \u2001 true >> \u2002 true >> \u2003 true >> \u2004 true >> \u2005 true >> \u2006 true >> \u2007 false >> \u2008 true >> \u2009 true >> \u200a true >> \u200b false <- unexpected >> \u200c false <- unexpected >> \u200d false <- unexpected >> \u202f false >> \u205f true >> \u3000 true > > thank you @hjohn ! this PR has been out for a while, could you sync it up > with the master please? @andy-goryachev-oracle thanks for testing this, what you discovered was a bug in the `computeTrailingWhiteSpace` code that went undetected because of the simplicity of the strings I tested with. The `positions` array containing all the character widths is shared amongst all `TextRun`s (it does mention that) but I accessed it as if it was 0 based per `TextRun`. This means it sometimes included the width of characters that weren't spaces at all... - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1911261917
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v3]
> 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 two additional commits since the last revision: - Use Character::isWhitespace to check for breakable spaces - Fix bug with shared positions array (it is not 0 indexed per TextRun) - Changes: - all: https://git.openjdk.org/jfx/pull/1236/files - new: https://git.openjdk.org/jfx/pull/1236/files/164f807e..14bedf5d Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 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: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v2]
> 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 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 two additional commits since the last revision: - Merge branch 'openjdk:master' into feature/wrapped-aligned-text-rendering - Fix to ignore trailing white space for wrapping and alignment - Changes: - all: https://git.openjdk.org/jfx/pull/1236/files - new: https://git.openjdk.org/jfx/pull/1236/files/bcca81e1..164f807e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=00-01 Stats: 355162 lines in 7219 files changed: 204987 ins; 105296 del; 44879 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: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Thu, 25 Jan 2024 23:23:42 GMT, John Hendrikx wrote: >> Regarding other whitespace characters. >> Using \u2003 em space, pasting the following text >> >> AAABBBCCC >> >> >> the results as seen with MS Word 16.78 (23100802) on macOS 14.2.1 indicate >> that it treats em space as whitespace: >> >>  >>  > >> Regarding other whitespace characters. Using \u2003 em space, pasting the >> following text > > I think we can use `Character.isWhitespace`. For the most part it aligns with > "is this a breaking space". It's not perfect, but we can do more when there > are complaints. Here's my test: > > > char isWhiteSpace (breaking space)? > \u0009 true > \u0020 true > \u00a0 false > \u1680 true > \u180e false <- unexpected > \u2001 true > \u2002 true > \u2003 true > \u2004 true > \u2005 true > \u2006 true > \u2007 false > \u2008 true > \u2009 true > \u200a true > \u200b false <- unexpected > \u200c false <- unexpected > \u200d false <- unexpected > \u202f false > \u205f true > \u3000 true thank you @hjohn ! this PR has been out for a while, could you sync it up with the master please? - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1911176710
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Thu, 25 Jan 2024 16:18:37 GMT, Andy Goryachev wrote: > Regarding other whitespace characters. Using \u2003 em space, pasting the > following text I think we can use `Character.isWhitespace`. For the most part it aligns with "is this a breaking space". It's not perfect, but we can do more when there are complaints. Here's my test: char isWhiteSpace (breaking space)? \u0009 true \u0020 true \u00a0 false \u1680 true \u180e false <- unexpected \u2001 true \u2002 true \u2003 true \u2004 true \u2005 true \u2006 true \u2007 false \u2008 true \u2009 true \u200a true \u200b false <- unexpected \u200c false <- unexpected \u200d false <- unexpected \u202f false \u205f true \u3000 true - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1911158165
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Sun, 10 Sep 2023 20:50:18 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, and IMHO does not need further attent... > Even if we forget other whitespace characters and use 0x20 spaces, there is a > weird problem where "CCC" jumps when changing the width of the window: > > I am testing this PR branch merged with the latest master and a slightly > different test program: This is because you don't have the same number of spaces between the words (first has 5 spaces, second has 6 spaces). I don't think it should do that, so I'll look into how this is happening. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1911155108
Integrated: 8308955: MediaPlayer/AudioClip skip data on seek/loop
On Tue, 23 Jan 2024 03:13:11 GMT, Alexander Matveev wrote: > This is regression from JDK-8262365. JDK-8262365 introduced support for > hardware pause for audio device. For some reason we will skip ~500 ms of > audio data after such pause. It is not noticeable for large audio files, but > for anything small like sound effects 1-3 seconds long it is noticeable and > makes short audio effects unusable without re-creating MediaPlayer after each > playback. Fix/workaround is to disable hardware pause. I did not figure out > why hardware pause skips audio data. This pull request has now been integrated. Changeset: 359ffb97 Author:Alexander Matveev URL: https://git.openjdk.org/jfx/commit/359ffb97931e19c6e2a8b9a35f6fee3b44639bdc Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod 8308955: MediaPlayer/AudioClip skip data on seek/loop Reviewed-by: kcr, arapte - PR: https://git.openjdk.org/jfx/pull/1346
Re: RFR: 8305418: [Linux] Replace obsolete XIM as Input Method Editor [v19]
On Wed, 17 Jan 2024 17:10:24 GMT, Thiago Milczarek Sayao wrote: >> This replaces obsolete XIM and uses gtk api for IME. >> Gtk uses [ibus](https://github.com/ibus/ibus) >> >> Gtk3+ uses relative positioning (as Wayland does), so I've added a Relative >> positioning on `InputMethodRequest`. >> >> [Screencast from 17-09-2023 >> 21:59:04.webm](https://github.com/openjdk/jfx/assets/30704286/6c398e39-55a3-4420-86a2-beff07b549d3) > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Add signals to avoid warnings The code and behavior look good to me. I did notice that you collapse the pango attribute list down to a single attribute but that didn't seem to affect the feedback negatively (the Mac ignores the OS-provided attributes completely so who am I to judge). When I drag a JavaFX window the JavaFX controls lose focus at the start of the drag and regain it at the end. This behavior is unique to Linux and dates back to at least JavaFX 20. This is a separate bug from the one addressed by this PR but it does cause an annoying interaction with the IME; if you drag the window while composing the text will either get committed or discarded (the IME controls which). - PR Review: https://git.openjdk.org/jfx/pull/1080#pullrequestreview-1844379573
Re: RFR: 8305418: [Linux] Replace obsolete XIM as Input Method Editor [v17]
On Wed, 17 Jan 2024 17:13:11 GMT, Thiago Milczarek Sayao wrote: >> modules/javafx.graphics/src/main/native-glass/gtk/glass_window_ime.cpp line >> 163: >> >>> 161: g_signal_connect(im_ctx.ctx, "preedit-changed", >>> G_CALLBACK(on_preedit_changed), this); >>> 162: g_signal_connect(im_ctx.ctx, "preedit-end", >>> G_CALLBACK(on_preedit_end), this); >>> 163: g_signal_connect(im_ctx.ctx, "commit", G_CALLBACK(on_commit), >>> this); >> >> On Ubuntu 22.04 using the Japanese (Mozc) input method I get a bunch of >> debug output from IBus. The message reads >> >> `IBUS-WARNING **: 08:50:12.994: java has no capability of surrounding-text >> feature` >> >> I was able to silence this by connecting to the surrounding text signal and >> simply returning TRUE from the callback. > > Fixed, although JavaFX does not support it, so the warning is true. Looks good to me. - PR Review Comment: https://git.openjdk.org/jfx/pull/1080#discussion_r1466766895
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
If you mean the changes proposed in Draft PR https://github.com/openjdk/jfx/pull/1349 those are too-intrusive for where we are in JavaFX 22. I see no chance that we will get agreement on the approach and be able to finish the code review and CSR in the next 4 business days. If we later get general agreement that the changes you propose are needed, we could consider them for JavaFX 23. For 22, I think we're down to either leaving it as is or reverting the thread checks and wrapping the implementation in a runLater. I don't think having the state change be asynchronous if you run it on a thread other than the application thread will be an issue. And, as I pointed out earlier, it is documented that the animation might not start or stop right away. -- Kevin On 1/25/2024 11:07 AM, Michael Strauß wrote: Hi Kevin, have you considered my proposal, which is basically the same approach: it uses runLater internally to dispatch the interaction with AbstractPrimaryTimer (instead of trying to make AbstractPrimaryTimer work under concurrent access). But from the point of view of the caller, the API works just as it worked before: all state changes of the Animation class are instantly visible, there is no surprising change as it would be with what you're currently proposing. With my proposal, play() can be called on a background thread, and the behavior of the Animation class remains the same. If you're reading getStatus() after calling play(), you will always see that the animation is currently running (even if internally, the animation hasn't yet been added to the primary timer). On Thu, Jan 25, 2024 at 6:30 PM Kevin Rushforth wrote: And that's why the "be careful" option is not a satisfying solution. Let's proceed with the option to change the implementation of play/pause/stop/start to do the work in a runLater, and change the docs accordingly. It's the only feasible option for JavaFX 22 (other than "do nothing"), and I also think it is a good choice. If we later want to evolve it for thread-safety, which I'm not convinced that we do, we can consider that for a future release. -- Kevin
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
Hi Kevin, have you considered my proposal, which is basically the same approach: it uses runLater internally to dispatch the interaction with AbstractPrimaryTimer (instead of trying to make AbstractPrimaryTimer work under concurrent access). But from the point of view of the caller, the API works just as it worked before: all state changes of the Animation class are instantly visible, there is no surprising change as it would be with what you're currently proposing. With my proposal, play() can be called on a background thread, and the behavior of the Animation class remains the same. If you're reading getStatus() after calling play(), you will always see that the animation is currently running (even if internally, the animation hasn't yet been added to the primary timer). On Thu, Jan 25, 2024 at 6:30 PM Kevin Rushforth wrote: > > And that's why the "be careful" option is not a satisfying solution. > > Let's proceed with the option to change the implementation of > play/pause/stop/start to do the work in a runLater, and change the docs > accordingly. It's the only feasible option for JavaFX 22 (other than "do > nothing"), and I also think it is a good choice. If we later want to evolve > it for thread-safety, which I'm not convinced that we do, we can consider > that for a future release. > > -- Kevin
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
Hi John, the threading restrictions are now removed. What I mean by those seemingly contradictory stratements is the following: This change doesn't make Animation inherently thread-safe, so if you configure the animation on thread A, you can't just call play() on thread B and expect it to work. What the change *does* allow you to do, is configure the animation on background thread A, and then call play() on the same background thread. On Thu, Jan 25, 2024 at 12:19 PM John Hendrikx wrote: > > Hi Michael, > > I'm not quite sure I see the point of this change. The PR did not > remove the threading restrictions on play/stop. > > I'm also confused by the seemingly contradictory statements: > > - this proposal does NOT allow Animation.play/stop/etc. to be "called on > any thread" > - It merely removes the requirement that these methods must be called on > the FX thread > > What does that mean exactly? > > Also, I think the "asynchronous" wording may apply to property changes > and action handlers that run at time index 0. These surely won't be ran > synchronous when calling "play", and I'm pretty sure they never were. > > --John > > On 24/01/2024 22:39, Michael Strauß wrote: > > Note: this proposal does NOT allow Animation.play/stop/etc. to be > > "called on any thread" as mentioned in JDK-8324658 [0]. > > > > It merely removes the requirement that these methods must be called on > > the FX thread, but this doesn't make the class inherently thread-safe. > > That is an important distinction to proposals that call for posting > > the play/stop calls directly to the FX thread. > > > > > > [0] https://bugs.openjdk.org/browse/JDK-8324658 > > > > > > On Wed, Jan 24, 2024 at 10:30 PM Michael Strauß > > wrote: > >> Here's another option, which I have implemented as a proof of concept [0]: > >> > >> The play/stop/etc. methods are currently specified to be > >> "asynchronous". This language should be removed, such that the methods > >> will be implied to execute synchronously from the point of view of the > >> caller (like every method that doesn't specify anything to the > >> contrary). > >> > >> All changes to observable state will remain instantly visible for the > >> calling thread. However, internally, interactions with > >> AbstractPrimaryTimer are posted to the FX thread if necessary. This is > >> not an unsurprising change, since the callback from the FX thread was > >> always occuring at an unspecified time in the future. > >> > >> To make this work, AbstractPrimaryTimer::pause/resume/nanos will have > >> to be synchronized to ensure field visibility across threads. > >> In the Animation class, interactions with AbstractPrimaryTimer will be > >> encapsulated in the new nested class AnimationPulseReceiver, which > >> also deduplicates redundant interactions with AbstractPrimaryTimer. > >> For example, repeatedly calling start() and stop() in quick succession > >> may require just a single interaction with AbstractPrimaryTimer in the > >> future (if we ended up in the running state), or no interaction at all > >> (if we ended up in the stopped state). > >> > >> > >> [0] https://github.com/openjdk/jfx/pull/1349
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
> 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 - Changes: - all: https://git.openjdk.org/jfx/pull/1348/files - new: https://git.openjdk.org/jfx/pull/1348/files/91061b8a..251d1d61 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1348&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1348&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Sun, 10 Sep 2023 20:50:18 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, and IMHO does not need further attent...
Even if we forget other whitespace characters and use 0x20 spaces, there is a
weird problem where "CCC" jumps when changing the width of the window:
I am testing this PR branch merged with the latest master and a slightly
different test program:
@Override
public void start(Stage stage) {
String text =
//"AAA\u2003\u2003\u2003\u2003BBB\u2003\u2003\u2003\u2003CCC\u2003\u2003\u2003\u2003";
"AAA BBB CCC ";
TextFlow t = new TextFlow(new Text(text));
t.setStyle("-fx-font-size:200%;");
t.setTextAlignment(TextAlignment.CENTER);
BorderPane root = new BorderPane(t);
Scene scene = new Scene(root, 595, 150, Color.BEIGE);
stage.setTitle("Text Alignment");
stage.setScene(scene);
stage.show();
}
-
PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1910568098
Re: RFR: 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.
Code changes look fine with one suggestion in the added code under `#if
PLATFOMR(JAVA)`.
I'll test it and finish my review.
modules/javafx.web/src/main/native/Source/WebCore/rendering/updating/RenderTreeBuilderMultiColumn.cpp
line 197:
> 195: continue;
> 196: placeholdersToRestore.append(&placeholder);
> 197: }
The added curly braces should be within `#if PLATFORM(JAVA)`.
Minor: the indentation is off in a couple places.
Minor: add a missing space after the `if` on line 191.
Here is a suggestion:
#if PLATFORM(JAVA)
if (spannerAndPlaceholder.value.get() != nullptr)
{
#endif
if (!placeholder.isDescendantOf(&container))
continue;
placeholdersToRestore.append(&placeholder);
#if PLATFORM(JAVA)
}
#endif
-
PR Review: https://git.openjdk.org/jfx/pull/1350#pullrequestreview-1844057055
PR Review Comment: https://git.openjdk.org/jfx/pull/1350#discussion_r1466568516
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text
On Sun, 10 Sep 2023 20:50:18 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, and IMHO does not need further attent... Regarding other whitespace characters. Using \u2003 em space, pasting the following text AAABBBCCC the results as seen with MS Word 16.78 (23100802) on macOS 14.2.1 indicate that it treats em space as whitespace:   - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1910538102
Re: Thought experiment: constructing Nodes on a different thread...
Yes, I think you are missing something. Unless you have an animation
that just decides all on it's own to find and attach a newly discovered
node to the scene and start rendering it, there will be a
synchronization as a result of calling runLater. Consider the following
code:
Label label = new Label();
label.setText("hi");
// Attach it to the scene on the FX thread.
Platform.runLater(() -> root.getChildren().add(label));
The "runLater" necessarily synchronizes with the FX Application thread
so that the runnable can be put on its queue and later taken off the
queue and executed on the FX Application thread. As specified in the
Platform::runLater API docs: Actions in a thread prior to submitting a
runnable to this method happen-before actions performed by the runnable
in the JavaFX Application Thread.
So I don't see the problem.
-- Kevin
On 1/25/2024 1:50 AM, John Hendrikx wrote:
All this threading talk has made me wonder something:
Let's say there are two threads, the FX thread and Thread 1. I do the
following:
- On thread 1: create Label
- On thread 1: set Label text to "xyz"
I now attach this label to an active Scene graph. This should be done
on the FX thread as we're going to be manipulating an active Scene
graph, so:
- On FX thread: attach Label to active scene graph
There is no synchronization in place on the Label's text field. What
guarantees do I have that when the label is shown on screen it will
show "xyz" ?
IMHO, there is no such guarantee, and so any creation or manipulation
of Nodes that will later be part of an active scene graph (and thus
accessed by the FX thread) **must** be done on the FX thread.
Involving any other thread in their creation or manipulation runs the
risk of seeing an object in the incorrect state (or even an
"impossible" state when multiple fields are involved that normally
change together).
Effectively, assuming that when you create Nodes you always have the
intention of showing them at some point, you can never construct Nodes
on any other thread than the FX thread...
Am I missing something?
--John
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
And that's why the "be careful" option is not a satisfying solution. Let's proceed with the option to change the implementation of play/pause/stop/start to do the work in a runLater, and change the docs accordingly. It's the only feasible option for JavaFX 22 (other than "do nothing"), and I also think it is a good choice. If we later want to evolve it for thread-safety, which I'm not convinced that we do, we can consider that for a future release. -- Kevin On 1/25/2024 5:46 AM, Nir Lisker wrote: Option 3 is basically document it as "be careful" and remove the thread restriction recently introduced, am I correct? Yes :) IMHO that can simply can't work at all, because this is what (theoretically) can happen: 1. Non-FX thread starts animation - Fields are manipulated in AbstractPrimaryTimer - There is no synchronization, so no guarantee anything is flushed (it may all reside in CPU caches) 2. FX Thread becomes involved to actually process the animation - Sees partially flushed state fields, and acts on garbage information (ie. number of animations > 0, but array contains only null's) There just is no safe way to do this in without synchronization or having everything immutable (and this extends to references to "thread safe" structures). What about something like a PauseTransition that doesn't manipulate properties? On Thu, Jan 25, 2024 at 11:02 AM John Hendrikx wrote: On 24/01/2024 22:06, Nir Lisker wrote: This is the ballpark of what I meant with "the downside might be some surprise when these methods behave differently". That can be documented, and basically already is (because that is what the "asynchronous" is alluding to, the fact that after calling "play" the state will change asynchronously). The example you give will only show a potential change if 'play' is not called from the FX thread. In such a case, what's the chance that this is an undeliberate misuse vs. an informed use? This brings me again to: what is the use case for running an animation from a background thread? The only possible use case I can think of is when using FX properties stand-alone (ie. only using javafx.base), without any FX thread involvement. Even in that case though one has to remember that properties themselves are not thread safe either. Any "animation" would need to be done on the same thread that is manipulating properties. However, Animation and Timeline simply can't work in such scenario's, as they're tied to javafx.graphics, to the FX system being started, and the FX thread. For such a use case you'd need to write your own system, or provide the option of specifying an Executor for Animations/Timelines. In your simple example, listening on the Status property would work. Also, while runLater makes no guarantees, I've never seen a non-negligible delay in its execution, so how surprising is it really going to be? If you want to be able to run an animation from a background thread with no race conditions, why not opt for option 3? Option 3 is basically document it as "be careful" and remove the thread restriction recently introduced, am I correct? IMHO that can simply can't work at all, because this is what (theoretically) can happen: 1. Non-FX thread starts animation - Fields are manipulated in AbstractPrimaryTimer - There is no synchronization, so no guarantee anything is flushed (it may all reside in CPU caches) 2. FX Thread becomes involved to actually process the animation - Sees partially flushed state fields, and acts on garbage information (ie. number of animations > 0, but array contains only null's) There just is no safe way to do this in without synchronization or having everything immutable (and this extends to references to "thread safe" structures). --John And option 1 is also new and surprising because now code that worked (or pretended to work) throws, which ruins properly written code (with respect to multithreading), or exposes a bug. On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß wrote: I am not in favor of option 2 if the implementation was simply "wrap the implementation in runLater", as this would be a surprising change. Consider the following code: var transition = new FadeTransition(); transition.play(); // Will always print "RUNNING" currently, but might print "STOPPED" // if we go with the proposed change: System.out.println(transition.getStatus()); Regardless of the race condition in AbstractPrimaryTimer, this code always seemed to be working quite fine (albeit superficially), because the play/stop/etc. methods
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
> > Option 3 is basically document it as "be careful" and remove the thread > restriction recently introduced, am I correct? > Yes :) IMHO that can simply can't work at all, because this is what > (theoretically) can happen: > 1. Non-FX thread starts animation > - Fields are manipulated in AbstractPrimaryTimer > - There is no synchronization, so no guarantee anything is flushed > (it may all reside in CPU caches) > 2. FX Thread becomes involved to actually process the animation > - Sees partially flushed state fields, and acts on garbage > information (ie. number of animations > 0, but array contains only null's) > There just is no safe way to do this in without synchronization or having > everything immutable (and this extends to references to "thread safe" > structures). What about something like a PauseTransition that doesn't manipulate properties? On Thu, Jan 25, 2024 at 11:02 AM John Hendrikx wrote: > > On 24/01/2024 22:06, Nir Lisker wrote: > > This is the ballpark of what I meant with "the downside might be some > surprise when these methods behave differently". > > That can be documented, and basically already is (because that is what the > "asynchronous" is alluding to, the fact that after calling "play" the state > will change asynchronously). > > > The example you give will only show a potential change if 'play' is not > called from the FX thread. In such a case, what's the chance that this is > an undeliberate misuse vs. an informed use? This brings me again to: what > is the use case for running an animation from a background thread? > > The only possible use case I can think of is when using FX properties > stand-alone (ie. only using javafx.base), without any FX thread > involvement. Even in that case though one has to remember that properties > themselves are not thread safe either. Any "animation" would need to be > done on the same thread that is manipulating properties. > > However, Animation and Timeline simply can't work in such scenario's, as > they're tied to javafx.graphics, to the FX system being started, and the FX > thread. For such a use case you'd need to write your own system, or > provide the option of specifying an Executor for Animations/Timelines. > > In your simple example, listening on the Status property would work. Also, > while runLater makes no guarantees, I've never seen a non-negligible delay > in its execution, so how surprising is it really going to be? > > If you want to be able to run an animation from a background thread with > no race conditions, why not opt for option 3? > > Option 3 is basically document it as "be careful" and remove the thread > restriction recently introduced, am I correct? > > IMHO that can simply can't work at all, because this is what > (theoretically) can happen: > > 1. Non-FX thread starts animation > - Fields are manipulated in AbstractPrimaryTimer > - There is no synchronization, so no guarantee anything is flushed > (it may all reside in CPU caches) > > 2. FX Thread becomes involved to actually process the animation > - Sees partially flushed state fields, and acts on garbage > information (ie. number of animations > 0, but array contains only null's) > > There just is no safe way to do this in without synchronization or having > everything immutable (and this extends to references to "thread safe" > structures). > > --John > > > And option 1 is also new and surprising because now code that worked (or > pretended to work) throws, which ruins properly written code (with respect > to multithreading), or exposes a bug. > > On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß > wrote: > >> I am not in favor of option 2 if the implementation was simply "wrap >> the implementation in runLater", as this would be a surprising change. >> Consider the following code: >> >> var transition = new FadeTransition(); >> transition.play(); >> >> // Will always print "RUNNING" currently, but might print "STOPPED" >> // if we go with the proposed change: >> System.out.println(transition.getStatus()); >> >> Regardless of the race condition in AbstractPrimaryTimer, this code >> always seemed to be working quite fine (albeit superficially), because >> the play/stop/etc. methods change that status of the animation as one >> would expect. >> >> You are proposing to change that, such that calling these methods will >> not predictably change the status of the animation. Instead, these >> methods now act more like "requests" that may be fulfilled at some >> later point in time, rather than statements of fact. >> This is not a change that I think we should be doing on an ad-hoc >> basis, since the same considerations potentially apply for many >> methods in many places. >> >> If we were to allow calling play/stop/etc. on a background thread, I >> would be in favor of keeping the semantics that these methods >> instantly and predictably affect the status of the animation. Only the >> internal operation
RFR: JDK-8324337: Cherry-pick WebKit 617.1 stabilization fixes
Cherry-picked changes related to webkit-2.42.4.Verified build on all platforms. Sanity testing looks fine. - Commit messages: - Resolve compilation error - Cherrypick changes related to 2.42.4 Changes: https://git.openjdk.org/jfx/pull/1350/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1350&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324337 Stats: 1294 lines in 161 files changed: 750 ins; 111 del; 433 mod Patch: https://git.openjdk.org/jfx/pull/1350.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1350/head:pull/1350 PR: https://git.openjdk.org/jfx/pull/1350
Re: Thought experiment: constructing Nodes on a different thread...
Isn't that exactly what the Worker interface and its Task and Service implementations are for? Service says A Service is a non-visual component encapsulating the information required > to perform some work on one or more background threads. As part of the > JavaFX UI library, the Service knows about the JavaFX Application thread > and is designed to relieve the application developer from the burden of > managing multithreaded code that interacts with the user interface. The example provided by Jurgen uses Task, which guarantees that the resulting Parent can be attached to the scenegraph on the FX thread (in 'succeeded()'). Up to here everything is fine. The problem starts when the code from inside Task#call, which is called on the background thread, starts an animation itself, an operation that interacts with the FX thread. Task warns about this: Because the Task is designed for use with JavaFX GUI applications, it > ensures that every change to its public properties, as well as change > notifications for state, errors, and for event handlers, all occur on the > main JavaFX application thread. Accessing these properties from a > background thread (including the call() method) will result in runtime > exceptions being raised. and Generally, Tasks should not interact directly with the UI. Doing so creates > a tight coupling between a specific Task implementation and a specific part > of your UI. However, when you do want to create such a coupling, you must > ensure that you use Platform.runLater so that any modifications of the > scene graph occur on the FX Application Thread. So I don't think you're missing something in your description of the problem, and I don't think anyone has argued that doing what you described is not a problem. Thankfully, I don't remember coming across code that does this. People seem to be vigilant about using javafx.concurrent tools for this purpose, which is a good sign. On Thu, Jan 25, 2024 at 11:50 AM John Hendrikx wrote: > All this threading talk has made me wonder something: > > Let's say there are two threads, the FX thread and Thread 1. I do the > following: > > - On thread 1: create Label > - On thread 1: set Label text to "xyz" > > I now attach this label to an active Scene graph. This should be done on > the FX thread as we're going to be manipulating an active Scene graph, so: > > - On FX thread: attach Label to active scene graph > > There is no synchronization in place on the Label's text field. What > guarantees do I have that when the label is shown on screen it will show > "xyz" ? > > IMHO, there is no such guarantee, and so any creation or manipulation of > Nodes that will later be part of an active scene graph (and thus > accessed by the FX thread) **must** be done on the FX thread. Involving > any other thread in their creation or manipulation runs the risk of > seeing an object in the incorrect state (or even an "impossible" state > when multiple fields are involved that normally change together). > > Effectively, assuming that when you create Nodes you always have the > intention of showing them at some point, you can never construct Nodes > on any other thread than the FX thread... > > Am I missing something? > > --John > >
Integrated: 8324270: Update boot JDK to 21.0.2
On Mon, 22 Jan 2024 13:45:35 GMT, Ambarish Rapte wrote: > JDK 21.0.2 is now released : https://jdk.java.net/21/ > Did a full CI build including Webkit and executed all headful tests. This pull request has now been integrated. Changeset: 9e7e8e1d Author:Ambarish Rapte URL: https://git.openjdk.org/jfx/commit/9e7e8e1d08a9d03611da99ef1a7e6e29cd870eca Stats: 11 lines in 2 files changed: 0 ins; 0 del; 11 mod 8324270: Update boot JDK to 21.0.2 Reviewed-by: kcr - PR: https://git.openjdk.org/jfx/pull/1345
Thought experiment: constructing Nodes on a different thread...
All this threading talk has made me wonder something: Let's say there are two threads, the FX thread and Thread 1. I do the following: - On thread 1: create Label - On thread 1: set Label text to "xyz" I now attach this label to an active Scene graph. This should be done on the FX thread as we're going to be manipulating an active Scene graph, so: - On FX thread: attach Label to active scene graph There is no synchronization in place on the Label's text field. What guarantees do I have that when the label is shown on screen it will show "xyz" ? IMHO, there is no such guarantee, and so any creation or manipulation of Nodes that will later be part of an active scene graph (and thus accessed by the FX thread) **must** be done on the FX thread. Involving any other thread in their creation or manipulation runs the risk of seeing an object in the incorrect state (or even an "impossible" state when multiple fields are involved that normally change together). Effectively, assuming that when you create Nodes you always have the intention of showing them at some point, you can never construct Nodes on any other thread than the FX thread... Am I missing something? --John
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
Hi Michael, I'm not quite sure I see the point of this change. The PR did not remove the threading restrictions on play/stop. I'm also confused by the seemingly contradictory statements: - this proposal does NOT allow Animation.play/stop/etc. to be "called on any thread" - It merely removes the requirement that these methods must be called on the FX thread What does that mean exactly? Also, I think the "asynchronous" wording may apply to property changes and action handlers that run at time index 0. These surely won't be ran synchronous when calling "play", and I'm pretty sure they never were. --John On 24/01/2024 22:39, Michael Strauß wrote: Note: this proposal does NOT allow Animation.play/stop/etc. to be "called on any thread" as mentioned in JDK-8324658 [0]. It merely removes the requirement that these methods must be called on the FX thread, but this doesn't make the class inherently thread-safe. That is an important distinction to proposals that call for posting the play/stop calls directly to the FX thread. [0] https://bugs.openjdk.org/browse/JDK-8324658 On Wed, Jan 24, 2024 at 10:30 PM Michael Strauß wrote: Here's another option, which I have implemented as a proof of concept [0]: The play/stop/etc. methods are currently specified to be "asynchronous". This language should be removed, such that the methods will be implied to execute synchronously from the point of view of the caller (like every method that doesn't specify anything to the contrary). All changes to observable state will remain instantly visible for the calling thread. However, internally, interactions with AbstractPrimaryTimer are posted to the FX thread if necessary. This is not an unsurprising change, since the callback from the FX thread was always occuring at an unspecified time in the future. To make this work, AbstractPrimaryTimer::pause/resume/nanos will have to be synchronized to ensure field visibility across threads. In the Animation class, interactions with AbstractPrimaryTimer will be encapsulated in the new nested class AnimationPulseReceiver, which also deduplicates redundant interactions with AbstractPrimaryTimer. For example, repeatedly calling start() and stop() in quick succession may require just a single interaction with AbstractPrimaryTimer in the future (if we ended up in the running state), or no interaction at all (if we ended up in the stopped state). [0] https://github.com/openjdk/jfx/pull/1349
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
On 24/01/2024 22:06, Nir Lisker wrote: This is the ballpark of what I meant with "the downside might be some surprise when these methods behave differently". That can be documented, and basically already is (because that is what the "asynchronous" is alluding to, the fact that after calling "play" the state will change asynchronously). The example you give will only show a potential change if 'play' is not called from the FX thread. In such a case, what's the chance that this is an undeliberate misuse vs. an informed use? This brings me again to: what is the use case for running an animation from a background thread? The only possible use case I can think of is when using FX properties stand-alone (ie. only using javafx.base), without any FX thread involvement. Even in that case though one has to remember that properties themselves are not thread safe either. Any "animation" would need to be done on the same thread that is manipulating properties. However, Animation and Timeline simply can't work in such scenario's, as they're tied to javafx.graphics, to the FX system being started, and the FX thread. For such a use case you'd need to write your own system, or provide the option of specifying an Executor for Animations/Timelines. In your simple example, listening on the Status property would work. Also, while runLater makes no guarantees, I've never seen a non-negligible delay in its execution, so how surprising is it really going to be? If you want to be able to run an animation from a background thread with no race conditions, why not opt for option 3? Option 3 is basically document it as "be careful" and remove the thread restriction recently introduced, am I correct? IMHO that can simply can't work at all, because this is what (theoretically) can happen: 1. Non-FX thread starts animation - Fields are manipulated in AbstractPrimaryTimer - There is no synchronization, so no guarantee anything is flushed (it may all reside in CPU caches) 2. FX Thread becomes involved to actually process the animation - Sees partially flushed state fields, and acts on garbage information (ie. number of animations > 0, but array contains only null's) There just is no safe way to do this in without synchronization or having everything immutable (and this extends to references to "thread safe" structures). --John And option 1 is also new and surprising because now code that worked (or pretended to work) throws, which ruins properly written code (with respect to multithreading), or exposes a bug. On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß wrote: I am not in favor of option 2 if the implementation was simply "wrap the implementation in runLater", as this would be a surprising change. Consider the following code: var transition = new FadeTransition(); transition.play(); // Will always print "RUNNING" currently, but might print "STOPPED" // if we go with the proposed change: System.out.println(transition.getStatus()); Regardless of the race condition in AbstractPrimaryTimer, this code always seemed to be working quite fine (albeit superficially), because the play/stop/etc. methods change that status of the animation as one would expect. You are proposing to change that, such that calling these methods will not predictably change the status of the animation. Instead, these methods now act more like "requests" that may be fulfilled at some later point in time, rather than statements of fact. This is not a change that I think we should be doing on an ad-hoc basis, since the same considerations potentially apply for many methods in many places. If we were to allow calling play/stop/etc. on a background thread, I would be in favor of keeping the semantics that these methods instantly and predictably affect the status of the animation. Only the internal operation of adding the animation to AbstractPrimaryTimer should be posted to the FX thread. If that is not possible, this suggests to me that we should choose option 1. Introducing new and surprising semantics to an existing API is not the way to go. On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker wrote: > > These are the options I see as reasonable: > > 1. Document that these methods *must* be run on the FX thread and throw an exception otherwise. This leaves it to the responsibility of the user. However, this will cause the backwards compatibility problems that Jugen brought up. As a side note, we do this in other methods already, but I always questioned why let the developer do something illegal - if there's only one execution path, force it. > 2. Document that these methods *are* run on the FX thread (the user doesn't need to do anything) and change the implementation to call runLater(...) internally unless they are al
