From 8945e0112f846c37453f298d0bbb8db7cb1870e4 Mon Sep 17 00:00:00 2001 From: Hixie Date: Thu, 8 Oct 2015 15:18:23 -0700 Subject: [PATCH] Changing themes caused crash The root cause was that we crawled the tree to mark anyone who depended on the updated theme dirty _after_ we crawled it to rebuild it. Thus, if anyone was already marked dirty when the process started, then got marked clean by the first (rebuild) walk, then got marked dirty again by the notification, they'd be clean when they got the notification, despite already being in the dirty list, which would cause an assertion. Also IconTheme didn't have an operator==, so it was independently too aggressive about updates. --- .../flutter/lib/src/widgets/framework.dart | 75 +++++++++++-------- packages/flutter/lib/src/widgets/icon.dart | 3 + 2 files changed, 47 insertions(+), 31 deletions(-) diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index f92219de40..dd36a8600b 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -7,6 +7,8 @@ import 'dart:collection'; import 'package:sky/rendering.dart'; +// KEYS + /// A Key is an identifier for [Widget]s and [Element]s. A new Widget will only /// be used to reconfigure an existing Element if its Key is the same as its /// original Widget's Key. @@ -171,6 +173,8 @@ class GlobalObjectKey extends GlobalKey { } +// WIDGETS + /// A Widget object describes the configuration for an [Element]. /// Widget subclasses should be immutable with const constructors. /// Widgets form a tree that is then inflated into an Element tree. @@ -193,6 +197,8 @@ abstract class Widget { void debugFillDescription(List description) { } } +// TODO(ianh): move the next four classes to below InheritedWidget + /// RenderObjectWidgets provide the configuration for [RenderObjectElement]s, /// which wrap [RenderObject]s, which provide the actual rendering of the /// application. @@ -382,16 +388,14 @@ abstract class State { } } -abstract class ProxyWidget extends StatelessComponent { - const ProxyWidget({ Key key, Widget this.child }) : super(key: key); +abstract class ProxyComponent extends Widget { + const ProxyComponent({ Key key, this.child }) : super(key: key); final Widget child; - - Widget build(BuildContext context) => child; } -abstract class ParentDataWidget extends ProxyWidget { - ParentDataWidget({ Key key, Widget child }) +abstract class ParentDataWidget extends ProxyComponent { + const ParentDataWidget({ Key key, Widget child }) : super(key: key, child: child); ParentDataElement createElement() => new ParentDataElement(this); @@ -405,7 +409,7 @@ abstract class ParentDataWidget extends ProxyWidget { void applyParentData(RenderObject renderObject); } -abstract class InheritedWidget extends ProxyWidget { +abstract class InheritedWidget extends ProxyComponent { const InheritedWidget({ Key key, Widget child }) : super(key: key, child: child); @@ -414,6 +418,9 @@ abstract class InheritedWidget extends ProxyWidget { bool updateShouldNotify(InheritedWidget oldWidget); } + +// ELEMENTS + bool _canUpdate(Widget oldWidget, Widget newWidget) { return oldWidget.runtimeType == newWidget.runtimeType && oldWidget.key == newWidget.key; @@ -637,6 +644,7 @@ abstract class Element implements BuildContext { assert(_debugLifecycleState == _ElementLifecycle.active); assert(widget != null); assert(newWidget != null); + assert(newWidget != widget); assert(depth != null); assert(_active); assert(_canUpdate(widget, newWidget)); @@ -970,8 +978,8 @@ abstract class BuildableElement extends Element { typedef Widget WidgetBuilder(BuildContext context); -/// Base class for the instantiation of StatelessComponent and StatefulComponent -/// widgets. +/// Base class for the instantiation of StatelessComponent, StatefulComponent, +/// and ProxyComponent widgets. abstract class ComponentElement extends BuildableElement { ComponentElement(T widget) : super(widget); @@ -1114,7 +1122,26 @@ class StatefulComponentElement> } } -class ParentDataElement extends StatelessComponentElement { +abstract class ProxyElement extends ComponentElement { + ProxyElement(T widget) : super(widget) { + _builder = (BuildContext context) => this.widget.child; + } + + void update(T newWidget) { + T oldWidget = widget; + assert(widget != null); + assert(widget != newWidget); + super.update(newWidget); + assert(widget == newWidget); + notifyDescendants(oldWidget); + _dirty = true; + rebuild(); + } + + void notifyDescendants(T oldWidget); +} + +class ParentDataElement extends ProxyElement { ParentDataElement(ParentDataWidget widget) : super(widget); void mount(Element parent, dynamic slot) { @@ -1134,15 +1161,7 @@ class ParentDataElement extends StatelessComponentElement { super.mount(parent, slot); } - void update(ParentDataWidget newWidget) { - ParentDataWidget oldWidget = widget; - super.update(newWidget); - assert(widget == newWidget); - if (widget != oldWidget) - _notifyDescendants(); - } - - void _notifyDescendants() { + void notifyDescendants(ParentDataWidget oldWidget) { void notifyChildren(Element child) { if (child is RenderObjectElement) child.updateParentData(widget); @@ -1153,20 +1172,14 @@ class ParentDataElement extends StatelessComponentElement { } } -class InheritedElement extends StatelessComponentElement { + + +class InheritedElement extends ProxyElement { InheritedElement(InheritedWidget widget) : super(widget); - void update(StatelessComponent newWidget) { - InheritedWidget oldWidget = widget; - super.update(newWidget); - assert(widget == newWidget); - if (widget.updateShouldNotify(oldWidget)) { - assert(widget != oldWidget); - _notifyDescendants(); - } - } - - void _notifyDescendants() { + void notifyDescendants(InheritedWidget oldWidget) { + if (!widget.updateShouldNotify(oldWidget)) + return; final Type ourRuntimeType = widget.runtimeType; void notifyChildren(Element child) { if (child._dependencies != null && diff --git a/packages/flutter/lib/src/widgets/icon.dart b/packages/flutter/lib/src/widgets/icon.dart index 5dcee05232..9b71b138c6 100644 --- a/packages/flutter/lib/src/widgets/icon.dart +++ b/packages/flutter/lib/src/widgets/icon.dart @@ -14,6 +14,9 @@ enum IconThemeColor { white, black } class IconThemeData { const IconThemeData({ this.color }); final IconThemeColor color; + + bool operator==(other) => other.runtimeType == runtimeType && other.color == color; + int get hashCode => color.hashCode; } class IconTheme extends InheritedWidget {