Skip to content

Commit

Permalink
Fix popping repeated pages unexpectedly (#4763)
Browse files Browse the repository at this point in the history
issue:flutter/flutter#132229

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [relevant style guides] and ran the
auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages
repo does use `dart format`.)
- [ ] I signed the [CLA].
- [ ] The title of the PR starts with the name of the package surrounded
by square brackets, e.g. `[shared_preferences]`
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated `pubspec.yaml` with an appropriate new version according
to the [pub versioning philosophy], or this PR is [exempt from version
changes].
- [ ] I updated `CHANGELOG.md` to add a description of the change,
[following repository CHANGELOG style].
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[relevant style guides]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
[pub versioning philosophy]: https://dart.dev/tools/pub/versioning
[exempt from version changes]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates
[following repository CHANGELOG style]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
  • Loading branch information
hangyujin committed Aug 28, 2023
1 parent 94ba82c commit 7cbde90
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 13 deletions.
5 changes: 3 additions & 2 deletions packages/go_router/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## NEXT
## 10.1.1

* Updates minimum supported SDK version to Flutter 3.7/Dart 2.19.
- Fixes mapping from `Page` to `RouteMatch`s.
- Updates minimum supported SDK version to Flutter 3.7/Dart 2.19.

## 10.1.0

Expand Down
33 changes: 23 additions & 10 deletions packages/go_router/lib/src/builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class RouteBuilder {

// Every Page should have a corresponding RouteMatch.
assert(keyToPage.values.flattened.every((Page<Object?> page) =>
pagePopContext.getRouteMatchForPage(page) != null));
pagePopContext.getRouteMatchesForPage(page) != null));
}

/// Clean up previous cache to prevent memory leak, making sure any nested
Expand Down Expand Up @@ -299,7 +299,8 @@ class RouteBuilder {
}
if (page != null) {
registry[page] = state;
pagePopContext._setRouteMatchForPage(page, match);
// Insert the route match in reverse order.
pagePopContext._insertRouteMatchAtStartForPage(page, match);
}
}

Expand Down Expand Up @@ -561,8 +562,10 @@ typedef _PageBuilderForAppType = Page<void> Function({
class _PagePopContext {
_PagePopContext._(this.onPopPageWithRouteMatch);

final Map<Page<dynamic>, RouteMatch> _routeMatchLookUp =
<Page<Object?>, RouteMatch>{};
/// A page can be mapped to a RouteMatch list, such as a const page being
/// pushed multiple times.
final Map<Page<dynamic>, List<RouteMatch>> _routeMatchesLookUp =
<Page<Object?>, List<RouteMatch>>{};

/// On pop page callback that includes the associated [RouteMatch].
final PopPageWithRouteMatchCallback onPopPageWithRouteMatch;
Expand All @@ -571,18 +574,28 @@ class _PagePopContext {
///
/// The [Page] must have been previously built via the [RouteBuilder] that
/// created this [PagePopContext]; otherwise, this method returns null.
RouteMatch? getRouteMatchForPage(Page<Object?> page) =>
_routeMatchLookUp[page];

void _setRouteMatchForPage(Page<Object?> page, RouteMatch match) =>
_routeMatchLookUp[page] = match;
List<RouteMatch>? getRouteMatchesForPage(Page<Object?> page) =>
_routeMatchesLookUp[page];

/// This is called in _buildRecursive to insert route matches in reverse order.
void _insertRouteMatchAtStartForPage(Page<Object?> page, RouteMatch match) {
_routeMatchesLookUp
.putIfAbsent(page, () => <RouteMatch>[])
.insert(0, match);
}

/// Function used as [Navigator.onPopPage] callback when creating Navigators.
///
/// This function forwards to [onPopPageWithRouteMatch], including the
/// [RouteMatch] associated with the popped route.
///
/// This assumes always pop the last route match for the page.
bool onPopPage(Route<dynamic> route, dynamic result) {
final Page<Object?> page = route.settings as Page<Object?>;
return onPopPageWithRouteMatch(route, result, _routeMatchLookUp[page]);

final RouteMatch match = _routeMatchesLookUp[page]!.last;
_routeMatchesLookUp[page]!.removeLast();

return onPopPageWithRouteMatch(route, result, match);
}
}
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -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: 10.1.0
version: 10.1.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

Expand Down
47 changes: 47 additions & 0 deletions packages/go_router/test/go_router_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,53 @@ void main() {
expect(find.byKey(settings), findsOneWidget);
});

testWidgets('can correctly pop stacks of repeated pages',
(WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/#132229.

final GlobalKey<NavigatorState> navKey = GlobalKey<NavigatorState>();
final List<GoRoute> routes = <GoRoute>[
GoRoute(
path: '/',
pageBuilder: (_, __) =>
const MaterialPage<Object>(child: HomeScreen()),
),
GoRoute(
path: '/page1',
pageBuilder: (_, __) =>
const MaterialPage<Object>(child: Page1Screen()),
),
GoRoute(
path: '/page2',
pageBuilder: (_, __) =>
const MaterialPage<Object>(child: Page2Screen()),
),
];
final GoRouter router =
await createRouter(routes, tester, navigatorKey: navKey);
expect(find.byType(HomeScreen), findsOneWidget);

router.push('/page1');
router.push('/page2');
router.push('/page1');
router.push('/page2');
await tester.pumpAndSettle();

expect(find.byType(HomeScreen), findsNothing);
expect(find.byType(Page1Screen), findsNothing);
expect(find.byType(Page2Screen), findsOneWidget);

router.pop();
await tester.pumpAndSettle();

final List<RouteMatch> matches =
router.routerDelegate.currentConfiguration.matches;
expect(matches.length, 4);
expect(find.byType(HomeScreen), findsNothing);
expect(find.byType(Page1Screen), findsOneWidget);
expect(find.byType(Page2Screen), findsNothing);
});

testWidgets('match sub-route', (WidgetTester tester) async {
final List<GoRoute> routes = <GoRoute>[
GoRoute(
Expand Down

0 comments on commit 7cbde90

Please sign in to comment.