Merge pull request #573 from Hixie/yak3-navigator-no-StateRoute

Remove StateRoutes

examples/stocks/lib/stock_home.dart

@@ -24,8 +24,8 @@ class StockHomeState extends State<StockHome> {
   String _searchQuery;
 
   void _handleSearchBegin() {
-    Navigator.of(context).push(new StateRoute(
-      onPop: () {
+    ModalRoute.of(context).addLocalHistoryEntry(new LocalHistoryEntry(
+      onRemove: () {
         setState(() {
           _isSearching = false;
           _searchQuery = null;

packages/flutter/lib/src/material/drawer.dart

@@ -86,7 +86,7 @@ class _DrawerRoute extends OverlayRoute {
     );
   }
 
-  void didPop(dynamic result) {
+  bool didPop(dynamic result) {
     // we don't call the superclass because we want to control the timing of the
     // call to finished().
     switch (_state) {
@@ -101,6 +101,7 @@ class _DrawerRoute extends OverlayRoute {
         finished();
         break;
     }
+    return true;
   }
 }
 

packages/flutter/lib/src/material/material_app.dart

@@ -84,9 +84,7 @@ class _MaterialAppState extends State<MaterialApp> {
     if (event.type == 'back') {
       NavigatorState navigator = _navigator.currentState;
       assert(navigator != null);
-      if (navigator.hasPreviousRoute)
-        navigator.pop();
-      else
+      if (!navigator.pop())
         activity.finishCurrentActivity();
     }
   }

packages/flutter/lib/src/material/scaffold.dart

@@ -175,8 +175,8 @@ class ScaffoldState extends State<Scaffold> {
     Performance performance = BottomSheet.createPerformance()
       ..forward();
     _PersistentBottomSheet bottomSheet;
-    Route route = new StateRoute(
-      onPop: () {
+    LocalHistoryEntry entry = new LocalHistoryEntry(
+      onRemove: () {
         assert(_currentBottomSheet._widget == bottomSheet);
         assert(bottomSheetKey.currentState != null);
         bottomSheetKey.currentState.close();
@@ -191,7 +191,7 @@ class ScaffoldState extends State<Scaffold> {
       performance: performance,
       onClosing: () {
         assert(_currentBottomSheet._widget == bottomSheet);
-        Navigator.of(context).remove(route);
+        entry.remove();
       },
       onDismissed: () {
         assert(_dismissedBottomSheets != null);
@@ -201,12 +201,12 @@ class ScaffoldState extends State<Scaffold> {
       },
       builder: builder
     );
-    Navigator.of(context).push(route);
+    ModalRoute.of(context).addLocalHistoryEntry(entry);
     setState(() {
       _currentBottomSheet = new ScaffoldFeatureController._(
         bottomSheet,
         completer,
-        () => Navigator.of(context).remove(route),
+        () => entry.remove(),
         setState
       );
     });

packages/flutter/lib/src/widgets/hero_controller.dart

@@ -27,9 +27,9 @@ class HeroController extends NavigatorObserver {
   void didPush(Route route, Route previousRoute) {
     assert(navigator != null);
     assert(route != null);
-    if (route is ModalRoute) { // as opposed to StateRoute, say
+    if (route is PageRoute) {
       assert(route.performance != null);
-      if (previousRoute is ModalRoute) // as opposed to the many other types of routes, or null
+      if (previousRoute is PageRoute) // could be null
         _from = previousRoute;
       _to = route;
       _performance = route.performance;
@@ -40,9 +40,9 @@ class HeroController extends NavigatorObserver {
   void didPop(Route route, Route previousRoute) {
     assert(navigator != null);
     assert(route != null);
-    if (route is ModalRoute) { // as opposed to StateRoute, say
+    if (route is PageRoute) {
       assert(route.performance != null);
-      if (previousRoute is ModalRoute) { // as opposed to the many other types of routes
+      if (previousRoute is PageRoute) {
         _to = previousRoute;
         _from = route;
         _performance = route.performance;

packages/flutter/lib/src/widgets/navigator.dart

@@ -12,7 +12,12 @@ abstract class Route<T> {
 
   List<OverlayEntry> get overlayEntries;
   void didPush(OverlayState overlay, OverlayEntry insertionPoint) { }
-  void didPop(T result) { }
+
+  /// A request was made to pop this route. If the route can handle it
+  /// internally (e.g. because it has its own stack of internal state) then
+  /// return false, otherwise return true. Returning false will prevent the
+  /// default behavior of NavigatorState.pop().
+  bool didPop(T result) => true;
 
   /// The given route has been pushed onto the navigator after this route.
   /// Return true if the route before this one should be notified also. The
@@ -88,7 +93,6 @@ class NavigatorState extends State<Navigator> {
     super.dispose();
   }
 
-  bool get hasPreviousRoute => _history.length > 1;
   OverlayState get overlay => _overlayKey.currentState;
 
   OverlayEntry get _currentOverlay {
@@ -128,58 +132,40 @@ class NavigatorState extends State<Navigator> {
     assert(() { _debugLocked = false; return true; });
   }
 
-  /// Pops the given route, if it's the current route. If it's not the current
-  /// route, removes it from the list of active routes without notifying any
-  /// observers or adjacent routes.
-  ///
-  /// Do not use this for ModalRoutes, or indeed anything other than
-  /// StateRoutes. Doing so would cause very odd results, e.g. ModalRoutes would
-  /// get confused about who is current.
-  ///
-  /// The type of the result argument, if provided, must match the type argument
-  /// of the class of the given route. (In practice, this is usually "dynamic".)
-  void remove(Route route, [dynamic result]) {
-    assert(!_debugLocked);
-    assert(_history.contains(route));
-    assert(route.overlayEntries.isEmpty);
-    if (_history.last == route) {
-      pop(result);
-    } else {
-      assert(() { _debugLocked = true; return true; });
-      assert(route._navigator == this);
-      setState(() {
-        _history.remove(route);
-        route.didPop(result);
-        route._navigator = null;
-      });
-      assert(() { _debugLocked = false; return true; });
-    }
-  }
-
   /// Removes the current route, notifying the observer (if any), and the
   /// previous routes (using [Route.didPopNext]).
   ///
   /// The type of the result argument, if provided, must match the type argument
   /// of the class of the current route. (In practice, this is usually
   /// "dynamic".)
-  void pop([dynamic result]) {
+  ///
+  /// Returns true if a route was popped; returns false if there are no further
+  /// previous routes.
+  bool pop([dynamic result]) {
     assert(!_debugLocked);
     assert(() { _debugLocked = true; return true; });
-    setState(() {
-      // We use setState to guarantee that we'll rebuild, since the routes can't
-      // do that for themselves, even if they have changed their own state (e.g.
-      // ModalScope.isCurrent).
-      assert(_history.length > 1);
-      Route route = _history.removeLast();
-      assert(route._navigator == this);
-      route.didPop(result);
-      int index = _history.length-1;
-      while (index >= 0 && _history[index].didPopNext(route))
-        index -= 1;
-      config.observer?.didPop(route, index >= 0 ? _history[index] : null);
-      route._navigator = null;
-    });
+    Route route = _history.last;
+    assert(route._navigator == this);
+    if (route.didPop(result)) {
+      if (_history.length > 1) {
+        setState(() {
+          // We use setState to guarantee that we'll rebuild, since the routes can't
+          // do that for themselves, even if they have changed their own state (e.g.
+          // ModalScope.isCurrent).
+          _history.removeLast();
+          int index = _history.length-1;
+          while (index >= 0 && _history[index].didPopNext(route))
+            index -= 1;
+          config.observer?.didPop(route, index >= 0 ? _history[index] : null);
+          route._navigator = null;
+        });
+      } else {
+        assert(() { _debugLocked = false; return true; });
+        return false;
+      }
+    }
     assert(() { _debugLocked = false; return true; });
+    return true;
   }
 
   Widget build(BuildContext context) {

packages/flutter/lib/src/widgets/routes.dart

@@ -17,23 +17,6 @@ import 'status_transitions.dart';
 
 const _kTransparent = const Color(0x00000000);
 
-class StateRoute extends Route {
-  StateRoute({ this.onPop });
-
-  final VoidCallback onPop;
-
-  List<OverlayEntry> get overlayEntries => const <OverlayEntry>[];
-
-  void didPop(dynamic result) {
-    assert(result == null);
-    if (onPop != null)
-      onPop();
-  }
-
-  bool willPushNext(Route nextRoute) => true;
-  bool didPopNext(Route nextRoute) => true;
-}
-
 abstract class OverlayRoute<T> extends Route<T> {
   List<WidgetBuilder> get builders => const <WidgetBuilder>[];
 
@@ -47,8 +30,9 @@ abstract class OverlayRoute<T> extends Route<T> {
   }
 
   // Subclasses shouldn't call this if they want to delay the finished() call.
-  void didPop(T result) {
+  bool didPop(T result) {
     finished();
+    return true;
   }
 
   /// Clears out the overlay entries.
@@ -121,10 +105,11 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
     super.didPush(overlay, insertionPoint);
   }
 
-  void didPop(T result) {
+  bool didPop(T result) {
     _result = result;
     _performance.reverse();
     _popCompleter?.complete(_result);
+    return true;
   }
 
   void finished() {
@@ -136,6 +121,55 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
   String toString() => '$runtimeType(performance: $_performance)';
 }
 
+class LocalHistoryEntry {
+  LocalHistoryEntry({ this.onRemove });
+  final VoidCallback onRemove;
+  LocalHistoryRoute _owner;
+  void remove() {
+    _owner.removeLocalHistoryEntry(this);
+  }
+  void _notifyRemoved() {
+    if (onRemove != null)
+      onRemove();
+  }
+}
+
+abstract class LocalHistoryRoute<T> extends TransitionRoute<T> {
+  LocalHistoryRoute({
+    Completer<T> popCompleter,
+    Completer<T> transitionCompleter
+  }) : super(
+    popCompleter: popCompleter,
+    transitionCompleter: transitionCompleter
+  );
+
+  List<LocalHistoryEntry> _localHistory;
+  void addLocalHistoryEntry(LocalHistoryEntry entry) {
+    assert(entry._owner == null);
+    entry._owner = this;
+    _localHistory ??= <LocalHistoryEntry>[];
+    _localHistory.add(entry);
+  }
+  void removeLocalHistoryEntry(LocalHistoryEntry entry) {
+    assert(entry != null);
+    assert(entry._owner == this);
+    assert(_localHistory.contains(entry));
+    _localHistory.remove(entry);
+    entry._owner = null;
+    entry._notifyRemoved();
+  }
+  bool didPop(T result) {
+    if (_localHistory != null && _localHistory.length > 0) {
+      LocalHistoryEntry entry = _localHistory.removeLast();
+      assert(entry._owner == this);
+      entry._owner = null;
+      entry._notifyRemoved();
+      return false;
+    }
+    return super.didPop(result);
+  }
+}
+
 class _ModalScopeStatus extends InheritedWidget {
   _ModalScopeStatus({
     Key key,
@@ -220,7 +254,7 @@ class ModalPosition {
   final double left;
 }
 
-abstract class ModalRoute<T> extends TransitionRoute<T> {
+abstract class ModalRoute<T> extends LocalHistoryRoute<T> {
   ModalRoute({
     Completer<T> completer,
     this.settings: const NamedRouteSettings()
@@ -286,25 +320,24 @@ abstract class ModalRoute<T> extends TransitionRoute<T> {
     super.didPush(overlay, insertionPoint);
   }
 
-  void didPop(T result) {
+  bool didPop(T result) {
     assert(_isCurrent);
-    _isCurrent = false;
-    super.didPop(result);
+    if (super.didPop(result)) {
+      _isCurrent = false;
+      return true;
+    }
+    return false;
   }
 
   bool willPushNext(Route nextRoute) {
-    if (nextRoute is ModalRoute) {
-      assert(_isCurrent);
-      _isCurrent = false;
-    }
+    assert(_isCurrent);
+    _isCurrent = false;
     return false;
   }
 
   bool didPopNext(Route nextRoute) {
-    if (nextRoute is ModalRoute) {
-      assert(!_isCurrent);
-      _isCurrent = true;
-    }
+    assert(!_isCurrent);
+    _isCurrent = true;
     return false;
   }
 

packages/unit/test/widget/heroes_test.dart

@@ -15,7 +15,6 @@ final Map<String, RouteBuilder> routes = <String, RouteBuilder>{
     new Container(height: 100.0, width: 100.0),
     new Card(child: new Hero(tag: 'a', child: new Container(height: 100.0, width: 100.0, key: firstKey))),
     new Container(height: 100.0, width: 100.0),
-    new FlatButton(child: new Text('state route please'), onPressed: () => Navigator.of(args.context).push(new StateRoute())),
     new FlatButton(child: new Text('button'), onPressed: () => Navigator.of(args.context).pushNamed('/two')),
   ]),
   '/two': (RouteArguments args) => new Block([
@@ -85,74 +84,4 @@ void main() {
 
     });
   });
-
-  test('Heroes animate even with intervening state routes', () {
-    testWidgets((WidgetTester tester) {
-
-      tester.pumpWidget(new Container()); // clear our memory
-
-      tester.pumpWidget(new MaterialApp(routes: routes));
-
-      // the initial setup.
-
-      expect(tester.findElementByKey(firstKey), isOnStage);
-      expect(tester.findElementByKey(firstKey), isInCard);
-      expect(tester.findElementByKey(secondKey), isNull);
-
-      // insert a state route
-
-      tester.tap(tester.findText('state route please'));
-      tester.pump();
-
-      expect(tester.findElementByKey(firstKey), isOnStage);
-      expect(tester.findElementByKey(firstKey), isInCard);
-      expect(tester.findElementByKey(secondKey), isNull);
-
-      tester.tap(tester.findText('button'));
-      tester.pump(); // begin navigation
-
-      // at this stage, the second route is off-stage, so that we can form the
-      // hero party.
-
-      expect(tester.findElementByKey(firstKey), isOnStage);
-      expect(tester.findElementByKey(firstKey), isInCard);
-      expect(tester.findElementByKey(secondKey), isOffStage);
-      expect(tester.findElementByKey(secondKey), isInCard);
-
-      tester.pump();
-
-      // at this stage, the heroes have just gone on their journey, we are
-      // seeing them at t=16ms. The original page no longer contains the hero.
-
-      expect(tester.findElementByKey(firstKey), isNull);
-      expect(tester.findElementByKey(secondKey), isOnStage);
-      expect(tester.findElementByKey(secondKey), isNotInCard);
-
-      tester.pump();
-
-      // t=32ms for the journey. Surely they are still at it.
-
-      expect(tester.findElementByKey(firstKey), isNull);
-      expect(tester.findElementByKey(secondKey), isOnStage);
-      expect(tester.findElementByKey(secondKey), isNotInCard);
-
-      tester.pump(new Duration(seconds: 1));
-
-      // t=1.032s for the journey. The journey has ended (it ends this frame, in
-      // fact). The hero should now be in the new page, on-stage.
-
-      expect(tester.findElementByKey(firstKey), isNull);
-      expect(tester.findElementByKey(secondKey), isOnStage);
-      expect(tester.findElementByKey(secondKey), isInCard);
-
-      tester.pump();
-
-      // Should not change anything.
-
-      expect(tester.findElementByKey(firstKey), isNull);
-      expect(tester.findElementByKey(secondKey), isOnStage);
-      expect(tester.findElementByKey(secondKey), isInCard);
-
-    });
-  });
 }