Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[deep link][ios] Update openURL method to reflect the result from framework #52643

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
+98 −43
Open

[deep link][ios] Update openURL method to reflect the result from framework #52643

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c762720
Update FlutterAppDelegate.mm
hangyujin May 7, 2024
b8d8083
Update FlutterAppDelegate.mm
hangyujin May 8, 2024
bbbb148
remove completion
hangyujin May 9, 2024
50dbd75
lint
hangyujin May 9, 2024
4fec71f
Update FlutterAppDelegate.mm
hangyujin May 9, 2024
6af454a
Update FlutterAppDelegate.mm
hangyujin May 9, 2024
9b28f1e
Update FlutterAppDelegate.mm
hangyujin May 9, 2024
1dcf138
Update FlutterAppDelegateTest.mm
hangyujin May 9, 2024
ca5e4d8
Update FlutterAppDelegateTest.mm
hangyujin May 9, 2024
7d00b0e
Update FlutterAppDelegateTest.mm
hangyujin May 9, 2024
62b38db
Update FlutterAppDelegate.mm
hangyujin May 9, 2024
2c38727
Update FlutterAppDelegateTest.mm
hangyujin May 9, 2024
b63243b
Update FlutterAppDelegateTest.mm
hangyujin May 9, 2024
308fdac
resolve comments
hangyujin May 14, 2024
17949bf
Update FlutterAppDelegate.mm
hangyujin May 14, 2024
4ec56ec
Update FlutterAppDelegate.mm
hangyujin May 14, 2024
77c56fe
Update FlutterAppDelegateTest.mm
hangyujin May 14, 2024
7fa8d0c
runloop
hangyujin May 14, 2024
2befe86
Update FlutterAppDelegate.mm
hangyujin May 16, 2024
8c595b7
Update FlutterAppDelegate.mm
hangyujin May 16, 2024
785fbc8
Update FlutterAppDelegate.mm
hangyujin May 17, 2024
1a6703e
throw url back to ios
hangyujin Jun 18, 2024
c9a38bd
format
hangyujin Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 40 additions & 22 deletions shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,33 +134,25 @@ - (void)userNotificationCenter:(UNUserNotificationCenter*)center
}
}

- (BOOL)openURL:(NSURL*)url {
- (BOOL)isFlutterDeepLinkingEnabled {
NSNumber* isDeepLinkingEnabled =
[[NSBundle mainBundle] objectForInfoDictionaryKey:@"FlutterDeepLinkingEnabled"];
if (!isDeepLinkingEnabled.boolValue) {
// Not set or NO.
return NO;
// if not set, return NO
return isDeepLinkingEnabled ? [isDeepLinkingEnabled boolValue] : NO;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this can be just isDeepLinkingEnabled.boolValue since sending message to nil will return NO.

Copy link
Contributor Author

@hangyujin hangyujin May 16, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep this and flip NO to YES in #52350

}

- (void)openURL:(NSURL*)url
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow up from meeting - can delete this.

options:(NSDictionary<UIApplicationOpenExternalURLOptionsKey, id>*)options
completionHandler:(void (^)(BOOL success))completion {
if (![self isFlutterDeepLinkingEnabled]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even this is No, we still want to go through plugins, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we want to do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for those who use uni-link or their own plugin to handle deeplink, they would set this to no, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes they would set this to no.
but for those using uni-link and didn't set isFlutterDeepLinkingEnabled flag, the current behavior is already return NO for these methods and don't invoke the framework method. This PR is not changing their experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, I misread the code, looks like the plugin would have been called already before reaching here.

completion(NO);
} else {
FlutterViewController* flutterViewController = [self rootFlutterViewController];
if (flutterViewController) {
[flutterViewController.engine
waitForFirstFrame:3.0
callback:^(BOOL didTimeout) {
if (didTimeout) {
FML_LOG(ERROR)
<< "Timeout waiting for the first frame when launching an URL.";
} else {
[flutterViewController.engine.navigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{
@"location" : url.absoluteString ?: [NSNull null],
}];
}
}];
return YES;
[flutterViewController openDeepLink:url completionHandler:completion];
} else {
FML_LOG(ERROR) << "Attempting to open an URL without a Flutter RootViewController.";
return NO;
completion(NO);
}
}
}
Expand All @@ -171,7 +163,20 @@ - (BOOL)application:(UIApplication*)application
if ([_lifeCycleDelegate application:application openURL:url options:options]) {
return YES;
}
return [self openURL:url];

__block BOOL openURLSuccess = NO; // Flag to track success
hangyujin marked this conversation as resolved.
Show resolved Hide resolved
// Use a dispatch semaphore to wait for the completion handler
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You won't want a dispatch_semaphore on the main thread, that will cause hangs and potentially deadlocks.

The run loop is a processing loop for event handling, and there's one assigned to the main thread. I believe what you want is to kick off the request, and spin the main run loop until it completes, then return the value from this method.

I didn't run this code so you should test it, but something like:

  __block BOOL openURLSuccess = NO;
  __block BOOL openURLCompleted = NO;
  [self openURL:url
                options:options
      completionHandler:^(BOOL success) {
        openURLSuccess = success;
        openURLCompleted = YES;
      }];

  while (!openURLCompleted) {
    [NSRunLoop.currentRunLoop runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.1]];
  }
  return openURLSuccess;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for educating me about the The run loop

I have some more questions :

  1. is a while loop a form of busy waiting and will it cause some performance issues? Should i also add a timeout to prevent an infinite loop?
  2. Looks like the async api [openURL:options:completionHandler:] is newer than the non-async api [application:openURL:options:] , is it possible the async one is replacing the non-async one? if so, it may be ok the async api returns the value from the framework but the old non-async api remains the old behavior to return the flag value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a while loop a form of busy waiting and will it cause some performance issues? Should i also add a timeout to prevent an infinite loop?

Are you concerned the code you're calling didTimeout logic won't work? You could pass in that timeout into the view controller method so it's obvious it handles timing out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also a bit concerned " navigationChannel invokeMethod:arguments:result: " will time out 😅 Is it possible?

Copy link
Member

@jmagman jmagman May 16, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add a timeout instead of the while loop, that would work too. And return NO if it times out.
There are a lot of timeouts going on here, it seems like it would be bad if it times out but the pushRouteInformation request has gone through, but not yet returned?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanna update my progress so far. So for the deadlock, I wasn't able to find anything useful online, but I did come up with a theory - the continueUserActivity runs on main queue (hence also the main thread); the viewDidLoad runs on main thread, but NOT the main queue. This can explain why it works in viewDidLoad, but deadlock in continueUserActivity.

If my theory is correct, I don't think there's a way to synchronize the async API. I am thinking about a workaround (which I am still hashing out). Basically we want to return YES synchronously (marking as "handled"), then call the async API. If the async API comes back as failure, then call UIAppilcation::openURL which should throw the url back to iOS. Something like:

- (BOOL) continueUserActivity {
  if vc not present { return NO; }
  runAsyncAPI(completion: (success) { 
    if (!success) { 
      // throw it back to iOS
      [UIApplication.sharedApplication openURL: url]
    }
  })
  return YES;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UIAppilcation::openURL which should throw the url back to iOS >> what will happen then? will it open the url in the browser?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hangyujin My understanding is that, it should open in the next best candidate app that registers the domain, and browser is the catch-all default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still playing around with the deep link API, but from my personal experience with Amazon:

Whenever i get an amazon package, I receive a link through text that starts with https://a.co/d/WHATEVER_CODE. When I click on it, it launches my Amazon app (which means amazon app registers https://a.co), and then after a few seconds, it redirect to Safari that displays the bar code.

This makes me think that Amazon app probably returned YES in continueUserActivity, asynchronously run some code that determines that the app can't handle it, so it throws the url back to iOS, which opens Safari as the next best candidate.

Copy link
Contributor Author

@hangyujin hangyujin Jun 18, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think throwing back it to iOS is a reasonable idea. I updated the code to add [UIApplication.sharedApplication openURL: url].

Also tested that custom URI scheme is using a different API application:openURL:options

I also tried return NO in application:continueUserActivity:restorationHandler for (universal link)and  application:openURL:options (for custom URI), return NO will not cause it to throw the URL back, the behavior looks the same as return YES. So I don't know how the return value here is used.

Also go_router package have a fallback error screen if the deep link is not linked to a page. So if you’re using go_router for deep linking, it will always consider the deep link handled and the throw back to iOS thing won’t happen.
unnamed-1


[self openURL:url
options:options
completionHandler:^(BOOL success) {
openURLSuccess = success;
dispatch_semaphore_signal(semaphore);
}];

dispatch_semaphore_wait(semaphore, 5 * NSEC_PER_SEC);
return openURLSuccess;
}

- (BOOL)application:(UIApplication*)application handleOpenURL:(NSURL*)url {
Expand Down Expand Up @@ -214,7 +219,20 @@ - (BOOL)application:(UIApplication*)application
restorationHandler:restorationHandler]) {
return YES;
}
return [self openURL:userActivity.webpageURL];

__block BOOL openURLSuccess = NO; // Flag to track success
// Use a dispatch semaphore to wait for the completion handler
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);

[self openURL:userActivity.webpageURL
options:@{}
completionHandler:^(BOOL success) {
openURLSuccess = success;
dispatch_semaphore_signal(semaphore);
}];

dispatch_semaphore_wait(semaphore, 5 * NSEC_PER_SEC);
return openURLSuccess;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you write a helper to avoid duplicate code?

}

#pragma mark - FlutterPluginRegistry methods. All delegating to the rootViewController
Expand Down
43 changes: 28 additions & 15 deletions shell/platform/darwin/ios/framework/Source/FlutterAppDelegateTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,20 @@ - (void)testLaunchUrl {
OCMStub([self.mockMainBundle objectForInfoDictionaryKey:@"FlutterDeepLinkingEnabled"])
.andReturn(@YES);

OCMStub([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route?query=test"}])
.andReturn(@YES);

BOOL result =
[self.appDelegate application:[UIApplication sharedApplication]
openURL:[NSURL URLWithString:@"http://myApp/custom/route?query=test"]
options:@{}];

[self waitForExpectationsWithTimeout:5.0 handler:nil];

XCTAssertTrue(result);
OCMVerify([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route?query=test"}]);
OCMVerifyAll(self.mockNavigationChannel);
}

- (void)testLaunchUrlWithDeepLinkingNotSet {
Expand All @@ -80,6 +86,7 @@ - (void)testLaunchUrlWithDeepLinkingNotSet {
[self.appDelegate application:[UIApplication sharedApplication]
openURL:[NSURL URLWithString:@"http://myApp/custom/route?query=test"]
options:@{}];
[self waitForExpectationsWithTimeout:5.0 handler:nil];
XCTAssertFalse(result);
OCMReject([self.mockNavigationChannel invokeMethod:OCMOCK_ANY arguments:OCMOCK_ANY]);
}
Expand All @@ -92,36 +99,41 @@ - (void)testLaunchUrlWithDeepLinkingDisabled {
[self.appDelegate application:[UIApplication sharedApplication]
openURL:[NSURL URLWithString:@"http://myApp/custom/route?query=test"]
options:@{}];
[self waitForExpectationsWithTimeout:5.0 handler:nil];
XCTAssertFalse(result);
OCMReject([self.mockNavigationChannel invokeMethod:OCMOCK_ANY arguments:OCMOCK_ANY]);
}

- (void)testLaunchUrlWithQueryParameterAndFragment {
OCMStub([self.mockMainBundle objectForInfoDictionaryKey:@"FlutterDeepLinkingEnabled"])
.andReturn(@YES);

OCMStub([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route?query=test#fragment"}])
.andReturn(@YES);
BOOL result = [self.appDelegate
application:[UIApplication sharedApplication]
openURL:[NSURL URLWithString:@"http://myApp/custom/route?query=test#fragment"]
options:@{}];
[self waitForExpectationsWithTimeout:5.0 handler:nil];
XCTAssertTrue(result);
OCMVerify([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route?query=test#fragment"}]);
OCMVerifyAll(self.mockNavigationChannel);
}

- (void)testLaunchUrlWithFragmentNoQueryParameter {
OCMStub([self.mockMainBundle objectForInfoDictionaryKey:@"FlutterDeepLinkingEnabled"])
.andReturn(@YES);

OCMStub([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route#fragment"}])
.andReturn(@YES);
BOOL result =
[self.appDelegate application:[UIApplication sharedApplication]
openURL:[NSURL URLWithString:@"http://myApp/custom/route#fragment"]
options:@{}];
[self waitForExpectationsWithTimeout:5.0 handler:nil];
XCTAssertTrue(result);
OCMVerify([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route#fragment"}]);
OCMVerifyAll(self.mockNavigationChannel);
}

- (void)testReleasesWindowOnDealloc {
Expand All @@ -145,7 +157,10 @@ - (void)testReleasesWindowOnDealloc {
- (void)testUniversalLinkPushRouteInformation {
OCMStub([self.mockMainBundle objectForInfoDictionaryKey:@"FlutterDeepLinkingEnabled"])
.andReturn(@YES);

OCMStub([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route?query=test"}])
.andReturn(@YES);
NSUserActivity* userActivity = [[NSUserActivity alloc] initWithActivityType:@"com.example.test"];
userActivity.webpageURL = [NSURL URLWithString:@"http://myApp/custom/route?query=test"];
BOOL result = [self.appDelegate
Expand All @@ -154,9 +169,7 @@ - (void)testUniversalLinkPushRouteInformation {
restorationHandler:^(NSArray<id<UIUserActivityRestoring>>* __nullable restorableObjects){
}];
XCTAssertTrue(result);
OCMVerify([self.mockNavigationChannel
invokeMethod:@"pushRouteInformation"
arguments:@{@"location" : @"http://myApp/custom/route?query=test"}]);
OCMVerifyAll(self.mockNavigationChannel);
}

@end
27 changes: 27 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1863,6 +1863,33 @@ - (void)handlePressEvent:(FlutterUIPressProxy*)press
[self.keyboardManager handlePress:press nextAction:next];
}

- (void)openDeepLink:(NSURL*)url completionHandler:(void (^)(BOOL success))completion {
[_engine.get()
waitForFirstFrame:3.0
callback:^(BOOL didTimeout) {
if (didTimeout) {
FML_LOG(ERROR) << "Timeout waiting for the first frame when launching an URL.";
completion(NO);
} else {
// invove the method and get the result
[[_engine.get() navigationChannel]
invokeMethod:@"pushRouteInformation"
arguments:@{
@"location" : url.absoluteString ?: [NSNull null],
}
result:^(id _Nullable result) {
BOOL success =
[result isKindOfClass:[NSNumber class]] && [result boolValue];
if (!success) {
// Logging the error if the result is not successful
FML_LOG(ERROR) << "Failed to handle route information in Flutter.";
}
completion(success);
}];
}
}];
}

// The documentation for presses* handlers (implemented below) is entirely
// unclear about how to handle the case where some, but not all, of the presses
// are handled here. I've elected to call super separately for each of the
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,12 @@ typedef void (^FlutterKeyboardAnimationCallback)(fml::TimePoint);
// handled.
- (void)handlePressEvent:(FlutterUIPressProxy*)press
nextAction:(void (^)())nextAction API_AVAILABLE(ios(13.4));
- (void)openDeepLink:(NSURL*)url completionHandler:(void (^)(BOOL success))completion;
- (void)addInternalPlugins;
- (void)deregisterNotifications;
- (int32_t)accessibilityFlags;

- (BOOL)supportsShowingSystemContextMenu;

@end

#endif // FLUTTER_SHELL_PLATFORM_DARWIN_IOS_FRAMEWORK_SOURCE_FLUTTERVIEWCONTROLLER_INTERNAL_H_