From 28643e2cf480f23493aa76450059df46965bc52b Mon Sep 17 00:00:00 2001 From: Hixie Date: Thu, 25 Jun 2015 17:08:06 -0700 Subject: [PATCH] Add asserts to catch potential misuses of the rendering framework. New asserts: - verify that after layout, the size fits the constraints - verify that after layout, the size isn't infinite - verify that you don't set the size in performLayout() if you have sizedByParent set - verify that nobody reads your size during layout except you, or your parent if they said parentUsesSize:true Fixes some bugs found by those asserts: - RenderBlock, RenderStack, and RenderScaffold were not always setting parentUsesSize correctly - RenderScaffold was setting its slot entries to null rather than removing them when the slot went away, which led to null derefs in certain circumstances Also, rename a local variable in RenderStack.performLayout() because it was shadowing a variable on the object itself, which was really confusing when I first tried to debug this function... R=abarth@chromium.org Review URL: https://codereview.chromium.org/1213473003. --- engine/core/painting/Size.dart | 2 + examples/rendering/sector_layout.dart | 11 ++++++ sdk/lib/rendering/README.md | 7 ++++ sdk/lib/rendering/block.dart | 3 +- sdk/lib/rendering/box.dart | 55 +++++++++++++++------------ sdk/lib/rendering/object.dart | 35 ++++++++++++++++- sdk/lib/rendering/stack.dart | 50 ++++++++++++------------ sdk/lib/widgets/scaffold.dart | 9 +++-- 8 files changed, 115 insertions(+), 57 deletions(-) diff --git a/engine/core/painting/Size.dart b/engine/core/painting/Size.dart index fefdf4f329d..7738fd5559e 100644 --- a/engine/core/painting/Size.dart +++ b/engine/core/painting/Size.dart @@ -22,6 +22,8 @@ class Size { Size operator +(Size other) => new Size(width + other.width, height + other.height); Size operator -(Size other) => new Size(width - other.width, height - other.height); + bool get isInfinite => width >= double.INFINITY || height >= double.INFINITY; + // does the equivalent of "return new Point(0,0) + this" Point toPoint() => new Point(this.width, this.height); diff --git a/examples/rendering/sector_layout.dart b/examples/rendering/sector_layout.dart index b17b31b98ce..85dcc41691f 100644 --- a/examples/rendering/sector_layout.dart +++ b/examples/rendering/sector_layout.dart @@ -75,6 +75,17 @@ abstract class RenderSector extends RenderObject { } SectorConstraints get constraints => super.constraints; + bool debugDoesMeetConstraints() { + assert(constraints != null); + assert(deltaRadius != null); + assert(deltaRadius < double.INFINITY); + assert(deltaTheta != null); + assert(deltaTheta < double.INFINITY); + return constraints.minDeltaRadius <= deltaRadius && + deltaRadius <= math.max(constraints.minDeltaRadius, constraints.maxDeltaRadius) && + constraints.minDeltaTheta <= deltaTheta && + deltaTheta <= math.max(constraints.minDeltaTheta, constraints.maxDeltaTheta); + } void performResize() { // default behaviour for subclasses that have sizedByParent = true deltaRadius = constraints.constrainDeltaRadius(0.0); diff --git a/sdk/lib/rendering/README.md b/sdk/lib/rendering/README.md index 8cd5c961ccc..e63e8075efc 100644 --- a/sdk/lib/rendering/README.md +++ b/sdk/lib/rendering/README.md @@ -85,6 +85,13 @@ preinitialise the `parentData` member to set its values before you add a node to its parent, you can preemptively call that future parent's `setParentData()` method with the future child as the argument. +TODO(ianh): Discuss putting per-child configuration information for +the parent on the child's parentData. + +If you change a child's parentData dynamically, you must also call +markNeedsLayout() on the parent, otherwise the new information will +not take effect until something else triggers a layout. + ### Box Model #### Dimensions diff --git a/sdk/lib/rendering/block.dart b/sdk/lib/rendering/block.dart index 3ab31711dfe..4cceb68a992 100644 --- a/sdk/lib/rendering/block.dart +++ b/sdk/lib/rendering/block.dart @@ -84,12 +84,11 @@ class RenderBlock extends RenderBox with ContainerRenderObjectMixin minHeight >= maxHeight; bool get isTight => hasTightWidth && hasTightHeight; + bool contains(Size size) { + return (minWidth <= size.width) && (size.width <= math.max(minWidth, maxWidth)) && + (minHeight <= size.height) && (size.height <= math.max(minHeight, maxHeight)); + } + bool operator ==(other) { if (identical(this, other)) return true; @@ -290,25 +295,13 @@ abstract class RenderBox extends RenderObject { return null; } - // This whole block should only be here in debug builds - bool _debugDoingThisLayout = false; - bool _debugCanParentUseSize; - void layoutWithoutResize() { - _debugDoingThisLayout = true; - _debugCanParentUseSize = false; - super.layoutWithoutResize(); - _debugCanParentUseSize = null; - _debugDoingThisLayout = false; - } - void layout(dynamic constraints, { bool parentUsesSize: false }) { - _debugDoingThisLayout = true; - _debugCanParentUseSize = parentUsesSize; - super.layout(constraints, parentUsesSize: parentUsesSize); - _debugCanParentUseSize = null; - _debugDoingThisLayout = false; - } - BoxConstraints get constraints => super.constraints; + bool debugDoesMeetConstraints() { + assert(constraints != null); + assert(_size != null); + assert(!_size.isInfinite); + return constraints.contains(_size); + } void performResize() { // default behaviour for subclasses that have sizedByParent = true size = constraints.constrain(Size.zero); @@ -329,19 +322,34 @@ abstract class RenderBox extends RenderObject { } void hitTestChildren(HitTestResult result, { Point position }) { } + // TODO(ianh): move size up to before constraints // TODO(ianh): In non-debug builds, this should all just be: // Size size = Size.zero; // In debug builds, however: Size _size = Size.zero; - Size get size => _size; + Size get size { + if (_size is _DebugSize) { + final _DebugSize _size = this._size; // TODO(ianh): Remove this once the analyzer is cleverer + assert(_size._owner == this); + if (RenderObject.debugActiveLayout != null) { + // we are always allowed to access our own size (for print debugging and asserts if nothing else) + // other than us, the only object that's allowed to read our size is our parent, if they're said they will + assert(debugDoingThisResize || debugDoingThisLayout || + (RenderObject.debugActiveLayout == parent && _size._canBeUsedByParent)); + } + assert(_size == this._size); // TODO(ianh): Remove this once the analyzer is cleverer + } + return _size; + } void set size(Size value) { assert(RenderObject.debugDoingLayout); - assert(_debugDoingThisLayout); + assert((sizedByParent && debugDoingThisResize) || + (!sizedByParent && debugDoingThisLayout)); if (value is _DebugSize) { assert(value._canBeUsedByParent); assert(value._owner.parent == this); } - _size = inDebugBuild ? new _DebugSize(value, this, _debugCanParentUseSize) : value; + _size = inDebugBuild ? new _DebugSize(value, this, debugCanParentUseSize) : value; } String debugDescribeSettings(String prefix) => '${super.debugDescribeSettings(prefix)}${prefix}size: ${size}\n'; @@ -1134,11 +1142,8 @@ class RenderView extends RenderObject with RenderObjectWithChildMixin assert(_size.height < double.INFINITY); assert(_size.width < double.INFINITY); - if (child != null) { + if (child != null) child.layout(new BoxConstraints.tight(_size)); - assert(child.size.width == width); - assert(child.size.height == height); - } } void rotate({ int oldAngle, int newAngle, Duration time }) { diff --git a/sdk/lib/rendering/object.dart b/sdk/lib/rendering/object.dart index 094796673bd..25083a2c9a9 100644 --- a/sdk/lib/rendering/object.dart +++ b/sdk/lib/rendering/object.dart @@ -80,11 +80,20 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { static List _nodesNeedingLayout = new List(); static bool _debugDoingLayout = false; static bool get debugDoingLayout => _debugDoingLayout; + bool _debugDoingThisResize = false; + bool get debugDoingThisResize => _debugDoingThisResize; + bool _debugDoingThisLayout = false; + bool get debugDoingThisLayout => _debugDoingThisLayout; + static RenderObject _debugActiveLayout = null; + static RenderObject get debugActiveLayout => _debugActiveLayout; + bool _debugCanParentUseSize; + bool get debugCanParentUseSize => _debugCanParentUseSize; bool _needsLayout = true; bool get needsLayout => _needsLayout; RenderObject _relayoutSubtreeRoot; Constraints _constraints; Constraints get constraints => _constraints; + bool debugDoesMeetConstraints(); // override this in a subclass to verify that your state matches the constraints object bool debugAncestorsAlreadyMarkedNeedsLayout() { if (_relayoutSubtreeRoot == null) return true; // we haven't yet done layout even once, so there's nothing for us to do @@ -152,7 +161,14 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { void layoutWithoutResize() { try { assert(_relayoutSubtreeRoot == this); + _debugCanParentUseSize = false; + _debugDoingThisLayout = true; + RenderObject debugPreviousActiveLayout = _debugActiveLayout; + _debugActiveLayout = this; performLayout(); + _debugActiveLayout = debugPreviousActiveLayout; + _debugDoingThisLayout = false; + _debugCanParentUseSize = null; } catch (e, stack) { print('Exception raised during layout of ${this}: ${e}'); print(stack); @@ -172,9 +188,20 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { return; _constraints = constraints; _relayoutSubtreeRoot = relayoutSubtreeRoot; - if (sizedByParent) + _debugCanParentUseSize = parentUsesSize; + if (sizedByParent) { + _debugDoingThisResize = true; performResize(); + _debugDoingThisResize = false; + } + _debugDoingThisLayout = true; + RenderObject debugPreviousActiveLayout = _debugActiveLayout; + _debugActiveLayout = this; performLayout(); + _debugActiveLayout = debugPreviousActiveLayout; + _debugDoingThisLayout = false; + _debugCanParentUseSize = null; + assert(debugDoesMeetConstraints()); _needsLayout = false; markNeedsPaint(); assert(parent == this.parent); // TODO(ianh): Remove this once the analyzer is cleverer @@ -250,6 +277,8 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { String toString([String prefix = '']) { + RenderObject debugPreviousActiveLayout = _debugActiveLayout; + _debugActiveLayout = null; String header = '${runtimeType}'; if (_relayoutSubtreeRoot != null && _relayoutSubtreeRoot != this) { int count = 1; @@ -265,7 +294,9 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { if (!attached) header += ' DETACHED'; prefix += ' '; - return '${header}\n${debugDescribeSettings(prefix)}${debugDescribeChildren(prefix)}'; + String result = '${header}\n${debugDescribeSettings(prefix)}${debugDescribeChildren(prefix)}'; + _debugActiveLayout = debugPreviousActiveLayout; + return result; } String debugDescribeSettings(String prefix) => '${prefix}parentData: ${parentData}\n${prefix}constraints: ${constraints}\n'; String debugDescribeChildren(String prefix) => ''; diff --git a/sdk/lib/rendering/stack.dart b/sdk/lib/rendering/stack.dart index be73d6f1aa4..33563e61bf9 100644 --- a/sdk/lib/rendering/stack.dart +++ b/sdk/lib/rendering/stack.dart @@ -150,45 +150,45 @@ class RenderStack extends RenderBox with ContainerRenderObjectMixin= 0.0 && x + child.size.width <= size.width); double y = 0.0; - if (parentData.top != null) - y = parentData.top; - else if (parentData.bottom != null) - y = size.height - parentData.bottom - child.size.height; + if (childData.top != null) + y = childData.top; + else if (childData.bottom != null) + y = size.height - childData.bottom - child.size.height; assert(y >= 0.0 && y + child.size.height <= size.height); - parentData.position = new Point(x, y); + childData.position = new Point(x, y); } - child = parentData.nextSibling; + child = childData.nextSibling; } } diff --git a/sdk/lib/widgets/scaffold.dart b/sdk/lib/widgets/scaffold.dart index e975e516b84..6f9208db97f 100644 --- a/sdk/lib/widgets/scaffold.dart +++ b/sdk/lib/widgets/scaffold.dart @@ -39,9 +39,12 @@ class RenderScaffold extends RenderBox { return; if (old != null) dropChild(old); - _slots[slot] = value; - if (value != null) + if (value == null) { + _slots.remove(slot); + } else { + _slots[slot] = value; adoptChild(value); + } markNeedsLayout(); } @@ -118,7 +121,7 @@ class RenderScaffold extends RenderBox { if (_slots[ScaffoldSlots.floatingActionButton] != null) { RenderBox floatingActionButton = _slots[ScaffoldSlots.floatingActionButton]; Size area = new Size(size.width - kButtonX, size.height - kButtonY); - floatingActionButton.layout(new BoxConstraints.loose(area)); + floatingActionButton.layout(new BoxConstraints.loose(area), parentUsesSize: true); assert(floatingActionButton.parentData is BoxParentData); floatingActionButton.parentData.position = (area - floatingActionButton.size).toPoint(); }