From 9752979e9905e77641a51906885f6e2f9ba4dc26 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Sat, 26 Sep 2015 16:15:55 -0700 Subject: [PATCH] Actually notify GlobalKey listeners in fn3 This patch makes a number of changes: 1) buildDirtyComponents now prevents all calls to setState, not just those higher in the tree. This change is necessary for consistency with MixedViewport and HomogeneousViewport because those widgets already build subwidgets with that restriction. If the "normal" build didn't enforce that rule, then some widgets would break when put inside a mixed or homogeneous viewport. 2) We now notify global key listeners in a microtask after beginFrame. That means setState is legal in these callbacks and that we'll produce another frame if someone calls setState in such a callback. --- sky/packages/sky/lib/src/fn3/binding.dart | 50 +++---- sky/packages/sky/lib/src/fn3/framework.dart | 88 ++++++++---- .../sky/lib/src/fn3/homogeneous_viewport.dart | 14 +- sky/unit/test/widget/build_scope_test.dart | 126 ++++++++++++++++++ 4 files changed, 212 insertions(+), 66 deletions(-) create mode 100644 sky/unit/test/widget/build_scope_test.dart 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); + }); +}