Skip to content

Conversation

@polina-c
Copy link
Contributor

The contract should be explained to make it clear what fixes will not break the functionality.

@polina-c polina-c requested a review from a team as a code owner December 10, 2022 02:01
@polina-c polina-c requested review from kenzieschmoll and removed request for a team December 10, 2022 02:01
@polina-c polina-c changed the title Document method contract for the case study. Add the widget tree to leak and document method contract for the case study. Dec 11, 2022
@polina-c polina-c changed the title Add the widget tree to leak and document method contract for the case study. case study: add the widget tree to the leak and document the method contract. Dec 12, 2022
MyIncrementer(this.increment);

MyIncrementer(this.increment, this.screen);
final Scaffold? screen;
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to store the widget in this class? It is bad practice to store widget objects, so I'm wondering if we should put this into a case study app. (unless of course, this is to demonstrate that it is bad practice to store Widget objects?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is bad practice and source of leak

Copy link
Contributor

Choose a reason for hiding this comment

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

A slightly more natural looking thing you could do would be to store a BuildContext object. Our quick fix to turn a function into a widget often accidentally adds a BuildContext as a parameter to a widget.

Copy link
Contributor Author

@polina-c polina-c Dec 13, 2022

Choose a reason for hiding this comment

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

Thanks. I moved screen calculation out of closure to make it leaking and we do not need to store widget to produce leak any more.
I added context, but it does not contribute to the leak as it is always the same. What is leaking here is every time built screen.

Update: #4942

@polina-c polina-c merged commit c95cf5d into flutter:master Dec 12, 2022
@polina-c polina-c deleted the case-study branch December 12, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants