Deep linking: automatically push the route hiearchy on load. #10894

Hixie · 2017-06-21T21:36:15Z

The main purpose of this PR is to make it so that when you set the
initial route and it's a hierarchical route (e.g. /a/b/c), it
implies multiple pushes, one for each step of the route (so in that
case, /, /a, /a/b, and /a/b/c, in that order). If any of those
routes don't exist, it falls back to '/'.

As part of doing that, I:

Changed the default for MaterialApp.initialRoute to honor the
actual initial route.
Added a MaterialApp.onUnknownRoute for handling bad routes.
Added a feature to flutter_driver that allows the host test script
and the device test app to communicate.
Added a test to make sure flutter drive --route works.
(Hopefully that will also prove flutter run --route works, though
this isn't testing the flutter tool's side of that. My main
concern is over whether the engine side works.)
Fixed flutter drive to output the right target file name.
Changed how the stocks app represents its data, so that we can
show a page for a stock before we know if it exists.
Made it possible to show a stock page that doesn't exist. It shows
a progress indicator if we're loading the data, or else shows a
message saying it doesn't exist.
Changed the pathing structure of routes in stocks to work more
sanely.
Made search in the stocks app actually work (before it only worked
if we happened to accidentally trigger a rebuild). Added a test.
Replaced some custom code in the stocks app with a BackButton.
Added a "color" feature to BackButton to support the stocks use case.
Spaced out the ErrorWidget text a bit more.
Added RouteSettings.copyWith, which I ended up not using.
Improved the error messages around routing.

While I was in some files I made a few formatting fixes, fixed some
code health issues, and also removed flaky: true from some devicelab
tests that have been stable for a while. Also added some documentation
here and there.

Hixie · 2017-06-21T21:37:49Z

Sorry this is such a giant patch.

cc @yjbanov for review of the flutter driver and devicelab parts
cc @HansMuller for review of everything else

Hixie · 2017-06-21T21:57:11Z

Fixes #10813
Fixes #618

yjbanov

LGTM w/ comments.

yjbanov · 2017-06-21T22:36:34Z

dev/devicelab/bin/tasks/routing_test.dart

There's a utility function in utils.dart (already imported) called section that you can use to print headings like this one.

yjbanov · 2017-06-21T22:41:34Z

dev/devicelab/bin/tasks/routing_test.dart

I'd say 2 minutes is optimistic but ok playing by ear.

removed timeout, i'll let cocoon deal with it

yjbanov · 2017-06-21T22:50:52Z

packages/flutter_driver/lib/src/driver.dart

This is 2mm away from being a custom request handler that takes a String and returns a String. 2 things would close the 2mm gap:

rename this method and DataHandler to something that implies "you can do anything in the DataHandler and return a result"

have DataHandler return Future<String> instead of String

I feel like we should do it now so we don't have to break API or introduce an almost identical API later because users want to do more than request data.

I've changed DataHandler to return a future string.

Any ideas what requestData, RequestData, DataHandler, and RequestDataResult should be called, if not those? I couldn't come up with anything more generic...

Ideas: RequestWithData, CustomRequest, SimpleRequest

I try to avoid words like Simple in APIs because if the API is simple, that should be obvious, and if it's not, the name will be a constant source of anger.

"customRequest()" doesn't really convey anything different than "requestData()" but is less verb-like, which is suboptimal for a method.

"requestWithData()" vs "requestData()" seems like a wash except the latter is shorter, so...

I dunno. I've left it as requestData() for now. I don't feel strongly about this though, feel free to change it.

yjbanov · 2017-06-21T22:52:29Z

packages/flutter_driver/lib/src/extension.dart

If this returned Future<String> it could be used for more use-cases (such as being able to wait for data to become available). You shouldn't lose anything in generality given that _requestData below and the network separation by its nature are already asynchronous.

yjbanov · 2017-06-21T22:54:19Z

packages/flutter_driver/lib/src/extension.dart

nit: += is not incorrect, but ++ is much more idiomatic in Dart code.

The problem with ++ is that in some situations, the postfix operator is less efficient than += 1. Since loops are often performance critical, people get all worried about this and end up writing ++moves. And the prefix operator is super ugly and horrible. So I'm on a mission to entirely kill the ++ and -- operators. :-)

yjbanov · 2017-06-21T22:59:11Z

packages/flutter_tools/lib/src/commands/drive.dart

What is this? The docs on addOption don't seem to explain.

Yeah I filed a bug on the docs being suboptimal.

It makes the help say that the syntax is --keep-app-running=<url>.

yjbanov · 2017-06-21T22:59:53Z

dev/integration_tests/ui/lib/main.dart

I think in flutter drive you still need the -t

As far as I can tell, this is not the case. I ran it with "flutter drive --route '/smuggle-it' lib/route.dart" and it worked.

collinjackson · 2017-06-21T23:14:18Z

Perhaps we should skip null intermediate route names rather than bailing out entirely.

We could make the routing behavior configurable via a delegate so that if developers don't like the default behavior for deep links, they could easily replace it with a different deep link handling algorithm. (This could be done in a separate CL, but should probably be addressed before make a lot of noise about this feature.)

lukef · 2017-06-22T16:04:26Z

I agree with @collinjackson. We may have a route like /users/:id and /users/create but no /users route so we wouldn't want the app to try to push/restore a /users route out of context or if it doesn't exist.

As a side, we don't structure our routes hierarchically like this. I'm not sure if that is the intention of routing in flutter or if this is just a convenience?

Also, it might be handy for navigator to have a push that allows pushing multiple routes / widgets at the same time (something like setViewControllers on the iOS NavigationController). That way you can decide where the pop will go to.

Hixie · 2017-06-22T16:25:04Z

@lukef Do you want the / route to be pushed when you start the app at /users/create? If so, would it work for you to name the route /users:create instead? Or is it important to you that it be named /users/create? (Just trying to understand the space here.)

lukef · 2017-06-22T16:32:36Z

Route names are generally of no consequence. It was more just an understanding that this is how you guys considered the routing to work. We didn't assume that if we have a /users/create route that it necessitated having a /users handler in between. If that's how Navigator works (or will) then its good to know.

re: root. I feel like there are times when you will want / to be restored every time and times when you don't want it. There are times where you might want a deep link to show an article and have to do the equivalent of FLAG_ACTIVITY_CLEAR_TOP or opening a new VC on iOS. To be clear, i'm not sure if this is in the scope of flutter.

The example I gave was around a news app displaying an article. If I get a push notification i might want to open the app and display the article. If the user hits close, then i want to just exit the app because the expectation is that the stack would only have the article screen/widget and nothing else. Other times I could see it would be desirable to have the option for pop to take me back to root. I would say it isn't something I'd expect flutter to make a choice for me on.

lukef · 2017-06-22T16:34:27Z

For iOS, see: https://developer.apple.com/library/content/documentation/General/Conceptual/AppSearch/UniversalLinks.html

Hixie · 2017-06-22T16:42:22Z

@lukef The current patch only applies this model if the strings starts with a /. Would that be sufficient control for you? (Basically, if you want to have the root route pushed, then the user-create route, you'd set the initialRoute to /users:create; if you wanted to just open an article, you'd set it to something like article:name-of-article-foo-bar, etc.)

Based on @collinjackson's idea, my long-term plan is to make this pluggable, so next to the routes table you could provide a route handler delegate that implements the behavior, and then you could just do whatever you want with the initialRoute (and presumably any other routes that get pushed later).

HansMuller · 2017-06-22T16:46:46Z

examples/stocks/lib/main.dart

A comment that explains what you're up to here would be helpful.

added lots of comments here.

lukef · 2017-06-22T16:48:56Z

That would work. It might be handy for me to show you our set up soon also because we're running a somewhat different setup (https://github.com/goposse/fluro) and that creates interesting side effects.

My personal wish would be a mechanism where we can just receive the incoming deep link info and deal with it ourselves manually if we wanted :)

HansMuller · 2017-06-22T16:50:18Z

examples/stocks/lib/stock_data.dart

Shouldn't we close the http client first?

HansMuller · 2017-06-22T16:55:43Z

examples/stocks/lib/stock_data.dart

Close the http client first? I assume that there's some finalization protocol that closes the connection at GC, but it seems at least tidier to just explicitly close it.

Hixie · 2017-06-22T17:01:01Z

My personal wish would be a mechanism where we can just receive the incoming deep link info and deal with it ourselves manually if we wanted :)

You can do that with this patch -- if you set initialRoute on the MaterialApp, it'll ignore the incoming initial route entirely and just do what you want.

lukef · 2017-06-22T17:03:27Z

That's what we are doing now. I set initial route to null and then I use onGenerateRoute to determine where to go. We're just seeing side effects from that approach.

HansMuller · 2017-06-22T17:04:07Z

examples/stocks/lib/stock_home.dart

Maybe a comment or an issue that explains what's needed to complete this?

HansMuller · 2017-06-22T17:07:13Z

examples/stocks/lib/stock_symbol_viewer.dart

We should probably use the NumberFormat class here, just to demo how to format prices.

I'm going to leave this for now and we can revisit it when we do i18n.

HansMuller · 2017-06-22T17:08:54Z

examples/stocks/lib/stock_symbol_viewer.dart

This is pretty slick!

HansMuller · 2017-06-22T17:12:11Z

examples/stocks/lib/stock_symbol_viewer.dart

Another way to format this, to make the if then else structure a little clearer:

secondChild: stock != null ? new _StockSymbolView( stock: stock, arrow: new Hero( tag: stock, child: new StockArrow(percentChange: stock.percentChange), ), ), : new Padding( padding: const EdgeInsets.all(20.0), child: new Center(child: new Text('$symbol not found')), ), ),

HansMuller · 2017-06-22T17:29:13Z

examples/stocks/test/search_test.dart

Maybe explain why you're waiting a minute at a time? I would have thought that a no-params pumpAndSettle() call would have been OK.

HansMuller · 2017-06-22T17:30:17Z