Fix `_debugElementWasRebuilt` global key asserts #105430

LongCatIsLooong · 2022-06-06T07:59:22Z

Byproduct of #105335

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.

LongCatIsLooong · 2022-06-06T08:03:14Z

packages/flutter/lib/src/widgets/framework.dart

Element.updateChild is also evidence that this is being rebuilt.
Instead of doing _debugElementWasRebuilt(context) in buildScope just for LayoutBuilder this feels slightly better?

To me this feels out of place here. We should detect before this place that the element was rebuild because it may potentially not have any children.

(same with the one below)

because it may potentially not have any children.

that case is already taken care of here:

flutter/packages/flutter/lib/src/widgets/framework.dart

Lines 3562 to 3565 in 2aa348b

assert(() {

child.owner!._debugElementWasRebuilt(child);

return true;

}());

But the above line doesn't call _debugElementWasRebuilt on the root element Element that initiated the subtree update, like in LayoutBuilder:

flutter/packages/flutter/lib/src/widgets/layout_builder.dart

Line 135 in 2aa348b

_child = updateChild(_child, built, null);

With _debugElementWasRebuilt's current implementation it's OK for it to be called on the same Element multiple times in a frame.

Another reason why this feels out of place here: technically you don't have to go through updateChild. You can just call the methods called from here directly and bypass updateChild completely.

Hm, but I guess if you do that you miss also some of the other debug key-locig at the end of this method?

…bal key reparenting

xu-baolin · 2022-06-06T13:07:58Z

packages/flutter/lib/src/widgets/framework.dart

        try {
          element.rebuild();
+          assert(() {
+            _debugElementWasRebuilt(element);


This is duplication with the updateChild's check?

Let _debugElementWasRebuilt returns true will simpler the assertion?

rebuild doesn't necessarily call updateChild. The widget may not have a child.

rebuild doesn't necessarily call updateChild. The widget may not have a child.

I mean if we check here, the new check you added to updateChild by this change is unnecessary.
But the check above when callback is non-null should retain.

Could you elaborate? You mean the 2 _debugElementWasRebuilt calls at the end of updateChild are not necessary?

Could you elaborate? You mean the 2 _debugElementWasRebuilt calls at the end of updateChild are not necessary?

Right.

LayoutBuilder doesn't call rebuild when it rebuilds in performLayout, nor does it parent call update on the layout builder so those are still needed. I don't think we want to call _debugElementWasRebuilt(context) in invokeLayoutCallback since it's not guaranteed that context will be rebuilt.

LayoutBuilder doesn't call rebuild when it rebuilds in performLayout, nor does it parent call update on the layout builder so those are still needed. I don't think we want to call _debugElementWasRebuilt(context) in invokeLayoutCallback since it's not guaranteed that context will be rebuilt.

Add a test for this?

It's covered by existing tests I think. I can add one that specifically verifies that _debugElementWasRebuilt(context) is no longer being called and there's no false negatives because we removed _debugElementWasRebuilt(context) but that one feels a little bit too specific to be useful?

We can add a test that failed #105492 to prevent regression by accident. : )

goderbauer · 2022-07-01T17:33:42Z

It looks like this is fixing two distinct issues? And there is no dependency between the two fixes? Maybe break this up into two separate PRs to make potential breakages easier to debug and roll-backs more targeted?

goderbauer · 2022-07-01T17:37:19Z

packages/flutter/lib/src/widgets/framework.dart

To me this feels out of place here. We should detect before this place that the element was rebuild because it may potentially not have any children.

goderbauer · 2022-07-01T17:39:23Z

packages/flutter/lib/src/widgets/framework.dart

(same with the one below)

goderbauer · 2022-07-01T17:44:11Z

packages/flutter/lib/src/widgets/slotted_render_object_widget.dart

  List<S>? _debugPreviousSlots;

-  void _updateChildren() {
+  void _updateChildren(SlottedMultiChildRenderObjectWidgetMixin<S> newWidget) {


This newly added parameter appears unused?

goderbauer · 2022-07-01T17:59:01Z

packages/flutter/lib/src/widgets/slotted_render_object_widget.dart

    }(), '${widget.runtimeType}.slots must not change.');
    assert(slottedMultiChildRenderObjectWidgetMixin.slots.toSet().length == slottedMultiChildRenderObjectWidgetMixin.slots.length, 'slots must be unique');

+    final Map<GlobalKey, S> globalKeyedSlots = _globalKeyToSlot;


Maybe rename this to oldGlobalKedSlots to avoid confusion?

goderbauer · 2022-07-01T18:03:45Z

packages/flutter/lib/src/widgets/slotted_render_object_widget.dart

-      _updateChild(slottedMultiChildRenderObjectWidgetMixin.childForSlot(slot), slot);
+      final Widget? widget = slottedMultiChildRenderObjectWidgetMixin.childForSlot(slot);
+      final Key? newWidgetKey = widget?.key;
+      if (widget != null && newWidgetKey is GlobalKey) {


This now has me wondering: Don't we need to do this for all keys, not just globalkeys? If a keyed widget changes slots, it should be matched with the element from the previous slot it was in?

goderbauer · 2022-07-15T18:50:30Z

Looking at this some more: The most obvious place to call _debugElementWasRebuilt would be in Element.rebuild right after we called Element.performRebuild instead of calling it from all these places (including the already existing ones). From what I can tell, that would fix the test case included in this PR (not sure if other things would break, though). I am wondering why we didn't just call it from there when this was first introduced...

LongCatIsLooong · 2022-07-15T19:02:17Z

RenderObjectElement doesn't call rebuild in its update I think?

goderbauer · 2022-07-15T19:20:07Z

RenderObjectElement doesn't call rebuild in its update I think?

You're right. That's probably the reason why we currently call _debugElementWasRebuilt after calling Element.update in updateChild.

I just experimented with this. If I only call _debugElementWasRebuilt from Element.update and Element.rebuild all tests (including the one from this PR) seem to pass. That actually seems to me the cleanest places to call this if we don't come up with a reason why that would be bad... WDYT about this approach?

LongCatIsLooong · 2022-07-15T20:10:08Z

LayoutBuilder would be an exception to this, it rebuilds itself in a layout callback and doesn't call either of this on itself.

flutter/packages/flutter/lib/src/widgets/layout_builder.dart

Line 135 in 2aa348b

_child = updateChild(_child, built, null);

I think we currently account for that by doing this:

flutter/packages/flutter/lib/src/widgets/framework.dart

Line 2611 in 8f834cf

_debugElementWasRebuilt(context);

But that doesn't seem to be the right place to do that.

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 6, 2022

LongCatIsLooong commented Jun 6, 2022

View reviewed changes

LongCatIsLooong force-pushed the global-key-asserts branch from 8cbca03 to fd52cff Compare June 6, 2022 08:08

Fix global key asserts & SlottedMultiChildRenderObjectWidgetMixin glo…

f894cc4

…bal key reparenting

LongCatIsLooong force-pushed the global-key-asserts branch from fd52cff to f894cc4 Compare June 6, 2022 08:10

xu-baolin reviewed Jun 6, 2022

View reviewed changes

LongCatIsLooong requested a review from Hixie June 6, 2022 16:41

LongCatIsLooong requested a review from goderbauer July 1, 2022 16:52

goderbauer reviewed Jul 1, 2022

View reviewed changes

LongCatIsLooong changed the title ~~Fix _debugElementWasRebuilt global key asserts & handle global key reparenting for SlottedMultiChildRenderObjectWidgetMixin~~ Fix _debugElementWasRebuilt global key asserts Jul 1, 2022

LongCatIsLooong mentioned this pull request Jul 1, 2022

Allow key reparenting between slots in SlottedMultiChildRenderObjectWidgetMixin #106977

Merged

8 tasks

discard unrelated changes

b24731c

LongCatIsLooong closed this Aug 9, 2022

	assert(() {
	child.owner!._debugElementWasRebuilt(child);
	return true;
	}());

Fix _debugElementWasRebuilt global key asserts #105430

Fix _debugElementWasRebuilt global key asserts #105430

Uh oh!

Conversation

LongCatIsLooong commented Jun 6, 2022

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jul 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jul 15, 2022

Uh oh!

LongCatIsLooong commented Jul 15, 2022

Uh oh!

goderbauer commented Jul 15, 2022

Uh oh!

LongCatIsLooong commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix `_debugElementWasRebuilt` global key asserts #105430

Fix `_debugElementWasRebuilt` global key asserts #105430

LongCatIsLooong Jun 6, 2022 •

edited

Loading

LongCatIsLooong commented Jul 15, 2022 •

edited

Loading