HomogeneousViewport support for Theme.of()

Previously, RenderObjectElements didn't support being marked dirty. This
is fine, except for MixedViewport and HomogeneousViewport, which have
builder functions to which they hand themselves as a BuildContext. If
those builder functions call, e.g., Theme.of(), then when the theme
changes, the Inherited logic tries to tell the RenderObjectElement
object that its dependencies changed and that doesn't go down well.

This patch fixes this by making RenderObjectElement a BuildableElement,
and making MixedViewport and HomogeneousViewport hook into that to
rebuild themselves appropriately.

Also, this was only found at all because ThemeData didn't implement
operator==, so we were aggressively marking the entire tree dirty all
the time. That's fixed here too.

Also, I changed card_collection.dart to have more features to make this
easier to test. This found bugs #1524, #1522, #1528, #1529, #1530, #1531.

examples/widgets/card_collection.dart

@@ -9,11 +9,13 @@ import 'package:sky/painting.dart';
 import 'package:sky/widgets.dart';
 
 class CardModel {
-  CardModel(this.value, this.height, this.color);
+  CardModel(this.value, this.height) {
+    label = "Item $value";
+  }
   int value;
   double height;
-  Color color;
-  String get label => "Item $value";
+  int get color => ((value % 9) + 1) * 100;
+  String label;
   Key get key => new ObjectKey(this);
 }
 
@@ -36,6 +38,7 @@ class CardCollectionState extends State<CardCollection> {
   final TextStyle backgroundTextStyle =
     Typography.white.title.copyWith(textAlign: TextAlign.center);
 
+  Map<int, Color> _primaryColor = Colors.deepPurple;
   List<CardModel> _cardModels;
   DismissDirection _dismissDirection = DismissDirection.horizontal;
   bool _snapToCenter = false;
@@ -50,19 +53,13 @@ class CardCollectionState extends State<CardCollection> {
       48.0, 63.0, 82.0, 146.0, 60.0, 55.0, 84.0, 96.0, 50.0,
       48.0, 63.0, 82.0, 146.0, 60.0, 55.0, 84.0, 96.0, 50.0
     ];
-    _cardModels = new List.generate(cardHeights.length, (i) {
-      Color color = Color.lerp(Colors.red[300], Colors.blue[900], i / cardHeights.length);
-      return new CardModel(i, cardHeights[i], color);
-    });
+    _cardModels = new List.generate(cardHeights.length, (i) => new CardModel(i, cardHeights[i]));
   }
 
   void _initFixedSizedCardModels() {
     const int cardCount = 27;
     const double cardHeight = 100.0;
-    _cardModels = new List.generate(cardCount, (i) {
-      Color color = Color.lerp(Colors.red[300], Colors.blue[900], i / cardCount);
-      return new CardModel(i, cardHeight, color);
-    });
+    _cardModels = new List.generate(cardCount, (i) => new CardModel(i, cardHeight));
   }
 
   void _initCardModels() {
@@ -126,9 +123,14 @@ class CardCollectionState extends State<CardCollection> {
           buildDrawerCheckbox("Fixed size cards", _fixedSizeCards, _toggleFixedSizeCards),
           buildDrawerCheckbox("Let the sun shine", _sunshine, _toggleSunshine),
           new DrawerDivider(),
-          buildDrawerRadioItem(DismissDirection.horizontal, 'action/code'),
-          buildDrawerRadioItem(DismissDirection.left, 'navigation/arrow_back'),
-          buildDrawerRadioItem(DismissDirection.right, 'navigation/arrow_forward'),
+          buildDrawerRadioItem("Deep Purple", Colors.deepPurple, _primaryColor, _selectColor),
+          buildDrawerRadioItem("Green", Colors.green, _primaryColor, _selectColor),
+          buildDrawerRadioItem("Amber", Colors.amber, _primaryColor, _selectColor),
+          buildDrawerRadioItem("Teal", Colors.teal, _primaryColor, _selectColor),
+          new DrawerDivider(),
+          buildDrawerRadioItem("Dismiss horizontally", DismissDirection.horizontal, _dismissDirection, _changeDismissDirection, icon: 'action/code'),
+          buildDrawerRadioItem("Dismiss left", DismissDirection.left, _dismissDirection, _changeDismissDirection, icon: 'navigation/arrow_back'),
+          buildDrawerRadioItem("Dismiss right", DismissDirection.right, _dismissDirection, _changeDismissDirection, icon: 'navigation/arrow_forward'),
         ])
       )
     );
@@ -158,6 +160,12 @@ class CardCollectionState extends State<CardCollection> {
     });
   }
 
+  void _selectColor(selection) {
+    setState(() {
+      _primaryColor = selection;
+    });
+  }
+
   void _changeDismissDirection(DismissDirection newDismissDirection) {
     setState(() {
       _dismissDirection = newDismissDirection;
@@ -175,16 +183,16 @@ class CardCollectionState extends State<CardCollection> {
     );
   }
 
-  Widget buildDrawerRadioItem(DismissDirection direction, String icon) {
+  Widget buildDrawerRadioItem(String label, itemValue, currentValue, RadioValueChanged onChanged, { String icon }) {
     return new DrawerItem(
       icon: icon,
-      onPressed: () { _changeDismissDirection(direction); },
+      onPressed: () { onChanged(itemValue); },
       child: new Row([
-        new Flexible(child: new Text(_dismissDirectionText(direction))),
+        new Flexible(child: new Text(label)),
         new Radio(
-          value: direction,
-          onChanged: _changeDismissDirection,
-          groupValue: _dismissDirection
+          value: itemValue,
+          groupValue: currentValue,
+          onChanged: onChanged
         )
       ])
     );
@@ -210,11 +218,22 @@ class CardCollectionState extends State<CardCollection> {
       onResized: () { _invalidator([index]); },
       onDismissed: () { dismissCard(cardModel); },
       child: new Card(
-        color: cardModel.color,
+        color: Theme.of(context).primarySwatch[cardModel.color],
         child: new Container(
           height: cardModel.height,
           padding: const EdgeDims.all(8.0),
-          child: new Center(child: new Text(cardModel.label, style: cardLabelStyle))
+          child: new Center(
+            child: new DefaultTextStyle(
+              style: cardLabelStyle,
+              child: new Input(
+                key: new GlobalObjectKey(cardModel),
+                initialValue: cardModel.label,
+                onChanged: (String value) {
+                  cardModel.label = value;
+                }
+              )
+            )
+          )
         )
       )
     );
@@ -341,9 +360,14 @@ class CardCollectionState extends State<CardCollection> {
       body = new Stack([body, indicator]);
     }
 
-    return new Scaffold(
-      toolBar: buildToolBar(),
-      body: body
+    return new Theme(
+      data: new ThemeData(
+        primarySwatch: _primaryColor
+      ),
+      child: new Scaffold(
+        toolBar: buildToolBar(),
+        body: body
+      )
     );
   }
 }
@@ -351,11 +375,6 @@ class CardCollectionState extends State<CardCollection> {
 void main() {
   runApp(new App(
     title: 'Cards',
-    theme: new ThemeData(
-      brightness: ThemeBrightness.light,
-      primarySwatch: Colors.blue,
-      accentColor: Colors.redAccent[200]
-    ),
     routes: {
       '/': (RouteArguments args) => new CardCollection(navigator: args.navigator),
     }

packages/flutter/lib/src/material/theme_data.dart

@@ -79,4 +79,38 @@ class ThemeData {
   Color get accentColor => _accentColor;
 
   final ThemeBrightness accentColorBrightness;
+
+  bool operator==(Object other) {
+    if (other.runtimeType != runtimeType)
+      return false;
+    ThemeData otherData = other;
+    return (otherData.brightness == brightness) &&
+           (otherData.primarySwatch == primarySwatch) &&
+           (otherData.canvasColor == canvasColor) &&
+           (otherData.cardColor == cardColor) &&
+           (otherData.dividerColor == dividerColor) &&
+           (otherData.hintColor == hintColor) &&
+           (otherData.highlightColor == highlightColor) &&
+           (otherData.selectedColor == selectedColor) &&
+           (otherData.hintOpacity == hintOpacity) &&
+           (otherData.text == text) &&
+           (otherData.primaryColorBrightness == primaryColorBrightness) &&
+           (otherData.accentColorBrightness == accentColorBrightness);
+  }
+  int get hashCode {
+    int value = 373;
+    value = 37 * value + brightness.hashCode;
+    value = 37 * value + primarySwatch.hashCode;
+    value = 37 * value + canvasColor.hashCode;
+    value = 37 * value + cardColor.hashCode;
+    value = 37 * value + dividerColor.hashCode;
+    value = 37 * value + hintColor.hashCode;
+    value = 37 * value + highlightColor.hashCode;
+    value = 37 * value + selectedColor.hashCode;
+    value = 37 * value + hintOpacity.hashCode;
+    value = 37 * value + text.hashCode;
+    value = 37 * value + primaryColorBrightness.hashCode;
+    value = 37 * value + accentColorBrightness.hashCode;
+    return value;
+  }
 }

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

@@ -751,8 +751,7 @@ abstract class Element<T extends Widget> implements BuildContext {
     assert(() { _debugLifecycleState = _ElementLifecycle.active; return true; });
   }
 
-  /// Called when an Element is removed from the tree.
-  /// Currently, an Element removed from the tree never returns.
+  /// Called when an Element is removed from the tree permanently.
   void unmount() {
     assert(_debugLifecycleState == _ElementLifecycle.inactive);
     assert(widget != null);
@@ -824,25 +823,21 @@ class ErrorWidget extends LeafRenderObjectWidget {
   RenderBox createRenderObject() => new RenderErrorBox();
 }
 
-typedef Widget WidgetBuilder(BuildContext context);
 typedef void BuildScheduler(BuildableElement element);
 
-/// Base class for the instantiation of StatelessComponent and StatefulComponent
-/// widgets.
+/// Base class for instantiations of widgets that have builders and can be
+/// marked dirty.
 abstract class BuildableElement<T extends Widget> extends Element<T> {
   BuildableElement(T widget) : super(widget);
 
-  WidgetBuilder _builder;
-  Element _child;
-
   /// Returns true if the element has been marked as needing rebuilding.
   bool get dirty => _dirty;
   bool _dirty = true;
 
-  // We to let component authors call setState from initState, didUpdateConfig,
-  // and build even when state is locked because its convenient and a no-op
-  // anyway. This flag ensures that this convenience is only allowed on the
-  // element currently undergoing initState, didUpdateConfig, or build.
+  // We let component authors call setState from initState, didUpdateConfig, and
+  // build even when state is locked because its convenient and a no-op anyway.
+  // This flag ensures that this convenience is only allowed on the element
+  // currently undergoing initState, didUpdateConfig, or build.
   bool _debugAllowIgnoredCallsToMarkNeedsBuild = false;
   bool _debugSetAllowIgnoredCallsToMarkNeedsBuild(bool value) {
     assert(_debugAllowIgnoredCallsToMarkNeedsBuild == !value);
@@ -850,73 +845,12 @@ abstract class BuildableElement<T extends Widget> extends Element<T> {
     return true;
   }
 
-  void mount(Element parent, dynamic newSlot) {
-    super.mount(parent, newSlot);
-    assert(_child == null);
-    assert(_active);
-    rebuild();
-    assert(_child != null);
-  }
-
-  static BuildableElement _debugCurrentBuildTarget;
-
-  /// Reinvokes the build() method of the StatelessComponent object (for
-  /// stateless components) or the State object (for stateful components) and
-  /// then updates the widget tree.
-  ///
-  /// Called automatically during mount() to generate the first build, by the
-  /// binding when scheduleBuild() has been called to mark this element dirty,
-  /// and by update() when the Widget has changed.
-  void rebuild() {
-    assert(_debugLifecycleState != _ElementLifecycle.initial);
-    if (!_active || !_dirty) {
-      _dirty = false;
-      return;
-    }
-    assert(_debugLifecycleState == _ElementLifecycle.active);
-    assert(_debugStateLocked);
-    assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(true));
-    BuildableElement debugPreviousBuildTarget;
-    assert(() {
-      debugPreviousBuildTarget = _debugCurrentBuildTarget;
-      _debugCurrentBuildTarget = this;
-     return true;
-    });
-    Widget built;
-    try {
-      built = _builder(this);
-      assert(built != null);
-    } catch (e, stack) {
-      _debugReportException('building ${_widget}', e, stack);
-      built = new ErrorWidget();
-    } finally {
-      // We delay marking the element as clean until after calling _builder so
-      // that attempts to markNeedsBuild() during build() will be ignored.
-      _dirty = false;
-      assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(false));
-    }
-
-    try {
-      _child = updateChild(_child, built, slot);
-      assert(_child != null);
-    } catch (e, stack) {
-      _debugReportException('building ${_widget}', e, stack);
-      built = new ErrorWidget();
-      _child = updateChild(null, built, slot);
-    }
-
-    assert(() {
-      assert(_debugCurrentBuildTarget == this);
-      _debugCurrentBuildTarget = debugPreviousBuildTarget;
-      return true;
-    });
-  }
-
   static BuildScheduler scheduleBuildFor;
 
   static int _debugStateLockLevel = 0;
   static bool get _debugStateLocked => _debugStateLockLevel > 0;
   static bool _debugBuilding = false;
+  static BuildableElement _debugCurrentBuildTarget;
 
   /// Establishes a scope in which component build functions can run.
   ///
@@ -989,17 +923,39 @@ abstract class BuildableElement<T extends Widget> extends Element<T> {
     scheduleBuildFor(this);
   }
 
-  void visitChildren(ElementVisitor visitor) {
-    if (_child != null)
-      visitor(_child);
+  /// Called by the binding when scheduleBuild() has been called to mark this
+  /// element dirty, and, in Components, by update() when the Widget has
+  /// changed.
+  void rebuild() {
+    assert(_debugLifecycleState != _ElementLifecycle.initial);
+    if (!_active || !_dirty) {
+      _dirty = false;
+      return;
+    }
+    assert(_debugLifecycleState == _ElementLifecycle.active);
+    assert(_debugStateLocked);
+    BuildableElement debugPreviousBuildTarget;
+    assert(() {
+      debugPreviousBuildTarget = _debugCurrentBuildTarget;
+      _debugCurrentBuildTarget = this;
+     return true;
+    });
+    try {
+      performRebuild();
+    } catch (e, stack) {
+      _debugReportException('rebuilding $this', e, stack);
+    } finally {
+      assert(() {
+        assert(_debugCurrentBuildTarget == this);
+        _debugCurrentBuildTarget = debugPreviousBuildTarget;
+        return true;
+      });
+    }
+    assert(!_dirty);
   }
 
-  bool detachChild(Element child) {
-    assert(child == _child);
-    _deactivateChild(_child);
-    _child = null;
-    return true;
-  }
+  /// Called by rebuild() after the appropriate checks have been made.
+  void performRebuild();
 
   void dependenciesChanged() {
     markNeedsBuild();
@@ -1012,8 +968,70 @@ abstract class BuildableElement<T extends Widget> extends Element<T> {
   }
 }
 
+typedef Widget WidgetBuilder(BuildContext context);
+
+/// Base class for the instantiation of StatelessComponent and StatefulComponent
+/// widgets.
+abstract class ComponentElement<T extends Widget> extends BuildableElement<T> {
+  ComponentElement(T widget) : super(widget);
+
+  WidgetBuilder _builder;
+  Element _child;
+
+  void mount(Element parent, dynamic newSlot) {
+    super.mount(parent, newSlot);
+    assert(_child == null);
+    assert(_active);
+    rebuild();
+    assert(_child != null);
+  }
+
+  /// Reinvokes the build() method of the StatelessComponent object (for
+  /// stateless components) or the State object (for stateful components) and
+  /// then updates the widget tree.
+  ///
+  /// Called automatically during mount() to generate the first build, and by
+  /// rebuild() when the element needs updating.
+  void performRebuild() {
+    assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(true));
+    Widget built;
+    try {
+      built = _builder(this);
+      assert(built != null);
+    } catch (e, stack) {
+      _debugReportException('building ${_widget}', e, stack);
+      built = new ErrorWidget();
+    } finally {
+      // We delay marking the element as clean until after calling _builder so
+      // that attempts to markNeedsBuild() during build() will be ignored.
+      _dirty = false;
+      assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(false));
+    }
+    try {
+      _child = updateChild(_child, built, slot);
+      assert(_child != null);
+    } catch (e, stack) {
+      _debugReportException('building ${_widget}', e, stack);
+      built = new ErrorWidget();
+      _child = updateChild(null, built, slot);
+    }
+  }
+
+  void visitChildren(ElementVisitor visitor) {
+    if (_child != null)
+      visitor(_child);
+  }
+
+  bool detachChild(Element child) {
+    assert(child == _child);
+    _deactivateChild(_child);
+    _child = null;
+    return true;
+  }
+}
+
 /// Instantiation of StatelessComponent widgets.
-class StatelessComponentElement<T extends StatelessComponent> extends BuildableElement<T> {
+class StatelessComponentElement<T extends StatelessComponent> extends ComponentElement<T> {
   StatelessComponentElement(T widget) : super(widget) {
     _builder = widget.build;
   }
@@ -1028,7 +1046,7 @@ class StatelessComponentElement<T extends StatelessComponent> extends BuildableE
 }
 
 /// Instantiation of StatefulComponent widgets.
-class StatefulComponentElement<T extends StatefulComponent, U extends State<T>> extends BuildableElement<T> {
+class StatefulComponentElement<T extends StatefulComponent, U extends State<T>> extends ComponentElement<T> {
   StatefulComponentElement(T widget)
     : _state = widget.createState(), super(widget) {
     assert(_state._debugTypesAreRight(widget)); // can't use T and U, since normally we don't actually set those
@@ -1142,8 +1160,10 @@ class InheritedElement extends StatelessComponentElement<InheritedWidget> {
     InheritedWidget oldWidget = widget;
     super.update(newWidget);
     assert(widget == newWidget);
-    if (widget.updateShouldNotify(oldWidget))
+    if (widget.updateShouldNotify(oldWidget)) {
+      assert(widget != oldWidget);
       _notifyDescendants();
+    }
   }
 
   void _notifyDescendants() {
@@ -1161,7 +1181,7 @@ class InheritedElement extends StatelessComponentElement<InheritedWidget> {
 }
 
 /// Base class for instantiations of RenderObjectWidget subclasses
-abstract class RenderObjectElement<T extends RenderObjectWidget> extends Element<T> {
+abstract class RenderObjectElement<T extends RenderObjectWidget> extends BuildableElement<T> {
   RenderObjectElement(T widget)
     : _renderObject = widget.createRenderObject(), super(widget);
 
@@ -1195,6 +1215,7 @@ abstract class RenderObjectElement<T extends RenderObjectWidget> extends Element
     ParentDataElement parentDataElement = _findAncestorParentDataElement();
     if (parentDataElement != null)
       updateParentData(parentDataElement.widget);
+    _dirty = false;
   }
 
   void update(T newWidget) {
@@ -1202,6 +1223,26 @@ abstract class RenderObjectElement<T extends RenderObjectWidget> extends Element
     super.update(newWidget);
     assert(widget == newWidget);
     widget.updateRenderObject(renderObject, oldWidget);
+    _dirty = false;
+  }
+
+  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.)
+    print('${runtimeType} failed to implement reinvokeBuilders(), but got marked dirty');
+    assert(() {
+      'reinvokeBuilders() not implemented';
+      return false;
+    });
   }
 
   /// Utility function for subclasses that have one or more lists of children.

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

@@ -37,8 +37,8 @@ class HomogeneousViewport extends RenderObjectWidget {
   RenderBlockViewport createRenderObject() => new RenderBlockViewport();
 
   bool isLayoutDifferentThan(HomogeneousViewport oldWidget) {
+    // changing the builder doesn't imply the layout changed
     return itemsWrap != oldWidget.itemsWrap ||
-           itemsWrap != oldWidget.itemsWrap ||
            itemExtent != oldWidget.itemExtent ||
            itemCount != oldWidget.itemCount ||
            direction != oldWidget.direction ||
@@ -89,6 +89,10 @@ class _HomogeneousViewportElement extends RenderObjectElement<HomogeneousViewpor
       _updateChildren();
   }
 
+  void reinvokeBuilders() {
+    _updateChildren();
+  }
+
   void layout(BoxConstraints constraints) {
     // We enter a build scope (meaning that markNeedsBuild() is forbidden)
     // because we are in the middle of layout and if we allowed people to set

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

@@ -164,27 +164,31 @@ class _MixedViewportElement extends RenderObjectElement<MixedViewport> {
     if (changes != _ChangeDescription.none || !isValid) {
       renderObject.markNeedsLayout();
     } else {
-      // we just need to redraw our existing widgets as-is
-      if (_childrenByKey.length > 0) {
-        assert(_firstVisibleChildIndex >= 0);
-        assert(renderObject != null);
-        final int startIndex = _firstVisibleChildIndex;
-        int lastIndex = startIndex + _childrenByKey.length - 1;
-        Element nextSibling = null;
-        for (int index = lastIndex; index > startIndex; index -= 1) {
-          final Widget newWidget = _buildWidgetAt(index);
-          final _ChildKey key = new _ChildKey.fromWidget(newWidget);
-          final Element oldElement = _childrenByKey[key];
-          assert(oldElement != null);
-          final Element newElement = updateChild(oldElement, newWidget, nextSibling);
-          assert(newElement != null);
-          _childrenByKey[key] = newElement;
-          // Verify that it hasn't changed size.
-          // If this assertion fires, it means you didn't call "invalidate"
-          // before changing the size of one of your items.
-          assert(_debugIsSameSize(newElement, index, _lastLayoutConstraints));
-          nextSibling = newElement;
-        }
+      reinvokeBuilders();
+    }
+  }
+
+  void reinvokeBuilders() {
+    // we just need to redraw our existing widgets as-is
+    if (_childrenByKey.length > 0) {
+      assert(_firstVisibleChildIndex >= 0);
+      assert(renderObject != null);
+      final int startIndex = _firstVisibleChildIndex;
+      int lastIndex = startIndex + _childrenByKey.length - 1;
+      Element nextSibling = null;
+      for (int index = lastIndex; index >= startIndex; index -= 1) {
+        final Widget newWidget = _buildWidgetAt(index);
+        final _ChildKey key = new _ChildKey.fromWidget(newWidget);
+        final Element oldElement = _childrenByKey[key];
+        assert(oldElement != null);
+        final Element newElement = updateChild(oldElement, newWidget, nextSibling);
+        assert(newElement != null);
+        _childrenByKey[key] = newElement;
+        // Verify that it hasn't changed size.
+        // If this assertion fires, it means you didn't call "invalidate"
+        // before changing the size of one of your items.
+        assert(_debugIsSameSize(newElement, index, _lastLayoutConstraints));
+        nextSibling = newElement;
       }
     }
   }

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

@@ -18,7 +18,6 @@ class RouteArguments {
 typedef Widget RouteBuilder(RouteArguments args);
 typedef RouteBuilder RouteGenerator(String name);
 typedef void StateRouteCallback(StateRoute route);
-typedef void _RouteCallback(Route route);
 
 class Navigator extends StatefulComponent {
   Navigator({

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

@@ -12,7 +12,7 @@ import 'package:sky/src/widgets/theme.dart';
 const sky.Color _kLightOffColor = const sky.Color(0x8A000000);
 const sky.Color _kDarkOffColor = const sky.Color(0xB2FFFFFF);
 
-typedef RadioValueChanged(Object value);
+typedef void RadioValueChanged(Object value);
 
 class Radio extends StatelessComponent {
   Radio({