Skip to content

Allow Navigator.popUntil to return a value #169341

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

alex-medinsh
Copy link
Contributor

@alex-medinsh alex-medinsh commented May 23, 2025

This PR adds a result param to Navigator.popUntil that allows to pass a value to the last pop call.

Fixes #30112

Pre-launch Checklist

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels May 23, 2025
@alex-medinsh alex-medinsh force-pushed the popuntil-return-data branch from 5d2a3c3 to 2577501 Compare May 23, 2025 13:48
@alex-medinsh alex-medinsh marked this pull request as ready for review May 23, 2025 15:27
@alex-medinsh
Copy link
Contributor Author

I don't like that predicate is getting called twice on each route, but that can be fixed. What I am not sure about is whether it is safe to assume that the predicate result won't change between checking it as a next route vs as a current route?

@chunhtai chunhtai self-requested a review May 23, 2025 18:37
@alex-medinsh alex-medinsh force-pushed the popuntil-return-data branch from 893e698 to bb4e943 Compare May 28, 2025 19:57
@alex-medinsh
Copy link
Contributor Author

@chunhtai I have made the changes, PTAL.
It still has a problem of testing the route as next, but I am not sure if there even is a different way in this case.
This way does give more control to the developer, but I feel like the majority of cases (judging from the issue) is still going to be

Navigator.popUntil(
   context,
  (Route<dynamic> route) => route.isFirst,
  (Route<dynamic> route) => route.isFirst ? true : null,
);

to just return a value to the first route.

@chunhtai
Copy link
Contributor

I think the current approach should be fine, if they really want a special case, they can write a helper function. I want to avoid having conflict that people want a slightly different variant of your use case and we would need to end up adding a different popUntil API.

@alex-medinsh
Copy link
Contributor Author

I think you are right. I am worried that there can be a predicate that when tested as the next route will return a different result to when it was tested as the current route. Do you think this can be an issue?

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

ah I didn't realized the resultPredicate is call on the next route to pop the current route. This doesn't look right. I imagine the correct way would be calling the resultpredicate on the current route. That will of course need some refactoring to correctly capture the last popped route, if they gave the last popped route an appropriate name, it should be fine I think

@alex-medinsh alex-medinsh force-pushed the popuntil-return-data branch from bb4e943 to 9dcf0ff Compare June 5, 2025 09:34
@alex-medinsh
Copy link
Contributor Author

@chunhtai I have reverted the changes, please take a look.

Regarding your last question:

I believe that any predicate that for some reason checks route.isCurrent (since it will now be false for all but the first checked route) or any other predicate that only becomes true when this route is the top one will break because of these changes.

Maybe I am overthinking this. Do you think this is an issue?

@alex-medinsh alex-medinsh requested a review from chunhtai June 5, 2025 10:19
@alex-medinsh
Copy link
Contributor Author

I think I found an example of such a predicate:

testWidgets('Route localHistory - popUntil', (WidgetTester tester) async {

To make the test pass, I had to leave this check:

if (predicate(candidate.route)) {
return;
}

This works, but it now has an issue with checking each route twice. Once as the next route, and once as the current route. This happens because checking willHandlePopInternally before the pop returns false, so we pop, but then if we don't check the predicate the second time, we will pop an extra time.

@chunhtai
Copy link
Contributor

chunhtai commented Jun 5, 2025

I see now, I will take another look

@alex-medinsh alex-medinsh force-pushed the popuntil-return-data branch from c2449be to dac9f04 Compare June 11, 2025 06:35
@alex-medinsh alex-medinsh requested a review from chunhtai June 11, 2025 09:06
Copy link
Contributor

@chunhtai chunhtai 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 requested a review from justinmc June 12, 2025 22:43
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

See the comment about the bug in the test, otherwise looks good. Thanks for wiring this up!

Comment on lines +5636 to +5644
final _RouteEntry? next = _lastRouteEntryWhereOrNull(
(_RouteEntry e) => _RouteEntry.isPresentPredicate(e) && e != candidate,
);

if (next != null && !next.route.willHandlePopInternally && predicate(next.route)) {
pop<T>(result);
} else {
pop();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are legitimate Google test failures. Some calls to popUntil ended up popping one more route than they were expected to. In fact, I think they are popping the root route twice?

Any thoughts on why this could cause more popping than before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess would be that there are predicates that return different results when checked as next or as current. We discussed this case in this issue. Any chance you could make a reproduction I can try?

I feel like we might find some predicates that won't let us land this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not a big fan of how this PR works, this can have all kinds of breakages or side effects. If some folks use the predicate to, for example, count the number of popped routes by having an increment in the predicate, this will now increment more times than they expect.

But then again I am not sure if there is a different way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

That might be what's happening in the Google tests. I will try to create a repro for this on Monday.

@alex-medinsh alex-medinsh force-pushed the popuntil-return-data branch from eb234b2 to 007972f Compare June 14, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return data with popUntil
3 participants