Fix Navigator.pop for named routes. #11289
Conversation
Pulling from main fork.
Pulling to make workspace up to date wrt master.
Bringing master up to date wrt workspace
Merging changes from flutter/flutter
Updating my flutter fork to master.
Pulling from flutter/flutter
| final String name; | ||
| final BuildContext context; | ||
|
|
||
| const PopWidget(this.context, this.name, this.value); |
There was a problem hiding this comment.
nit: constructor always comes first
| final String name; | ||
| final BuildContext context; | ||
|
|
||
| const PopWidget(this.context, this.name, this.value); |
There was a problem hiding this comment.
passing a context to a widget is pretty crazy. Maybe instead of a PopWidget, use a Builder?
There was a problem hiding this comment.
Yeah, I thought it was pretty crazy too, I was just looking for a way to delay the pop until after building was done. Thanks for the suggestion: it's much simpler now.
| <excludeFolder url="file://$MODULE_DIR$/packages" /> | ||
| <excludeFolder url="file://$MODULE_DIR$/test/calculator/packages" /> | ||
| <excludeFolder url="file://$MODULE_DIR$/test/packages" /> | ||
| <excludeFolder url="file://$MODULE_DIR$/tool/packages" /> |
packages/flutter/flutter.iml
Outdated
| @@ -1,5 +1,5 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <module type="WEB_MODULE" version="4"> | |||
| <module type="FLUTTER_MODULE_TYPE" version="4"> | |||
|
It's a pity that there's no way to dynamically fill the type there, but: |
|
Well, but where would it get the type from?: we don't know the type until pop is called, so it kind of has to be dynamic. Anyhow, thanks for the review! |
|
The route knows the type when it's created. A language with good RTTI could let us do it. But yeah, not in Dart. :-) |
|
But for named routes, there is no type that the user specifies, since they just supply a builder, and we create the MaterialPageRoute for them. The pop is what determines the type for named routes. For other routes, it'll fail when calling didPop if the result is the wrong type. Hey, what's the deal with Travis? Seems like most of my runs for this PR have been failing through no fault of my code... |
|
The pop doesn't determine the type. The type is there to ensure the pop is providing the right type. The type is decided by the person who defined the route. For example, a page where you edit a contact might be defined as returning a Not sure what's up with Travis. |
|
Did you mean to add the |
|
Dammmit. No. I was trying to bring my branch up to current flutter/master. I went through my local master, which must have had that in it still. From now on, I'm nuking my local repos before each change. :-) Removed. |
|
Yes, the |
|
Yeah. Like I said, it's a pity. :-) Not much I can think to do about it. Anyway, all is green, so I've landed your patch. Thanks! |
3 participants
This fixes a problem with Navigator.pop where the routes were created with MaterialPageRoute<Null>, which breaks any call to Navigator.pop with an argument other than Null.