From 44fc781fb84206f29aa1840356108db0c82d1931 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Thu, 1 Dec 2022 13:34:56 -0800 Subject: [PATCH] [go_router] Refactors GoRouter.pop to handle pageless route (#2879) * Refactors GoRouter.pop to handle pageless route * update --- packages/go_router/CHANGELOG.md | 4 + packages/go_router/lib/src/builder.dart | 54 ++++--- packages/go_router/lib/src/delegate.dart | 164 +++++++++++++------- packages/go_router/lib/src/router.dart | 11 +- packages/go_router/pubspec.yaml | 2 +- packages/go_router/test/builder_test.dart | 2 +- packages/go_router/test/delegate_test.dart | 15 +- packages/go_router/test/go_router_test.dart | 125 +++++++++++++-- packages/go_router/test/test_helpers.dart | 2 +- 9 files changed, 273 insertions(+), 106 deletions(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index 333e564857..ff7402ee90 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 5.2.1 + +- Refactors `GoRouter.pop` to be able to pop individual pageless route with result. + ## 5.2.0 - Fixes `GoRouterState.location` and `GoRouterState.param` to return correct value. diff --git a/packages/go_router/lib/src/builder.dart b/packages/go_router/lib/src/builder.dart index 42989d741d..04c6656fb8 100644 --- a/packages/go_router/lib/src/builder.dart +++ b/packages/go_router/lib/src/builder.dart @@ -54,7 +54,7 @@ class RouteBuilder { Widget build( BuildContext context, RouteMatchList matchList, - VoidCallback pop, + PopPageCallback onPopPage, bool routerNeglect, ) { if (matchList.isEmpty) { @@ -69,14 +69,14 @@ class RouteBuilder { try { final Map, GoRouterState> newRegistry = , GoRouterState>{}; - final Widget result = tryBuild(context, matchList, pop, + final Widget result = tryBuild(context, matchList, onPopPage, routerNeglect, configuration.navigatorKey, newRegistry); _registry.updateRegistry(newRegistry); return GoRouterStateRegistryScope( registry: _registry, child: result); } on _RouteBuilderError catch (e) { - return _buildErrorNavigator( - context, e, matchList.uri, pop, configuration.navigatorKey); + return _buildErrorNavigator(context, e, matchList.uri, onPopPage, + configuration.navigatorKey); } }, ), @@ -91,7 +91,7 @@ class RouteBuilder { Widget tryBuild( BuildContext context, RouteMatchList matchList, - VoidCallback pop, + PopPageCallback onPopPage, bool routerNeglect, GlobalKey navigatorKey, Map, GoRouterState> registry, @@ -99,9 +99,9 @@ class RouteBuilder { return builderWithNav( context, _buildNavigator( - pop, - buildPages( - context, matchList, pop, routerNeglect, navigatorKey, registry), + onPopPage, + buildPages(context, matchList, onPopPage, routerNeglect, navigatorKey, + registry), navigatorKey, observers: observers, ), @@ -114,15 +114,15 @@ class RouteBuilder { List> buildPages( BuildContext context, RouteMatchList matchList, - VoidCallback onPop, + PopPageCallback onPopPage, bool routerNeglect, GlobalKey navigatorKey, Map, GoRouterState> registry) { try { final Map, List>> keyToPage = , List>>{}; - _buildRecursive(context, matchList, 0, onPop, routerNeglect, keyToPage, - navigatorKey, registry); + _buildRecursive(context, matchList, 0, onPopPage, routerNeglect, + keyToPage, navigatorKey, registry); return keyToPage[navigatorKey]!; } on _RouteBuilderError catch (e) { return >[ @@ -135,7 +135,7 @@ class RouteBuilder { BuildContext context, RouteMatchList matchList, int startIndex, - VoidCallback pop, + PopPageCallback onPopPage, bool routerNeglect, Map, List>> keyToPages, GlobalKey navigatorKey, @@ -163,8 +163,8 @@ class RouteBuilder { keyToPages.putIfAbsent(goRouteNavKey, () => >[]).add(page); - _buildRecursive(context, matchList, startIndex + 1, pop, routerNeglect, - keyToPages, navigatorKey, registry); + _buildRecursive(context, matchList, startIndex + 1, onPopPage, + routerNeglect, keyToPages, navigatorKey, registry); } else if (route is ShellRoute) { // The key for the Navigator that will display this ShellRoute's page. final GlobalKey parentNavigatorKey = navigatorKey; @@ -184,12 +184,12 @@ class RouteBuilder { final int shellPageIdx = keyToPages[parentNavigatorKey]!.length; // Build the remaining pages - _buildRecursive(context, matchList, startIndex + 1, pop, routerNeglect, - keyToPages, shellNavigatorKey, registry); + _buildRecursive(context, matchList, startIndex + 1, onPopPage, + routerNeglect, keyToPages, shellNavigatorKey, registry); // Build the Navigator final Widget child = _buildNavigator( - pop, keyToPages[shellNavigatorKey]!, shellNavigatorKey); + onPopPage, keyToPages[shellNavigatorKey]!, shellNavigatorKey); // Build the Page for this route final Page page = @@ -203,7 +203,7 @@ class RouteBuilder { } Navigator _buildNavigator( - VoidCallback pop, + PopPageCallback onPopPage, List> pages, Key? navigatorKey, { List observers = const [], @@ -213,13 +213,7 @@ class RouteBuilder { restorationScopeId: restorationScopeId, pages: pages, observers: observers, - onPopPage: (Route route, dynamic result) { - if (!route.didPop(result)) { - return false; - } - pop(); - return true; - }, + onPopPage: onPopPage, ); } @@ -393,10 +387,14 @@ class RouteBuilder { ); /// Builds a Navigator containing an error page. - Widget _buildErrorNavigator(BuildContext context, _RouteBuilderError e, - Uri uri, VoidCallback pop, GlobalKey navigatorKey) { + Widget _buildErrorNavigator( + BuildContext context, + _RouteBuilderError e, + Uri uri, + PopPageCallback onPopPage, + GlobalKey navigatorKey) { return _buildNavigator( - pop, + onPopPage, >[ _buildErrorPage(context, e, uri), ], diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index 2ccbd4c1e4..3bb303cda7 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -11,6 +11,7 @@ import 'builder.dart'; import 'configuration.dart'; import 'match.dart'; import 'matching.dart'; +import 'misc/errors.dart'; import 'typedefs.dart'; /// GoRouter implementation of [RouterDelegate]. @@ -59,38 +60,19 @@ class GoRouterDelegate extends RouterDelegate final Map _pushCounts = {}; final RouteConfiguration _configuration; + _NavigatorStateIterator _createNavigatorStateIterator() => + _NavigatorStateIterator(_matchList, navigatorKey.currentState!); + @override Future popRoute() async { - // Iterate backwards through the RouteMatchList until seeing a GoRoute with - // a non-null parentNavigatorKey or a ShellRoute with a non-null - // parentNavigatorKey and pop from that Navigator instead of the root. - final int matchCount = _matchList.matches.length; - for (int i = matchCount - 1; i >= 0; i -= 1) { - final RouteMatch match = _matchList.matches[i]; - final RouteBase route = match.route; - - if (route is GoRoute && route.parentNavigatorKey != null) { - final bool didPop = - await route.parentNavigatorKey!.currentState!.maybePop(); - - // Continue if didPop was false. - if (didPop) { - return didPop; - } - } else if (route is ShellRoute) { - final bool didPop = await route.navigatorKey.currentState!.maybePop(); - - // Continue if didPop was false. - if (didPop) { - return didPop; - } + final _NavigatorStateIterator iterator = _createNavigatorStateIterator(); + while (iterator.moveNext()) { + final bool didPop = await iterator.current.maybePop(); + if (didPop) { + return true; } } - - // Use the root navigator if no ShellRoute Navigators were found and didn't - // pop - final NavigatorState navigator = navigatorKey.currentState!; - return navigator.maybePop(); + return false; } /// Pushes the given location onto the page stack @@ -117,29 +99,25 @@ class GoRouterDelegate extends RouterDelegate /// Returns `true` if the active Navigator can pop. bool canPop() { - // Loop through navigators in reverse and call canPop() - final int matchCount = _matchList.matches.length; - for (int i = matchCount - 1; i >= 0; i -= 1) { - final RouteMatch match = _matchList.matches[i]; - final RouteBase route = match.route; - if (route is GoRoute && route.parentNavigatorKey != null) { - final bool canPop = - route.parentNavigatorKey!.currentState?.canPop() ?? false; - - // Continue if canPop is false. - if (canPop) { - return canPop; - } - } else if (route is ShellRoute) { - final bool canPop = route.navigatorKey.currentState?.canPop() ?? false; - - // Continue if canPop is false. - if (canPop) { - return canPop; - } + final _NavigatorStateIterator iterator = _createNavigatorStateIterator(); + while (iterator.moveNext()) { + if (iterator.current.canPop()) { + return true; } } - return navigatorKey.currentState?.canPop() ?? false; + return false; + } + + /// Pops the top-most route. + void pop([T? result]) { + final _NavigatorStateIterator iterator = _createNavigatorStateIterator(); + while (iterator.moveNext()) { + if (iterator.current.canPop()) { + iterator.current.pop(result); + return; + } + } + throw GoError('There is nothing to pop'); } void _debugAssertMatchListNotEmpty() { @@ -150,14 +128,16 @@ class GoRouterDelegate extends RouterDelegate ); } - /// Pop the top page off the GoRouter's page stack. - void pop() { + bool _onPopPage(Route route, Object? result) { + if (!route.didPop(result)) { + return false; + } _matchList.pop(); assert(() { _debugAssertMatchListNotEmpty(); return true; }()); - notifyListeners(); + return true; } /// Replaces the top-most page of the page stack with the given one. @@ -187,7 +167,7 @@ class GoRouterDelegate extends RouterDelegate return builder.build( context, _matchList, - pop, + _onPopPage, routerNeglect, ); } @@ -204,6 +184,84 @@ class GoRouterDelegate extends RouterDelegate } } +/// An iterator that iterates through navigators that [GoRouterDelegate] +/// created from the inner to outer. +/// +/// The iterator starts with the navigator that hosts the top-most route. This +/// navigator may not be the inner-most navigator if the top-most route is a +/// pageless route, such as a dialog or bottom sheet. +class _NavigatorStateIterator extends Iterator { + _NavigatorStateIterator(this.matchList, this.root) + : index = matchList.matches.length; + + final RouteMatchList matchList; + int index = 0; + final NavigatorState root; + @override + late NavigatorState current; + + @override + bool moveNext() { + if (index < 0) { + return false; + } + for (index -= 1; index >= 0; index -= 1) { + final RouteMatch match = matchList.matches[index]; + final RouteBase route = match.route; + if (route is GoRoute && route.parentNavigatorKey != null) { + final GlobalKey parentNavigatorKey = + route.parentNavigatorKey!; + final ModalRoute? parentModalRoute = + ModalRoute.of(parentNavigatorKey.currentContext!); + // The ModalRoute can be null if the parentNavigatorKey references the + // root navigator. + if (parentModalRoute == null) { + index = -1; + assert(root == parentNavigatorKey.currentState); + current = root; + return true; + } + // It must be a ShellRoute that holds this parentNavigatorKey; + // otherwise, parentModalRoute would have been null. Updates the index + // to the ShellRoute + for (index -= 1; index >= 0; index -= 1) { + final RouteBase route = matchList.matches[index].route; + if (route is ShellRoute) { + if (route.navigatorKey == parentNavigatorKey) { + break; + } + } + } + // There may be a pageless route on top of ModalRoute that the + // NavigatorState of parentNavigatorKey is in. For example, an open + // dialog. In that case we want to find the navigator that host the + // pageless route. + if (parentModalRoute.isCurrent == false) { + continue; + } + + current = parentNavigatorKey.currentState!; + return true; + } else if (route is ShellRoute) { + // Must have a ModalRoute parent because the navigator ShellRoute + // created must not be the root navigator. + final ModalRoute parentModalRoute = + ModalRoute.of(route.navigatorKey.currentContext!)!; + // There may be pageless route on top of ModalRoute that the + // parentNavigatorKey is in. For example an open dialog. + if (parentModalRoute.isCurrent == false) { + continue; + } + current = route.navigatorKey.currentState!; + return true; + } + } + assert(index == -1); + current = root; + return true; + } +} + /// The route match that represent route pushed through [GoRouter.push]. // TODO(chunhtai): Removes this once imperative API no longer insert route match. class ImperativeRouteMatch extends RouteMatch { diff --git a/packages/go_router/lib/src/router.dart b/packages/go_router/lib/src/router.dart index 120bfd244f..548747e5ce 100644 --- a/packages/go_router/lib/src/router.dart +++ b/packages/go_router/lib/src/router.dart @@ -142,7 +142,7 @@ class GoRouter extends ChangeNotifier implements RouterConfig { String get location => _location; String _location = '/'; - /// Returns `true` if there is more than 1 page on the stack. + /// Returns `true` if there is at least two or more route can be pop. bool canPop() => _routerDelegate.canPop(); void _handleStateMayChange() { @@ -272,13 +272,16 @@ class GoRouter extends ChangeNotifier implements RouterConfig { ); } - /// Pop the top page off the GoRouter's page stack. - void pop() { + /// Pop the top-most route off the current screen. + /// + /// If the top-most route is a pop up or dialog, this method pops it instead + /// of any GoRoute under it. + void pop([T? result]) { assert(() { log.info('popping $location'); return true; }()); - _routerDelegate.pop(); + _routerDelegate.pop(result); } /// Refresh the route. diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index 5655bd6d89..64eb598047 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: 5.2.0 +version: 5.2.1 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/builder_test.dart b/packages/go_router/test/builder_test.dart index ba4f7f5f43..04e75432df 100644 --- a/packages/go_router/test/builder_test.dart +++ b/packages/go_router/test/builder_test.dart @@ -346,7 +346,7 @@ class _BuilderTestWidget extends StatelessWidget { @override Widget build(BuildContext context) { return MaterialApp( - home: builder.tryBuild(context, matches, () {}, false, + home: builder.tryBuild(context, matches, (_, __) => false, false, routeConfiguration.navigatorKey, , GoRouterState>{}), // builder: (context, child) => , ); diff --git a/packages/go_router/test/delegate_test.dart b/packages/go_router/test/delegate_test.dart index 3974223def..7df8504c33 100644 --- a/packages/go_router/test/delegate_test.dart +++ b/packages/go_router/test/delegate_test.dart @@ -36,24 +36,21 @@ void main() { testWidgets('removes the last element', (WidgetTester tester) async { final GoRouter goRouter = await createGoRouter(tester) ..push('/error'); + await tester.pumpAndSettle(); - goRouter.routerDelegate.addListener(expectAsync0(() {})); final RouteMatch last = goRouter.routerDelegate.matches.matches.last; - goRouter.routerDelegate.pop(); + await goRouter.routerDelegate.popRoute(); expect(goRouter.routerDelegate.matches.matches.length, 1); expect(goRouter.routerDelegate.matches.matches.contains(last), false); }); - testWidgets('throws when it pops more than matches count', + testWidgets('pops more than matches count should return false', (WidgetTester tester) async { final GoRouter goRouter = await createGoRouter(tester) ..push('/error'); - expect( - () => goRouter.routerDelegate - ..pop() - ..pop(), - throwsA(isAssertionError), - ); + await tester.pumpAndSettle(); + await goRouter.routerDelegate.popRoute(); + expect(await goRouter.routerDelegate.popRoute(), isFalse); }); }); diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index d301742f75..35198e0b88 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -2549,15 +2549,6 @@ void main() { }); group('Imperative navigation', () { - testWidgets('pop triggers pop on routerDelegate', - (WidgetTester tester) async { - final GoRouter router = await createGoRouter(tester) - ..push('/error'); - router.routerDelegate.addListener(expectAsync0(() {})); - router.pop(); - await tester.pump(); - }); - group('canPop', () { testWidgets( 'It should return false if Navigator.canPop() returns false.', @@ -2736,7 +2727,55 @@ void main() { expect(router.canPop(), true); }, ); + testWidgets('Pageless route should include in can pop', + (WidgetTester tester) async { + final GlobalKey root = + GlobalKey(debugLabel: 'root'); + final GlobalKey shell = + GlobalKey(debugLabel: 'shell'); + + final GoRouter router = GoRouter( + navigatorKey: root, + routes: [ + ShellRoute( + navigatorKey: shell, + builder: + (BuildContext context, GoRouterState state, Widget child) { + return Scaffold( + body: Center( + child: Column( + children: [ + const Text('Shell'), + Expanded(child: child), + ], + ), + ), + ); + }, + routes: [ + GoRoute( + path: '/', + builder: (_, __) => const Text('A Screen'), + ), + ], + ), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + + expect(router.canPop(), isFalse); + expect(find.text('A Screen'), findsOneWidget); + expect(find.text('Shell'), findsOneWidget); + showDialog( + context: root.currentContext!, + builder: (_) => const Text('A dialog')); + await tester.pumpAndSettle(); + expect(find.text('A dialog'), findsOneWidget); + expect(router.canPop(), isTrue); + }); }); + group('pop', () { testWidgets( 'Should pop from the correct navigator when parentNavigatorKey is set', @@ -2815,6 +2854,74 @@ void main() { expect(find.text('Shell'), findsNothing); }, ); + + testWidgets('Should pop dialog if it is present', + (WidgetTester tester) async { + final GlobalKey root = + GlobalKey(debugLabel: 'root'); + final GlobalKey shell = + GlobalKey(debugLabel: 'shell'); + + final GoRouter router = GoRouter( + initialLocation: '/a', + navigatorKey: root, + routes: [ + GoRoute( + path: '/', + builder: (BuildContext context, _) { + return const Scaffold( + body: Text('Home'), + ); + }, + routes: [ + ShellRoute( + navigatorKey: shell, + builder: (BuildContext context, GoRouterState state, + Widget child) { + return Scaffold( + body: Center( + child: Column( + children: [ + const Text('Shell'), + Expanded(child: child), + ], + ), + ), + ); + }, + routes: [ + GoRoute( + path: 'a', + builder: (_, __) => const Text('A Screen'), + ), + ], + ), + ], + ), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + + expect(router.canPop(), isTrue); + expect(find.text('A Screen'), findsOneWidget); + expect(find.text('Shell'), findsOneWidget); + expect(find.text('Home'), findsNothing); + final Future resultFuture = showDialog( + context: root.currentContext!, + builder: (_) => const Text('A dialog')); + await tester.pumpAndSettle(); + expect(find.text('A dialog'), findsOneWidget); + expect(router.canPop(), isTrue); + + router.pop(true); + await tester.pumpAndSettle(); + expect(find.text('A Screen'), findsOneWidget); + expect(find.text('Shell'), findsOneWidget); + expect(find.text('A dialog'), findsNothing); + final bool? result = await resultFuture; + expect(result, isTrue); + }); }); }); } diff --git a/packages/go_router/test/test_helpers.dart b/packages/go_router/test/test_helpers.dart index be35707d98..9ead9a688c 100644 --- a/packages/go_router/test/test_helpers.dart +++ b/packages/go_router/test/test_helpers.dart @@ -130,7 +130,7 @@ class GoRouterPopSpy extends GoRouter { bool popped = false; @override - void pop() { + void pop([T? result]) { popped = true; } }