Merge pull request #1369 from abarth/notify_global_key_listeners

Actually notify GlobalKey listeners in fn3

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<BuildableElement> _dirtyElements = new List<BuildableElement>();
-
-  int _debugBuildingAtDepth;
+  List<BuildableElement> _dirtyElements = new List<BuildableElement>();
 
   /// 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<BuildableElement> 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<BuildableElement> sortedDirtyElements = new List<BuildableElement>();
-    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

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<T extends StatefulComponent> {
   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<T extends Widget> extends Element<T> {
   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<T extends Widget> extends Element<T> {
     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<T extends Widget> extends Element<T> {
   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<T extends Widget> extends Element<T> {
   /// 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<StatefulComponent> {
     : _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<StatefulComponent> {
       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<StatefulComponent> {
     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;
   }

sky/packages/sky/lib/src/fn3/homogeneous_viewport.dart

@@ -90,12 +90,14 @@ class HomogeneousViewportElement extends RenderObjectElement<HomogeneousViewport
   }
 
   void layout(BoxConstraints constraints) {
-    // we lock the framework state (meaning that no elements can call markNeedsBuild()) because we are
-    // in the middle of layout and if we allowed people to set state, they'd expect to have that state
-    // reflected immediately, which, if we were to try to honour it, would potentially result in
-    // assertions since you can't normally mutate the render object tree during layout. (If there was
-    // a way to limit this to only descendants of this, it'd be ok, since we are exempt from that
-    // assert since we are actively doing our own layout still.)
+    // 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
+    // state, they'd expect to have that state reflected immediately, which, if
+    // we were to try to honour it, would potentially result in assertions
+    // because you can't normally mutate the render object tree during layout.
+    // (If there were a way to limit these writes to descendants of this, it'd
+    // be ok because we are exempt from that assert since we are still actively
+    // doing our own layout.)
     BuildableElement.lockState(() {
       double mainAxisExtent = widget.direction == ScrollDirection.vertical ? constraints.maxHeight : constraints.maxWidth;
       double offset;

sky/unit/test/widget/build_scope_test.dart

@@ -0,0 +1,126 @@
+import 'package:sky/src/fn3.dart';
+import 'package:test/test.dart';
+
+import '../fn3/widget_tester.dart';
+import '../fn3/test_widgets.dart';
+
+class ProbeWidget extends StatefulComponent {
+  ProbeWidgetState createState() => new ProbeWidgetState();
+}
+
+class ProbeWidgetState extends State<ProbeWidget> {
+  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<BadWidgetParent> {
+  Widget build(BuildContext context) {
+    return new BadWidget(this);
+  }
+}
+
+class BadDisposeWidget extends StatefulComponent {
+  BadDisposeWidgetState createState() => new BadDisposeWidgetState();
+}
+
+class BadDisposeWidgetState extends State<BadDisposeWidget> {
+  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);
+  });
+}