From dd2251ecf509689a48b5dbc7627e3bac12b50019 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Mon, 29 Aug 2016 10:35:18 -0700 Subject: [PATCH] Fix some hero observer bugs (#5633) 1: If a route is already dismissed when it's popped, there's no point trying to animate heroes, because it's going to be gone before the heroes code can look at it. 2: If a hero animation finishes just as a new one is starting, we previously blew away the state for the starting one. Now we correctly segregate the "starting up quest" variables from the "actively ongoing quest" variables. --- .../flutter/lib/src/scheduler/binding.dart | 6 ++ packages/flutter/lib/src/scheduler/debug.dart | 6 ++ packages/flutter/lib/src/widgets/heroes.dart | 30 ++++++++-- packages/flutter/test/widget/heroes_test.dart | 60 +++++++++++++++++++ 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/packages/flutter/lib/src/scheduler/binding.dart b/packages/flutter/lib/src/scheduler/binding.dart index 19d2b39c4d..76156d16d0 100644 --- a/packages/flutter/lib/src/scheduler/binding.dart +++ b/packages/flutter/lib/src/scheduler/binding.dart @@ -504,6 +504,12 @@ abstract class SchedulerBinding extends BindingBase { Timeline.finishSync(); + assert(() { + if (debugPrintEndFrameBanner) + debugPrint('━━━━━━━┫ End of Frame ┣━━━━━━━'); + return true; + }); + // All frame-related callbacks have been executed. Run lower-priority tasks. _runTasks(); } diff --git a/packages/flutter/lib/src/scheduler/debug.dart b/packages/flutter/lib/src/scheduler/debug.dart index 7bfa357fac..405551e72b 100644 --- a/packages/flutter/lib/src/scheduler/debug.dart +++ b/packages/flutter/lib/src/scheduler/debug.dart @@ -4,3 +4,9 @@ /// Print a banner at the beginning of each frame. bool debugPrintBeginFrameBanner = false; + +/// Print a banner at the end of each frame. +/// +/// Combined with [debugPrintBeginFrameBanner], this can be helpful for +/// determining if code is running during a frame or between frames. +bool debugPrintEndFrameBanner = false; diff --git a/packages/flutter/lib/src/widgets/heroes.dart b/packages/flutter/lib/src/widgets/heroes.dart index b5ca5b3414..90c6c65ff8 100644 --- a/packages/flutter/lib/src/widgets/heroes.dart +++ b/packages/flutter/lib/src/widgets/heroes.dart @@ -5,6 +5,7 @@ import 'dart:collection'; import 'package:meta/meta.dart'; +import 'package:flutter/foundation.dart'; import 'basic.dart'; import 'binding.dart'; @@ -424,10 +425,15 @@ class HeroController extends NavigatorObserver { ); } + // The current party, if they're on a quest. _HeroParty _party; - Animation _animation; + + // The settings used to prepare the next quest. + // These members are only non-null between the didPush/didPop call and the + // corresponding _updateQuest call. PageRoute _from; PageRoute _to; + Animation _animation; final List _overlayEntries = new List(); @@ -451,9 +457,9 @@ class HeroController extends NavigatorObserver { assert(route != null); if (route is PageRoute) { assert(route.animation != null); - if (previousRoute is PageRoute) { - _to = previousRoute; + if (route.animation.status != AnimationStatus.dismissed && previousRoute is PageRoute) { _from = route; + _to = previousRoute; _animation = route.animation; _checkForHeroQuest(); } @@ -475,16 +481,17 @@ class HeroController extends NavigatorObserver { void _checkForHeroQuest() { if (_from != null && _to != null && _from != _to && _questsEnabled) { + assert(_animation != null); _to.offstage = _to.animation.status != AnimationStatus.completed; WidgetsBinding.instance.addPostFrameCallback(_updateQuest); + } else { + // this isn't a valid quest + _clearPendingHeroQuest(); } } void _handleQuestFinished() { _removeHeroesFromOverlay(); - _from = null; - _to = null; - _animation = null; } Rect _getAnimationArea(BuildContext context) { @@ -514,8 +521,11 @@ class HeroController extends NavigatorObserver { void _updateQuest(Duration timeStamp) { if (navigator == null) { // The navigator was removed before this end-of-frame callback was called. + _clearPendingHeroQuest(); return; } + assert(_from.subtreeContext != null); + assert(_to.subtreeContext != null); Map heroesFrom = _party.isEmpty ? Hero._of(_from.subtreeContext) : _party.getHeroesToAnimate(); @@ -541,5 +551,13 @@ class HeroController extends NavigatorObserver { navigator.overlay ); } + + _clearPendingHeroQuest(); + } + + void _clearPendingHeroQuest() { + _from = null; + _to = null; + _animation = null; } } diff --git a/packages/flutter/test/widget/heroes_test.dart b/packages/flutter/test/widget/heroes_test.dart index f70cc29850..7cea2e0bc8 100644 --- a/packages/flutter/test/widget/heroes_test.dart +++ b/packages/flutter/test/widget/heroes_test.dart @@ -286,4 +286,64 @@ void main() { await tester.tap(find.text('bar')); expect(log, equals(['bar'])); }); + + testWidgets('Popping on first frame does not cause hero observer to crash', (WidgetTester tester) async { + await tester.pumpWidget(new MaterialApp( + onGenerateRoute: (RouteSettings settings) { + return new MaterialPageRoute( + settings: settings, + builder: (BuildContext context) => new Hero(tag: 'test', child: new Container()), + ); + }, + )); + await tester.pump(); + + Finder heroes = find.byType(Hero); + expect(heroes, findsOneWidget); + + Navigator.pushNamed(heroes.evaluate().first, 'test'); + await tester.pump(); // adds the new page to the tree... + + Navigator.pop(heroes.evaluate().first); + await tester.pump(); // ...and removes it straight away (since it's already at 0.0) + + // this is verifying that there's no crash + + // TODO(ianh): once https://github.com/flutter/flutter/issues/5631 is fixed, remove this line: + await tester.pump(const Duration(hours: 1)); + }); + + testWidgets('Overlapping starting and ending a hero transition works ok', (WidgetTester tester) async { + await tester.pumpWidget(new MaterialApp( + onGenerateRoute: (RouteSettings settings) { + return new MaterialPageRoute( + settings: settings, + builder: (BuildContext context) => new Hero(tag: 'test', child: new Container()), + ); + }, + )); + await tester.pump(); + + Finder heroes = find.byType(Hero); + expect(heroes, findsOneWidget); + + Navigator.pushNamed(heroes.evaluate().first, 'test'); + await tester.pump(); + await tester.pump(const Duration(hours: 1)); + + Navigator.pushNamed(heroes.evaluate().first, 'test'); + await tester.pump(); + await tester.pump(const Duration(hours: 1)); + + Navigator.pop(heroes.evaluate().first); + await tester.pump(); + Navigator.pop(heroes.evaluate().first); + await tester.pump(const Duration(hours: 1)); // so the first transition is finished, but the second hasn't started + await tester.pump(); + + // this is verifying that there's no crash + + // TODO(ianh): once https://github.com/flutter/flutter/issues/5631 is fixed, remove this line: + await tester.pump(const Duration(hours: 1)); + }); }