Added the ability to set the initial route via launch urls. #21336

gaaclarke · 2020-09-22T21:23:11Z

Description

Started setting the intiailRoute and entryPoint via launch urls if no plugin handles the URL.

Related Issues

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A
change in behaviour with no test covering it will likely get reverted
accidentally sooner or later. PRs must include tests for all
changed/updated/fixed behaviors. See testing the engine for instructions on
writing and running engine tests.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-10-01T23:49:59Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke · 2020-10-21T20:06:49Z

ping @goderbauer @chunhtai

chunhtai

Change looks good. just need a test

chunhtai · 2020-10-22T23:31:40Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

 #import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterAppDelegate.h"

-#include "flutter/fml/logging.h"
+#import "flutter/fml/logging.h"


How do we decide whether we use import vs include? I originally thought we always include c++ file and import objective c file

If the file is objective-c or objective-c++ just always use import.

chunhtai · 2020-10-23T19:04:39Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

+                   }];
+      return YES;
+    } else {
+      FML_LOG(ERROR) << "Attempting to open an URL without a Flutter RootViewController.";


In what situation this can happen?

It's unlikely but if you edit the storyboard for the iOS project this could happen. Also, if you were doing add to app and you call into a FlutterAppDelegate it could be a problem.

gaaclarke · 2020-10-23T22:54:18Z

@chunhtai tests added, thanks for pushing on that.

chunhtai · 2020-10-23T23:12:15Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

-  if ([viewController isKindOfClass:[FlutterViewController class]]) {
-    return (FlutterViewController*)viewController;
+- (FlutterViewController*)rootFlutterViewController {
+  if (_rootFlutterViewControllerGetter != nil) {


Is this only used in test? It seems weird that we inject the getter this way, is it possible for the test to override this class and override this method to inject the getter there?

I could, but it's the class I'm testing. In order to do that I'd have to make a partial mock of the class I actually want to test which is weird, too.

I could do more traditional dependency injection and pass in an object that can get the root FlutterViewController in with the FlutterAppDelegate's initializer. This is basically the same thing but with a setter. I wanted to introduce the least amount of overhead possible, that's why it's a pointer check that will basically be ignored with branch prediction. If I always passed the logic through a block or a protocol we would always incur a dynamic dispatch.

chunhtai · 2020-10-23T23:13:05Z

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm


 - (void)dealloc {
  [_lifeCycleDelegate release];
+  [_rootFlutterViewControllerGetter release];


This class does not create the getter, it seems weird that this class is responsible for destroying it.

It actually does create it in its declaration of the property. This is related to your other comment, too. The file FlutterAppDelegate_Test is a thing called a category which allows you to extend classes or make methods selectively visible. You can think of this as declaring that property as @VisibleForTesting.

gaaclarke · 2020-10-24T00:36:45Z

@chunhtai let me know what you want to do. You said "looks good" for the code but had some questions on the tests. I just want to make sure you get your concerns addressed before landing this.

chunhtai

LGTM

…lutter/engine#21336)

…lutter#21336)" This reverts commit 25bb31e.

…21336)" (#22337)

…lutter#21336)

…21336)

…lutter#21336)" (flutter#22337)

flutter-dashboard bot added the platform-ios label Sep 22, 2020

googlebot added the cla: yes label Sep 22, 2020

gaaclarke force-pushed the initial-route-launch-url branch from 83cd04a to 3a2ab05 Compare September 22, 2020 22:11

gaaclarke force-pushed the initial-route-launch-url branch 3 times, most recently from f800400 to 4287dd0 Compare October 1, 2020 23:48

gaaclarke marked this pull request as ready for review October 1, 2020 23:49

gaaclarke requested review from chunhtai and goderbauer October 1, 2020 23:50

chunhtai reviewed Oct 23, 2020

View reviewed changes

gaaclarke added 7 commits October 23, 2020 16:48

Started passing URLs as routes down to the Framework.

fad38fd

made sure to return NO if we have no flutterviewcontroller

1fa2fd2

added test for wait for first frame

e4d598c

added test for launching urls

51e56d6

added test for openURL

5441cec

added missing license entries

fc2023b

fixed licenses file

43e792f

gaaclarke force-pushed the initial-route-launch-url branch from c276c88 to 43e792f Compare October 23, 2020 23:48

chunhtai approved these changes Oct 26, 2020

View reviewed changes

gaaclarke merged commit 25bb31e into flutter:master Oct 26, 2020

engine-flutter-autoroll mentioned this pull request Oct 26, 2020

Roll Engine from 1857470267f1 to 828b5ce3224f (5 revisions) flutter/flutter#69049

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 26, 2020

25bb31e Added the ability to set the initial route via launch urls. (f…

9071264

…lutter/engine#21336)

engine-flutter-autoroll mentioned this pull request Oct 27, 2020

Roll Engine from 1857470267f1 to 828b5ce3224f (5 revisions) flutter/flutter#69075

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2020

25bb31e Added the ability to set the initial route via launch urls. (f…

cbd85f5

…lutter/engine#21336)

engine-flutter-autoroll mentioned this pull request Oct 27, 2020

Roll Engine from 1857470267f1 to 26a0f88b5502 (10 revisions) flutter/flutter#69081

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2020

25bb31e Added the ability to set the initial route via launch urls. (f…

ca2a721

…lutter/engine#21336)

engine-flutter-autoroll mentioned this pull request Oct 27, 2020

Roll Engine from 1857470267f1 to 64e659991025 (11 revisions) flutter/flutter#69085

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2020

25bb31e Added the ability to set the initial route via launch urls. (f…

a8b7dac

…lutter/engine#21336)

chunhtai added a commit to chunhtai/engine that referenced this pull request Nov 5, 2020

Revert "Added the ability to set the initial route via launch urls. (f…

b5b3407

…lutter#21336)" This reverts commit 25bb31e.

chunhtai mentioned this pull request Nov 5, 2020

Revert "Added the ability to set the initial route via launch urls. (… #22337

Merged

13 tasks

fluttergithubbot pushed a commit that referenced this pull request Nov 6, 2020

Revert "Added the ability to set the initial route via launch urls. (#…

acaef6f

…21336)" (#22337)

xster mentioned this pull request Nov 6, 2020

Ability to turn off Android deeplink flutter/flutter#69786

Closed

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Nov 6, 2020

Reland Added the ability to set the initial route via launch urls. (f…

28154a9

…lutter#21336)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Added the ability to set the initial route via launch urls. (flutter#…

696a5cb

…21336)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Revert "Added the ability to set the initial route via launch urls. (f…

bb42aa3

…lutter#21336)" (flutter#22337)

Added the ability to set the initial route via launch urls. #21336

Added the ability to set the initial route via launch urls. #21336

Uh oh!

Conversation

gaaclarke commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Oct 1, 2020

Uh oh!

gaaclarke commented Oct 21, 2020

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Oct 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Oct 24, 2020

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented Sep 22, 2020 •

edited

Loading