Allow key reparenting between slots in SlottedMultiChildRenderObjectWidgetMixin (#106977)

Author: LongCatIsLooong

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

@@ -182,14 +182,24 @@ mixin SlottedContainerRenderObjectMixin<S> on RenderBox {
       adoptChild(child);
     }
   }
+
+  void _moveChild(RenderBox child, S slot, S oldSlot) {
+    assert(slot != oldSlot);
+    final RenderBox? oldChild = _slotToChild[oldSlot];
+    if (oldChild == child) {
+      _setChild(null, oldSlot);
+    }
+    _setChild(child, slot);
+  }
 }
 
 /// Element used by the [SlottedMultiChildRenderObjectWidgetMixin].
 class SlottedRenderObjectElement<S> extends RenderObjectElement {
   /// Creates an element that uses the given widget as its configuration.
   SlottedRenderObjectElement(SlottedMultiChildRenderObjectWidgetMixin<S> super.widget);
 
-  final Map<S, Element> _slotToChild = <S, Element>{};
+  Map<S, Element> _slotToChild = <S, Element>{};
+  Map<Key, Element> _keyedChildren = <Key, Element>{};
 
   @override
   SlottedContainerRenderObjectMixin<S> get renderObject => super.renderObject as SlottedContainerRenderObjectMixin<S>;
@@ -231,21 +241,73 @@ class SlottedRenderObjectElement<S> extends RenderObjectElement {
     }(), '${widget.runtimeType}.slots must not change.');
     assert(slottedMultiChildRenderObjectWidgetMixin.slots.toSet().length == slottedMultiChildRenderObjectWidgetMixin.slots.length, 'slots must be unique');
 
+    final Map<Key, Element> oldKeyedElements = _keyedChildren;
+    _keyedChildren = <Key, Element>{};
+    final Map<S, Element> oldSlotToChild = _slotToChild;
+    _slotToChild = <S, Element>{};
+
+    Map<Key, List<Element>>? debugDuplicateKeys;
+
     for (final S slot in slottedMultiChildRenderObjectWidgetMixin.slots) {
-      _updateChild(slottedMultiChildRenderObjectWidgetMixin.childForSlot(slot), slot);
+      final Widget? widget = slottedMultiChildRenderObjectWidgetMixin.childForSlot(slot);
+      final Key? newWidgetKey = widget?.key;
+
+      final Element? oldSlotChild = oldSlotToChild[slot];
+      final Element? oldKeyChild = oldKeyedElements[newWidgetKey];
+
+      // Try to find the slot for the correct Element that `widget` should update.
+      // If key matching fails, resort to `oldSlotChild` from the same slot.
+      final Element? fromElement;
+      if (oldKeyChild != null) {
+        fromElement = oldSlotToChild.remove(oldKeyChild.slot as S);
+      } else if (oldSlotChild?.widget.key == null) {
+        fromElement = oldSlotToChild.remove(slot);
+      } else {
+        // The only case we can't use `oldSlotChild` is when its widget has a key.
+        assert(oldSlotChild!.widget.key != newWidgetKey);
+        fromElement = null;
+      }
+      final Element? newChild = updateChild(fromElement, widget, slot);
+
+      if (newChild != null) {
+        _slotToChild[slot] = newChild;
+
+        if (newWidgetKey != null) {
+          assert(() {
+            final Element? existingElement = _keyedChildren[newWidgetKey];
+            if (existingElement != null) {
+              (debugDuplicateKeys ??= <Key, List<Element>>{})
+                .putIfAbsent(newWidgetKey, () => <Element>[existingElement])
+                .add(newChild);
+            }
+            return true;
+          }());
+          _keyedChildren[newWidgetKey] = newChild;
+        }
+      }
     }
+    oldSlotToChild.values.forEach(deactivateChild);
+    assert(_debugDuplicateKeys(debugDuplicateKeys));
+    assert(_keyedChildren.values.every(_slotToChild.values.contains), '_keyedChildren ${_keyedChildren.values} should be a subset of ${_slotToChild.values}');
   }
 
-  void _updateChild(Widget? widget, S slot) {
-    final Element? oldChild = _slotToChild[slot];
-    assert(oldChild == null || oldChild.slot == slot); // Reason why [moveRenderObjectChild] is not reachable.
-    final Element? newChild = updateChild(oldChild, widget, slot);
-    if (oldChild != null) {
-      _slotToChild.remove(slot);
+  bool _debugDuplicateKeys(Map<Key, List<Element>>? debugDuplicateKeys) {
+    if (debugDuplicateKeys == null) {
+      return true;
     }
-    if (newChild != null) {
-      _slotToChild[slot] = newChild;
+    for (final MapEntry<Key, List<Element>> duplicateKey in debugDuplicateKeys.entries) {
+      throw FlutterError.fromParts(<DiagnosticsNode>[
+        ErrorSummary('Multiple widgets used the same key in ${widget.runtimeType}.'),
+        ErrorDescription(
+          'The key ${duplicateKey.key} was used by multiple widgets. The parents of those widgets were:\n'
+        ),
+        for (final Element element in duplicateKey.value) ErrorDescription('  - $element\n'),
+        ErrorDescription(
+          'A key can only be specified on one widget at a time in the same parent widget.',
+        ),
+      ]);
     }
+    return true;
   }
 
   @override
@@ -256,14 +318,14 @@ class SlottedRenderObjectElement<S> extends RenderObjectElement {
 
   @override
   void removeRenderObjectChild(RenderBox child, S slot) {
-    assert(renderObject._slotToChild[slot] == child);
-    renderObject._setChild(null, slot);
-    assert(renderObject._slotToChild[slot] == null);
+    if (renderObject._slotToChild[slot] == child) {
+      renderObject._setChild(null, slot);
+      assert(renderObject._slotToChild[slot] == null);
+    }
   }
 
   @override
-  void moveRenderObjectChild(RenderBox child, Object? oldSlot, Object? newSlot) {
-    // Existing elements are never moved to a new slot, see assert in [_updateChild].
-    assert(false, 'not reachable');
+  void moveRenderObjectChild(RenderBox child, S oldSlot, S newSlot) {
+    renderObject._moveChild(child, newSlot, oldSlot);
   }
 }

packages/flutter/test/widgets/slotted_render_object_widget_test.dart

@@ -139,6 +139,72 @@ void main() {
     expect(_RenderTest().publicNameForSlot(slot), slot.toString());
   });
 
+  testWidgets('key reparenting', (WidgetTester tester) async {
+    const Widget widget1 = SizedBox(key: ValueKey<String>('smol'), height: 10, width: 10);
+    const Widget widget2 = SizedBox(key: ValueKey<String>('big'), height: 100, width: 100);
+    const Widget nullWidget = SizedBox(key: ValueKey<String>('null'), height: 50, width: 50);
+
+    await tester.pumpWidget(buildWidget(topLeft: widget1, bottomRight: widget2, nullSlot: nullWidget));
+    final _RenderDiagonal renderObject = tester.renderObject(find.byType(_Diagonal));
+    expect(renderObject._topLeft!.size, const Size(10, 10));
+    expect(renderObject._bottomRight!.size, const Size(100, 100));
+    expect(renderObject._nullSlot!.size, const Size(50, 50));
+
+    final Element widget1Element = tester.element(find.byWidget(widget1));
+    final Element widget2Element = tester.element(find.byWidget(widget2));
+    final Element nullWidgetElement = tester.element(find.byWidget(nullWidget));
+
+    // Swapping 1 and 2.
+    await tester.pumpWidget(buildWidget(topLeft: widget2, bottomRight: widget1, nullSlot: nullWidget));
+    expect(renderObject._topLeft!.size, const Size(100, 100));
+    expect(renderObject._bottomRight!.size, const Size(10, 10));
+    expect(renderObject._nullSlot!.size, const Size(50, 50));
+    expect(widget1Element, same(tester.element(find.byWidget(widget1))));
+    expect(widget2Element, same(tester.element(find.byWidget(widget2))));
+    expect(nullWidgetElement, same(tester.element(find.byWidget(nullWidget))));
+
+    // Shifting slots
+    await tester.pumpWidget(buildWidget(topLeft: nullWidget, bottomRight: widget2, nullSlot: widget1));
+    expect(renderObject._topLeft!.size, const Size(50, 50));
+    expect(renderObject._bottomRight!.size, const Size(100, 100));
+    expect(renderObject._nullSlot!.size, const Size(10, 10));
+    expect(widget1Element, same(tester.element(find.byWidget(widget1))));
+    expect(widget2Element, same(tester.element(find.byWidget(widget2))));
+    expect(nullWidgetElement, same(tester.element(find.byWidget(nullWidget))));
+
+    // Moving + Deleting.
+    await tester.pumpWidget(buildWidget(bottomRight: widget2));
+    expect(renderObject._topLeft, null);
+    expect(renderObject._bottomRight!.size, const Size(100, 100));
+    expect(renderObject._nullSlot, null);
+    expect(widget1Element.debugIsDefunct, isTrue);
+    expect(nullWidgetElement.debugIsDefunct, isTrue);
+    expect(widget2Element, same(tester.element(find.byWidget(widget2))));
+
+    // Moving.
+    await tester.pumpWidget(buildWidget(topLeft: widget2));
+    expect(renderObject._topLeft!.size, const Size(100, 100));
+    expect(renderObject._bottomRight, null);
+    expect(widget2Element, same(tester.element(find.byWidget(widget2))));
+  });
+
+  testWidgets('duplicated key error message', (WidgetTester tester) async {
+    const Widget widget1 = SizedBox(key: ValueKey<String>('widget 1'), height: 10, width: 10);
+    const Widget widget2 = SizedBox(key: ValueKey<String>('widget 1'), height: 100, width: 100);
+    const Widget widget3 = SizedBox(key: ValueKey<String>('widget 1'), height: 50, width: 50);
+
+    await tester.pumpWidget(buildWidget(topLeft: widget1, bottomRight: widget2, nullSlot: widget3));
+
+    expect((tester.takeException() as FlutterError).toString(), equalsIgnoringHashCodes(
+     'Multiple widgets used the same key in _Diagonal.\n'
+     "The key [<'widget 1'>] was used by multiple widgets. The parents of those widgets were:\n"
+     "  - SizedBox-[<'widget 1'>](width: 50.0, height: 50.0, renderObject: RenderConstrainedBox#00000 NEEDS-LAYOUT NEEDS-PAINT)\n"
+     "  - SizedBox-[<'widget 1'>](width: 10.0, height: 10.0, renderObject: RenderConstrainedBox#00000 NEEDS-LAYOUT NEEDS-PAINT)\n"
+     "  - SizedBox-[<'widget 1'>](width: 100.0, height: 100.0, renderObject: RenderConstrainedBox#a4685 NEEDS-LAYOUT NEEDS-PAINT)\n"
+     'A key can only be specified on one widget at a time in the same parent widget.'
+    ));
+  });
+
   testWidgets('debugDescribeChildren', (WidgetTester tester) async {
     await tester.pumpWidget(buildWidget(
       topLeft: const SizedBox(
@@ -178,14 +244,15 @@ _RenderDiagonal#00000 relayoutBoundary=up1
   });
 }
 
-Widget buildWidget({Widget? topLeft, Widget? bottomRight}) {
+Widget buildWidget({Widget? topLeft, Widget? bottomRight, Widget? nullSlot}) {
   return Directionality(
     textDirection: TextDirection.ltr,
     child: Align(
       alignment: Alignment.topLeft,
       child: _Diagonal(
         topLeft: topLeft,
         bottomRight: bottomRight,
+        nullSlot: nullSlot,
       ),
     ),
   );
@@ -196,21 +263,25 @@ enum _DiagonalSlot {
   bottomRight,
 }
 
-class _Diagonal extends RenderObjectWidget with SlottedMultiChildRenderObjectWidgetMixin<_DiagonalSlot> {
+class _Diagonal extends RenderObjectWidget with SlottedMultiChildRenderObjectWidgetMixin<_DiagonalSlot?> {
   const _Diagonal({
     this.topLeft,
     this.bottomRight,
+    this.nullSlot,
   });
 
   final Widget? topLeft;
   final Widget? bottomRight;
+  final Widget? nullSlot;
 
   @override
-  Iterable<_DiagonalSlot> get slots => _DiagonalSlot.values;
+  Iterable<_DiagonalSlot?> get slots => <_DiagonalSlot?>[null, ..._DiagonalSlot.values];
 
   @override
-  Widget? childForSlot(_DiagonalSlot slot) {
+  Widget? childForSlot(_DiagonalSlot? slot) {
     switch (slot) {
+      case null:
+        return nullSlot;
       case _DiagonalSlot.topLeft:
         return topLeft;
       case _DiagonalSlot.bottomRight:
@@ -219,16 +290,17 @@ class _Diagonal extends RenderObjectWidget with SlottedMultiChildRenderObjectWid
   }
 
   @override
-  SlottedContainerRenderObjectMixin<_DiagonalSlot> createRenderObject(
+  SlottedContainerRenderObjectMixin<_DiagonalSlot?> createRenderObject(
     BuildContext context,
   ) {
     return _RenderDiagonal();
   }
 }
 
-class _RenderDiagonal extends RenderBox with SlottedContainerRenderObjectMixin<_DiagonalSlot> {
+class _RenderDiagonal extends RenderBox with SlottedContainerRenderObjectMixin<_DiagonalSlot?> {
   RenderBox? get _topLeft => childForSlot(_DiagonalSlot.topLeft);
   RenderBox? get _bottomRight => childForSlot(_DiagonalSlot.bottomRight);
+  RenderBox? get _nullSlot => childForSlot(null);
 
   @override
   void performLayout() {
@@ -251,9 +323,17 @@ class _RenderDiagonal extends RenderBox with SlottedContainerRenderObjectMixin<_
       bottomRightSize = _bottomRight!.size;
     }
 
+    Size nullSlotSize = Size.zero;
+    final RenderBox? nullSlot = _nullSlot;
+    if (nullSlot != null) {
+      nullSlot.layout(childConstraints, parentUsesSize: true);
+      _positionChild(nullSlot, Offset.zero);
+      nullSlotSize = nullSlot.size;
+    }
+
     size = constraints.constrain(Size(
-      topLeftSize.width + bottomRightSize.width,
-      topLeftSize.height + bottomRightSize.height,
+      topLeftSize.width + bottomRightSize.width + nullSlotSize.width,
+      topLeftSize.height + bottomRightSize.height + nullSlotSize.height,
     ));
   }