Add a RepeatingAnimationBuilder API#174014
Conversation
53cabd8 to
3dcc5eb
Compare
| /// Unlike the default constructor, [onEnd] is not supported for repeating | ||
| /// animations since they never end. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Could you move the Ownership section up above the code samples?
| /// ## 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]. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
5b96c99 to
e62152e
Compare
|
I need your help @loic-sharma, I have a leak test failing and I believe it is because of |
|
@bernaferrari I would go ahead and make the widget non- 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 |
| /// | ||
| /// 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; |
There was a problem hiding this comment.
TweenAnimationBuilder.builder has type ValueWidgetBuilder<T> - should we use the same type here to be consistent?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 theSizeTransitionexample, we should consider switching toClipRect+Aligninstead. ForRotationTransition, we should consider switching to aTransforminstead. - 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 aT. - Option 3: Don't use
RepeatingTweenAnimationBuilder. Go back to an explicit animation instead. Examples for this category:AnimatedIconanimations,FadeTransitionanimations (this has important optimizations over usingOpacitydirectly), etc.
Let me know if you have any feedback or questions!
|
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 We can:
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). |
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Should I call |
|
@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. |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
I drafted a suggestion below: https://github.com/flutter/flutter/pull/174014/files#r2519860585
dkwingsmt
left a comment
There was a problem hiding this comment.
Comprehensive tests. Clear documents. Thank you so much!
| /// The animatable to drive repeatedly. Typically this is a [Tween] or a | ||
| /// [TweenSequence], but any [Animatable] that produces values of type [T] | ||
| /// is accepted. |
There was a problem hiding this comment.
| /// 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. |
There was a problem hiding this comment.
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'; |
loic-sharma
left a comment
There was a problem hiding this comment.
I left some minor documentation suggestions.
Thank you for doing this! This is truly outstanding work :)
|
@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 :) |
|
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 |
|
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. |


Implements / Close #174011