Revise iOS add-to-app usage page #3309

xster · 2019-11-27T03:10:52Z

Staging on https://add-to-app-2019.firebaseapp.com/docs/development/add-to-app/ios/add-flutter-screen

xster · 2019-11-27T03:15:36Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+demonstrate creating a `FlutterEngine`, exposed as a property, on app startup in
+the app delegate.
+
+<ul class="nav nav-tabs" id="engine-language" role="tablist">


@matthew-carroll see staging at https://add-to-app-2019.firebaseapp.com/docs/development/add-to-app/ios/add-flutter-screen.

This seems like a reasonable way of toggling languages for instructions in a way consistent with the aesthetics of the rest of the website.

dnfield · 2019-11-27T05:28:26Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+It's generally recommended pre-warm a long-lived `FlutterEngine` for your
+application. This way,
+
+- Flutter screens will render their first frames faster.


nit: Flutter will start the work to get to its first frame earlier.

Not sure how much the precision matters here, but having no VC doesn't actually speed up frame time.

VC doesn't actually speed up frame time

Not sure I understood the comment about having no VCs. I rephrased it a little bit nevertheless.

dnfield · 2019-11-27T05:29:06Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+application. This way,
+
+- Flutter screens will render their first frames faster.
+- Your Flutter and Dart state could outlive one `FlutterViewController`.


I'm always a bit thrown by what Flutter means in this context. Do we just mean Flutter-specific application state?

"could" or "will"?

@dnfield I'm actually just kinda thinking anything you can conceptually assign to the term 'Flutter' and that has a state. Can you think of counter examples?

Replaced could with will

Is Flutter here the Flutter framework? The Flutter engine excluding the embedding? The embedding? Plugin code (dart/platform)?

Doesn't the sentence apply the same way for all those definitions?

Tangent: I don't think the word embedding means anything for people outside the team and I'm tempted to keep it so for as long as we can since the distinction isn't necessarily useful unless you're trying to contribute to the engine code.

I guess a way of looking at this is: here you use Flutter in a way that excludesFlutterViewController. Later in the document you're using Flutter in a way that explicitly means showing the FlutterViewController.

Isn't it a bit like having the sentences:

Having a backup of your movie will make it outlive one VHS tape

'showMovie'

and then asking whether readers will be struggling understanding whether 'movie' here encompasses the mylar tape ribbons or not :)

It's likely immaterial whether they assume one way or another.

dnfield · 2019-11-27T05:34:16Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

-  self.flutterEngine = [[FlutterEngine alloc] initWithName:@"io.flutter" project:nil];
-  [self.flutterEngine runWithEntrypoint:nil];
+  self.flutterEngine = [[FlutterEngine alloc] initWithName:@"my flutter engine"];
+  // Runs the default Dart entrypoint with a default Flutter route.


I think we should avoid mentioning the default Flutter route here. If anything, we should outline how to use platform channels to communicate with the Dart side to set up any non-default state.

We do have a default flutter route on the flutter side https://api.flutter.dev/flutter/widgets/Navigator/defaultRouteName-constant.html

It exists but it causes much confusion and frequently doesn't work as expected

Which part specifically are you commenting on?

The fact that there's a cascade going on in https://api.flutter.dev/flutter/material/MaterialApp-class.html and the top level navigator behavior? It's indeed confusing but it's just how Flutter works at the moment. I don't think it should ever not work as expected. This is a mostly orthogonal concept to the timing of calls to setInitialRoute.

It's confusing becuase you can only set it at one point in the lifecycle of starting the engine, and you lose access to that completely if you set it up in certain ways, and if you call it too late it just silently fails, and there's no good way to fix any of those problems with the API so if you open a bug about it you'll be told you should be doing it differently anyway.

Ya, the mechanism for changing the default route is confusing as mentioned below. But it doesn't change the fact that there is a default route when you don't change it.

dnfield · 2019-11-27T05:34:28Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

  override func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
-    self.flutterEngine = FlutterEngine(name: "io.flutter", project: nil);
-    self.flutterEngine?.run(withEntrypoint: nil);
+    // Runs the default Dart entrypoint with a default Flutter route.


dnfield · 2019-11-27T05:35:05Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+- (void)viewDidLoad {
+    [super viewDidLoad];
+
+    // Make a button to call the showFlutter function below when pressed.


nit: maybe showFlutterViewController?

I don't think it adds much. Which misunderstanding are you trying to avoid for the reader?

what Flutter is.

Which user-self-assigned definition of the term Flutter would be problematic?

dnfield · 2019-11-27T05:35:43Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+
+    // Make a button to call the showFlutter function below when pressed.
+    let button = UIButton(type:UIButton.ButtonType.custom)
+    button.addTarget(self, action: #selector(showFlutter), for: .touchUpInside)


dnfield · 2019-11-27T05:36:59Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+sometimes useful if the Flutter screen is rarely shown and there are no good
+heuristics to determine when the Dart VM should be started.


if the Flutter screen is rarely shown and does not need to persist state between apperances?

added into an enumeration

dnfield · 2019-11-27T05:37:46Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+- Forwarding status bar taps (which can only be detected in the AppDelegate) to
+  Flutter for scroll-to-top behavior.
+
+If your app delegate cannot directly subclass `FlutterAppDelegate`, you can


s/can/must right?

changed to should. Must is a bit strong since it's not like the app won't run anymore without it. We can point out the reasoning and let them decide.

dnfield · 2019-11-27T05:40:05Z

src/docs/development/add-to-app/ios/add-flutter-screen.md


-## Building and running your app
+An initial route can be set for your Flutter [`WidgetsApp`]({{site.api}}/flutter/widgets/WidgetsApp-class.html)
+when constructing the engine.


If you choose to do this, you _must_ do so before running the engine, and any calls after running the engine will be ignored.

IMO, we should avoid documenting this API, deprecate it, and remove it. It's the source of a lot of confusion and it has no clear fix.

Added a warning box.

Actually there is already a warning box.

dnfield · 2019-11-27T05:42:48Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+In order to change your Flutter route after the `FlutterEngine` is already
+running, use [pushRoute]({{site.api}}/objcdoc/Classes/FlutterViewController.html#/c:objc(cs)FlutterViewController(im)pushRoute:)
+or [popRoute]({{site.api}}/objcdoc/Classes/FlutterViewController.html#/c:objc(cs)FlutterViewController(im)popRoute)
+on the `FlutterViewController`.


This is markedly different from setting an initial route, since it mutates the route stack, whereas setInitialRoute initializes the route stack to a specific spot.

I wasn't suggesting that they are equivalent. It's in a tangential tip box.

Added a few more words to clarify difference.

dnfield · 2019-11-27T05:45:02Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+The above only illustrates a few examples of ways to customize how a Flutter
+instance is initiated. Using [platform channels](/docs/development/platform-integration/platform-channels),
+you're free to push data or prepare your Flutter environment in any way you'd
+like before presenting the Flutter UI via a `FlutterViewController`.


IME, the routing logic for add-to-app is pretty limited and does unexpected things. I think it would be better to document how to use platform channels in this way here, and then we could mention that you can use any of the framework-provided channels as well (such as the navigation channel).

In other words, I think a user is far more likely to want to tell the running Dart program what state it should initialize to before presenting, and that it's very unlikely that the right way to do that will be setting an initial route (that only works at a specific point in execution) or pushing/popping routes blindly from the background.

Document platform channels in this way here -> document how to use a custom platform channel to communicate with Dart

I generally agree with you and using a custom channel to push an application specific DI-like bundle is what I would do too.

Though on the flip side, there's a bit of a personal taste involved in the trade off in the sense that writing platform channels is extremely boring and tedious (and error-prone in its own right as well) currently until we do flutter/flutter#32930.

Routes is also a non-trivially important concept in Flutter and we're not necessarily going to be avoiding pains for developers if we avoided this whole topic in documentations.

TL;DR, I think all present solutions have annoyances and we should properly handle this very common initialization use case and present a proper best practice with infrastructure in the coming phases, but we should have some docs in the meantime.

Added a bit more caveating for more transparency in the docs in the meantime.

oh look there's flutter/flutter#34170 already

SG. I can understand including this - my comment above sketches out a bit more of why (I think users will read this, get it wrong, think it's a bug, and be frustrated when we tell them it's by design). But maybe that's what we have right now.

ya, I hear you. There are tons of bugs around this already and I don't think routes are generally useful APIs, but I don't think we can move the weight of the community right now and we might get more confusion if the code is in the repo but we avoided talking about it.

sfshaza2

Won't merge yet but LGTM.

jmagman · 2019-11-27T18:04:25Z

src/docs/development/add-to-app/ios/add-flutter-screen.md


-```objective-c
+{{site.alert.tip}}
+It's generally recommended pre-warm a long-lived `FlutterEngine` for your


missing "to": "recommended to pre-warm"

jmagman · 2019-11-27T18:04:53Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+application. This way,
+
+- Flutter screens will render their first frames faster.
+- Your Flutter and Dart state could outlive one `FlutterViewController`.


"could" or "will"?

jmagman · 2019-11-27T18:05:06Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+
+- Flutter screens will render their first frames faster.
+- Your Flutter and Dart state could outlive one `FlutterViewController`.
+- You and your plugins can interact with Flutter and your Dart logic before


"Your application and plugins"?

sg, changed

jmagman · 2019-11-27T18:10:00Z

src/docs/development/add-to-app/ios/add-flutter-screen.md


 #import "AppDelegate.h"

 @implementation AppDelegate

-// This override can be omitted if you do not have any Flutter Plugins.
 - (BOOL)application:(UIApplication *)application
    didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {


I know you didn't edit this, but it should be NSDictionary<UIApplicationLaunchOptionsKey, id> *)

jmagman · 2019-11-27T18:12:58Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

-  var flutterEngine : FlutterEngine?;
-  // Only if you have Flutter plugins.
+class AppDelegate: FlutterAppDelegate { // More on the FlutterAppDelegate below.
+  lazy var flutterEngine = FlutterEngine(name: "my flutter engine")


Do we have engine naming conventions?

looking into it a bit, this only adds a debug label for the thread in instruments. I want to get rid of it so I'll not over-emphasize it here for now

jmagman · 2019-11-27T18:14:45Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+    UIButton *button = [UIButton buttonWithType:UIButtonTypeCustom];
+    [button addTarget:self
+               action:@selector(showFlutter)
+     forControlEvents:UIControlEventTouchUpInside];


I think this is missing a few tabs in the front.

this is just lining up the semi-columns. Did I understand correctly?

jmagman · 2019-11-27T18:16:32Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+               action:@selector(showFlutter)
+     forControlEvents:UIControlEventTouchUpInside];
+    [button setTitle:@"Show Flutter!" forState:UIControlStateNormal];
+    [button setBackgroundColor:[UIColor blueColor]];


I would love to start "modernizing" (~2012) at least our sample code with dot notation.

button.backgroundColor = UIColor.blueColor;

square brackets forever!

jmagman · 2019-11-27T18:19:40Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+
+- (void)showFlutter {
+    FlutterEngine *flutterEngine =
+        [(AppDelegate *)[[UIApplication sharedApplication] delegate] flutterEngine];


UIApplication.sharedApplication.delegate.flutterEngine

<I didn't try it yet> don't we need to keep the cast to AppDelegate? </>

Oh yeah ((AppDelegate *)UIApplication.sharedApplication.delegate).flutterEngine

jmagman · 2019-11-27T18:22:31Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+<div class="tab-pane active" id="vc-objc-tab" role="tabpanel" aria-labelledby="vc-objc-tab" markdown="1">
+
+<?code-excerpt "ViewController.m" title?>
+```objectivec


I thought the markdown language marker was objective-c?

I'm not sure which syntax highlighter library we're using for the website. objective-c and objc don't work.

jmagman · 2019-11-27T19:03:33Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

 ```

- Swift:
+or


Should the two languages be in tabs?

lol you caught me. I got tired of writing all that html boilerplate

jmagman · 2019-11-27T19:42:20Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

@@ -346,7 +622,7 @@ Connected views:

 Attach to specific isolates instead in two steps:

-1. Name the Flutter root isolate of interest in its Dart source.
+1- Name the Flutter root isolate of interest in its Dart source.


Is 1- the normal markdown way to number steps on the website?

It's not, but the code highlighter makes the markdown parser crap out (it would have the wrong indent with the code block anyway)

jmagman · 2019-11-27T20:54:25Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

@@ -358,7 +637,7 @@ void main() {
 }
 ```

-2. Run `flutter attach` with the `--isolate-filter` option.
+2- Run `flutter attach` with the `--isolate-filter` option.

 ```bash


I think this is supposed to be ```terminal

oh nice, it looks way nicer, thanks

jmagman · 2019-11-27T20:56:03Z

src/docs/development/add-to-app/ios/add-flutter-screen.md

+
+- (void)showFlutter {
+    FlutterEngine *flutterEngine =
+        [(AppDelegate *)[[UIApplication sharedApplication] delegate] flutterEngine];


Oh yeah ((AppDelegate *)UIApplication.sharedApplication.delegate).flutterEngine

xster added 3 commits November 25, 2019 18:09

review iOS usage instructions

c055681

add an ios usage page

24cb7fd

one more note on routes

6a843b0

googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Nov 27, 2019

xster requested review from sfshaza2, jmagman and dnfield November 27, 2019 03:10

xster commented Nov 27, 2019

View reviewed changes

dnfield reviewed Nov 27, 2019

View reviewed changes

sfshaza2 approved these changes Nov 27, 2019

View reviewed changes

jmagman reviewed Nov 27, 2019

View reviewed changes

partial review

c0e93d7

jmagman reviewed Nov 27, 2019

View reviewed changes

review

495d1a1

dnfield approved these changes Nov 28, 2019

View reviewed changes

xster merged commit 8768f66 into flutter:staging-add-to-app-do-not-delete-until-2020 Nov 28, 2019

xster deleted the ios-usage branch November 28, 2019 01:42

		sometimes useful if the Flutter screen is rarely shown and there are no good
		heuristics to determine when the Dart VM should be started.

               ```
-              - Swift:
+              or

Revise iOS add-to-app usage page #3309

Revise iOS add-to-app usage page #3309

Conversation

xster commented Nov 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster Nov 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfshaza2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster commented Nov 27, 2019 •

edited

xster Nov 28, 2019 •

edited