Skip to content

Convert AnimatedSize to a StatefulWidget#80554

Merged
fluttergithubbot merged 1 commit intoflutter:masterfrom
jason-simmons:animated_size_leak_2
Apr 22, 2021
Merged

Convert AnimatedSize to a StatefulWidget#80554
fluttergithubbot merged 1 commit intoflutter:masterfrom
jason-simmons:animated_size_leak_2

Conversation

@jason-simmons
Copy link
Member

@jason-simmons jason-simmons commented Apr 16, 2021

This fixes a memory leak in which RenderAnimatedSize instances were
creating tickers in the TickerProvider but were not disposing them.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Apr 16, 2021

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Apr 16, 2021
@google-cla google-cla bot added the cla: yes label Apr 16, 2021
@jason-simmons jason-simmons mentioned this pull request Apr 16, 2021
Closed
goderbauer
goderbauer approved these changes Apr 16, 2021
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/widgets/animated_size.dart Outdated
Copy link
Member

@goderbauer goderbauer Apr 16, 2021

Choose a reason for hiding this comment

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

nit: these deprecation notices need to follow a certain pattern, see "Breaking Changes" in the wiki.

Copy link
Member Author

@jason-simmons jason-simmons Apr 16, 2021

Choose a reason for hiding this comment

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

done

packages/flutter/lib/src/widgets/animated_size.dart Outdated
Copy link
Member

@goderbauer goderbauer Apr 16, 2021

Choose a reason for hiding this comment

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

keep some doc on the constructor?

Copy link
Member Author

@jason-simmons jason-simmons Apr 16, 2021

Choose a reason for hiding this comment

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

done

packages/flutter/lib/src/widgets/animated_size.dart Outdated
Copy link
Member

@goderbauer goderbauer Apr 16, 2021

Choose a reason for hiding this comment

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

this doc can probably be removed since the constructor is private and this doesn't contain much information...

Copy link
Member Author

@jason-simmons jason-simmons Apr 16, 2021

Choose a reason for hiding this comment

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

done

@jason-simmons jason-simmons force-pushed the animated_size_leak_2 branch 2 times, most recently from 259bcf7 to ca76263 Compare April 16, 2021 22:42
jason-simmons added a commit to jason-simmons/flutter_gallery that referenced this pull request Apr 20, 2021
@jason-simmons
Remove the AnimatedSize vsync parameter
0b5bcb1 
This is needed in order to land flutter/flutter#80554,
which will deprecate AnimatedSize.vsync
@jason-simmons jason-simmons mentioned this pull request Apr 20, 2021
Merged
jason-simmons added a commit to jason-simmons/rainbowmonkey that referenced this pull request Apr 20, 2021
@jason-simmons
Remove the AnimatedSize vsync parameter
7d5cdd5 
This is needed in order to land flutter/flutter#80554,
which will deprecate AnimatedSize.vsync
@jason-simmons jason-simmons mentioned this pull request Apr 20, 2021
Merged
@jason-simmons
Copy link
Member Author

jason-simmons commented Apr 20, 2021

Removed the @deprecated attribute in order to simplify landing the API change.

AnimatedSize.vsync will be @deprecated in a follow-up PR.

PTAL @goderbauer

jason-simmons added a commit to jason-simmons/flutter_gallery that referenced this pull request Apr 21, 2021
@jason-simmons
Remove the AnimatedSize vsync parameter
9817044 
This is needed in order to land flutter/flutter#80554,
which will deprecate AnimatedSize.vsync
goderbauer
goderbauer approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/widgets/animated_size.dart Outdated
Copy link
Member

@goderbauer goderbauer Apr 21, 2021

Choose a reason for hiding this comment

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

Can you phrase this as a "TODO(xx): deprecate when customer tests are updated"?

Copy link
Member Author

@jason-simmons jason-simmons Apr 21, 2021

Choose a reason for hiding this comment

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

done

@jason-simmons
Convert AnimatedSize to a StatefulWidget
13aacfc 
This fixes a memory leak in which RenderAnimatedSize instances were
creating tickers in the TickerProvider but were not disposing them.
@fluttergithubbot fluttergithubbot merged commit a21a22a into flutter:master Apr 22, 2021
jason-simmons added a commit to flutter/gallery that referenced this pull request Apr 22, 2021
@jason-simmons
Remove the AnimatedSize vsync parameter (#480)
e06144f 
This is needed in order to land flutter/flutter#80554,
which will deprecate AnimatedSize.vsync
Hixie pushed a commit to jocosocial/rainbowmonkey that referenced this pull request Apr 22, 2021
@jason-simmons
Remove the AnimatedSize vsync parameter (#335)
99f5ec2 
This is needed in order to land flutter/flutter#80554,
which will deprecate AnimatedSize.vsync
renyou pushed a commit to renyou/flutter that referenced this pull request Apr 26, 2021
@jason-simmons @renyou
Convert AnimatedSize to a StatefulWidget (flutter#80554)
44462d6
renyou added a commit that referenced this pull request Apr 26, 2021
@renyou @jmagman @jason-simmons
Revert "[RenderEditable] Dont paint caret when selection is invalid" #…
d72bfb8 
…81076 (#81155)

* Revert "Replace some `dynamic` to `Object?` type (#80772)" (#80965)

This reverts commit 12a2e68.

* Add frontend_server_client to dependency allowlist (#80912)

* Revert "[RenderEditable] Dont paint caret when selection is invalid (#79607)" (#81076)

This reverts commit 0f8148e.

* Convert AnimatedSize to a StatefulWidget (#80554)

Co-authored-by: Jenn Magder <[email protected]>
Co-authored-by: Jason Simmons <[email protected]>
Seven-112 pushed a commit to Seven-112/gallery that referenced this pull request May 16, 2023
Remove the AnimatedSize vsync parameter (#480)
22feefb 
This is needed in order to land flutter/flutter#80554,
which will deprecate AnimatedSize.vsync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer approved these changes

@Hixie Hixie Awaiting requested review from Hixie

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jason-simmons @goderbauer @fluttergithubbot