Fix Container's child state loss. #163419
Conversation
|
There is one failing test in This test tries to get the element of the Could you please advise what to do in this case? |
|
@LongCatIsLooong FYI I think we were discussing this problem before you went on leave, see #161698. |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Hey from triage, @LongCatIsLooong and @ksokolovskyi , what is the status of this PR? |
99b8473 to
9fc1456
Compare
|
Hi @Piinks. From my side, the PR is ready for review, but I am unsure how to proceed with one failing test in the |
There was a problem hiding this comment.
I think this PR will require a lot of work to prove this is a valid solution that is not negatively affecting performance, but I appreciate you taking on such a difficult and fundamental problem @ksokolovskyi!
This is the failing rfw test. Can you see if it's something trivial that can be updated? It's probably just depending on Container producing a ColoredBox even when not given a color. https://github.com/flutter/packages/blob/ff7724c18a803542fc709f942c3bf1cecf7e4864/packages/rfw/test/runtime_test.dart#L894
| @@ -243,7 +243,7 @@ class DecoratedBox extends SingleChildRenderObjectWidget { | |||
| /// [InkResponse] and [InkWell] splashes to paint over them. | |||
| /// * Cookbook: [Animate the properties of a container](https://docs.flutter.dev/cookbook/animation/animated-container) | |||
| /// * The [catalog of layout widgets](https://docs.flutter.dev/ui/widgets/layout). | |||
| class Container extends StatelessWidget { | |||
| class Container extends StatefulWidget { | |||
There was a problem hiding this comment.
I wonder what the performance implications are about changing this to a stateful widget since Container is used all over the place.
| } | ||
|
|
||
| class _ContainerState extends State<Container> { | ||
| final GlobalKey _childGlobalKey = GlobalKey(); |
There was a problem hiding this comment.
This also might be a performance concern. Somewhat related: #161698 (comment)
|
|
||
| if (child == null && (constraints == null || !constraints!.isTight)) { | ||
| if (current != null) { | ||
| current = KeyedSubtree(key: _childGlobalKey, child: current); |
There was a problem hiding this comment.
@chunhtai I think you and I talked about solving this problem with a KeyedSubtree before and decided against it, do you remember the details at all?
Fixes #161698
Description
Containerwidget to beStatefuland useGlobalKeyto preserve the child's stateContainerinframework_test.dartwith custom_Wrapperwidget to retain tests readability. Without this change, we would have to check forKeyedSubtree-[GlobalKey#00000]in tests, instead ofContainer-[<1>]which decreases readability.ContainerwithSizedBoxinlookup_boundary_test.dart, because the test didn't expect the additionalKeyedSubtreewidget inside theContainer.ContainerwithKeyedSubtreeinpackages/flutter_test/lib/src/binding.dart. This is because using theContainerinTestWidgetsFlutterBinding._runTestBodyintroduces two additionalGlobalKeywhich affect existingGlobalKeytests.Pre-launch Checklist
///).