Skip to content

Add a RepeatingAnimationBuilder API#174014

Merged
auto-submit[bot] merged 9 commits intoflutter:masterfrom
bernaferrari:tween
Nov 15, 2025
Merged

Add a RepeatingAnimationBuilder API#174014
auto-submit[bot] merged 9 commits intoflutter:masterfrom
bernaferrari:tween

Conversation

@bernaferrari
Copy link
Contributor

@bernaferrari bernaferrari commented Aug 19, 2025

Implements / Close #174011

diff

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Aug 19, 2025
@bernaferrari bernaferrari force-pushed the tween branch 2 times, most recently from 53cabd8 to 3dcc5eb Compare August 19, 2025 06:27
@github-actions github-actions bot added the a: text input Entering text in a text field or keyboard related problems label Aug 19, 2025
Comment on lines +146 to +147
/// Unlike the default constructor, [onEnd] is not supported for repeating
/// animations since they never end.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd consider dropping this line - calling out something that's not present might confuse users.

/// * [AnimationController.repeat], which this constructor replaces for simple use cases.
/// * [TweenAnimationBuilder], the default constructor for single animations.
///
/// ## Ownership
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move the Ownership section up above the code samples?

Comment on lines +194 to +199
/// ## Ownership
///
/// The [TweenAnimationBuilder] takes full ownership of the provided [tween]
/// instance and mutates it. Once a [Tween] has been passed to a
/// [TweenAnimationBuilder], its properties should not be accessed or changed
/// anymore to avoid interference with the [TweenAnimationBuilder].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This content mirrors but is slightly different from the "Ownership of the Tween" content on TweenAnimationBuilder. I'd consider making a template out of the original content and just use the template here.

https://github.com/flutter/flutter/blob/6cbd98adc517ca7a20c724c4f0c5a5a385b95f74/packages/flutter/lib/src/widgets/tween_animation_builder.dart#L58C1-L67C34

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue of making a template (now) where it is a separate component is that I can't repeat "The [TweenAnimationBuilder] takes full ownership" unless it accepts variable so might make sense to repeat now :( or just omit or duplicate.

@bernaferrari bernaferrari force-pushed the tween branch 5 times, most recently from 5b96c99 to e62152e Compare August 22, 2025 06:03
@bernaferrari
Copy link
Contributor Author

I need your help @loic-sharma, I have a leak test failing and I believe it is because of assert() which checks if tween is valid (has begin and end). I don't know how to solve that. I can move the assert to the constructor, but then it can't be const, or I can just remove the assert (but then if someone forgets begin or end will get runtime issues). What do you think? Shall I remove const or is there a simple good way to solve this problem?

@loic-sharma
Copy link
Member

loic-sharma commented Sep 4, 2025

@bernaferrari I would go ahead and make the widget non-const for now.

It looks like leaking objects in exceptional situations (like a test for error conditions) is a known issue: #157470. When we fix this, we can make this widget const :)

@bernaferrari bernaferrari marked this pull request as ready for review September 5, 2025 18:57
///
/// The builder receives the animation object and the optional child.
/// The current value can be accessed via animation.value.
final Widget Function(BuildContext context, Animation<T> animation, Widget? child) builder;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TweenAnimationBuilder.builder has type ValueWidgetBuilder<T> - should we use the same type here to be consistent?

Copy link
Contributor Author

@bernaferrari bernaferrari Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky. ValueWidgetBuilder takes the actual value T, while RepeatingTweenAnimationBuilder is currently taking Animation.
The reason is, many examples require animation, so it doesn't make sense for us to do animation.value and then create a new FixedAnimation(value) or something like that, I thought it made more sense to just use animation directly instead of animation.value, but it makes it different than TweenAnimationBuilder, so it is tricky.

I wish Repeating had value instead of animation, but then it will make the code for example worse than right now. Any thoughts?

Examples like this would suffer:
image

Copy link
Member

@loic-sharma loic-sharma Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR: We should use ValueWidgetBuilder<T> here as well.

Whether the builder accepts a T or a Animation<T> has implications on how many times the builder should be called:

FooAnimationBuilder(
  tween: Tween<double>(begin: 0, end: 1),
  builder: (context, double sizeFactor, child) {
    // This builder is called each time sizeFactor changes.
    // The subtree is rebuilt on each tick.
    return ...;
  },
);

FooAnimationBuilder(
  tween: Tween<double>(begin: 0, end: 1),
  builder: (context, Animation<double> sizeFactorAnimation, child) {
    // This builder should be called ONCE. You or some child widget needs an
    // AnimatedBuilder to trigger a callback each time sizeFactorAnimation
    // changes. This lets you minimize the subtree that is rebuilt each
    // time the animation ticks.
    return ...;
  },
);

Since TweenAnimationBuilder uses the former pattern (T), RepeatingTweenAnimationBuilder should use the same pattern - it'd be confusing otherwise. I do think the latter pattern (Animation<T>) is something we should consider, however let's punt that until after this PR as I'd like to come up with a new naming convention that makes the distinction clear.


Counterintuitively, the AlwaysStoppedAnimation thing is a good thing. It's a code smell that indicates we're using some API incorrectly:

  • Option 1: Switch to a child widget that doesn't accept an Animation<T>. For the SizeTransition example, we should consider switching to ClipRect + Align instead. For RotationTransition, we should consider switching to a Transform instead.
  • Option 2: Consider whether the child widget actually needs to accept an Animation<T>. If the child widget rebuilds itself on each tick, it should might as well just accept a T.
  • Option 3: Don't use RepeatingTweenAnimationBuilder. Go back to an explicit animation instead. Examples for this category: AnimatedIcon animations, FadeTransition animations (this has important optimizations over using Opacity directly), etc.

Let me know if you have any feedback or questions!

@bernaferrari
Copy link
Contributor Author

bernaferrari commented Sep 23, 2025

Well, I finished it, but I guess I took too long :( This completely breaks my PR: #174605

We can still merge it, but I think it will only be useful for samples. The biggest reason for this PR (progress indicator) is now even more complex with custom controller. I could do something like, if no custom controller is provided, use RepeatingTweenAnimationBuilder else do the standard behavior, but it will be duplicated code, so I'm not sure how to progress.

We can:

  • When not using custom controller, use RepeatingTweenAnimationBuilder (but might have duplicated rendering logic)
  • I guess allow custom AnimationController inside RepeatingTweenAnimationBuilder
  • Merge but only use on examples, not on ProgressIndicator
  • Don't merge.

Any thoughts? For now, I'm going for option 1, it is a bit more code but should work. I'll fix whatever is causing the golden later (interesting there was no golden issue before).

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #174014 at sha a096c63

@bernaferrari
Copy link
Contributor Author

Should I call autosubmit or does anyone want to review anything? @loic-sharma

@loic-sharma
Copy link
Member

@bernaferrari Let me take another look tomorrow since there were significant changes since I approved. Thanks for bearing with us, we’re almost there! 😂

/// Each iteration starts over from the beginning once 1.0 is reached.
restart,

/// Each iteration runs forward, then reverses back to the beginning.
Copy link
Contributor

@dkwingsmt dkwingsmt Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document whether this means the duration is effectively doubled for each actual iteration, or that the curve is calculated at double speed?

(Alternatively we can document it in RepeatingAnimationBuilder or even in RepeatingAnimationBuilder.duration but I think documenting it here is more useful if this enum is to be used in other widgets. But feel free to do it this way if you find it easier.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive tests. Clear documents. Thank you so much!

Comment on lines +57 to +59
/// The animatable to drive repeatedly. Typically this is a [Tween] or a
/// [TweenSequence], but any [Animatable] that produces values of type [T]
/// is accepted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The animatable to drive repeatedly. Typically this is a [Tween] or a
/// [TweenSequence], but any [Animatable] that produces values of type [T]
/// is accepted.
/// The animatable to drive repeatedly.
///
/// Typically this is a [Tween] or a [TweenSequence], but any [Animatable]
/// that produces values of type [T] is accepted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this is needed because the first paragraph is what shows up in the IDE. The IDE tooltip is small, so for readability a single sentence is best.


import 'framework.dart';
import 'implicit_animations.dart';
import 'transitions.dart';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some minor documentation suggestions.

Thank you for doing this! This is truly outstanding work :)

@loic-sharma
Copy link
Member

@bernaferrari There's a few minor nitpicks left and then we can land this! Let us know if you'd like us to just apply the docs suggestions directly :)

@bernaferrari
Copy link
Contributor Author

I need to find the answer for this. Don't worry. I'll get that later today. Thanks for collaborating over 3 months for this.
image

@bernaferrari bernaferrari added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Nov 15, 2025
Merged via the queue into flutter:master with commit 1814874 Nov 15, 2025
76 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2025
@jmagman
Copy link
Member

jmagman commented Feb 11, 2026

Thanks you so much for this contribution @bernaferrari, shout out in the Flutter 3.41 blog post: https://blog.flutter.dev/whats-new-in-flutter-3-41-302ec140e632#3c0b

@bernaferrari
Copy link
Contributor Author

Yay! I'm very happy! Thanks everybody for the help too!! If you have anything interesting but lack the time to implement/test, don't hesitate in asking for my help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: animation Animation APIs d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a RepeatedTweenAnimationBuilder API

7 participants