From ffe14b0d27de94edf4e6e541e53ca4802c36d8b9 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Fri, 14 Oct 2016 16:05:45 -0700 Subject: [PATCH] Navigator.push and Navigator.pushNamed should return Futures (#6333) These futures complete when the route is popped off the navigator. This generalizes and simplifies a mechanism already in place for dialogs and menus. Fixes #5283 --- .../lib/demo/shrine/shrine_home.dart | 16 ++--- .../lib/demo/shrine/shrine_order.dart | 5 +- .../flutter_gallery/lib/demo/shrine_demo.dart | 5 +- .../lib/src/material/bottom_sheet.dart | 8 +-- packages/flutter/lib/src/material/dialog.dart | 8 +-- .../flutter/lib/src/material/drop_down.dart | 4 +- packages/flutter/lib/src/material/page.dart | 5 +- .../flutter/lib/src/material/popup_menu.dart | 8 +-- .../flutter/lib/src/widgets/navigator.dart | 72 +++++++++++++++---- packages/flutter/lib/src/widgets/pages.dart | 5 +- packages/flutter/lib/src/widgets/routes.dart | 54 +++++++------- packages/flutter/test/widget/routes_test.dart | 6 ++ 12 files changed, 104 insertions(+), 92 deletions(-) diff --git a/examples/flutter_gallery/lib/demo/shrine/shrine_home.dart b/examples/flutter_gallery/lib/demo/shrine/shrine_home.dart index 8733d33918b..6b77e689e46 100644 --- a/examples/flutter_gallery/lib/demo/shrine/shrine_home.dart +++ b/examples/flutter_gallery/lib/demo/shrine/shrine_home.dart @@ -296,18 +296,10 @@ class _ShrineHomeState extends State { static final GlobalKey scrollableKey = new GlobalKey(); static final GridDelegate gridDelegate = new ShrineGridDelegate(); - void handleCompletedOrder(Order completedOrder) { - assert(completedOrder.product != null); - if (completedOrder.quantity == 0) - _shoppingCart.remove(completedOrder.product); - } - - void showOrderPage(Product product) { + Future showOrderPage(Product product) async { final Order order = _shoppingCart[product] ?? new Order(product: product); - final Completer completer = new Completer(); - Navigator.push(context, new ShrineOrderRoute( + final Order completedOrder = await Navigator.push(context, new ShrineOrderRoute( order: order, - completer: completer, builder: (BuildContext context) { return new OrderPage( order: order, @@ -316,7 +308,9 @@ class _ShrineHomeState extends State { ); } )); - completer.future.then(handleCompletedOrder); + assert(completedOrder.product != null); + if (completedOrder.quantity == 0) + _shoppingCart.remove(completedOrder.product); } @override diff --git a/examples/flutter_gallery/lib/demo/shrine/shrine_order.dart b/examples/flutter_gallery/lib/demo/shrine/shrine_order.dart index 08da4736ce4..37c125e4322 100644 --- a/examples/flutter_gallery/lib/demo/shrine/shrine_order.dart +++ b/examples/flutter_gallery/lib/demo/shrine/shrine_order.dart @@ -2,8 +2,6 @@ // 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:flutter/material.dart'; import '../shrine_demo.dart' show ShrinePageRoute; @@ -226,9 +224,8 @@ class ShrineOrderRoute extends ShrinePageRoute { ShrineOrderRoute({ this.order, WidgetBuilder builder, - Completer completer, RouteSettings settings: const RouteSettings() - }) : super(builder: builder, completer: completer, settings: settings) { + }) : super(builder: builder, settings: settings) { assert(order != null); } diff --git a/examples/flutter_gallery/lib/demo/shrine_demo.dart b/examples/flutter_gallery/lib/demo/shrine_demo.dart index 22fea5850b2..0345536f450 100644 --- a/examples/flutter_gallery/lib/demo/shrine_demo.dart +++ b/examples/flutter_gallery/lib/demo/shrine_demo.dart @@ -2,8 +2,6 @@ // 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:flutter/material.dart'; import 'shrine/shrine_home.dart' show ShrineHome; @@ -26,9 +24,8 @@ Widget buildShrine(Widget child) { class ShrinePageRoute extends MaterialPageRoute { ShrinePageRoute({ WidgetBuilder builder, - Completer completer, RouteSettings settings: const RouteSettings() - }) : super(builder: builder, completer: completer, settings: settings); + }) : super(builder: builder, settings: settings); @override Widget buildPage(BuildContext context, Animation animation, Animation forwardAnimation) { diff --git a/packages/flutter/lib/src/material/bottom_sheet.dart b/packages/flutter/lib/src/material/bottom_sheet.dart index 8ab653bc9b1..df60efdbf16 100644 --- a/packages/flutter/lib/src/material/bottom_sheet.dart +++ b/packages/flutter/lib/src/material/bottom_sheet.dart @@ -204,10 +204,9 @@ class _ModalBottomSheetState extends State<_ModalBottomSheet> { class _ModalBottomSheetRoute extends PopupRoute { _ModalBottomSheetRoute({ - Completer completer, this.builder, this.theme, - }) : super(completer: completer); + }); final WidgetBuilder builder; final ThemeData theme; @@ -260,11 +259,8 @@ class _ModalBottomSheetRoute extends PopupRoute { Future showModalBottomSheet/**/({ BuildContext context, WidgetBuilder builder }) { assert(context != null); assert(builder != null); - final Completer completer = new Completer(); - Navigator.push(context, new _ModalBottomSheetRoute( - completer: completer, + return Navigator.push(context, new _ModalBottomSheetRoute( builder: builder, theme: Theme.of(context, shadowThemeOnly: true), )); - return completer.future; } diff --git a/packages/flutter/lib/src/material/dialog.dart b/packages/flutter/lib/src/material/dialog.dart index 0a890ce997e..b6aa3260ba6 100644 --- a/packages/flutter/lib/src/material/dialog.dart +++ b/packages/flutter/lib/src/material/dialog.dart @@ -278,10 +278,9 @@ class SimpleDialog extends StatelessWidget { class _DialogRoute extends PopupRoute { _DialogRoute({ - Completer completer, this.child, this.theme, - }) : super(completer: completer); + }); final Widget child; final ThemeData theme; @@ -324,11 +323,8 @@ class _DialogRoute extends PopupRoute { /// * [Dialog] /// * Future showDialog/**/({ BuildContext context, Widget child }) { - Completer completer = new Completer(); - Navigator.push(context, new _DialogRoute( - completer: completer, + return Navigator.push(context, new _DialogRoute( child: child, theme: Theme.of(context, shadowThemeOnly: true), )); - return completer.future; } diff --git a/packages/flutter/lib/src/material/drop_down.dart b/packages/flutter/lib/src/material/drop_down.dart index e1873df96fa..e1c8120d00a 100644 --- a/packages/flutter/lib/src/material/drop_down.dart +++ b/packages/flutter/lib/src/material/drop_down.dart @@ -288,14 +288,13 @@ class _DropdownRouteResult { class _DropdownRoute extends PopupRoute<_DropdownRouteResult> { _DropdownRoute({ - Completer<_DropdownRouteResult> completer, this.items, this.buttonRect, this.selectedIndex, this.elevation: 8, this.theme, TextStyle style, - }) : _style = style, super(completer: completer) { + }) : _style = style { assert(style != null); } @@ -502,7 +501,6 @@ class _DropdownButtonState extends State> { final Rect itemRect = itemBox.localToGlobal(Point.origin) & itemBox.size; final Completer<_DropdownRouteResult> completer = new Completer<_DropdownRouteResult>(); _currentRoute = new _DropdownRoute( - completer: completer, items: config.items, buttonRect: _kMenuHorizontalPadding.inflateRect(itemRect), selectedIndex: _selectedIndex, diff --git a/packages/flutter/lib/src/material/page.dart b/packages/flutter/lib/src/material/page.dart index 7048c47dc4d..0ec86ec40fc 100644 --- a/packages/flutter/lib/src/material/page.dart +++ b/packages/flutter/lib/src/material/page.dart @@ -2,8 +2,6 @@ // 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:flutter/widgets.dart'; import 'material.dart'; import 'theme.dart'; @@ -167,10 +165,9 @@ class MaterialPageRoute extends PageRoute { /// Creates a page route for use in a material design app. MaterialPageRoute({ this.builder, - Completer completer, RouteSettings settings: const RouteSettings(), this.maintainState: true, - }) : super(completer: completer, settings: settings) { + }) : super(settings: settings) { assert(builder != null); assert(opaque); } diff --git a/packages/flutter/lib/src/material/popup_menu.dart b/packages/flutter/lib/src/material/popup_menu.dart index 7b2dac193cf..48962fa6d00 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -370,13 +370,12 @@ class _PopupMenuRouteLayout extends SingleChildLayoutDelegate { class _PopupMenuRoute extends PopupRoute { _PopupMenuRoute({ - Completer completer, this.position, this.items, this.initialValue, this.elevation, this.theme - }) : super(completer: completer); + }); final RelativeRect position; final List> items; @@ -439,16 +438,13 @@ Future showMenu/**/({ }) { assert(context != null); assert(items != null && items.length > 0); - Completer completer = new Completer(); - Navigator.push(context, new _PopupMenuRoute( - completer: completer, + return Navigator.push(context, new _PopupMenuRoute( position: position, items: items, initialValue: initialValue, elevation: elevation, theme: Theme.of(context, shadowThemeOnly: true), )); - return completer.future; } /// A callback that is passed the value of the PopupMenuItem that caused diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index 9ea1f274403..fa0dcd514cc 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.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:flutter/rendering.dart'; import 'package:meta/meta.dart'; @@ -32,6 +34,12 @@ abstract class Route { /// will initialize its [Focus] to this key. GlobalKey get focusKey => null; + /// A future that completes when this route is popped off the navigator. + /// + /// The future completes with the value given to [Navigator.pop], if any. + Future get popped => _popCompleter.future; + final Completer _popCompleter = new Completer(); + /// Called when the route is inserted into the navigator. /// /// Use this to populate overlayEntries and add them to the overlay @@ -40,9 +48,13 @@ abstract class Route { /// responsible for _removing_ the entries and this way it's symmetric.) /// /// The overlay argument will be null if this is the first route inserted. + @protected + @mustCallSuper void install(OverlayEntry insertionPoint) { } /// Called after install() when the route is pushed onto the navigator. + @protected + @mustCallSuper void didPush() { } /// When this route is popped (see [Navigator.pop]) if the result isn't @@ -50,6 +62,8 @@ abstract class Route { T get currentResult => null; /// Called after install() when the route replaced another in the navigator. + @protected + @mustCallSuper void didReplace(Route oldRoute) { } /// A request was made to pop this route. If the route can handle it @@ -59,19 +73,28 @@ abstract class Route { /// /// If this is called, the Navigator will not call dispose(). It is the /// responsibility of the Route to later call dispose(). - bool didPop(T result) => true; + @protected + @mustCallSuper + bool didPop(T result) { + _popCompleter.complete(result); + return true; + } /// Whether calling didPop() would return false. bool get willHandlePopInternally => false; /// The given route, which came after this one, has been popped off the /// navigator. + @protected + @mustCallSuper void didPopNext(Route nextRoute) { } /// This route's next route has changed to the given new route. This is called /// on a route whenever the next route changes for any reason, except for /// cases when didPopNext() would be called, so long as it is in the history. /// nextRoute will be null if there's no next route. + @protected + @mustCallSuper void didChangeNext(Route nextRoute) { } /// The route should remove its overlays and free any other resources. @@ -79,6 +102,7 @@ abstract class Route { /// A call to didPop() implies that the Route should call dispose() itself, /// but it is possible for dispose() to be called directly (e.g. if the route /// is replaced, or if the navigator itself is disposed). + @mustCallSuper void dispose() { } /// If the route's transition can be popped via a user gesture (e.g. the iOS @@ -248,8 +272,10 @@ class Navigator extends StatefulWidget { /// /// The route name will be passed to that navigator's [onGenerateRoute] /// callback. The returned route will be pushed into the navigator. - static void pushNamed(BuildContext context, String routeName) { - Navigator.of(context).pushNamed(routeName); + /// + /// Returns a Future that completes when the pushed route is popped. + static Future pushNamed(BuildContext context, String routeName) { + return Navigator.of(context).pushNamed(routeName); } /// Push a route onto the navigator that most tightly encloses the given context. @@ -258,8 +284,10 @@ class Navigator extends StatefulWidget { /// The route will have didPush() and didChangeNext() called on it; the /// previous route, if any, will have didChangeNext() called on it; and the /// Navigator observer, if any, will have didPush() called on it. - static void push(BuildContext context, Route route) { - Navigator.of(context).push(route); + /// + /// Returns a Future that completes when the pushed route is popped. + static Future push(BuildContext context, Route route) { + return Navigator.of(context).push(route); } /// Pop a route off the navigator that most tightly encloses the given context. @@ -269,10 +297,10 @@ class Navigator extends StatefulWidget { /// (if any) is notified using its didPop() method, and the previous route is /// notified using [Route.didChangeNext]. /// - /// If non-null, [result] will be used as the result of the route. Routes + /// If non-null, `result` will be used as the result of the route. Routes /// such as dialogs or popup menus typically use this mechanism to return the /// value selected by the user to the widget that created their route. The - /// type of [result], if provided, must match the type argument of the class + /// type of `result`, if provided, must match the type argument of the class /// of the current route. (In practice, this is usually "dynamic".) /// /// Returns true if a route was popped; returns false if there are no further @@ -299,11 +327,20 @@ class Navigator extends StatefulWidget { } /// Executes a simple transaction that both pops the current route off and - /// pushes a named route into the navigator that most tightly encloses the given context. - static void popAndPushNamed(BuildContext context, String routeName) { - Navigator.of(context) - ..pop() - ..pushNamed(routeName); + /// pushes a named route into the navigator that most tightly encloses the + /// given context. + /// + /// If non-null, `result` will be used as the result of the route that is + /// popped. Routes such as dialogs or popup menus typically use this mechanism + /// to return the value selected by the user to the widget that created their + /// route. The type of `result`, if provided, must match the type argument of + /// the class of the current route. (In practice, this is usually "dynamic".) + /// + /// Returns a Future that completes when the pushed route is popped. + static Future popAndPushNamed(BuildContext context, String routeName, { dynamic result }) { + NavigatorState navigator = Navigator.of(context); + navigator.pop(result); + return navigator.pushNamed(routeName); } /// The state from the closest instance of this class that encloses the given context. @@ -378,7 +415,9 @@ class NavigatorState extends State with TickerProviderStateMixin { /// Looks up the route with the given name using [Navigator.onGenerateRoute], /// and then [push]es that route. - void pushNamed(String name) { + /// + /// Returns a Future that completes when the pushed route is popped. + Future pushNamed(String name) { assert(!_debugLocked); assert(name != null); RouteSettings settings = new RouteSettings(name: name); @@ -388,7 +427,7 @@ class NavigatorState extends State with TickerProviderStateMixin { route = config.onUnknownRoute(settings); assert(route != null); } - push(route); + return push(route); } /// Adds the given route to the navigator's history, and transitions to it. @@ -400,7 +439,9 @@ class NavigatorState extends State with TickerProviderStateMixin { /// /// Ongoing gestures within the current route are canceled when a new route is /// pushed. - void push(Route route) { + /// + /// Returns a Future that completes when the pushed route is popped. + Future push(Route route) { assert(!_debugLocked); assert(() { _debugLocked = true; return true; }); assert(route != null); @@ -418,6 +459,7 @@ class NavigatorState extends State with TickerProviderStateMixin { }); assert(() { _debugLocked = false; return true; }); _cancelActivePointers(); + return route.popped; } /// Replaces a route that is not currently visible with a new route. diff --git a/packages/flutter/lib/src/widgets/pages.dart b/packages/flutter/lib/src/widgets/pages.dart index a810da0f686..51a726db1ed 100644 --- a/packages/flutter/lib/src/widgets/pages.dart +++ b/packages/flutter/lib/src/widgets/pages.dart @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:async'; - import 'basic.dart'; import 'navigator.dart'; import 'overlay.dart'; @@ -13,9 +11,8 @@ import 'routes.dart'; abstract class PageRoute extends ModalRoute { /// Creates a modal route that replaces the entire screen. PageRoute({ - Completer completer, RouteSettings settings: const RouteSettings() - }) : super(completer: completer, settings: settings); + }) : super(settings: settings); @override bool get opaque => true; diff --git a/packages/flutter/lib/src/widgets/routes.dart b/packages/flutter/lib/src/widgets/routes.dart index d78abd47043..5240e07b06d 100644 --- a/packages/flutter/lib/src/widgets/routes.dart +++ b/packages/flutter/lib/src/widgets/routes.dart @@ -32,21 +32,23 @@ abstract class OverlayRoute extends Route { assert(_overlayEntries.isEmpty); _overlayEntries.addAll(createOverlayEntries()); navigator.overlay?.insertAll(_overlayEntries, above: insertionPoint); + super.install(insertionPoint); } - /// 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(). + /// Controls whether [didPop] calls [finished]. /// - /// If this is called, the Navigator will not call dispose(). It is the - /// responsibility of the Route to later call dispose(). - /// - /// Subclasses shouldn't call this if they want to delay the finished() call. + /// If true, this route removes its overlay entries during [didPop]. + /// Subclasses can override this getter if they want to delay the [finished] + /// call (for example to animate the route's exit before removing it from the + /// overlay). + @protected + bool get finishedWhenPopped => true; + @override bool didPop(T result) { - finished(); - return true; + if (finishedWhenPopped) + finished(); + return super.didPop(result); } /// Clears out the overlay entries. @@ -56,6 +58,7 @@ abstract class OverlayRoute extends Route { /// overlay removal. /// /// Do not call this method outside of this context. + @protected void finished() { for (OverlayEntry entry in _overlayEntries) entry.remove(); @@ -65,6 +68,7 @@ abstract class OverlayRoute extends Route { @override void dispose() { finished(); + super.dispose(); } } @@ -72,26 +76,20 @@ abstract class OverlayRoute extends Route { abstract class TransitionRoute extends OverlayRoute { /// Creates a route with entrance and exit transitions. TransitionRoute({ - Completer popCompleter, Completer transitionCompleter - }) : _popCompleter = popCompleter, - _transitionCompleter = transitionCompleter; + }) : _transitionCompleter = transitionCompleter; /// The same as the default constructor but callable with mixins. TransitionRoute.explicit( - Completer popCompleter, Completer transitionCompleter - ) : this(popCompleter: popCompleter, transitionCompleter: transitionCompleter); - - /// This future completes once the animation has been dismissed. For - /// ModalRoutes, this will be after the completer that's passed in, since that - /// one completes before the animation even starts, as soon as the route is - /// popped. - Future get popped => _popCompleter?.future; - final Completer _popCompleter; + ) : this(transitionCompleter: transitionCompleter); /// This future completes only once the transition itself has finished, after /// the overlay entries have been removed from the navigator's overlay. + /// + /// This future completes once the animation has been dismissed. That will be + /// after [popped], because [popped] completes before the animation even + /// starts, as soon as the route is popped. Future get completed => _transitionCompleter?.future; final Completer _transitionCompleter; @@ -104,6 +102,9 @@ abstract class TransitionRoute extends OverlayRoute { /// the opaque route will not be built to save resources. bool get opaque; + @override + bool get finishedWhenPopped => false; + /// The animation that drives the route's transition and the previous route's /// forward transition. Animation get animation => _animation; @@ -192,8 +193,7 @@ abstract class TransitionRoute extends OverlayRoute { bool didPop(T result) { _result = result; _controller.reverse(); - _popCompleter?.complete(_result); - return true; + return super.didPop(result); } @override @@ -455,9 +455,8 @@ class _ModalScopeState extends State<_ModalScope> { abstract class ModalRoute extends TransitionRoute with LocalHistoryRoute { /// Creates a route that blocks interaction with previous routes. ModalRoute({ - Completer completer, this.settings: const RouteSettings() - }) : super.explicit(completer, null); + }) : super.explicit(null); // The API for general users of this class @@ -678,9 +677,6 @@ abstract class ModalRoute extends TransitionRoute with LocalHistoryRoute extends ModalRoute { - /// Creates a modal route that overlays a widget over the current route. - PopupRoute({ Completer completer }) : super(completer: completer); - @override bool get opaque => false; diff --git a/packages/flutter/test/widget/routes_test.dart b/packages/flutter/test/widget/routes_test.dart index b057e9d3efd..32f7d34e059 100644 --- a/packages/flutter/test/widget/routes_test.dart +++ b/packages/flutter/test/widget/routes_test.dart @@ -35,16 +35,19 @@ class TestRoute extends LocalHistoryRoute { _entries.add(entry); navigator.overlay?.insert(entry, above: insertionPoint); routes.add(this); + super.install(insertionPoint); } @override void didPush() { log('didPush'); + super.didPush(); } @override void didReplace(@checked TestRoute oldRoute) { log('didReplace ${oldRoute.name}'); + super.didReplace(oldRoute); } @override @@ -59,11 +62,13 @@ class TestRoute extends LocalHistoryRoute { @override void didPopNext(@checked TestRoute nextRoute) { log('didPopNext ${nextRoute.name}'); + super.didPopNext(nextRoute); } @override void didChangeNext(@checked TestRoute nextRoute) { log('didChangeNext ${nextRoute?.name}'); + super.didChangeNext(nextRoute); } @override @@ -72,6 +77,7 @@ class TestRoute extends LocalHistoryRoute { _entries.forEach((OverlayEntry entry) { entry.remove(); }); _entries.clear(); routes.remove(this); + super.dispose(); } }