Merge pull request #972 from abarth/parent_data_madness

Positioned 'remembers' things it shouldn't

packages/flutter/lib/rendering/flex.dart

@@ -9,10 +9,10 @@ import 'package:sky/rendering/object.dart';
 
 export 'package:sky/rendering/object.dart' show EventDisposition;
 
-class FlexBoxParentData extends BoxParentData with ContainerParentDataMixin<RenderBox> {
+class FlexParentData extends BoxParentData with ContainerParentDataMixin<RenderBox> {
   int flex;
 
-  void merge(FlexBoxParentData other) {
+  void merge(FlexParentData other) {
     if (other.flex != null)
       flex = other.flex;
     super.merge(other);
@@ -41,8 +41,8 @@ enum FlexAlignItems {
 
 typedef double _ChildSizingFunction(RenderBox child, BoxConstraints constraints);
 
-class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, FlexBoxParentData>,
-                                        RenderBoxContainerDefaultsMixin<RenderBox, FlexBoxParentData> {
+class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, FlexParentData>,
+                                        RenderBoxContainerDefaultsMixin<RenderBox, FlexParentData> {
   // lays out RenderBox children using flexible layout
 
   RenderFlex({
@@ -98,8 +98,8 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
   double _overflow;
 
   void setupParentData(RenderBox child) {
-    if (child.parentData is! FlexBoxParentData)
-      child.parentData = new FlexBoxParentData();
+    if (child.parentData is! FlexParentData)
+      child.parentData = new FlexParentData();
   }
 
   double _getIntrinsicSize({ BoxConstraints constraints,
@@ -133,7 +133,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
         } else {
           inflexibleSpace += childSize(child, childConstraints);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
       double mainSize = maxFlexFractionSoFar * totalFlex + inflexibleSpace;
@@ -196,7 +196,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
           inflexibleSpace += mainSize;
           maxCrossSize = math.max(maxCrossSize, crossSize);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
 
@@ -224,7 +224,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
           }
           maxCrossSize = math.max(maxCrossSize, crossSize);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
 
@@ -276,7 +276,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
   }
 
   int _getFlex(RenderBox child) {
-    assert(child.parentData is FlexBoxParentData);
+    assert(child.parentData is FlexParentData);
     return child.parentData.flex != null ? child.parentData.flex : 0;
   }
 
@@ -301,7 +301,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
     double freeSpace = canFlex ? mainSize : 0.0;
     RenderBox child = firstChild;
     while (child != null) {
-      assert(child.parentData is FlexBoxParentData);
+      assert(child.parentData is FlexParentData);
       totalChildren++;
       int flex = _getFlex(child);
       if (flex > 0) {
@@ -392,7 +392,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
           if (distance != null)
             maxBaselineDistance = math.max(maxBaselineDistance, distance);
         }
-        assert(child.parentData is FlexBoxParentData);
+        assert(child.parentData is FlexParentData);
         child = child.parentData.nextSibling;
       }
     }
@@ -461,7 +461,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl
     double childMainPosition = leadingSpace;
     child = firstChild;
     while (child != null) {
-      assert(child.parentData is FlexBoxParentData);
+      assert(child.parentData is FlexParentData);
       double childCrossPosition;
       switch (_alignItems) {
         case FlexAlignItems.stretch:

packages/flutter/lib/widgets/basic.dart

@@ -485,22 +485,64 @@ class Stack extends MultiChildRenderObjectWrapper {
 
   RenderStack createNode() => new RenderStack();
   RenderStack get renderObject => super.renderObject;
+
+  void updateParentData(RenderObject child, Positioned positioned) {
+    if (positioned == null)
+      _updateParentDataWithValues(child, null, null, null, null);
+    else
+      _updateParentDataWithValues(child, positioned.top, positioned.right, positioned.bottom, positioned.left);
+  }
+
+  void _updateParentDataWithValues(RenderObject child, double top, double right, double bottom, double left) {
+    assert(child.parentData is StackParentData);
+    final StackParentData parentData = child.parentData;
+    bool needsLayout = false;
+    if (parentData.top != top) {
+      parentData.top = top;
+      needsLayout = true;
+    }
+
+    if (parentData.right != right) {
+      parentData.right = right;
+      needsLayout = true;
+    }
+
+    if (parentData.bottom != bottom) {
+      parentData.bottom = bottom;
+      needsLayout = true;
+    }
+
+    if (parentData.left != left) {
+      parentData.left = left;
+      needsLayout = true;
+    }
+
+    if (needsLayout)
+      renderObject.markNeedsLayout();
+  }
 }
 
 class Positioned extends ParentDataNode {
   Positioned({
     Key key,
     Widget child,
-    double top,
-    double right,
-    double bottom,
-    double left
-  }) : super(child,
-             new StackParentData()..top = top
-                                  ..right = right
-                                  ..bottom = bottom
-                                  ..left = left,
-             key: key);
+    this.top,
+    this.right,
+    this.bottom,
+    this.left
+  }) : super(key: key, child: child);
+
+  final double top;
+  final double right;
+  final double bottom;
+  final double left;
+
+   void debugValidateAncestor(Widget ancestor) {
+     assert(() {
+       'Positioned must placed directly inside a Stack';
+       return ancestor is Stack;
+     });
+   }
 }
 
 class Grid extends MultiChildRenderObjectWrapper {
@@ -548,6 +590,19 @@ class Flex extends MultiChildRenderObjectWrapper {
     renderObject.alignItems = alignItems;
     renderObject.textBaseline = textBaseline;
   }
+
+  void updateParentData(RenderObject child, Flexible flexible) {
+    _updateParentDataWithValues(child, flexible == null ? null : flexible.flex);
+  }
+
+  void _updateParentDataWithValues(RenderObject child, int flex) {
+    assert(child.parentData is FlexParentData);
+    final FlexParentData parentData = child.parentData;
+    if (parentData.flex != flex) {
+      parentData.flex = flex;
+      renderObject.markNeedsLayout();
+    }
+  }
 }
 
 class Row extends Flex {
@@ -569,8 +624,17 @@ class Column extends Flex {
 }
 
 class Flexible extends ParentDataNode {
-  Flexible({ Key key, int flex: 1, Widget child })
-    : super(child, new FlexBoxParentData()..flex = flex, key: key);
+  Flexible({ Key key, this.flex: 1, Widget child })
+    : super(key: key, child: child);
+
+  final int flex;
+
+  void debugValidateAncestor(Widget ancestor) {
+    assert(() {
+      'Flexible must placed directly inside a Flex';
+      return ancestor is Flex;
+    });
+  }
 }
 
 class Paragraph extends LeafRenderObjectWrapper {

packages/flutter/lib/widgets/framework.dart

@@ -441,8 +441,10 @@ bool _canSync(Widget a, Widget b) {
 // stylistic information, etc.
 abstract class TagNode extends Widget {
 
-  TagNode(Widget child, { Key key })
-    : this.child = child, super(key: key);
+  TagNode({ Key key, Widget child })
+    : this.child = child, super(key: key) {
+    assert(child != null);
+  }
 
   // TODO(jackson): Remove this workaround for limitation of Dart mixins
   TagNode._withKey(Widget child, Key key)
@@ -480,10 +482,15 @@ abstract class TagNode extends Widget {
 
 }
 
-class ParentDataNode extends TagNode {
-  ParentDataNode(Widget child, this.parentData, { Key key })
-    : super(child, key: key);
-  final ParentData parentData;
+abstract class ParentDataNode extends TagNode {
+  ParentDataNode({ Key key, Widget child })
+    : super(key: key, child: child);
+
+  /// Subclasses should override this function to ensure that they are placed
+  /// inside widgets that expect them.
+  ///
+  /// The given ancestor is the first non-component ancestor of this widget.
+  void debugValidateAncestor(Widget ancestor);
 }
 
 abstract class Inherited extends TagNode {
@@ -553,7 +560,7 @@ class Listener extends TagNode  {
          onPointerUp: onPointerUp,
          custom: custom
        ),
-       super(child, key: key);
+       super(key: key, child: child);
 
   final Map<String, EventListener> listeners;
 
@@ -975,24 +982,30 @@ abstract class RenderObjectWrapper extends Widget {
     // we give them their own slots for them to fit into us.
   }
 
+  /// Override this function if your RenderObjectWrapper uses a [ParentDataNode]
+  /// to program parent data into children.
+  void updateParentData(RenderObject child, ParentDataNode node) { }
+
   void syncRenderObject(RenderObjectWrapper old) {
     assert(old == null || old.renderObject == renderObject);
-    ParentData parentData = null;
-    Widget ancestor = parent;
-    while (ancestor != null && ancestor is! RenderObjectWrapper) {
-      if (ancestor is ParentDataNode && ancestor.parentData != null) {
-        if (parentData != null)
-          parentData.merge(ancestor.parentData); // this will throw if the types aren't the same
-        else
-          parentData = ancestor.parentData;
+    ParentDataNode parentDataNode = null;
+    for (Widget current = parent; current != null; current = current.parent) {
+      assert(() {
+        if (current is ParentDataNode) {
+          Widget ancestor = current.parent;
+          while (ancestor != null && ancestor is Component)
+            ancestor = ancestor.parent;
+          current.debugValidateAncestor(ancestor);
+        }
+        return true;
+      });
+      if (current is ParentDataNode) {
+        assert(parentDataNode == null);
+        parentDataNode = current;
+      } else if (current is RenderObjectWrapper) {
+        current.updateParentData(renderObject, parentDataNode);
+        break;
       }
-      ancestor = ancestor.parent;
-    }
-    if (parentData != null) {
-      assert(renderObject.parentData != null);
-      renderObject.parentData.merge(parentData); // this will throw if the types aren't appropriate
-      if (ancestor != null && ancestor.renderObject != null)
-        ancestor.renderObject.markNeedsLayout();
     }
   }
 

packages/unit/test/widget/stack_test.dart

+import 'package:sky/widgets.dart';
+import 'package:test/test.dart';
+
+import 'widget_tester.dart';
+
+void main() {
+  test('Can change position data', () {
+    WidgetTester tester = new WidgetTester();
+
+    tester.pumpFrame(() {
+      return new Stack([
+        new Positioned(
+          left: 10.0,
+          child: new Container(
+            width: 10.0,
+            height: 10.0
+          )
+        )
+      ]);
+    });
+
+    Container container = tester.findWidget((Widget widget) => widget is Container);
+    expect(container.renderObject.parentData.top, isNull);
+    expect(container.renderObject.parentData.right, isNull);
+    expect(container.renderObject.parentData.bottom, isNull);
+    expect(container.renderObject.parentData.left, equals(10.0));
+
+    tester.pumpFrame(() {
+      return new Stack([
+        new Positioned(
+          right: 10.0,
+          child: new Container(
+            width: 10.0,
+            height: 10.0
+          )
+        )
+      ]);
+    });
+
+    container = tester.findWidget((Widget widget) => widget is Container);
+    expect(container.renderObject.parentData.top, isNull);
+    expect(container.renderObject.parentData.right, equals(10.0));
+    expect(container.renderObject.parentData.bottom, isNull);
+    expect(container.renderObject.parentData.left, isNull);
+  });
+}