Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[quick_actions] handle cold start on iOS correctly #3811

Merged
merged 4 commits into from
Apr 19, 2021

Conversation

mvanbeusekom
Copy link
Contributor

According to Apple's documentation (see the "Discussion" section here) the application:performActionForShortcutItem: method is not executed when the willFinishLaunchingWithOptions: or the didFinishLaunchingWithOptions: return NO. In these cases the willFinishLaunchingWithOptions: or didFinishLaunchingWithOptions: method are responsible for handling quick actions.

Basic Flutter applications will always return YES for the willFinishLaunchingWithOptions: and didFinishLaunchingWithOptions: methods, however in more advanced cases developers might override these methods and provide custom implementations which might result in one of these method to return NO. In these situations the current implementation of the quick_actions plugin will not work.

This PR will solve the problem and correctly handle quick actions in the didFinishLaunchingWithOptions: delegate of the plugin. It also contains an additional XCUITest to proof this method is working correctly. To be able to simulate the situation the didFinishLaunchingWithOptions: method in the example/ios/Runner/AppDelegate.m is updated to always return NO.

Running the XCUITest on the current codebase will fail if you update the didFinishLaunchingWithOptions: method in the example/ios/Runner/AppDelegate.m to return NO. Running the same test on the code modified in this PR the test will succeed.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@@ -10,7 +10,9 @@ @interface FLTQuickActionsPlugin ()
@property(nonatomic, retain) FlutterMethodChannel *channel;
@end

@implementation FLTQuickActionsPlugin
@implementation FLTQuickActionsPlugin {
NSString *shortcutType;
Copy link
Contributor

Choose a reason for hiding this comment

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

nits: Let's make this a property instead of ivar to match the code style of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

#pragma mark Private functions

- (void)_handleShortcut:(NSString *)shortcut {
Copy link
Contributor

Choose a reason for hiding this comment

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

nits: OBJC method should not start with handleShortcut is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

- (BOOL)application:(UIApplication *)application
didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
if (@available(iOS 9.0, *)) {
UIApplicationShortcutItem *shortcutItem =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we directly call handleShortcut here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to wait here for the application to become fully active before the method channel are fully initialised. When we directly call the handleShortcut in the didFinishLaunchingWithOptions method the channels on the Dart side are not initialised yet and the invokeMethod:@"launch" isn't processed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah makes sense. Let's add a comment here for future reference.

Why do we return No below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to Apple's documentation you should return NO if you already handled the shortcut action which will make sure the application:performActionFor: method and not handle the same action again.

The explanation can be found in the last alinea of the "Discussion" section of this article: https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1623032-application

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Let's also add comment here too. Then I think we are good to go!

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 @cyanglaz, I have added comments as explanation.

@mvanbeusekom
Copy link
Contributor Author

@cyanglaz thank you very much for your feedback. I corrected the "nits" and answered the question regarding why not to call the handleShortcut directly from thedidFinishLaunchingWithOptions method. Would appreciate it if you could have another look.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@cyanglaz cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Apr 18, 2021
@fluttergithubbot fluttergithubbot merged commit e2844d2 into flutter:master Apr 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 19, 2021
bhaveshptl pushed a commit to bhaveshptl/plugins that referenced this pull request Apr 23, 2021
* master: (397 commits)
  [in_app_purchase] Implementation of platform interface (flutter#3781)
  [google_sign_in] Add todo WRT correctly setting X-Goog-AuthUser header (flutter#3819)
  [tools] fix version check command not working for new packages (flutter#3818)
  [camera] android-rework part 1: Base classes to support Android Camera features (flutter#3795)
  fix MD (flutter#3815)
  Path provider windows crash fix (flutter#3814)
  [local_auth] docs update (flutter#3103)
  Update PULL_REQUEST_TEMPLATE.md (flutter#3801)
  [quick_actions] handle cold start on iOS correctly (flutter#3811)
  Replace path_provider_linux widget tests with simple unit tests (flutter#3812)
  [sensors] format dart code based on the new dart formatter (flutter#3809)
  [google_sign_in] Fix "pick account" on iOS (flutter#3805)
  [image_picker_platform_interface] Added pickMultiImage (flutter#3782)
  [in_app_purchase] Added currency code and numerical price to product detail model. (flutter#3794)
  [local_auth] Fix iOS crash when no localizedReason (flutter#3780)
  Fix and update version checks (flutter#3792)
  [in_app_purchase] Configured example app to use StoreKit Testing on iOS 14 (flutter#3772)
  [local_auth] Unnecessary reassignment in example removed (flutter#2983)
  [flutter_webview] Fix `allowsInlineMediaPlayback` ignored on iOS (flutter#3791)
  Switch script/tools over to the new analysis options (flutter#3777)
  ...
yasargil added a commit to yasargil/plugins that referenced this pull request Apr 28, 2021
* master: (79 commits)
  Fix grammatical error in contributing guide (flutter#3217)
  [google_sign_in_web] fix README typos.
  [tool] combine run and runAndExitOnError (flutter#3827)
  [camera] android-rework part 2: Android auto focus feature (flutter#3796)
  [in_app_purchase_platform_interface] Added additional fields to ProductDetails (flutter#3826)
  Move all null safety packages' min dart sdk to 2.12.0 (flutter#3822)
  [path_provider_*] code cleanup: sort directives (flutter#3823)
  [in_app_purchase] Implementation of platform interface (flutter#3781)
  [google_sign_in] Add todo WRT correctly setting X-Goog-AuthUser header (flutter#3819)
  [tools] fix version check command not working for new packages (flutter#3818)
  [camera] android-rework part 1: Base classes to support Android Camera features (flutter#3795)
  fix MD (flutter#3815)
  Path provider windows crash fix (flutter#3814)
  [local_auth] docs update (flutter#3103)
  Update PULL_REQUEST_TEMPLATE.md (flutter#3801)
  [quick_actions] handle cold start on iOS correctly (flutter#3811)
  Replace path_provider_linux widget tests with simple unit tests (flutter#3812)
  [sensors] format dart code based on the new dart formatter (flutter#3809)
  [google_sign_in] Fix "pick account" on iOS (flutter#3805)
  [image_picker_platform_interface] Added pickMultiImage (flutter#3782)
  ...
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
@mvanbeusekom mvanbeusekom deleted the issue/13634 branch September 21, 2021 09:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes p: quick_actions platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
3 participants