-
Notifications
You must be signed in to change notification settings - Fork 28.6k
Reland "Do not rebuild Routes when a new opaque Route is pushed on top" #49376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@dkwingsmt for review of the changes to the stepper test: https://github.com/flutter/flutter/pull/49376/files#diff-cf08a14b525117a713c3170d35000be3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LTGM on the fix in stepper_test.dart
49be385
to
18020a9
Compare
This should not be submitted until the upstream fixes have been submitted to the internal google code base. |
18020a9
to
97cfd7a
Compare
Upstream fixes are in, this is ready for submission when Cirrus is done. |
Before, active overlays used the TickerMode of the tree above their parent Overlay widget. This worked nicely with nested Overlays/Navigators, because all animations could be disabled at by the topmost navigator. After the change, a regression was introduced where Overlays that were offstage (inside of another Overlay) would continue ticking, consuming CPU and GPU time for animations that wouldn't be seen.
See #48900 for full description of this change.
The reason for the revert turned out to be erroneous user code that was missing setState calls. The code was instead relying on the implicit rebuilds caused by pushing/popping routes which this PR removes. The google-internal tests in question have been fixed.
Reverts #49366
Migration guide: flutter/website#3581