Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[go_router] Fixes an issue where route future does not complete when … #6596

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

chunhtai
Copy link
Contributor

…popping shell route.

fixes flutter/flutter#147196

Pre-launch Checklist

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

@@ -147,8 +147,12 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
}

void _completeRouteMatch(Object? result, RouteMatchBase match) {
if (match is ImperativeRouteMatch) {
match.complete(result);
RouteMatchBase walker = match;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's more clear to walk the tree in _handlePopPage instead of _completeRouteMatch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's odd that the match here is not a final match, you still have to walk the tree.

but if you walk it right after assigning the match, it looks more clear?

 bool _handlePopPage(Route<Object?> route, Object? result) {
    final Page<Object?> page = route.settings as Page<Object?>;
    final RouteMatchBase match = _pageToRouteMatchBase[page]!;
+++
walk the tree 
+++
    return widget.onPopPageWithRouteMatch(route, result, match);
  }
``

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The match is the correct match we want to remove, but it can be a shell route match, which means we want to remove the entire branch of that shell route match. Also in this case it should only have one leaf node, otherwise the pop would not remove the shell route match in the first place.

The problem is that the completer is stored in the leaf node of that shell route match, that's why i have to find the leave node here.

I think put it in here is correct, we do want to complete and remove the entire shell route branch. as of how we complete this branch is the implementation detail in this method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the explanation!

@chunhtai chunhtai requested a review from hannah-hyj April 23, 2024 17:00
Copy link
Member

@hannah-hyj hannah-hyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 24, 2024
@auto-submit auto-submit bot merged commit 890ec36 into flutter:main Apr 24, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 25, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 25, 2024
flutter/packages@cf6d280...fde908d

2024-04-25 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.3 to 4.1.4 (flutter/packages#6609)
2024-04-24 [email protected] [go_router] Add `GoRouterState state` parameter to `GoRouterData.onExit` (flutter/packages#6495)
2024-04-24 [email protected] Add CI steps to test iOS and macOS plugins with both CocoaPods and Swift Package Manager (flutter/packages#6557)
2024-04-24 [email protected] Roll Flutter from 77043ba to dba4f77 (30 revisions) (flutter/packages#6607)
2024-04-24 [email protected] [camera] Finish converting iOS to Pigeon (flutter/packages#6601)
2024-04-24 [email protected] [go_router] Fixes an issue where route future does not complete when â�¦ (flutter/packages#6596)
2024-04-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Bump legacy all_packages project AGP version to 7.0.0, Gradle version to 7.0.2 (#6591)" (flutter/packages#6605)
2024-04-23 [email protected] [in_app_purchase_android] Readme update for Alternative billing (flutter/packages#6578)
2024-04-23 [email protected] Bump legacy all_packages project AGP version to 7.0.0, Gradle version to 7.0.2 (flutter/packages#6591)
2024-04-23 [email protected] Roll Flutter from 140edb9 to 77043ba (21 revisions) (flutter/packages#6599)
2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.1 to 3.25.2 (flutter/packages#6597)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: go_router
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[go_router] push future does not complete if popping shellroute
2 participants