Do not dispose CupertinoSheetTransition animation on update and throw ticker error #180609
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a ticker-related exception in CupertinoSheetTransition by adjusting the lifecycle of _stretchDragController. Moving its initialization to initState and disposal to dispose is the right approach to prevent issues during widget updates. The new test case provides good coverage for the fix. I have one suggestion to clean up some unused code in the test file.
Comment on lines
16
to
77
| class _RebuildingPage extends StatefulWidget { | ||
| const _RebuildingPage({required this.scaffoldKey}); | ||
|
|
||
| final GlobalKey scaffoldKey; | ||
|
|
||
| @override | ||
| _RebuildingPageState createState() => _RebuildingPageState(); | ||
| } | ||
|
|
||
| class _RebuildingPageState extends State<_RebuildingPage> { | ||
| late int counter; | ||
|
|
||
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| counter = 0; | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return CupertinoApp( | ||
| home: CupertinoPageScaffold( | ||
| key: widget.scaffoldKey, | ||
| child: Center( | ||
| child: Column( | ||
| children: <Widget>[ | ||
| const Text('Page 1'), | ||
| CupertinoButton( | ||
| onPressed: () { | ||
| Navigator.push<void>( | ||
| widget.scaffoldKey.currentContext!, | ||
| CupertinoSheetRoute<void>( | ||
| builder: (BuildContext context) { | ||
| return CupertinoPageScaffold( | ||
| child: Center( | ||
| child: Column( | ||
| children: <Widget>[ | ||
| const Text('Page two'), | ||
| Text('Counter Value: $counter'), | ||
| CupertinoButton( | ||
| onPressed: () => setState(() { | ||
| counter++; | ||
| }), | ||
| child: const Text('Increase Count'), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ); | ||
| }, | ||
| ), | ||
| ); | ||
| }, | ||
| child: const Text('Push Page 2'), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The _RebuildingPage widget and its state _RebuildingPageState appear to be unused in this test file. To improve code clarity and maintainability, it's best to remove this dead code. If it was added for debugging purposes, it should be removed before merging.
|
|
||
| final newAnimation = AnimationController(vsync: const TestVSync()); | ||
|
|
||
| // Should not throw an exception. |
Contributor
There was a problem hiding this comment.
Drive by nit, we can probably assert this with expect(tester.takeException(), isNull);
since otherwise this would throw an AssertionError ?
chunhtai
approved these changes
Jan 7, 2026
Contributor
chunhtai
left a comment
There was a problem hiding this comment.
LGTM, except for minor comments
| reverseCurve: Curves.easeInToLinear, | ||
| parent: widget.secondaryRouteAnimation, | ||
| ); | ||
| _stretchDragController = AnimationController( |
Contributor
There was a problem hiding this comment.
should we do a _stretchDragController.reset() here?
Contributor
Author
There was a problem hiding this comment.
I don't think so. I don't see a need to reseting the value and casing the jump on this update flow. I'd think we would want to hold the old status and value if it's non-zero.
|
|
||
| final newAnimation = AnimationController(vsync: const TestVSync()); | ||
|
|
||
| // Should not throw an exception. |
d09bdbd to
75fc3cd
Compare
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2026
…nd throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2026
…nd throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2026
…nd throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2026
…nd throw ticker error (flutter/flutter#180609)
This was referenced Jan 8, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 9, 2026
…nd throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 9, 2026
… and throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 9, 2026
…nd throw ticker error (flutter/flutter#180609)
This was referenced Jan 9, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 9, 2026
…nd throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 10, 2026
…nd throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 10, 2026
…nd throw ticker error (flutter/flutter#180609)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 10, 2026
…nd throw ticker error (flutter/flutter#180609)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #179337 by moving the disposal of an animation controller outside of the didUpdate method flow. It was unnecessary to rebuild this controller on update, and was originally put there to follow the pattern of the other animations. However, unlike the other animations, this one is not chained to animations from outside of the transition's place in the tree, and was throwing exceptions from the ticker mixin in some cases.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.