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
