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
Convert AnimatedSize to a StatefulWidget #80554
Convert AnimatedSize to a StatefulWidget #80554
Conversation
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. |
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
this.alignment = Alignment.center, | ||
this.curve = Curves.linear, | ||
required this.duration, | ||
this.reverseDuration, | ||
required this.vsync, | ||
@Deprecated('Ignored') TickerProvider? vsync, |
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.
nit: these deprecation notices need to follow a certain pattern, see "Breaking Changes" in the wiki.
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.
done
@@ -49,21 +50,23 @@ import 'framework.dart'; | |||
/// See also: | |||
/// | |||
/// * [SizeTransition], which changes its size based on an [Animation]. | |||
class AnimatedSize extends SingleChildRenderObjectWidget { | |||
/// Creates a widget that animates its size to match that of its child. |
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.
keep some doc on the constructor?
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.
done
} | ||
|
||
class _AnimatedSize extends SingleChildRenderObjectWidget { | ||
/// Creates a widget that animates its size to match that of its child. |
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.
this doc can probably be removed since the constructor is private and this doesn't contain much information...
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.
done
259bcf7
to
ca76263
Compare
This is needed in order to land flutter/flutter#80554, which will deprecate AnimatedSize.vsync
This is needed in order to land flutter/flutter#80554, which will deprecate AnimatedSize.vsync
ca76263
to
6d29469
Compare
Removed the
PTAL @goderbauer |
This is needed in order to land flutter/flutter#80554, which will deprecate AnimatedSize.vsync
6d29469
to
b44fa03
Compare
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
this.alignment = Alignment.center, | ||
this.curve = Curves.linear, | ||
required this.duration, | ||
this.reverseDuration, | ||
required this.vsync, | ||
// This field is now ignored and will be deprecated. |
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.
Can you phrase this as a "TODO(xx): deprecate when customer tests are updated"?
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.
done
This fixes a memory leak in which RenderAnimatedSize instances were creating tickers in the TickerProvider but were not disposing them.
b44fa03
to
13aacfc
Compare
This is needed in order to land flutter/flutter#80554, which will deprecate AnimatedSize.vsync
This is needed in order to land flutter/flutter#80554, which will deprecate AnimatedSize.vsync
…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 <magder@google.com> Co-authored-by: Jason Simmons <jason-simmons@users.noreply.github.com>
This is needed in order to land flutter/flutter#80554, which will deprecate AnimatedSize.vsync
This is needed in order to land flutter/flutter#80554, which will deprecate AnimatedSize.vsync
This fixes a memory leak in which RenderAnimatedSize instances were
creating tickers in the TickerProvider but were not disposing them.