Implement detachChild for LayoutBuilder (#5860)

Fixes https://github.com/flutter/flutter/issues/5840

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

@@ -471,6 +471,12 @@ class RenderObjectToWidgetElement<T extends RenderObject> extends RootRenderObje
       visitor(_child);
   }
 
+  @override
+  void detachChild(Element child) {
+    assert(child == _child);
+    _child = null;
+  }
+
   @override
   void mount(Element parent, dynamic newSlot) {
     assert(parent == null);

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

@@ -1143,6 +1143,13 @@ class _InactiveElements {
 
   void _unmount(Element element) {
     assert(element._debugLifecycleState == _ElementLifecycle.inactive);
+    assert(() {
+      if (debugPrintGlobalKeyedWidgetLifecycle) {
+        if (element.widget.key is GlobalKey)
+          debugPrint('Discarding $element from inactive elements list.');
+      }
+      return true;
+    });
     element.unmount();
     assert(element._debugLifecycleState == _ElementLifecycle.defunct);
     element.visitChildren((Element child) {
@@ -1720,7 +1727,15 @@ abstract class Element implements BuildContext {
     visitChildren(visitor);
   }
 
-  bool detachChild(Element child) => false;
+  /// Remove the given child.
+  ///
+  /// This updates the child model such that [visitChildren] does not walk that
+  /// child anymore.
+  ///
+  /// The element must have already been deactivated when this is called,
+  /// meaning that its parent should be null.
+  @protected
+  void detachChild(Element child);
 
   /// This method is the core of the system.
   ///
@@ -1868,11 +1883,14 @@ abstract class Element implements BuildContext {
       return null;
     assert(() {
       if (debugPrintGlobalKeyedWidgetLifecycle)
-        debugPrint('Attempting to take $element from ${element._parent ?? "inactive elements list"} to put in $this');
+        debugPrint('Attempting to take $element from ${element._parent ?? "inactive elements list"} to put in $this.');
       return true;
     });
-    if (element._parent != null && !element._parent.detachChild(element))
-      return null;
+    final Element parent = element._parent;
+    if (parent != null) {
+      parent.deactivateChild(element);
+      parent.detachChild(element);
+    }
     assert(element._parent == null);
     owner._inactiveElements.remove(element);
     return element;
@@ -1930,6 +1948,11 @@ abstract class Element implements BuildContext {
   void _activateWithParent(Element parent, dynamic newSlot) {
     assert(_debugLifecycleState == _ElementLifecycle.inactive);
     _parent = parent;
+    assert(() {
+      if (debugPrintGlobalKeyedWidgetLifecycle)
+        debugPrint('Reactivating $this (now child of $_parent).');
+      return true;
+    });
     _updateDepth(_parent.depth);
     _activateRecursively(this);
     attachRenderObject(newSlot);
@@ -1947,11 +1970,6 @@ abstract class Element implements BuildContext {
   /// instead of being unmounted (see [unmount]).
   @mustCallSuper
   void activate() {
-    assert(() {
-      if (debugPrintGlobalKeyedWidgetLifecycle)
-        debugPrint('Reactivating $this (child of $_parent).');
-      return true;
-    });
     assert(_debugLifecycleState == _ElementLifecycle.inactive);
     assert(widget != null);
     assert(owner != null);
@@ -2387,11 +2405,9 @@ abstract class ComponentElement extends BuildableElement {
   }
 
   @override
-  bool detachChild(Element child) {
+  void detachChild(Element child) {
     assert(child == _child);
-    deactivateChild(_child);
     _child = null;
-    return true;
   }
 }
 
@@ -3026,6 +3042,11 @@ abstract class RootRenderObjectElement extends RenderObjectElement {
 class LeafRenderObjectElement extends RenderObjectElement {
   LeafRenderObjectElement(LeafRenderObjectWidget widget): super(widget);
 
+  @override
+  void detachChild(Element child) {
+    assert(false);
+  }
+
   @override
   void insertChildRenderObject(RenderObject child, dynamic slot) {
     assert(false);
@@ -3064,11 +3085,9 @@ class SingleChildRenderObjectElement extends RenderObjectElement {
   }
 
   @override
-  bool detachChild(Element child) {
+  void detachChild(Element child) {
     assert(child == _child);
-    deactivateChild(_child);
     _child = null;
-    return true;
   }
 
   @override
@@ -3157,10 +3176,10 @@ class MultiChildRenderObjectElement extends RenderObjectElement {
   }
 
   @override
-  bool detachChild(Element child) {
+  void detachChild(Element child) {
+    assert(_children.contains(child));
+    assert(!_detachedChildren.contains(child));
     _detachedChildren.add(child);
-    deactivateChild(child);
-    return true;
   }
 
   @override

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

@@ -69,6 +69,12 @@ class _LayoutBuilderElement extends RenderObjectElement {
       visitor(_child);
   }
 
+  @override
+  void detachChild(Element child) {
+    assert(child == _child);
+    _child = null;
+  }
+
   @override
   void mount(Element parent, dynamic newSlot) {
     super.mount(parent, newSlot); // Creates the renderObject.

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

@@ -510,6 +510,18 @@ class _LazyBlockElement extends RenderObjectElement {
       visitor(child);
   }
 
+  @override
+  void detachChild(Element child) {
+    assert(() {
+      // TODO(ianh): implement detachChild for LazyBlock
+      throw new FlutterError(
+        'LazyBlock does not yet support GlobalKey reparenting of its children.\n'
+        'As a temporary workaround, wrap the child with the GlobalKey in a '
+        'Container or other harmless child.'
+      );
+    });
+  }
+
   @override
   void mount(Element parent, dynamic newSlot) {
     super.mount(parent, newSlot);
@@ -585,7 +597,7 @@ class _LazyBlockElement extends RenderObjectElement {
   void performRebuild() {
     IndexedWidgetBuilder builder = widget.delegate.buildItem;
     List<Widget> widgets = <Widget>[];
-    for (int i = 0; i < _children.length; ++i) {
+    for (int i = 0; i < _children.length; i += 1) {
       int logicalIndex = _firstChildLogicalIndex + i;
       Widget childWidget = _callBuilder(builder, logicalIndex);
       if (childWidget == null)

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

@@ -438,9 +438,10 @@ class _TheatreElement extends RenderObjectElement {
     if (child == _onstage) {
       _onstage = null;
     } else {
+      assert(_offstage.contains(child));
+      assert(!_detachedOffstageChildren.contains(child));
       _detachedOffstageChildren.add(child);
     }
-    deactivateChild(child);
     return true;
   }
 

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

@@ -279,7 +279,6 @@ class _TableElement extends RenderObjectElement {
   @override
   bool detachChild(Element child) {
     _detachedChildren.add(child);
-    deactivateChild(child);
     return true;
   }
 }

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

@@ -100,6 +100,18 @@ abstract class VirtualViewportElement extends RenderObjectElement {
       visitor(child);
   }
 
+  @override
+  void detachChild(Element child) {
+    assert(() {
+      // TODO(ianh): implement detachChild for VirtualViewport
+      throw new FlutterError(
+        '$runtimeType does not yet support GlobalKey reparenting of its children.\n'
+        'As a temporary workaround, wrap the child with the GlobalKey in a '
+        'Container or other harmless child.'
+      );
+    });
+  }
+
   _WidgetProvider _widgetProvider;
 
   @override

packages/flutter/test/widget/reparent_state_with_layout_builder_test.dart

@@ -0,0 +1,72 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter/material.dart';
+import 'package:flutter_test/flutter_test.dart' hide TypeMatcher;
+
+// This is a regression test for https://github.com/flutter/flutter/issues/5840.
+
+class Bar extends StatefulWidget {
+  @override
+  BarState createState() => new BarState();
+}
+
+class BarState extends State<Bar> {
+  final GlobalKey _fooKey = new GlobalKey();
+
+  bool _mode = false;
+
+  void trigger() {
+    setState(() {
+      _mode = !_mode;
+    });
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    if (_mode) {
+      return new SizedBox(
+        child: new LayoutBuilder(
+          builder: (BuildContext context, BoxConstraints constraints) => new StatefulCreationCounter(key: _fooKey),
+        ),
+      );
+    } else {
+      return new LayoutBuilder(
+        builder: (BuildContext context, BoxConstraints constraints) => new StatefulCreationCounter(key: _fooKey),
+      );
+    }
+  }
+}
+
+class StatefulCreationCounter extends StatefulWidget {
+  StatefulCreationCounter({ Key key }) : super(key: key);
+
+  @override
+  StatefulCreationCounterState createState() => new StatefulCreationCounterState();
+}
+
+class StatefulCreationCounterState extends State<StatefulCreationCounter> {
+  static int creationCount = 0;
+
+  @override
+  void initState() {
+    super.initState();
+    creationCount += 1;
+  }
+
+  @override
+  Widget build(BuildContext context) => new Container();
+}
+
+void main() {
+  testWidgets('reparent state with layout builder', (WidgetTester tester) async {
+    expect(StatefulCreationCounterState.creationCount, 0);
+    await tester.pumpWidget(new Bar());
+    expect(StatefulCreationCounterState.creationCount, 1);
+    BarState s = tester.state/*<BarState>*/(find.byType(Bar));
+    s.trigger();
+    await tester.pump();
+    expect(StatefulCreationCounterState.creationCount, 1);
+  });
+}