diff --git a/packages/flutter/lib/src/animation/listener_helpers.dart b/packages/flutter/lib/src/animation/listener_helpers.dart index ad72ef7667..1f03100efb 100644 --- a/packages/flutter/lib/src/animation/listener_helpers.dart +++ b/packages/flutter/lib/src/animation/listener_helpers.dart @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:collection'; import 'dart:ui' show VoidCallback; import 'package:flutter/foundation.dart'; @@ -34,9 +35,11 @@ abstract class AnimationLazyListenerMixin implements _ListenerMixin { } /// Called when the number of listeners changes from zero to one. + @protected void didStartListening(); /// Called when the number of listeners changes from one to zero. + @protected void didStopListening(); /// Whether there are any listeners. @@ -61,7 +64,7 @@ abstract class AnimationEagerListenerMixin implements _ListenerMixin { /// A mixin that implements the addListener/removeListener protocol and notifies /// all the registered listeners when notifyListeners is called. abstract class AnimationLocalListenersMixin extends _ListenerMixin { - final List _listeners = []; + final Set _listeners = new LinkedHashSet(); /// Calls the listener every time the value of the animation changes. /// @@ -84,9 +87,24 @@ abstract class AnimationLocalListenersMixin extends _ListenerMixin { /// If listeners are added or removed during this function, the modifications /// will not change which listeners are called during this iteration. void notifyListeners() { - List localListeners = new List.from(_listeners); - for (VoidCallback listener in localListeners) - listener(); + final List localListeners = new List.from(_listeners); + for (VoidCallback listener in localListeners) { + try { + if (_listeners.contains(listener)) + listener(); + } catch (exception, stack) { + FlutterError.reportError(new FlutterErrorDetails( + exception: exception, + stack: stack, + library: 'animation library', + context: 'while notifying listeners for $runtimeType', + informationCollector: (StringBuffer information) { + information.writeln('The $runtimeType notifying listeners was:'); + information.write(' $this'); + } + )); + } + } } } @@ -94,7 +112,7 @@ abstract class AnimationLocalListenersMixin extends _ListenerMixin { /// and notifies all the registered listeners when notifyStatusListeners is /// called. abstract class AnimationLocalStatusListenersMixin extends _ListenerMixin { - final List _statusListeners = []; + final Set _statusListeners = new LinkedHashSet(); /// Calls listener every time the status of the animation changes. /// @@ -117,8 +135,23 @@ abstract class AnimationLocalStatusListenersMixin extends _ListenerMixin { /// If listeners are added or removed during this function, the modifications /// will not change which listeners are called during this iteration. void notifyStatusListeners(AnimationStatus status) { - List localListeners = new List.from(_statusListeners); - for (AnimationStatusListener listener in localListeners) - listener(status); + final List localListeners = new List.from(_statusListeners); + for (AnimationStatusListener listener in localListeners) { + try { + if (_statusListeners.contains(listener)) + listener(status); + } catch (exception, stack) { + FlutterError.reportError(new FlutterErrorDetails( + exception: exception, + stack: stack, + library: 'animation library', + context: 'while notifying status listeners for $runtimeType', + informationCollector: (StringBuffer information) { + information.writeln('The $runtimeType notifying status listeners was:'); + information.write(' $this'); + } + )); + } + } } } diff --git a/packages/flutter/lib/src/foundation/change_notifier.dart b/packages/flutter/lib/src/foundation/change_notifier.dart index f09dd2d058..08761f3c17 100644 --- a/packages/flutter/lib/src/foundation/change_notifier.dart +++ b/packages/flutter/lib/src/foundation/change_notifier.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:collection'; + import 'package:meta/meta.dart'; import 'assertions.dart'; @@ -37,11 +39,11 @@ abstract class Listenable { /// It is O(N) for adding and removing listeners and O(N²) for dispatching /// notifications (where N is the number of listeners). class ChangeNotifier extends Listenable { - List _listeners; + Set _listeners = new LinkedHashSet(); bool _debugAssertNotDisposed() { assert(() { - if (_listeners == const []) { + if (_listeners == null) { throw new FlutterError( 'A $runtimeType was used after being disposed.\n' 'Once you have called dispose() on a $runtimeType, it can no longer be used.' @@ -58,7 +60,6 @@ class ChangeNotifier extends Listenable { @override void addListener(VoidCallback listener) { assert(_debugAssertNotDisposed); - _listeners ??= []; _listeners.add(listener); } @@ -71,7 +72,7 @@ class ChangeNotifier extends Listenable { @override void removeListener(VoidCallback listener) { assert(_debugAssertNotDisposed); - _listeners?.remove(listener); + _listeners.remove(listener); } /// Discards any resources used by the object. After this is called, the @@ -83,7 +84,7 @@ class ChangeNotifier extends Listenable { @mustCallSuper void dispose() { assert(_debugAssertNotDisposed); - _listeners = const []; + _listeners = null; } /// Call all the registered listeners. @@ -101,7 +102,7 @@ class ChangeNotifier extends Listenable { void notifyListeners() { assert(_debugAssertNotDisposed); if (_listeners != null) { - List localListeners = new List.from(_listeners); + final List localListeners = new List.from(_listeners); for (VoidCallback listener in localListeners) { try { if (_listeners.contains(listener)) diff --git a/packages/flutter/lib/src/material/page.dart b/packages/flutter/lib/src/material/page.dart index 55c6506790..70f789430f 100644 --- a/packages/flutter/lib/src/material/page.dart +++ b/packages/flutter/lib/src/material/page.dart @@ -167,14 +167,6 @@ class _CupertinoBackGestureController extends NavigationGestureController { } void handleStatusChanged(AnimationStatus status) { - // This can happen if an earlier status listener ends up calling dispose() - // on this object. - // TODO(abarth): Consider changing AnimationController not to call listeners - // that were removed while calling other listeners. - // See . - if (controller == null) - return; - if (status == AnimationStatus.dismissed) { navigator.pop(); assert(controller == null); diff --git a/packages/flutter/test/animation/iteration_patterns_test.dart b/packages/flutter/test/animation/iteration_patterns_test.dart new file mode 100644 index 0000000000..50001994cb --- /dev/null +++ b/packages/flutter/test/animation/iteration_patterns_test.dart @@ -0,0 +1,123 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:flutter/animation.dart'; +import 'package:flutter/widgets.dart'; + +void main() { + setUp(() { + WidgetsFlutterBinding.ensureInitialized(); + WidgetsBinding.instance.resetEpoch(); + }); + + test('AnimationController with mutating listener', () { + final AnimationController controller = new AnimationController( + duration: const Duration(milliseconds: 100), + vsync: const TestVSync(), + ); + final List log = []; + + final VoidCallback listener1 = () { log.add('listener1'); }; + final VoidCallback listener3 = () { log.add('listener3'); }; + final VoidCallback listener4 = () { log.add('listener4'); }; + final VoidCallback listener2 = () { + log.add('listener2'); + controller.removeListener(listener1); + controller.removeListener(listener3); + controller.addListener(listener4); + }; + + controller.addListener(listener1); + controller.addListener(listener2); + controller.addListener(listener3); + controller.value = 0.2; + expect(log, ['listener1', 'listener2']); + log.clear(); + + controller.value = 0.3; + expect(log, ['listener2', 'listener4']); + log.clear(); + + controller.value = 0.4; + expect(log, ['listener2', 'listener4']); + log.clear(); + }); + + test('AnimationController with mutating status listener', () { + final AnimationController controller = new AnimationController( + duration: const Duration(milliseconds: 100), + vsync: const TestVSync(), + ); + final List log = []; + + final AnimationStatusListener listener1 = (AnimationStatus status) { log.add('listener1'); }; + final AnimationStatusListener listener3 = (AnimationStatus status) { log.add('listener3'); }; + final AnimationStatusListener listener4 = (AnimationStatus status) { log.add('listener4'); }; + final AnimationStatusListener listener2 = (AnimationStatus status) { + log.add('listener2'); + controller.removeStatusListener(listener1); + controller.removeStatusListener(listener3); + controller.addStatusListener(listener4); + }; + + controller.addStatusListener(listener1); + controller.addStatusListener(listener2); + controller.addStatusListener(listener3); + controller.forward(); + expect(log, ['listener1', 'listener2']); + log.clear(); + + controller.reverse(); + expect(log, ['listener2', 'listener4']); + log.clear(); + + controller.forward(); + expect(log, ['listener2', 'listener4']); + log.clear(); + + controller.dispose(); + }); + + testWidgets('AnimationController with throwing listener', (WidgetTester tester) async { + final AnimationController controller = new AnimationController( + duration: const Duration(milliseconds: 100), + vsync: const TestVSync(), + ); + final List log = []; + + final VoidCallback listener1 = () { log.add('listener1'); }; + final VoidCallback badListener = () { log.add('badListener'); throw null; }; + final VoidCallback listener2 = () { log.add('listener2'); }; + + controller.addListener(listener1); + controller.addListener(badListener); + controller.addListener(listener2); + controller.value = 0.2; + expect(log, ['listener1', 'badListener', 'listener2']); + expect(tester.takeException(), isNullThrownError); + log.clear(); + }); + + testWidgets('AnimationController with throwing status listener', (WidgetTester tester) async { + final AnimationController controller = new AnimationController( + duration: const Duration(milliseconds: 100), + vsync: const TestVSync(), + ); + final List log = []; + + final AnimationStatusListener listener1 = (AnimationStatus status) { log.add('listener1'); }; + final AnimationStatusListener badListener = (AnimationStatus status) { log.add('badListener'); throw null; }; + final AnimationStatusListener listener2 = (AnimationStatus status) { log.add('listener2'); }; + + controller.addStatusListener(listener1); + controller.addStatusListener(badListener); + controller.addStatusListener(listener2); + controller.forward(); + expect(log, ['listener1', 'badListener', 'listener2']); + expect(tester.takeException(), isNullThrownError); + log.clear(); + controller.dispose(); + }); +} diff --git a/packages/flutter/test/foundation/change_notifier_test.dart b/packages/flutter/test/foundation/change_notifier_test.dart index 24cb71bf47..d5ec519624 100644 --- a/packages/flutter/test/foundation/change_notifier_test.dart +++ b/packages/flutter/test/foundation/change_notifier_test.dart @@ -24,12 +24,12 @@ void main() { test.addListener(listener); test.addListener(listener); test.notify(); - expect(log, ['listener', 'listener']); + expect(log, ['listener']); log.clear(); test.removeListener(listener); test.notify(); - expect(log, ['listener']); + expect(log, []); log.clear(); test.removeListener(listener); @@ -79,7 +79,7 @@ void main() { test.removeListener(listener2); test.addListener(listener2); test.notify(); - expect(log, ['badListener', 'listener1', 'listener2']); + expect(log, ['badListener', 'listener2']); expect(tester.takeException(), isNullThrownError); log.clear(); }); @@ -110,7 +110,7 @@ void main() { log.clear(); test.notify(); - expect(log, ['listener2', 'listener4', 'listener4']); + expect(log, ['listener2', 'listener4']); log.clear(); });