Add a RepeatingAnimationBuilder API
#174014
Conversation
github-actions
bot
added
framework
bernaferrari
force-pushed
the
tween
branch
2 times, most recently
from
53cabd8 to
3dcc5eb
Compare
github-actions
bot
added
the
a: text input
| /// 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.
bernaferrari
force-pushed
the
tween
branch
5 times, most recently
from
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?
Examples like this would suffer:
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. |
flutter-dashboard
bot
removed
the
autosubmit
…#10455) Manual roll Flutter from 9b5ad8031646 to cc14ef529014 (143 revisions) Manual roll requested by [email protected] flutter/flutter@9b5ad80...cc14ef5 2025-11-17 [email protected] [ Widget Preview ] Fix crash when `widget_preview_scaffold/.dart_tool` doesn't exist (flutter/flutter#178662) 2025-11-17 [email protected] Dev proxy correctly copy query parameters from original request (flutter/flutter#178162) 2025-11-17 [email protected] Roll Dart SDK from ac5942fe75d0 to cf94632d94a1 (1 revision) (flutter/flutter#178664) 2025-11-17 [email protected] Improve code quality `FlutterViewTest.java` (flutter/flutter#178594) 2025-11-17 [email protected] Roll customer tests (flutter/flutter#178652) 2025-11-17 [email protected] Corrects invalid Flutter wiki links (flutter/flutter#178158) 2025-11-17 [email protected] Fix LateInitializationError in didChangeTextScaleFactor code example. (flutter/flutter#178375) 2025-11-17 [email protected] Roll Skia from d877de9fc23e to 84c83c0dfb4a (3 revisions) (flutter/flutter#178653) 2025-11-17 [email protected] Small refactor in `DartMessenger.java` to use method reference (flutter/flutter#178584) 2025-11-17 [email protected] fix typos in `PlatformChannel.java` docs (flutter/flutter#178589) 2025-11-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Enable UIScene Migration and update create templates (#178328)" (flutter/flutter#178665) 2025-11-17 [email protected] Remove unnecessary `String.valueOf` in `ListenableEditingState.java` (flutter/flutter#178597) 2025-11-17 [email protected] Update `Metadata.java` to use `getBytes` in a static context (flutter/flutter#178587) 2025-11-17 [email protected] Reduce the data copying in CanvasPath related to the SkPathBuilder API migration (flutter/flutter#178512) 2025-11-17 [email protected] Roll Packages from 799b62c to ce44ebb (10 revisions) (flutter/flutter#178649) 2025-11-17 [email protected] Enable UIScene Migration and update create templates (flutter/flutter#178328) 2025-11-17 [email protected] Roll Skia from 5fb44352b232 to d877de9fc23e (1 revision) (flutter/flutter#178647) 2025-11-17 [email protected] Revert "Separate copying dsym into its own target (#178261)" (flutter/flutter#178545) 2025-11-17 [email protected] [web] Reduce Skwasm test shards to 2 (flutter/flutter#178239) 2025-11-17 [email protected] Roll Dart SDK from ee793f732959 to ac5942fe75d0 (1 revision) (flutter/flutter#178637) 2025-11-17 [email protected] Roll Skia from b30859d35b89 to 5fb44352b232 (1 revision) (flutter/flutter#178636) 2025-11-17 [email protected] Roll Skia from 5b7f8f61528e to b30859d35b89 (1 revision) (flutter/flutter#178634) 2025-11-17 [email protected] Roll Skia from aad0515710a9 to 5b7f8f61528e (7 revisions) (flutter/flutter#178631) 2025-11-17 [email protected] Roll Skia from 761e1fe67e15 to aad0515710a9 (1 revision) (flutter/flutter#178624) 2025-11-16 [email protected] Roll Skia from 5f9157717150 to 761e1fe67e15 (1 revision) (flutter/flutter#178614) 2025-11-16 [email protected] Roll Skia from 1b435b65e754 to 5f9157717150 (1 revision) (flutter/flutter#178612) 2025-11-16 [email protected] Roll Dart SDK from dffc5f3f9be6 to ee793f732959 (1 revision) (flutter/flutter#178607) 2025-11-16 [email protected] Roll Skia from ce2168cf2b6c to 1b435b65e754 (1 revision) (flutter/flutter#178604) 2025-11-15 [email protected] Roll Dart SDK from 50eca21d80c4 to dffc5f3f9be6 (1 revision) (flutter/flutter#178603) 2025-11-15 [email protected] [macOS] Implement dialog window (flutter/flutter#176893) 2025-11-15 [email protected] Roll Dart SDK from f6f88ed5e5a1 to 50eca21d80c4 (1 revision) (flutter/flutter#178599) 2025-11-15 [email protected] Roll Dart SDK from 9eda5b525ca9 to f6f88ed5e5a1 (1 revision) (flutter/flutter#178576) 2025-11-15 [email protected] Roll Skia from c5c3399b3a84 to ce2168cf2b6c (1 revision) (flutter/flutter#178577) 2025-11-15 [email protected] Add a `RepeatingAnimationBuilder` API (flutter/flutter#174014) 2025-11-15 [email protected] Roll Dart SDK from 486be9c17488 to 9eda5b525ca9 (1 revision) (flutter/flutter#178562) 2025-11-15 [email protected] Roll Skia from f5d29842bf45 to c5c3399b3a84 (1 revision) (flutter/flutter#178559) 2025-11-14 [email protected] Roll Skia from f58b1b8052a2 to f5d29842bf45 (3 revisions) (flutter/flutter#178556) 2025-11-14 [email protected] Roll Dart SDK from c6c5f6c169a9 to 486be9c17488 (1 revision) (flutter/flutter#178555) 2025-11-14 [email protected] [ Widget Preview ] Gracefully handle unexpected analysis context disposal (flutter/flutter#178550) 2025-11-14 [email protected] docs: Add documentation for running devicelab tests in a PR (flutter/flutter#178554) 2025-11-14 [email protected] [Reland] Generate new Gradle lockfiles and delete ignore files (flutter/flutter#178553) 2025-11-14 [email protected] Fix crash when doing a SaveLayer under a non-invertible transform (flutter/flutter#178507) 2025-11-14 [email protected] [Reland] Generate ignore lockfiles and Update Android Embedder Dependencies (flutter/flutter#178538) 2025-11-14 [email protected] Roll Skia from 4c02d9fc79c1 to f58b1b8052a2 (1 revision) (flutter/flutter#178543) ...
Implements / Close flutter#174011 Probably there are 400 LOC of comments + tests, so this removes more code than adds. <img width="1509" height="649" alt="diff" src="https://github.com/user-attachments/assets/cd961ddc-3b79-4ffd-8e9a-a806d5743f17" />
Implements / Close flutter#174011 Probably there are 400 LOC of comments + tests, so this removes more code than adds. <img width="1509" height="649" alt="diff" src="https://github.com/user-attachments/assets/cd961ddc-3b79-4ffd-8e9a-a806d5743f17" />
Implements / Close flutter#174011 Probably there are 400 LOC of comments + tests, so this removes more code than adds. <img width="1509" height="649" alt="diff" src="https://github.com/user-attachments/assets/cd961ddc-3b79-4ffd-8e9a-a806d5743f17" />
6 participants
Implements / Close #174011