diff --git a/sky/packages/sky/lib/src/fn3/binding.dart b/sky/packages/sky/lib/src/fn3/binding.dart index ccf83ead6a0..00ebaed5fc2 100644 --- a/sky/packages/sky/lib/src/fn3/binding.dart +++ b/sky/packages/sky/lib/src/fn3/binding.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:async'; + import 'package:sky/animation.dart'; import 'package:sky/rendering.dart'; import 'package:sky/src/fn3/framework.dart'; @@ -15,7 +17,7 @@ class WidgetFlutterBinding extends FlutterBinding { } /// Ensures that there is a FlutterBinding object instantiated. - static void initBinding() { + static void ensureInitialized() { if (FlutterBinding.instance == null) new WidgetFlutterBinding(); assert(FlutterBinding.instance is WidgetFlutterBinding); @@ -38,16 +40,14 @@ class WidgetFlutterBinding extends FlutterBinding { void beginFrame(double timeStamp) { buildDirtyElements(); super.beginFrame(timeStamp); + scheduleMicrotask(GlobalKey.checkForDuplicatesAndNotifyListeners); } - final List _dirtyElements = new List(); - - int _debugBuildingAtDepth; + List _dirtyElements = new List(); /// Adds an element to the dirty elements list so that it will be rebuilt /// when buildDirtyElements is called. void scheduleBuildFor(BuildableElement element) { - assert(_debugBuildingAtDepth == null || element.depth > _debugBuildingAtDepth); assert(!_dirtyElements.contains(element)); assert(element.dirty); if (_dirtyElements.isEmpty) @@ -55,46 +55,30 @@ class WidgetFlutterBinding extends FlutterBinding { _dirtyElements.add(element); } - void _absorbDirtyElements(List list) { - assert(_debugBuildingAtDepth != null); - assert(!_dirtyElements.any((element) => element.depth <= _debugBuildingAtDepth)); - _dirtyElements.sort((BuildableElement a, BuildableElement b) => a.depth - b.depth); - list.addAll(_dirtyElements); - _dirtyElements.clear(); - } - /// Builds all the elements that were marked as dirty using schedule(), in depth order. /// If elements are marked as dirty while this runs, they must be deeper than the algorithm /// has yet reached. /// This is called by beginFrame(). void buildDirtyElements() { - assert(_debugBuildingAtDepth == null); if (_dirtyElements.isEmpty) return; - assert(() { _debugBuildingAtDepth = 0; return true; }); - List sortedDirtyElements = new List(); - int index = 0; - do { - _absorbDirtyElements(sortedDirtyElements); - for (; index < sortedDirtyElements.length; index += 1) { - BuildableElement element = sortedDirtyElements[index]; - assert(() { - if (element.depth > _debugBuildingAtDepth) - _debugBuildingAtDepth = element.depth; - return element.depth == _debugBuildingAtDepth; - }); + BuildableElement.lockState(() { + _dirtyElements.sort((BuildableElement a, BuildableElement b) => a.depth - b.depth); + for (BuildableElement element in _dirtyElements) element.rebuild(); - } - } while (_dirtyElements.isNotEmpty); - assert(() { _debugBuildingAtDepth = null; return true; }); + _dirtyElements.clear(); + }); + assert(_dirtyElements.isEmpty); } } void runApp(Widget app) { - WidgetFlutterBinding.initBinding(); - WidgetFlutterBinding.instance.renderViewElement.update( - WidgetFlutterBinding.instance.describeApp(app) - ); + WidgetFlutterBinding.ensureInitialized(); + BuildableElement.lockState(() { + WidgetFlutterBinding.instance.renderViewElement.update( + WidgetFlutterBinding.instance.describeApp(app) + ); + }); } /// This class provides a bridge from a RenderObject to an Element tree. The diff --git a/sky/packages/sky/lib/src/fn3/framework.dart b/sky/packages/sky/lib/src/fn3/framework.dart index 7c6df961c9a..ba0f5d77d81 100644 --- a/sky/packages/sky/lib/src/fn3/framework.dart +++ b/sky/packages/sky/lib/src/fn3/framework.dart @@ -118,8 +118,7 @@ abstract class GlobalKey extends Key { assert(removed); } - // TODO(ianh): call this - static void _notifyListeners() { + static void checkForDuplicatesAndNotifyListeners() { assert(() { String message = ''; for (GlobalKey key in _debugDuplicates.keys) { @@ -326,12 +325,7 @@ abstract class State { void setState(void fn()) { assert(_debugLifecycleState != _StateLifecycle.defunct); fn(); - if (_element._builder != null) { - // _element._builder is set after initState(). We verify that we're past - // that before calling markNeedsBuild() so that setState()s triggered - // during initState() during lockState() don't cause any trouble. - _element.markNeedsBuild(); - } + _element.markNeedsBuild(); } /// Called when this object is removed from the tree. Override this to clean @@ -645,6 +639,17 @@ abstract class BuildableElement extends Element { 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. + bool _debugAllowIgnoredCallsToMarkNeedsBuild = false; + bool _debugSetAllowIgnoredCallsToMarkNeedsBuild(bool value) { + assert(_debugAllowIgnoredCallsToMarkNeedsBuild == !value); + _debugAllowIgnoredCallsToMarkNeedsBuild = value; + return true; + } + void mount(Element parent, dynamic newSlot) { super.mount(parent, newSlot); assert(_child == null); @@ -664,21 +669,27 @@ abstract class BuildableElement extends Element { if (!_dirty) return; assert(_debugLifecycleState == _ElementLifecycle.mounted); - _dirty = false; + assert(_debugStateLocked); + assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(true)); Widget built; try { built = _builder(this); assert(built != null); } catch (e, stack) { - _debugReportException('building $this', 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 $this', e, stack); + _debugReportException('building ${_widget}', e, stack); built = new ErrorWidget(); _child = updateChild(null, built, slot); } @@ -689,12 +700,16 @@ abstract class BuildableElement extends Element { static int _debugStateLockLevel = 0; static bool get _debugStateLocked => _debugStateLockLevel > 0; - /// Calls the callback argument synchronously, but in a context where calls to - /// State.setState() will fail. Use this when it is possible that you will - /// trigger code in components but want to make sure that there is no - /// possibility that any components will be marked dirty, for example because - /// you are in the middle of layout and you are not going to be flushing the - /// build queue (since that could mutate the layout tree). + /// Establishes a scope in which component build functions can run. + /// + /// Inside a build scope, component build functions are allowed to run, but + /// State.setState() is forbidden. This mechanism prevents build functions + /// from transitively requiring other build functions to run, potentially + /// causing infinite loops. + /// + /// After unwinding the last build scope on the stack, the framework verifies + /// that each global key is used at most once and notifies listeners about + /// changes to global keys. static void lockState(void callback()) { _debugStateLockLevel += 1; try { @@ -712,10 +727,12 @@ abstract class BuildableElement extends Element { /// components dirty during event handlers before the frame begins, not during /// the build itself. void markNeedsBuild() { - assert(!_debugStateLocked); - assert(_debugLifecycleState == _ElementLifecycle.mounted); + assert(_debugLifecycleState != _ElementLifecycle.defunct); + assert(!_debugStateLocked || (_debugAllowIgnoredCallsToMarkNeedsBuild && _dirty)); if (_dirty) return; + assert(_debugLifecycleState == _ElementLifecycle.mounted); + assert(!_debugStateLocked); _dirty = true; assert(scheduleBuildFor != null); scheduleBuildFor(this); @@ -757,10 +774,17 @@ class StatefulComponentElement extends BuildableElement { : _state = widget.createState(), super(widget) { assert(_state._element == null); _state._element = this; + assert(_builder == null); + _builder = _state.build; assert(_state._config == null); _state._config = widget; assert(_state._debugLifecycleState == _StateLifecycle.created); - _state.initState(this); + try { + _debugSetAllowIgnoredCallsToMarkNeedsBuild(true); + _state.initState(this); + } finally { + _debugSetAllowIgnoredCallsToMarkNeedsBuild(false); + } assert(() { if (_state._debugLifecycleState == _StateLifecycle.initialized) return true; @@ -768,10 +792,6 @@ class StatefulComponentElement extends BuildableElement { return false; }); assert(() { _state._debugLifecycleState = _StateLifecycle.ready; return true; }); - assert(_builder == null); - // see State.setState() for why it's important that _builder be set after - // initState() is called. - _builder = _state.build; } State get state => _state; @@ -781,16 +801,30 @@ class StatefulComponentElement extends BuildableElement { super.update(newWidget); assert(widget == newWidget); StatefulComponent oldConfig = _state._config; - _state._config = widget; - _state.didUpdateConfig(oldConfig); + // Notice that we mark ourselves as dirty before calling didUpdateConfig to + // let authors call setState from within didUpdateConfig without triggering + // asserts. _dirty = true; + _state._config = widget; + try { + _debugSetAllowIgnoredCallsToMarkNeedsBuild(true); + _state.didUpdateConfig(oldConfig); + } finally { + _debugSetAllowIgnoredCallsToMarkNeedsBuild(false); + } rebuild(); } void unmount() { super.unmount(); _state.dispose(); - assert(_state._debugLifecycleState == _StateLifecycle.defunct); + assert(() { + if (_state._debugLifecycleState == _StateLifecycle.defunct) + return true; + print('${_state.runtimeType}.dispose failed to call super.dispose'); + return false; + }); + assert(!_dirty); // See BuildableElement.unmount for why this is important. _state._element = null; _state = null; } diff --git a/sky/packages/sky/lib/src/fn3/homogeneous_viewport.dart b/sky/packages/sky/lib/src/fn3/homogeneous_viewport.dart index 16d4e9930ec..f5a28b49c31 100644 --- a/sky/packages/sky/lib/src/fn3/homogeneous_viewport.dart +++ b/sky/packages/sky/lib/src/fn3/homogeneous_viewport.dart @@ -90,12 +90,14 @@ class HomogeneousViewportElement extends RenderObjectElement new ProbeWidgetState(); +} + +class ProbeWidgetState extends State { + static int buildCount = 0; + + void initState(BuildContext context) { + super.initState(context); + setState(() {}); + } + + void didUpdateConfig(ProbeWidget oldConfig) { + setState(() {}); + } + + Widget build(BuildContext context) { + setState(() {}); + buildCount++; + return new Container(); + } +} + +class BadWidget extends StatelessComponent { + BadWidget(this.parentState); + + final State parentState; + + Widget build(BuildContext context) { + parentState.setState(() {}); + return new Container(); + } +} + +class BadWidgetParent extends StatefulComponent { + BadWidgetParentState createState() => new BadWidgetParentState(); +} + +class BadWidgetParentState extends State { + Widget build(BuildContext context) { + return new BadWidget(this); + } +} + +class BadDisposeWidget extends StatefulComponent { + BadDisposeWidgetState createState() => new BadDisposeWidgetState(); +} + +class BadDisposeWidgetState extends State { + Widget build(BuildContext context) { + return new Container(); + } + + void dispose() { + setState(() {}); + super.dispose(); + } +} + +void main() { + dynamic cachedException; + + setUp(() { + assert(cachedException == null); + debugWidgetsExceptionHandler = (String context, dynamic exception, StackTrace stack) { + cachedException = exception; + }; + }); + + tearDown(() { + assert(cachedException == null); + cachedException = null; + debugWidgetsExceptionHandler = null; + }); + + test('Legal times for setState', () { + WidgetTester tester = new WidgetTester(); + + GlobalKey flipKey = new GlobalKey(); + expect(ProbeWidgetState.buildCount, equals(0)); + tester.pumpFrame(new ProbeWidget()); + expect(ProbeWidgetState.buildCount, equals(1)); + tester.pumpFrame(new ProbeWidget()); + expect(ProbeWidgetState.buildCount, equals(2)); + tester.pumpFrame(new FlipComponent( + key: flipKey, + left: new Container(), + right: new ProbeWidget() + )); + expect(ProbeWidgetState.buildCount, equals(2)); + (flipKey.currentState as FlipComponentState).flip(); + tester.pumpFrameWithoutChange(); + expect(ProbeWidgetState.buildCount, equals(3)); + (flipKey.currentState as FlipComponentState).flip(); + tester.pumpFrameWithoutChange(); + expect(ProbeWidgetState.buildCount, equals(3)); + tester.pumpFrame(new Container()); + expect(ProbeWidgetState.buildCount, equals(3)); + }); + + test('Setting parent state during build is forbidden', () { + WidgetTester tester = new WidgetTester(); + + expect(cachedException, isNull); + tester.pumpFrame(new BadWidgetParent()); + expect(cachedException, isNotNull); + cachedException = null; + tester.pumpFrame(new Container()); + expect(cachedException, isNull); + }); + + test('Setting state during dispose is forbidden', () { + WidgetTester tester = new WidgetTester(); + + tester.pumpFrame(new BadDisposeWidget()); + expect(() { + tester.pumpFrame(new Container()); + }, throws); + }); +}