-
Notifications
You must be signed in to change notification settings - Fork 28.7k
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
base: master
Are you sure you want to change the base?
Conversation
5d2a3c3
to
2577501
Compare
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? |
893e698
to
bb4e943
Compare
@chunhtai I have made the changes, PTAL. Navigator.popUntil(
context,
(Route<dynamic> route) => route.isFirst,
(Route<dynamic> route) => route.isFirst ? true : null,
); to just return a value to the first route. |
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. |
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? |
There was a problem hiding this 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
bb4e943
to
9dcf0ff
Compare
@chunhtai I have reverted the changes, please take a look. Regarding your last question: I believe that any predicate that for some reason checks Maybe I am overthinking this. Do you think this is an issue? |
I think I found an example of such a predicate:
To make the test pass, I had to leave this check: flutter/packages/flutter/lib/src/widgets/navigator.dart Lines 5632 to 5634 in c2449be
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 |
I see now, I will take another look |
c2449be
to
dac9f04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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!
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(); | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
eb234b2
to
007972f
Compare
This PR adds a
result
param toNavigator.popUntil
that allows to pass a value to the lastpop
call.Fixes #30112
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.