Skip to content

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

Merged
merged 3 commits into from
Jan 28, 2020

Conversation

goderbauer
Copy link
Member

@goderbauer goderbauer commented Jan 23, 2020

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

@fluttergithubbot fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 23, 2020
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

@goderbauer
Copy link
Member Author

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.

LTGM on the fix in stepper_test.dart

@goderbauer goderbauer force-pushed the revert-49366-revert-48900-theatre2 branch from 49be385 to 18020a9 Compare January 23, 2020 23:54
@goderbauer
Copy link
Member Author

This should not be submitted until the upstream fixes have been submitted to the internal google code base.

@goderbauer goderbauer force-pushed the revert-49366-revert-48900-theatre2 branch from 18020a9 to 97cfd7a Compare January 24, 2020 17:51
@goderbauer goderbauer added waiting for tree to go green and removed f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Jan 24, 2020
@goderbauer
Copy link
Member Author

Upstream fixes are in, this is ready for submission when Cirrus is done.

@goderbauer goderbauer merged commit bc5ea0a into master Jan 28, 2020
@goderbauer goderbauer deleted the revert-49366-revert-48900-theatre2 branch January 28, 2020 07:56
ds84182 added a commit that referenced this pull request Feb 7, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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.
@liyuqian liyuqian added perf: speed Performance issues related to (mostly rendering) speed c: performance Relates to speed or footprint issues (see "perf:" labels) labels May 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) framework flutter/packages/flutter repository. See also f: labels. perf: speed Performance issues related to (mostly rendering) speed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants