Add Navigator.pushReplacement() and Navigator.pushReplacementNamed() #7611
Conversation
| void install(OverlayEntry insertionPoint) { } | ||
|
|
||
| /// Called after install() when the route is pushed onto the navigator. | ||
| /// Called after install() when the route is pushed onto the navigator. The |
There was a problem hiding this comment.
I'd add a paragraph break after the first sentence.
The first line of the dartdoc becomes the summary for the method that is displayed on the class page on the web site.
| return navigator.pushNamed(routeName); | ||
| } | ||
|
|
||
| static Future<dynamic> pushReplacementNamed(BuildContext context, String routeName, { dynamic result }) { |
| assert(newRoute.overlayEntries.isEmpty); | ||
|
|
||
| setState(() { | ||
| final int index = _history.indexOf(oldRoute); |
There was a problem hiding this comment.
This seems unnecessarily expensive. We know the old route is the last one in the history, so we can compute this index from the length. We can assert that we got it right with indexOf if you like.
There was a problem hiding this comment.
You're right. The indexOf is now just part of an assert as you suggested.
|
|
||
| setState(() { | ||
| final int index = _history.indexOf(oldRoute); | ||
| assert(index >= 0); |
There was a problem hiding this comment.
On line 779, you handle the empty history case but here you assert that we have an old route. We should be consistent about whether we handle that case.
There was a problem hiding this comment.
That's my mistake on line 779, thanks for catching that.
| newRoute.install(_currentOverlayEntry); | ||
| _history[index] = newRoute; | ||
| newRoute.didPush().then<dynamic>((Null _) { | ||
| // The old route's exit is not animated. We're assuming that the |
There was a problem hiding this comment.
We need to re-check mounted when this future resolves. Otherwise, we can end up disposing oldRoute twice (once when we unmount and once when the future resolves).
| // The old route's exit is not animated. We're assuming that the | ||
| // new route completely obscures the old one. | ||
| oldRoute | ||
| .._popCompleter.complete(result ?? oldRoute.currentResult) |
There was a problem hiding this comment.
We should have docs somewhere that explain all these lifecycle callbacks. It's a bit surprising that we can complete this future without calling didPop.
There was a problem hiding this comment.
I think that the names of the Route lifecycle callback methods and their roles warrant a little thought.
I've filed an API stability issue: #7617
| assert(config.onUnknownRoute != null); | ||
| newRoute = config.onUnknownRoute(settings); | ||
| assert(newRoute != null); | ||
| } |
There was a problem hiding this comment.
A bunch of this code is repeated with pushNamed. Maybe break out into a common subroutine?
There was a problem hiding this comment.
Good idea. I added _routeNamed(String name)
|
|
3 participants
Replace the current route by pushing a new route and disposing the old one.
Route.didPush() now returns a Future that completes when the push operation completes.
Fixes: #7365
Fixes: #6544