Skip to content
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

Clarify what AnimatedBuilder really speeds up when using child #114506

Closed
wants to merge 1 commit into from

Conversation

fzyzcjy
Copy link
Contributor

@fzyzcjy fzyzcjy commented Nov 2, 2022

When I was reading AnimatedBuilder doc (when not that familiar w/ Flutter), I wrongly have the following conclusion: Suppose I have the following, and MyWidget constructs a huge tree:

return AnimatedBuilder(
  builder: () => AnimatedOpacity(child: Padding(padding: ..., child: MyWidget())),
);

vs

return AnimatedBuilder(
  builder: (child) => AnimatedOpacity(child: child),
  child: Padding(padding: ..., child: MyWidget()),
);

Then, reading the doc, I think this will be very little performance boost when using this child. It is because,

If your builder function contains a subtree that does not depend on the animation, it's more efficient to build that subtree once instead of rebuilding it on every animation tick.

So I thought: Yes, I do have a subtree (Padding + MyWidget) that does not change, and this trick can make them built once instead of many times. But it is very fast, because it just creates two objects! (Here, I understood the "a subtree" in the doc as the Padding+MyWidget two objects, while in reality we know it should mean the whole subtree that MyWidget and its descedent widgets build.)

Thus, I add a few lines to clarify this. Hope future flutter new learners do not get confused as me!

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 2, 2022
@fzyzcjy fzyzcjy marked this pull request as ready for review November 2, 2022 14:03
@@ -1020,6 +1020,12 @@ class DefaultTextStyleTransition extends AnimatedWidget {
/// Using this pre-built child is entirely optional, but can improve
/// performance significantly in some cases and is therefore a good practice.
///
/// When putting something into [child] instead of [builder], it is not the
Copy link
Member

Choose a reason for hiding this comment

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

This is basically repeating the information from the previous three paragraphs without really adding anything new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then maybe I should rephrase the paragraphs above. As is said in the post content of the PR, the original paragraphs can make new learners somehow confused and have wrong thoughts

@goderbauer
Copy link
Member

I am going to close this as it doesn't add any new information to the doc.

@goderbauer goderbauer closed this Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants