From 0327141c370a5dacd80f1a0cb7525b354fa8a9e3 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Thu, 10 Mar 2016 17:13:45 -0800 Subject: [PATCH] Prepare to make RenderObjectElement buildable This patch prepares us to pass a BuildContext to RenderObjectWidgets, which will make it possible to rebuild RenderObjectElements: * Delay creation of the render object until mount(). That will let us pass `this` to createRenderObject and have the inherited elements be initialized. * Cleanup widgets that take builder closures to prepare for their RenderObjectElement to be rebuilt more often. * Add a test for the interaction between inherited widgets and MixedViewport. Related to #2598 --- packages/flutter/lib/src/widgets/basic.dart | 8 ++++ .../flutter/lib/src/widgets/framework.dart | 30 ++---------- .../lib/src/widgets/mixed_viewport.dart | 16 ++++--- .../lib/src/widgets/virtual_viewport.dart | 2 + .../test/widget/mixed_viewport_test.dart | 46 ++++++++++++++++++- 5 files changed, 69 insertions(+), 33 deletions(-) diff --git a/packages/flutter/lib/src/widgets/basic.dart b/packages/flutter/lib/src/widgets/basic.dart index a6e746cb57..e48f4d738e 100644 --- a/packages/flutter/lib/src/widgets/basic.dart +++ b/packages/flutter/lib/src/widgets/basic.dart @@ -2319,9 +2319,17 @@ class KeyedSubtree extends StatelessComponent { Widget build(BuildContext context) => child; } +/// A platonic widget that invokes a closure to obtain its child widget. class Builder extends StatelessComponent { Builder({ Key key, this.builder }) : super(key: key); + + /// Called to obtain the child widget. + /// + /// This function is invoked whether this widget is included in its parent's + /// build and the old widget (if any) that it synchronizes with has a distinct + /// object identity. final WidgetBuilder builder; + Widget build(BuildContext context) => builder(context); } diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index 899ea85576..699d7c6c42 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -1482,14 +1482,11 @@ class InheritedElement extends _ProxyElement { /// Base class for instantiations of RenderObjectWidget subclasses abstract class RenderObjectElement extends BuildableElement { - RenderObjectElement(T widget) - : _renderObject = widget.createRenderObject(), super(widget) { - assert(() { debugUpdateRenderObjectOwner(); return true; }); - } + RenderObjectElement(T widget) : super(widget); /// The underlying [RenderObject] for this element RenderObject get renderObject => _renderObject; - final RenderObject _renderObject; + RenderObject _renderObject; RenderObjectElement _ancestorRenderObjectElement; @@ -1512,8 +1509,9 @@ abstract class RenderObjectElement extends Buildab void mount(Element parent, dynamic newSlot) { super.mount(parent, newSlot); - assert(_slot == newSlot); + _renderObject = widget.createRenderObject(); assert(() { debugUpdateRenderObjectOwner(); return true; }); + assert(_slot == newSlot); attachRenderObject(newSlot); _dirty = false; } @@ -1532,29 +1530,9 @@ abstract class RenderObjectElement extends Buildab } void performRebuild() { - reinvokeBuilders(); _dirty = false; } - void reinvokeBuilders() { - // There's no way to mark a normal RenderObjectElement dirty. - // We inherit from BuildableElement so that subclasses can themselves - // implement reinvokeBuilders() if they do provide a way to mark themeselves - // dirty, e.g. if they have a builder callback. (Builder callbacks have a - // 'BuildContext' argument which you can pass to Theme.of() and other - // InheritedWidget APIs which eventually trigger a rebuild.) - assert(() { - throw new WidgetError( - '$runtimeType failed to implement reinvokeBuilders(), but got marked dirty.\n' - 'If a RenderObjectElement subclass supports being marked dirty, then the ' - 'reinvokeBuilders() method must be implemented.\n' - 'If a RenderObjectElement uses a builder callback, it must support being ' - 'marked dirty, because builder callbacks can register the object as having ' - 'an Inherited dependency.' - ); - }); - } - /// Utility function for subclasses that have one or more lists of children. /// Attempts to update the given old children list using the given new /// widgets, removing obsolete elements and introducing new ones as necessary, diff --git a/packages/flutter/lib/src/widgets/mixed_viewport.dart b/packages/flutter/lib/src/widgets/mixed_viewport.dart index 4068b25137..84c6e2c366 100644 --- a/packages/flutter/lib/src/widgets/mixed_viewport.dart +++ b/packages/flutter/lib/src/widgets/mixed_viewport.dart @@ -192,15 +192,18 @@ class _MixedViewportElement extends RenderObjectElement { // includes some of the state from that component. The component calls // setState() on itself. It rebuilds. Part of that involves rebuilding // us, but now what? If we don't reinvoke the builders. then they will - // not be rebuilt, and so the new state won't be used. - // Note that if the builders are to change so much that the _sizes_ of + // not be rebuilt, and so the new state won't be used. Therefore, we use + // the object identity of the widget to determine whether to reinvoke the + // builders. + // + // If the builders are to change so much that the _sizes_ of // the children would change, then the parent must change the 'token'. if (!renderObject.needsLayout) - reinvokeBuilders(); + performRebuild(); } } - void reinvokeBuilders() { + void performRebuild() { // we just need to redraw our existing widgets as-is if (_childrenByKey.length > 0) { assert(_firstVisibleChildIndex >= 0); @@ -223,6 +226,7 @@ class _MixedViewportElement extends RenderObjectElement { previousChild = newElement; } } + super.performRebuild(); } void layout(BoxConstraints constraints) { @@ -242,7 +246,7 @@ class _MixedViewportElement extends RenderObjectElement { final double newExtent = _didReachLastChild ? _childOffsets.last : double.INFINITY; Size contentSize; switch (widget.direction) { - case Axis.vertical: + case Axis.vertical: contentSize = new Size(containerSize.width, newExtent); break; case Axis.horizontal: @@ -257,7 +261,7 @@ class _MixedViewportElement extends RenderObjectElement { _lastReportedDimensions = dimensions; Offset overrideOffset = widget.onPaintOffsetUpdateNeeded(dimensions); switch (widget.direction) { - case Axis.vertical: + case Axis.vertical: assert(overrideOffset.dx == 0.0); _overrideStartOffset = overrideOffset.dy; break; diff --git a/packages/flutter/lib/src/widgets/virtual_viewport.dart b/packages/flutter/lib/src/widgets/virtual_viewport.dart index 28857f1239..af672f1092 100644 --- a/packages/flutter/lib/src/widgets/virtual_viewport.dart +++ b/packages/flutter/lib/src/widgets/virtual_viewport.dart @@ -256,6 +256,8 @@ class _LazyWidgetProvider extends _WidgetProvider { List _widgets; void didUpdateWidget(VirtualViewportFromBuilder oldWidget, VirtualViewportFromBuilder newWidget) { + // TODO(abarth): We shouldn't check the itemBuilder closure for equality with. + // instead, we should use the widget's identity to decide whether to rebuild. if (_length != newWidget.itemCount || oldWidget?.itemBuilder != newWidget.itemBuilder) { _length = newWidget.itemCount; _base = null; diff --git a/packages/flutter/test/widget/mixed_viewport_test.dart b/packages/flutter/test/widget/mixed_viewport_test.dart index 770e2bf23c..a376ef9da1 100644 --- a/packages/flutter/test/widget/mixed_viewport_test.dart +++ b/packages/flutter/test/widget/mixed_viewport_test.dart @@ -3,7 +3,7 @@ // found in the LICENSE file. import 'package:flutter_test/flutter_test.dart'; -import 'package:flutter/widgets.dart'; +import 'package:flutter/material.dart'; import 'package:test/test.dart'; import 'test_widgets.dart'; @@ -196,4 +196,48 @@ void main() { text.clear(); }); }); + + test('MixedViewport reinvoke builders', () { + testWidgets((WidgetTester tester) { + StateSetter setState; + ThemeData themeData = new ThemeData.light(); + + IndexedBuilder itemBuilder = (BuildContext context, int i) { + return new Container( + key: new ValueKey(i), + width: 500.0, // this should be ignored + height: 220.0, + decoration: new BoxDecoration( + backgroundColor: Theme.of(context).primaryColor + ), + child: new Text("$i") + ); + }; + + Widget viewport = new MixedViewport(builder: itemBuilder); + + tester.pumpWidget( + new StatefulBuilder( + builder: (BuildContext context, StateSetter setter) { + setState = setter; + return new Theme(data: themeData, child: viewport); + } + ) + ); + + DecoratedBox widget = tester.findWidgetOfType(DecoratedBox); + BoxDecoration decoraton = widget.decoration; + expect(decoraton.backgroundColor, equals(Colors.blue[500])); + + setState(() { + themeData = new ThemeData(primarySwatch: Colors.green); + }); + + tester.pump(); + + widget = tester.findWidgetOfType(DecoratedBox); + decoraton = widget.decoration; + expect(decoraton.backgroundColor, equals(Colors.green[500])); + }); + }); }