If there's an agreement, I can do it tomorrow. It will effectively revert JDK-8159048 and supersede JDK-8324219.
On Wed, Jan 24, 2024 at 8:42 PM Kevin Rushforth <kevin.rushfo...@oracle.com> wrote: > I also now favor option 2 and was in the process of writing something up > recommending that we do that. Phil and I (and a couple others) had an > offline discussion and think this is the way to go. > > Given the short amount of time to get this into 22, I will file the JBS > issue in the next hour or so. > > Nir: if you want to take it, that would be great. Otherwise, I will do it. > We need the PR and CSR filed before the end of this week. > > Regarding other methods that choose option 1, many of them are more > complicated (e.g., scene mutation can be done on a background thread as > long as the scene is not "showing") or must be synchronous. Animation > starts something that conceptually happens later on the FX animation thread > anyway, so wrapping it in a runLater (if not already on the right thread) > doesn't really change the semantics in an appreciable way. > > -- Kevin > > > On 1/24/2024 10:26 AM, 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 already on the FX thread. This > will be backwards compatible for the most part (see option 3). The downside > might be some surprise when these methods behave differently. > 3. Document that it's *recommended* that these methods be run on the FX > thread and let the user be responsible for the threading. We can explain > that manipulating nodes that are attached to an active scenegraph should be > avoided. > > I prefer option 2 over 1 regardless of the backwards compatibility issue > even, but would like to know if I'm missing something here because in > theory this change could be done to any "must run on the FX thread" method > and I question why the user had the option to get an exception. > Option 3 is risky and I wager a guess that it will be used wrongly more > often than not. It does allow some (what I would call) valid niche uses. I > never did it. > > On Wed, Jan 24, 2024 at 4:21 PM Kevin Rushforth < > kevin.rushfo...@oracle.com> wrote: > >> I'd like to hear from the others on this. I don't see any fundamental >> problem with having the play/pause/stop methods wrap their implementation >> in a runLater (if not on the FX Application thread already), and >> documenting that it does so, if we can get general agreement. >> >> -- Kevin >> >> >> On 1/24/2024 5:29 AM, Jurgen Doll wrote: >> >> Hi Kevin >> >> If I may make one more final appeal then to an alternative solution >> please. >> >> Could we then instead of throwing an Exception rather invoke runLater if >> needed inside play, stop, and resume. >> >> Putting the onus on the developer is fine if it is the developer that is >> invoking the call, but if it's in a library then it's a no go. >> >> In my application I have two libraries that I know of where this happens. >> The major problem is that with FX22 as it now stands my application just >> crashes because play() does an FX thread check and throws an Exception >> which it never did before. There are bound to be other applications out >> there that are going to find themselves in a similar position. >> >> PLEASE ! >> >> Regards >> Jurgen >> >> >> >> On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth >> <kevin.rushfo...@oracle.com> <kevin.rushfo...@oracle.com> wrote: >> >> Thank you to Jurgen for raising the question and to Nir, John, and >> Michael for evaluating it. >> >> I conclude that there is insufficient motivation to revert the change in >> behavior implemented by JDK-8159048 to allow calling the play/pause/stop >> methods of Animation on a background thread. Doing so without making it >> fully multi-thread-safe would be asking for problems, and making it fully >> multi-thread-safe would be a fair bit of work to do it right without a >> clear benefit. >> >> We will proceed with the current approach and let JDK-8159048 stand. >> Further, we will proceed with https://bugs.openjdk.org/browse/JDK-8324219 >> which is under review in https://github.com/openjdk/jfx/pull/1342 >> <https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/1342__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHiVuWiFRR$> >> >> -- Kevin >> >> On 1/24/2024 12:30 AM, Nir Lisker wrote: >> >> After playing around with the code sample, I think that this is not the >> right way to use the animation. The reason is that there is no point in >> starting the animation before the control is attached to the scenegraph, or >> even made visible. A small refactoring where, e.g., the controller class >> exposes a method to start the animation in onSucceeded or just calls it on >> the FX thread is enough. I never start an animation as part of the >> construction because it's not the right time. John suggested tying the >> lifecycle of the animation to the showing of the node, which also solves >> the problem. >> >> There are animations like PauseTransition or other non-interfering >> Timelines that could reasonably be run on a background thread. Or maybe >> just on an unconnected control. This could be a reason to not limit >> animation methods to the FX thread at the expense of possible user errors, >> but document the pitfall. >> >> I don't see a good use case for modifying controls in a background thread >> while still interacting with the scenegraph, hence for adding multithread >> support. >> >> - Nir >> >> On Mon, Jan 22, 2024, 12:59 Jurgen Doll <jav...@ivoryemr.co.za> wrote: >> >>> Here's an example as requested by Nir: >>> >>> public class FxTimeLineTest extends Application >>> >>> { >>> >>> private BorderPane bp = new BorderPane( new Label("Loading") ); >>> >>> public static void main( String[] args ) { >>> >>> launch( FxTimeLineTest.class, args ); >>> >>> } >>> >>> @Override >>> >>> public void start( Stage primaryStage ) throws Exception { >>> >>> new Thread( new LoadScene() ).start(); >>> >>> primaryStage.setScene( new Scene( bp, 300, 200 ) ); >>> >>> primaryStage.setTitle( "Memory Usage" ); >>> >>> primaryStage.show(); >>> >>> } >>> >>> private class LoadScene extends Task<Parent> { >>> >>> @Override protected Parent call() throws Exception { >>> >>> Parent p = FXMLLoader.load( getClass( ).getResource("TestView.fxml") ); >>> >>> Thread.sleep( 1000 ); >>> >>> return p; >>> >>> } >>> >>> @Override protected void succeeded() { >>> >>> bp.setCenter( getValue() ); >>> >>> } >>> >>> @Override protected void failed() { >>> >>> getException().printStackTrace(); >>> >>> } >>> >>> } >>> >>> } >>> >>> >>> ------------------------------------------------------------------------------------------------------ >>> >>> public class TestView >>> >>> { >>> >>> @FXML private Label memory; >>> >>> private static final double MEGABYTE = 1024 * 1024; >>> >>> @FXML private void initialize() >>> >>> { >>> >>> var updater = new Timeline >>> >>> ( >>> >>> new K eyFrame( Duration.seconds(2.5), event -> >>> >>> { >>> >>> var runtime = Runtime.getRuntime(); >>> >>> double maxMemory = runtime.maxMemory() / MEGABYTE; >>> >>> double usedMemory = (runtime.totalMemory() - runtime.freeMemory()) / >>> MEGABYTE; >>> >>> memory.setText( (int) usedMemory + " MB / " + (int) maxMemory +" MB" ); >>> >>> }) >>> >>> ); >>> >>> updater.setCycleCount(Animation.INDEFINITE); // This FXML is being >>> loaded on a background thread >>> >>> updater.play(); >>> >>> } >>> >>> } >>> >>> >>> ------------------------------------------------------------------------------------------------------ >>> >>> TestView.fxml >>> >>> <?xml version="1.0" encoding="UTF-8"?> >>> >>> <?import javafx.scene.control.Label?> >>> >>> <?import javafx.scene.layout.StackPane?> >>> >>> <StackPane xmlns:fx="http://javafx.com/fxml/1 >>> <https://urldefense.com/v3/__http://javafx.com/fxml/1__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHia1cBAoi$>" >>> fx:controller="TestView"> >>> >>> <children> >>> >>> <Label fx:id="memory" text="Current / Max MB" > >>> >>> <properties hashCode="12345" /> >>> >>> </Label> >>> >>> </children> >>> >>> </StackPane> >>> >>> >>> >>> On Sat, 20 Jan 2024 17:08:41 +0200, Nir Lisker <nlis...@gmail.com> >>> wrote: >>> >>> Hi Jurgen, >>> >>> What I'm confused about the most is what it is you are actually trying >>> to do that necessitates the use of animations outside of the FX thread. You >>> said that you need to initialize controls on another thread, and that you >>> are using Task (both of which are fine), but how does playing animations >>> relate? Playing an animation is something that is done explicitly, usually >>> in order to manipulate data. Can you give a real use case, like a minimized >>> version of what you're doing? >>> >>> - Nir >>> >>> >> >> >> >> -- >> Using Opera's mail client: http://www.opera.com/mail/ >> <https://urldefense.com/v3/__http://www.opera.com/mail/__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHifb0G69Z$> >> >> >> >
