diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index 09b0e883479a..9c71d389a31a 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 15.1.2 + +- Fixes `PopScope.onPopInvokedWithResult` getting called twice when popping a route if `GoRouteData.onExit` is not null + ## 15.1.1 - Adds missing `caseSensitive` to `GoRouteData.$route`. diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index aa95fdb39eb3..bc0751b6df3b 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -66,11 +66,12 @@ class GoRouterDelegate extends RouterDelegate // Fallback to onExit if maybePop did not handle the pop final GoRoute lastRoute = currentConfiguration.last.route; if (lastRoute.onExit != null && navigatorKey.currentContext != null) { - return !(await lastRoute.onExit!( - navigatorKey.currentContext!, - currentConfiguration.last - .buildState(_configuration, currentConfiguration), - )); + return !(await lastRoute.onExit!.call( + navigatorKey.currentContext!, + currentConfiguration.last + .buildState(_configuration, currentConfiguration), + ) ?? + true); // @TODO(techouse) not sure if returning true by default is correct } return false; @@ -146,11 +147,14 @@ class GoRouterDelegate extends RouterDelegate // a microtask in case the onExit callback want to launch dialog or other // navigator operations. scheduleMicrotask(() async { - final bool onExitResult = await routeBase.onExit!( + final bool? onExitResult = await routeBase.onExit!.call( navigatorKey.currentContext!, match.buildState(_configuration, currentConfiguration), ); - if (onExitResult) { + if (onExitResult == null) { + route.didPop(result); + } + if (onExitResult ?? true) { _completeRouteMatch(result, match); } }); @@ -298,14 +302,17 @@ class GoRouterDelegate extends RouterDelegate return SynchronousFuture(false); } - final FutureOr exitFuture = goRoute.onExit!( + final FutureOr? exitFuture = goRoute.onExit?.call( context, match.buildState(_configuration, currentConfiguration), ); if (exitFuture is bool) { return handleOnExitResult(exitFuture); } - return exitFuture.then(handleOnExitResult); + return exitFuture?.then(handleOnExitResult) ?? + SynchronousFuture( + true, // @TODO(techouse) not sure if returning true by default is correct + ); } Future _setCurrentConfiguration(RouteMatchList configuration) { diff --git a/packages/go_router/lib/src/route.dart b/packages/go_router/lib/src/route.dart index d16e1dfd658b..5eddb5c4a6c0 100644 --- a/packages/go_router/lib/src/route.dart +++ b/packages/go_router/lib/src/route.dart @@ -67,7 +67,7 @@ typedef NavigatorBuilder = Widget Function( /// /// If the return value is true or the future resolve to true, the route will /// exit as usual. Otherwise, the operation will abort. -typedef ExitCallback = FutureOr Function( +typedef ExitCallback = FutureOr? Function( BuildContext context, GoRouterState state); /// The base class for [GoRoute] and [ShellRoute]. @@ -1453,6 +1453,7 @@ class _RestorableRouteMatchList extends RestorableProperty { RouteMatchList get value => _value; RouteMatchList _value = RouteMatchList.empty; + set value(RouteMatchList newValue) { if (newValue != _value) { _value = newValue; diff --git a/packages/go_router/lib/src/route_data.dart b/packages/go_router/lib/src/route_data.dart index e288182de8ac..a5252a5b4735 100644 --- a/packages/go_router/lib/src/route_data.dart +++ b/packages/go_router/lib/src/route_data.dart @@ -66,7 +66,7 @@ abstract class GoRouteData extends RouteData { /// Called when this route is removed from GoRouter's route history. /// /// Corresponds to [GoRoute.onExit]. - FutureOr onExit(BuildContext context, GoRouterState state) => true; + FutureOr? onExit(BuildContext context, GoRouterState state) => null; /// A helper function used by generated code. /// @@ -112,7 +112,7 @@ abstract class GoRouteData extends RouteData { FutureOr redirect(BuildContext context, GoRouterState state) => factoryImpl(state).redirect(context, state); - FutureOr onExit(BuildContext context, GoRouterState state) => + FutureOr? onExit(BuildContext context, GoRouterState state) => factoryImpl(state).onExit(context, state); return GoRoute( diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index 31e9debc0b99..dcf98a3d9839 100644 --- a/packages/go_router/pubspec.yaml +++ b/packages/go_router/pubspec.yaml @@ -1,7 +1,7 @@ name: go_router description: A declarative router for Flutter based on Navigation 2 supporting deep linking, data-driven routes and more -version: 15.1.1 +version: 15.1.2 repository: https://github.com/flutter/packages/tree/main/packages/go_router issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22 diff --git a/packages/go_router/test/on_exit_test.dart b/packages/go_router/test/on_exit_test.dart index 8a7bd83ab648..997db069ef08 100644 --- a/packages/go_router/test/on_exit_test.dart +++ b/packages/go_router/test/on_exit_test.dart @@ -471,4 +471,133 @@ void main() { expect(onExitState2.fullPath, '/route-2/:id2'); }, ); + + testWidgets( + 'PopScope onPopInvokedWithResult should be called only once', + (WidgetTester tester) async { + int counter = 0; + + final GoRouter goRouter = GoRouter( + initialLocation: '/', + routes: [ + GoRoute(path: '/', builder: (_, __) => const DummyStatefulWidget()), + GoRoute( + path: '/page-1', + builder: (_, __) => PopScope( + onPopInvokedWithResult: (bool didPop, __) { + if (didPop) { + counter++; + return; + } + }, + child: const Text('Page 1'), + ), + ), + ], + ); + + addTearDown(goRouter.dispose); + + await tester.pumpWidget(MaterialApp.router( + routeInformationProvider: goRouter.routeInformationProvider, + routeInformationParser: goRouter.routeInformationParser, + routerDelegate: goRouter.routerDelegate, + )); + + goRouter.push('/page-1'); + + await tester.pumpAndSettle(); + + expect(find.text('Page 1'), findsOneWidget); + expect( + goRouter.routerDelegate.currentConfiguration.matches.length, + equals(2), + ); + expect(goRouter.routerDelegate.canPop(), true); + + goRouter.routerDelegate.pop(); + + await tester.pumpAndSettle(); + + expect(counter, equals(1)); + }, + ); + + testWidgets( + r'PopScope onPopInvokedWithResult should be called only once with GoRouteData.$route', + (WidgetTester tester) async { + int counter = 0; + + final GoRouter goRouter = GoRouter( + initialLocation: '/', + routes: [ + GoRoute(path: '/', builder: (_, __) => const DummyStatefulWidget()), + GoRouteData.$route( + path: '/page-1', + factory: (GoRouterState state) => _Page1( + onPop: () { + counter++; + }, + ), + ), + ], + ); + + addTearDown(goRouter.dispose); + + await tester.pumpWidget(MaterialApp.router( + routeInformationProvider: goRouter.routeInformationProvider, + routeInformationParser: goRouter.routeInformationParser, + routerDelegate: goRouter.routerDelegate, + )); + + goRouter.push('/page-1'); + + await tester.pumpAndSettle(); + + expect(find.text('Page 1'), findsOneWidget); + expect( + goRouter.routerDelegate.currentConfiguration.matches.length, + equals(2), + ); + expect(goRouter.routerDelegate.canPop(), true); + + goRouter.routerDelegate.pop(); + + await tester.pumpAndSettle(); + + expect(counter, equals(1)); + }, + ); +} + +class _Page1 extends GoRouteData { + const _Page1({required this.onPop}); + + final VoidCallback onPop; + + @override + Page buildPage(_, __) => MaterialPage( + child: PopScope( + onPopInvokedWithResult: (bool didPop, _) { + if (didPop) { + onPop(); + return; + } + }, + child: const Text('Page 1'), + ), + ); +} + +class DummyStatefulWidget extends StatefulWidget { + const DummyStatefulWidget({super.key}); + + @override + State createState() => _DummyStatefulWidgetState(); +} + +class _DummyStatefulWidgetState extends State { + @override + Widget build(_) => const SizedBox(); }