New API: Animation/Timeline improvement

[John Hendrikx]

I don't think that's the correct solution, as it opens up a whole new 
avenue for subtle bugs, bringing GC/JVM/OS whims and settings into the 
mix. We want the clean-up to be easier to reason about, not harder.

Even if it were a good idea, it's likely also going to be a breaking 
change to add weak references at this stage, without some kind of opt-in 
(which would then require new API, in which case we might as well go for 
a solution that has no need of weak references).

I feel I have to keep repeating this, but there almost zero guarantees 
made by the JVM or Java with regards to references; they are fine for 
internal processes carefully designed to have no user visible side 
effects, but burdening the user with side effects (delayed clean-up, or 
early unexpected clean-up due to lack of a hard reference) without the 
user actually choosing to use a reference type themselves is not a good 
design.

--John

On 18/12/2023 17:18, Andy Goryachev wrote:
>
> Would making Timeline to use WeakReferences solve the issue without 
> the need for a new API?
>
> -andy
>
> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of John 
> Hendrikx <john.hendrikx at gmail.com>
> *Date: *Friday, December 15, 2023 at 21:53
> *To: *openjfx-dev <openjfx-dev at openjdk.org>
> *Subject: *New API: Animation/Timeline improvement
>
> Hi list,
>
> I've noticed that Animations and Timelines are often a source of leaks,
> and their clean-up is often either non-existent or incorrect.  The
> reason they cause leaks easily is because a running animation or
> timeline is globally referred from a singleton PrimaryTimer.  The
> animation or timeline then refers to properties or event handlers which
> refer to controls (which refer to parents and the entire scene).
>
> For example:
>
> - ScrollBarBehavior uses a Timeline, but neglects to clean it up.  If it
> was running at the time a Scene is detached from a Window, and that
> Scene is left to go out of scope, it won't because Timeline refers it;
> this can happen if the behavior never receives a key released event.
>
> - ScrollBarBehavior has no dispose method overridden, so swapping Skins
> while the animation is running will leave a Timeline active (it uses
> Animation.INDEFINITE)
>
> - SpinnerBehavior has flawed clean up; it attaches a Scene listener and
> disables its timeline when the scene changed, but the scene doesn't have
> to change for it to go out of scope as a whole... Result is that if you
> have a spinner timeline running, and you close the entire window (no
> Scene change happens), the entire Scene will still be referred.  It also
> uses an indefinite cycle count.  It also lacks a dispose method, so
> swapping Skins at a bad moment can also leave a reference.
>
> I think these mistakes are common, and far too easy to make.  The most
> common use cases for animations revolve around modifying properties on
> visible controls, and although animations can be used for purposes other
> than animating UI controls, this is extremely rare. So it is safe to
> say that in 99% of cases you want the animation to stop once a some Node
> is no longer showing. For both the mentioned buggy behaviors above, this
> would be perfect.  A spinner stops spinning when no longer showing, and
> a scroll bar stops scrolling when no longer showing. It is also likely
> to apply for many other uses of timelines and animations.
>
> I therefore want to propose a new API, either on Node or Animation (or
> both):
>
>      /**
>       * Creates a new timeline which is stopped automatically when 
> this Node
>       * is no longer showing. Stopping timelines is essential as they
> may refer
>       * nodes even after they are no longer used anywhere, preventing
> them from
>       * being garbage collected.
>       */
>      Node.createTimeline();  // and variants with the various Timeline
> constructors
>
> And/or:
>
>      /**
>       * Links this Animation to the given Node, and stops the animation
>       * automatically when the Node is no longer showing. Stopping
> animations
>       * is essential as they may refer nodes even after they are no
> longer used
>       * anywhere, preventing them from being garbage collected.
>       */
>      void stopWhenHidden(Node node);
>
> The above API for Animation could also be provided through another
> constructor, which takes a Node which will it be linked to.
>
> Alternatives:
>
> - Be a lot more diligent about cleaning up animations and timelines
> (essentially maintain the status quo which has led to above bugs)
> - Use this lengthy code fragment below:
>
>      Timeline timeline = new Timeline();
>
>      someNode.sceneProperty()
>          .when(timeline.statusProperty().map(status -> status !=
> Status.STOPPED))
>          .flatMap(Scene::windowProperty)
>          .flatMap(Window::showingProperty)
>          .orElse(false)
>          .subscribe(showing -> {
>              if (!showing) timeline.stop();
>          });
>
> The `when` line ensures that the opposite problem (Nodes forever
> referencing Timelines) doesn't occur if you are creating a new Timeline
> for each use (not recommended, but nonetheless a common occurrence).
>
> --John
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20231219/944df4ef/attachment-0001.htm>