From 585e20a15f561a13fbfe88920d42e7e17d0744c7 Mon Sep 17 00:00:00 2001 From: Enguerrand ARMINJON <37028599+EArminjon@users.noreply.github.com> Date: Wed, 12 Feb 2025 23:36:49 +0100 Subject: [PATCH] feat: removeRoute now call didComplete (#157725) **(edited 4 december) PR EDITED TO FIT LATEST COMMENTS** Developper may want to remove a route in the background while an other one is displayed. To do so, developer can use removeRoute(). RemoveRoute() didn't resolve futures created by Navigator.push and so code which await for Navigator.push will never end. By calling complete() inside removeRoute, futures are well resolved. In addition that also allow to pass some result through removeRoute. Ex: ```dart Navigator.of(context).removeRoute(route, "test"); Navigator.of(context).removeRoute(route, object); ``` This PR aim to fix this issue : https://github.com/flutter/flutter/issues/157505 ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../lib/fix_data/fix_widgets/fix_widgets.yaml | 67 +-- .../flutter/lib/src/widgets/navigator.dart | 135 +++--- .../flutter/test/widgets/navigator_test.dart | 435 +++++++++++++++++- .../flutter/test_fixes/widgets/widgets.dart | 31 ++ .../test_fixes/widgets/widgets.dart.expect | 31 ++ 5 files changed, 585 insertions(+), 114 deletions(-) diff --git a/packages/flutter/lib/fix_data/fix_widgets/fix_widgets.yaml b/packages/flutter/lib/fix_data/fix_widgets/fix_widgets.yaml index 977148f5c5..203efaf28f 100644 --- a/packages/flutter/lib/fix_data/fix_widgets/fix_widgets.yaml +++ b/packages/flutter/lib/fix_data/fix_widgets/fix_widgets.yaml @@ -104,7 +104,7 @@ transforms: - title: "Remove 'vsync'" date: 2023-01-30 element: - uris: ['widgets.dart', 'material.dart', 'cupertino.dart'] + uris: [ 'widgets.dart', 'material.dart', 'cupertino.dart' ] constructor: '' inClass: 'AnimatedSize' changes: @@ -115,7 +115,7 @@ transforms: - title: "Migrate to 'boldTextOf'" date: 2022-10-28 element: - uris: ['widgets.dart', 'material.dart', 'cupertino.dart'] + uris: [ 'widgets.dart', 'material.dart', 'cupertino.dart' ] method: 'boldTextOverride' inClass: 'MediaQuery' changes: @@ -855,8 +855,8 @@ transforms: field: 'clipToSize' inClass: 'ListWheelViewport' changes: - - kind: 'rename' - newName: 'clipBehavior' + - kind: 'rename' + newName: 'clipBehavior' # Changes made in https://docs.flutter.dev/release/breaking-changes/clip-behavior - title: "Migrate to 'clipBehavior'" @@ -866,28 +866,28 @@ transforms: constructor: '' inClass: 'ListWheelViewport' oneOf: - - if: "clipToSize == 'true'" - changes: - - kind: 'addParameter' - index: 13 - name: 'clipBehavior' - style: optional_named - argumentValue: - expression: 'Clip.hardEdge' - requiredIf: "clipToSize == 'true'" - - kind: 'removeParameter' - name: 'clipToSize' - - if: "clipToSize == 'false'" - changes: - - kind: 'addParameter' - index: 13 - name: 'clipBehavior' - style: optional_named - argumentValue: - expression: 'Clip.none' - requiredIf: "clipToSize == 'false'" - - kind: 'removeParameter' - name: 'clipToSize' + - if: "clipToSize == 'true'" + changes: + - kind: 'addParameter' + index: 13 + name: 'clipBehavior' + style: optional_named + argumentValue: + expression: 'Clip.hardEdge' + requiredIf: "clipToSize == 'true'" + - kind: 'removeParameter' + name: 'clipToSize' + - if: "clipToSize == 'false'" + changes: + - kind: 'addParameter' + index: 13 + name: 'clipBehavior' + style: optional_named + argumentValue: + expression: 'Clip.none' + requiredIf: "clipToSize == 'false'" + - kind: 'removeParameter' + name: 'clipToSize' variables: clipToSize: kind: 'fragment' @@ -938,7 +938,18 @@ transforms: method: 'buildViewportChrome' inClass: 'ScrollBehavior' changes: - - kind: 'rename' - newName: 'buildOverscrollIndicator' + - kind: 'rename' + newName: 'buildOverscrollIndicator' + + # Changes made in https://github.com/flutter/flutter/pull/157725 + - title: "Migrate to 'markForComplete'" + date: 2025-02-10 + element: + uris: [ 'widgets.dart' ] + method: 'markForRemove' + inClass: 'RouteTransitionRecord' + changes: + - kind: 'rename' + newName: 'markForComplete' # Before adding a new fix: read instructions at the top of this file. diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index 4281901b2b..e6f63dea5e 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -952,7 +952,12 @@ abstract class RouteTransitionRecord { /// During [TransitionDelegate.resolve], this can be called on an exiting /// route to indicate that the route should be removed from the [Navigator] /// without completing and without an animated transition. - void markForRemove(); + @Deprecated( + 'Call markForComplete instead. ' + 'This will let route associated future to complete when route is removed. ' + 'This feature was deprecated after v3.27.0-1.0.pre.', + ) + void markForRemove() => markForComplete(); } /// The delegate that decides how pages added and removed from [Navigator.pages] @@ -987,11 +992,11 @@ abstract class RouteTransitionRecord { /// } /// for (final RouteTransitionRecord exitingPageRoute in locationToExitingPageRoute.values) { /// if (exitingPageRoute.isWaitingForExitingDecision) { -/// exitingPageRoute.markForRemove(); +/// exitingPageRoute.markForComplete(); /// final List? pagelessRoutes = pageRouteToPagelessRoutes[exitingPageRoute]; /// if (pagelessRoutes != null) { /// for (final RouteTransitionRecord pagelessRoute in pagelessRoutes) { -/// pagelessRoute.markForRemove(); +/// pagelessRoute.markForComplete(); /// } /// } /// } @@ -1112,8 +1117,7 @@ abstract class TransitionDelegate { /// route requires explicit decision on how it should transition off the /// Navigator. To make a decision for a removed route, call /// [RouteTransitionRecord.markForPop], - /// [RouteTransitionRecord.markForComplete] or - /// [RouteTransitionRecord.markForRemove]. It is possible that decisions are + /// [RouteTransitionRecord.markForComplete]. It is possible that decisions are /// not required for routes in the `locationToExitingPageRoute`. This can /// happen if the routes have already been popped in earlier page updates and /// are still waiting for popping animations to finish. In such case, those @@ -1159,8 +1163,6 @@ abstract class TransitionDelegate { /// without an animated transition. /// * [RouteTransitionRecord.markForPop], which makes route exit the screen /// with an animated transition. - /// * [RouteTransitionRecord.markForRemove], which does not complete the - /// route and makes it exit the screen without an animated transition. /// * [RouteTransitionRecord.markForComplete], which completes the route and /// makes it exit the screen without an animated transition. /// * [DefaultTransitionDelegate.resolve], which implements the default way @@ -2187,9 +2189,9 @@ class Navigator extends StatefulWidget { /// and [Route.didChangeNext]). If the [Navigator] has any /// [Navigator.observers], they will be notified as well (see /// [NavigatorObserver.didPush] and [NavigatorObserver.didRemove]). The - /// removed routes are disposed, without being notified, once the new route - /// has finished animating. The futures that had been returned from pushing - /// those routes will not complete. + /// removed routes are disposed, once the new route has finished animating, + /// and the futures that had been returned from pushing those routes + /// will complete. /// /// Ongoing gestures within the current route are canceled when a new route is /// pushed. @@ -2463,7 +2465,7 @@ class Navigator extends StatefulWidget { /// they will be notified as well (see [NavigatorObserver.didPush] and /// [NavigatorObserver.didRemove]). The removed routes are disposed of and /// notified, once the new route has finished animating. The futures that had - /// been returned from pushing those routes will not complete. + /// been returned from pushing those routes will complete. /// /// Ongoing gestures within the current route are canceled when a new route is /// pushed. @@ -2544,16 +2546,14 @@ class Navigator extends StatefulWidget { /// _does_ animate the new route, and delays removing the old route until the /// new route has finished animating. /// - /// The removed route is removed without being completed, so this method does - /// not take a return value argument. + /// The removed route is removed and completed with a `null` value. /// /// The new route, the route below the new route (if any), and the route above /// the new route, are all notified (see [Route.didReplace], /// [Route.didChangeNext], and [Route.didChangePrevious]). If the [Navigator] /// has any [Navigator.observers], they will be notified as well (see - /// [NavigatorObserver.didReplace]). The removed route is disposed without - /// being notified. The future that had been returned from pushing that routes - /// will not complete. + /// [NavigatorObserver.didReplace]). The removed route is disposed with its + /// future completed. /// /// This can be useful in combination with [removeRouteBelow] when building a /// non-linear user experience. @@ -2615,16 +2615,14 @@ class Navigator extends StatefulWidget { /// _does_ animate the new route, and delays removing the old route until the /// new route has finished animating. /// - /// The removed route is removed without being completed, so this method does - /// not take a return value argument. + /// The removed route is removed and completed with a `null` value. /// /// The new route, the route below the new route (if any), and the route above /// the new route, are all notified (see [Route.didReplace], /// [Route.didChangeNext], and [Route.didChangePrevious]). If the [Navigator] /// has any [Navigator.observers], they will be notified as well (see - /// [NavigatorObserver.didReplace]). The removed route is disposed without - /// being notified. The future that had been returned from pushing that routes - /// will not complete. + /// [NavigatorObserver.didReplace]). The removed route is disposed with its + /// future completed. /// /// The `T` type argument is the type of the return value of the new route. /// {@endtemplate} @@ -2815,27 +2813,35 @@ class Navigator extends StatefulWidget { /// the given context, and [Route.dispose] it. /// /// {@template flutter.widgets.navigator.removeRoute} - /// The removed route is removed without being completed, so this method does - /// not take a return value argument. No animations are run as a result of - /// this method call. + /// No animations are run as a result of this method call. /// /// The routes below and above the removed route are notified (see /// [Route.didChangeNext] and [Route.didChangePrevious]). If the [Navigator] /// has any [Navigator.observers], they will be notified as well (see - /// [NavigatorObserver.didRemove]). The removed route is disposed without - /// being notified. The future that had been returned from pushing that routes - /// will not complete. + /// [NavigatorObserver.didRemove]). The removed route is disposed with its + /// future completed. /// /// The given `route` must be in the history; this method will throw an /// exception if it is not. /// + /// If non-null, `result` will be used as the result of the route that is + /// removed; the future that had been returned from pushing the removed route + /// will complete with `result`. If provided, must match the type argument of + /// the class of the popped route (`T`). + /// + /// The `T` type argument is the type of the return value of the popped route. + /// + /// The type of `result`, if provided, must match the type argument of the + /// class of the removed route (`T`). + /// /// Ongoing gestures within the current route are canceled. /// {@endtemplate} /// /// This method is used, for example, to instantly dismiss dropdown menus that /// are up when the screen's orientation changes. - static void removeRoute(BuildContext context, Route route) { - return Navigator.of(context).removeRoute(route); + @optionalTypeArgs + static void removeRoute(BuildContext context, Route route, [T? result]) { + return Navigator.of(context).removeRoute(route, result); } /// Immediately remove a route from the navigator that most tightly encloses @@ -2843,24 +2849,36 @@ class Navigator extends StatefulWidget { /// one below the given `anchorRoute`. /// /// {@template flutter.widgets.navigator.removeRouteBelow} - /// The removed route is removed without being completed, so this method does - /// not take a return value argument. No animations are run as a result of - /// this method call. + /// No animations are run as a result of this method call. /// /// The routes below and above the removed route are notified (see /// [Route.didChangeNext] and [Route.didChangePrevious]). If the [Navigator] /// has any [Navigator.observers], they will be notified as well (see - /// [NavigatorObserver.didRemove]). The removed route is disposed without - /// being notified. The future that had been returned from pushing that routes - /// will not complete. + /// [NavigatorObserver.didRemove]). The removed route is disposed with its + /// future completed. /// /// The given `anchorRoute` must be in the history and must have a route below /// it; this method will throw an exception if it is not or does not. /// + /// If non-null, `result` will be used as the result of the route that is + /// removed; the future that had been returned from pushing the removed route + /// will complete with `result`. If provided, must match the type argument of + /// the class of the popped route (`T`). + /// + /// The `T` type argument is the type of the return value of the popped route. + /// + /// The type of `result`, if provided, must match the type argument of the + /// class of the removed route (`T`). + /// /// Ongoing gestures within the current route are canceled. /// {@endtemplate} - static void removeRouteBelow(BuildContext context, Route anchorRoute) { - return Navigator.of(context).removeRouteBelow(anchorRoute); + @optionalTypeArgs + static void removeRouteBelow( + BuildContext context, + Route anchorRoute, [ + T? result, + ]) { + return Navigator.of(context).removeRouteBelow(anchorRoute, result); } /// The state from the closest instance of this class that encloses the given @@ -3348,21 +3366,6 @@ class _RouteEntry extends RouteTransitionRecord { bool _reportRemovalToObserver = true; - // Route is removed without being completed. - void remove({bool isReplaced = false}) { - assert( - !pageBased || isWaitingForExitingDecision, - 'A page-based route cannot be completed using imperative api, provide a ' - 'new list without the corresponding Page to Navigator.pages instead. ', - ); - if (currentState.index >= _RouteLifecycle.remove.index) { - return; - } - assert(isPresent); - _reportRemovalToObserver = !isReplaced; - currentState = _RouteLifecycle.remove; - } - // Route completes with `result` and is removed. void complete(T result, {bool isReplaced = false}) { assert( @@ -3561,18 +3564,6 @@ class _RouteEntry extends RouteTransitionRecord { _isWaitingForExitingDecision = false; } - @override - void markForRemove() { - assert( - !isWaitingForEnteringDecision && isWaitingForExitingDecision && isPresent, - 'This route cannot be marked for remove. Either a decision has already ' - 'been made or it does not require an explicit decision on how to transition ' - 'out.', - ); - remove(); - _isWaitingForExitingDecision = false; - } - bool get restorationEnabled => route.restorationScopeId.value != null; set restorationEnabled(bool value) { assert(!value || restorationId != null); @@ -5315,7 +5306,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res _history.add(entry); while (index >= 0 && !predicate(_history[index].route)) { if (_history[index].isPresent) { - _history[index].remove(); + _history[index].complete(null); } index -= 1; } @@ -5400,7 +5391,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res ); final bool wasCurrent = oldRoute.isCurrent; _history.insert(index + 1, entry); - _history[index].remove(isReplaced: true); + _history[index].complete(null, isReplaced: true); _flushHistoryUpdates(); assert(() { _debugLocked = false; @@ -5490,7 +5481,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res } assert(index >= 0, 'There are no routes below the specified anchorRoute.'); _history.insert(index + 1, entry); - _history[index].remove(isReplaced: true); + _history[index].complete(null, isReplaced: true); _flushHistoryUpdates(); assert(() { _debugLocked = false; @@ -5654,7 +5645,8 @@ class NavigatorState extends State with TickerProviderStateMixin, Res /// Immediately remove `route` from the navigator, and [Route.dispose] it. /// /// {@macro flutter.widgets.navigator.removeRoute} - void removeRoute(Route route) { + @optionalTypeArgs + void removeRoute(Route route, [T? result]) { assert(!_debugLocked); assert(() { _debugLocked = true; @@ -5663,7 +5655,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res assert(route._navigator == this); final bool wasCurrent = route.isCurrent; final _RouteEntry entry = _history.firstWhere(_RouteEntry.isRoutePredicate(route)); - entry.remove(); + entry.complete(result); _flushHistoryUpdates(rearrangeOverlay: false); assert(() { _debugLocked = false; @@ -5678,7 +5670,8 @@ class NavigatorState extends State with TickerProviderStateMixin, Res /// route to be removed is the one below the given `anchorRoute`. /// /// {@macro flutter.widgets.navigator.removeRouteBelow} - void removeRouteBelow(Route anchorRoute) { + @optionalTypeArgs + void removeRouteBelow(Route anchorRoute, [T? result]) { assert(!_debugLocked); assert(() { _debugLocked = true; @@ -5699,7 +5692,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res index -= 1; } assert(index >= 0, 'There are no routes below the specified anchorRoute.'); - _history[index].remove(); + _history[index].complete(result); _flushHistoryUpdates(rearrangeOverlay: false); assert(() { _debugLocked = false; diff --git a/packages/flutter/test/widgets/navigator_test.dart b/packages/flutter/test/widgets/navigator_test.dart index ba22b44994..9f855732d6 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -16,6 +16,14 @@ import 'navigator_utils.dart'; import 'observer_tester.dart'; import 'semantics_tester.dart'; +@pragma('vm:entry-point') +Route _routeBuilder(BuildContext context, Object? arguments) { + return MaterialPageRoute( + settings: const RouteSettings(name: 'route'), + builder: (BuildContext context) => Container(), + ); +} + class FirstWidget extends StatelessWidget { const FirstWidget({super.key}); @override @@ -1459,12 +1467,409 @@ void main() { await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A await tester.pumpAndSettle(); - pageValue.then((String? value) { - assert(false); - }); final NavigatorState navigator = tester.state(find.byType(Navigator)); - navigator.removeRoute(routes['/A']!); // stack becomes /, pageValue will not complete + navigator.removeRoute( + routes['/A']!, + 'B', + ); // stack becomes /, pageValue will complete and return 'B' + expect(await pageValue, 'B'); + }); + + testWidgets('remove route below an other one whose value is awaited', ( + WidgetTester tester, + ) async { + late Future pageValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': + (BuildContext context) => OnTapPage( + id: '/A', + onTap: () { + Navigator.pushNamed(context, '/B'); + }, + ), + '/B': + (BuildContext context) => OnTapPage( + id: 'B', + onTap: () { + Navigator.pop(context, 'B'); + }, + ), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + await tester.tap(find.text('/A')); // pushNamed('/B'), stack becomes /, /A, /B + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + navigator.removeRouteBelow( + routes['/B']!, + 'A', + ); // stack becomes /, /B, pageValue will complete and return 'A' + expect(await pageValue, 'A'); + }); + + testWidgets('replace route by an other whose value is awaited', (WidgetTester tester) async { + late Future pageValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': (BuildContext context) => const OnTapPage(id: '/A'), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + + final MaterialPageRoute routeB = MaterialPageRoute( + builder: (BuildContext context) => const OnTapPage(id: '/B'), + ); + navigator.replace( + oldRoute: routes['/A']!, + newRoute: routeB, + ); // stack becomes /, /B, pageValue will complete and return 'A' + expect(await pageValue, null); + }); + + testWidgets('replace route by an other whose value is awaited', (WidgetTester tester) async { + late Future pageValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': (BuildContext context) => const OnTapPage(id: '/A'), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + + final MaterialPageRoute routeB = MaterialPageRoute( + builder: (BuildContext context) => const OnTapPage(id: '/B'), + ); + navigator.replace( + oldRoute: routes['/A']!, + newRoute: routeB, + ); // stack becomes /, /B, pageValue will complete and return 'A' + expect(await pageValue, null); + }); + + testWidgets('restorable replace route by an other whose value is awaited', ( + WidgetTester tester, + ) async { + late Future pageAValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageAValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': (BuildContext context) => const OnTapPage(id: '/A'), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + + navigator.restorableReplace( + oldRoute: routes['/A']!, + newRouteBuilder: _routeBuilder, + ); // stack becomes /, /route, pageValue will complete and return 'A' + expect(await pageAValue, null); + }); + + testWidgets('push named route and remove until where routes values are awaited', ( + WidgetTester tester, + ) async { + late Future pageAValue; + late Future pageBValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageAValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': + (BuildContext context) => OnTapPage( + id: '/A', + onTap: () { + pageBValue = Navigator.pushNamed(context, '/B'); + }, + ), + '/B': (BuildContext context) => const OnTapPage(id: '/B'), + '/C': (BuildContext context) => const OnTapPage(id: '/C'), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + await tester.tap(find.text('/A')); // pushNamed('/B'), stack becomes /, /A, /B + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + + navigator.pushNamedAndRemoveUntil( + '/C', + ModalRoute.withName('/'), + ); // stack becomes /, /C, pageAValue & pageBValue will complete and return null + expect(await pageAValue, null); + expect(await pageBValue, null); + }); + + testWidgets('push route and remove until where routes values are awaited', ( + WidgetTester tester, + ) async { + late Future pageAValue; + late Future pageBValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageAValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': + (BuildContext context) => OnTapPage( + id: '/A', + onTap: () { + pageBValue = Navigator.pushNamed(context, '/B'); + }, + ), + '/B': (BuildContext context) => const OnTapPage(id: '/B'), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + await tester.tap(find.text('/A')); // pushNamed('/B'), stack becomes /, /A, /B + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + + final MaterialPageRoute routeC = MaterialPageRoute( + builder: (BuildContext context) => const OnTapPage(id: '/C'), + ); + navigator.pushAndRemoveUntil( + routeC, + ModalRoute.withName('/'), + ); // stack becomes /, /C, pageAValue & pageBValue will complete and return null + expect(await pageAValue, null); + expect(await pageBValue, null); + }); + + testWidgets('restorable push named and remove until where routes values are awaited', ( + WidgetTester tester, + ) async { + late Future pageAValue; + late Future pageBValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageAValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': + (BuildContext context) => OnTapPage( + id: '/A', + onTap: () { + pageBValue = Navigator.pushNamed(context, '/B'); + }, + ), + '/B': (BuildContext context) => const OnTapPage(id: '/B'), + '/C': (BuildContext context) => const OnTapPage(id: '/C'), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + await tester.tap(find.text('/A')); // pushNamed('/B'), stack becomes /, /A, /B + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + + navigator.restorablePushNamedAndRemoveUntil( + '/C', + ModalRoute.withName('/'), + ); // stack becomes /, /C, pageAValue & pageBValue will complete and return null + expect(await pageAValue, null); + expect(await pageBValue, null); + }); + + testWidgets('restorable push and remove until where routes values are awaited', ( + WidgetTester tester, + ) async { + late Future pageAValue; + late Future pageBValue; + final Map pageBuilders = { + '/': + (BuildContext context) => OnTapPage( + id: '/', + onTap: () { + pageAValue = Navigator.pushNamed(context, '/A'); + }, + ), + '/A': + (BuildContext context) => OnTapPage( + id: '/A', + onTap: () { + pageBValue = Navigator.pushNamed(context, '/B'); + }, + ), + '/B': (BuildContext context) => const OnTapPage(id: '/B'), + }; + final Map> routes = >{}; + + await tester.pumpWidget( + MaterialApp( + onGenerateRoute: (RouteSettings settings) { + routes[settings.name!] = PageRouteBuilder( + settings: settings, + pageBuilder: (BuildContext context, Animation _, Animation _) { + return pageBuilders[settings.name!]!(context); + }, + ); + return routes[settings.name]; + }, + ), + ); + + await tester.tap(find.text('/')); // pushNamed('/A'), stack becomes /, /A + await tester.pumpAndSettle(); + await tester.tap(find.text('/A')); // pushNamed('/B'), stack becomes /, /A, /B + + final NavigatorState navigator = tester.state(find.byType(Navigator)); + + navigator.restorablePushAndRemoveUntil( + _routeBuilder, + ModalRoute.withName('/'), + ); // stack becomes /, /route, pageAValue & pageBValue will complete and return null + expect(await pageAValue, null); + expect(await pageBValue, null); }); testWidgets('replacing route can be observed', (WidgetTester tester) async { @@ -4027,8 +4432,8 @@ void main() { transitionDelegate: transitionDelegate, ), ); - // The pageless route of initial page route should be removed without complete. - expect(initialPageless1Completed, false); + // The pageless route of initial page route should be removed and completed. + expect(initialPageless1Completed, true); expect(secondPageless1Completed, false); expect(secondPageless2Completed, false); expect(thirdPageless1Completed, false); @@ -4044,9 +4449,9 @@ void main() { ), ); await tester.pumpAndSettle(); - expect(initialPageless1Completed, false); - expect(secondPageless1Completed, false); - expect(secondPageless2Completed, false); + expect(initialPageless1Completed, true); + expect(secondPageless1Completed, true); + expect(secondPageless2Completed, true); expect(thirdPageless1Completed, false); myPages = [const TestPage(key: ValueKey('4'), name: 'forth')]; @@ -4060,10 +4465,10 @@ void main() { ), ); await tester.pump(); - expect(initialPageless1Completed, false); - expect(secondPageless1Completed, false); - expect(secondPageless2Completed, false); - expect(thirdPageless1Completed, false); + expect(initialPageless1Completed, true); + expect(secondPageless1Completed, true); + expect(secondPageless2Completed, true); + expect(thirdPageless1Completed, true); expect(find.text('forth'), findsOneWidget); }); @@ -5859,12 +6264,12 @@ class AlwaysRemoveTransitionDelegate extends TransitionDelegate { final RouteTransitionRecord exitingPageRoute = locationToExitingPageRoute[location]!; if (exitingPageRoute.isWaitingForExitingDecision) { final bool hasPagelessRoute = pageRouteToPagelessRoutes.containsKey(exitingPageRoute); - exitingPageRoute.markForRemove(); + exitingPageRoute.markForComplete(); if (hasPagelessRoute) { final List pagelessRoutes = pageRouteToPagelessRoutes[exitingPageRoute]!; for (final RouteTransitionRecord pagelessRoute in pagelessRoutes) { - pagelessRoute.markForRemove(); + pagelessRoute.markForComplete(); } } } diff --git a/packages/flutter/test_fixes/widgets/widgets.dart b/packages/flutter/test_fixes/widgets/widgets.dart index e4c4cdff55..8f73103800 100644 --- a/packages/flutter/test_fixes/widgets/widgets.dart +++ b/packages/flutter/test_fixes/widgets/widgets.dart @@ -4,6 +4,32 @@ import 'package:flutter/widgets.dart'; +class _TestRouteTransitionRecord extends RouteTransitionRecord { + @override + bool get isWaitingForEnteringDecision => throw UnimplementedError(); + + @override + bool get isWaitingForExitingDecision => throw UnimplementedError(); + + @override + void markForAdd() {} + + @override + void markForComplete([dynamic result]) {} + + @override + void markForPop([dynamic result]) {} + + @override + void markForPush() {} + + @override + void markForRemove() {} + + @override + Route get route => throw UnimplementedError(); +} + void main() { // Generic reference variables. BuildContext context; @@ -189,4 +215,9 @@ void main() { // Changes made in https://github.com/flutter/flutter/pull/139260 final NavigatorState state = Navigator.of(context); state.focusScopeNode; + + // Changes made in https://github.com/flutter/flutter/pull/157725 + final _TestRouteTransitionRecord testRouteTransitionRecord = + _TestRouteTransitionRecord(); + testRouteTransitionRecord.markForComplete(); } diff --git a/packages/flutter/test_fixes/widgets/widgets.dart.expect b/packages/flutter/test_fixes/widgets/widgets.dart.expect index df89aaaf09..c95137b8f7 100644 --- a/packages/flutter/test_fixes/widgets/widgets.dart.expect +++ b/packages/flutter/test_fixes/widgets/widgets.dart.expect @@ -4,6 +4,32 @@ import 'package:flutter/widgets.dart'; +class _TestRouteTransitionRecord extends RouteTransitionRecord { + @override + bool get isWaitingForEnteringDecision => throw UnimplementedError(); + + @override + bool get isWaitingForExitingDecision => throw UnimplementedError(); + + @override + void markForAdd() {} + + @override + void markForComplete([dynamic result]) {} + + @override + void markForPop([dynamic result]) {} + + @override + void markForPush() {} + + @override + void markForRemove() {} + + @override + Route get route => throw UnimplementedError(); +} + void main() { // Generic reference variables. BuildContext context; @@ -189,4 +215,9 @@ void main() { // Changes made in https://github.com/flutter/flutter/pull/139260 final NavigatorState state = Navigator.of(context); state.focusNode.enclosingScope!; + + // Changes made in https://github.com/flutter/flutter/pull/157725 + final _TestRouteTransitionRecord testRouteTransitionRecord = + _TestRouteTransitionRecord(); + testRouteTransitionRecord.markForComplete(); }