[quick_actions] Android support only calling initialize once #4204

monmanuela · 2021-07-29T08:19:27Z

Pre-launch Checklist

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

stuartmorgan-g

I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.

While this does change a test file, there's no actually testing of the new functionality. Per separate discussion, that will need a native integration test that actually drives a quick actions flow.

stuartmorgan-g · 2021-07-29T20:07:41Z

.../quick_actions/android/src/main/java/io/flutter/plugins/quickactions/QuickActionsPlugin.java

  private static final String CHANNEL_ID = "plugins.flutter.io/quick_actions";
+  protected static boolean isInitialized = false;


Why is this static? Wouldn't each instance of the plugin need to be initialized?

sounds like the use case is to support a single initialization. If that is the case, it could still be an instance field.

That said, I don't there's such a case as registering/initializing the same plugin name more than once.

Removed as it's unnecessary. Thanks.

stuartmorgan-g · 2021-07-29T20:10:00Z

.../quick_actions/android/src/main/java/io/flutter/plugins/quickactions/QuickActionsPlugin.java

@@ -43,6 +47,10 @@ public void onDetachedFromEngine(FlutterPluginBinding binding) {
  @Override
  public void onAttachedToActivity(ActivityPluginBinding binding) {
    handler.setActivity(binding.getActivity());
+    binding.addOnNewIntentListener(this);
+    if (isInitialized) {


Why is this (and the new method) needed? I don't see any deferment logic, so couldn't it just send the call unconditionally?

Ah my bad. Initially I thought it'll call onNewIntent when the Dart side isn't initialized yet, which means it'll make a redundant method call which would just vanish. But after checking again, the method call does go through to Dart.

Removed the if block, the static field, and the new method. Thanks for catching this.

stuartmorgan-g · 2021-07-29T20:11:17Z

...ctions/quick_actions_platform_interface/lib/method_channel/method_channel_quick_actions.dart

@@ -24,6 +25,7 @@ class MethodChannelQuickActions extends QuickActionsPlatform {
      assert(call.method == 'launch');
      handler(call.arguments);
    });
+    unawaited(channel.invokeMethod<String?>('initialize'));


If you're going to call a new method, you should make it a no-op on iOS to avoid getting useless error messages (although per other comments, it's not clear to me that this is necessary).

Removed as it's not necessary.

stuartmorgan-g · 2021-07-29T20:14:25Z

Adding @blasten to review the Android intent flow aspect of this.

blasten · 2021-07-29T21:12:53Z

.../quick_actions/android/src/main/java/io/flutter/plugins/quickactions/QuickActionsPlugin.java

@@ -60,6 +69,15 @@ public void onDetachedFromActivityForConfigChanges() {
    onDetachedFromActivity();
  }

+  @Override
+  public boolean onNewIntent(Intent intent) {
+    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N_MR1) return false;


Suggested change

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N_MR1) return false;

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N_MR1) {

return false;

}

Thanks, done.

blasten · 2021-07-29T23:21:04Z

.../quick_actions/android/src/main/java/io/flutter/plugins/quickactions/QuickActionsPlugin.java

+    }
+    return false;
+  }
+
  private void setupChannel(BinaryMessenger messenger, Context context, Activity activity) {
    channel = new MethodChannel(messenger, CHANNEL_ID);
    handler = new MethodCallHandlerImpl(context, activity);


MethodCallHandlerImpl could take an object that implements an interface. Then, QuickActionsPlugin could implement the interface. Lastly, pass the reference when it constructs MethodCallHandlerImpl.

The interface implementation sets isInitialized = true;

Thanks for the suggestion. But as per stuartmorgan@'s comment above, removed the field as it's not necessary.

blasten · 2021-07-29T23:32:31Z

.../quick_actions/android/src/main/java/io/flutter/plugins/quickactions/QuickActionsPlugin.java

@@ -43,6 +47,10 @@ public void onDetachedFromEngine(FlutterPluginBinding binding) {
  @Override
  public void onAttachedToActivity(ActivityPluginBinding binding) {
    handler.setActivity(binding.getActivity());
+    binding.addOnNewIntentListener(this);
+    if (isInitialized) {


could we add comments to the code? That will help the next maintainer

monmanuela · 2021-07-30T06:21:05Z

Thanks for the reviews! 😄 Responded to the comments.

blasten

Implementation LGTM, but it may also need a test.

@bparrishMines is there a plan to backfill this plugin as well?

stuartmorgan-g

Implementation LGTM, but it may also need a test.

Yes, this will need a test per my initial comment.

@bparrishMines is there a plan to backfill this plugin as well?

The scaffolding is in place now so it should be possible to add an integration test to this PR.

monmanuela · 2021-08-17T09:52:38Z

Per @bparrishMines 's suggestions in a separate discussion, added unit tests for the new functionality

stuartmorgan-g

It wasn't actually clear to me from the offline discussion why unit tests are better here than UI tests of the actual flow (which would ensure that whole user flow behaves as expected), but it looks like this has reasonable coverage of the change so LGTM.

…lutter/plugins#4204)

…lutter/plugins#4204) (#88363)

…lutter/plugins#4204) (flutter#88363)

…#4204) Fixes flutter/flutter#87259

monmanuela added 6 commits July 29, 2021 15:04

Support only calling initialize once

bac3863

Fix typo in dartdoc

13cc651

Add changelog

e1cf07c

Add method channel quick actions test

b6fb66b

Mark channel method call initialize unawaited

1b13087

Bump version

3615911

google-cla bot added the cla: yes label Jul 29, 2021

github-actions bot added p: quick_actions platform-android labels Jul 29, 2021

stuartmorgan-g reviewed Jul 29, 2021

View reviewed changes

stuartmorgan-g requested a review from blasten July 29, 2021 20:14

blasten reviewed Jul 29, 2021

View reviewed changes

monmanuela force-pushed the qa-onnewintent branch from cfcd743 to 8a1b505 Compare July 30, 2021 05:12

Call onNewIntent unconditionally

f9d1341

monmanuela force-pushed the qa-onnewintent branch from 8a1b505 to f9d1341 Compare July 30, 2021 05:13

Add comments

0463301

monmanuela requested review from blasten and stuartmorgan-g August 2, 2021 02:50

blasten approved these changes Aug 3, 2021

View reviewed changes

stuartmorgan-g suggested changes Aug 4, 2021

View reviewed changes

Merge branch 'flutter:master' into qa-onnewintent

4af9343

bparrishMines mentioned this pull request Aug 16, 2021

Add unit tests to quick_actions plugin #4245

Merged

11 tasks

monmanuela added 2 commits August 17, 2021 11:17

Merge branch 'flutter:master' into qa-onnewintent

b7544e5

Add unit tests for onNewIntent

67f48eb

monmanuela requested a review from stuartmorgan-g August 17, 2021 09:52

stuartmorgan-g approved these changes Aug 17, 2021

View reviewed changes

stuartmorgan-g merged commit d58036f into flutter:master Aug 17, 2021

engine-flutter-autoroll mentioned this pull request Aug 17, 2021

Roll Plugins from c52ae9fdf175 to d58036f45d82 (1 revision) flutter/flutter#88363

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 17, 2021

d58036f [quick_actions] Android support only calling initialize once (f…

69e4162

…lutter/plugins#4204)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Aug 17, 2021

d58036f [quick_actions] Android support only calling initialize once (f…

2d0f40e

…lutter/plugins#4204) (#88363)

blasten pushed a commit to blasten/flutter that referenced this pull request Aug 19, 2021

d58036f [quick_actions] Android support only calling initialize once (f…

6dc6f87

…lutter/plugins#4204) (flutter#88363)

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

[quick_actions] Android support only calling initialize once (flutter…

5fba6b3

…#4204) Fixes flutter/flutter#87259

amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021

[quick_actions] Android support only calling initialize once (flutter…

fe39ff5

…#4204) Fixes flutter/flutter#87259

stuartmorgan-g mentioned this pull request Jan 29, 2024

Split Android and iOS plugin capabilities in note on 'Writing custom platform-specific code' page flutter/website#5636

Closed

		private static final String CHANNEL_ID = "plugins.flutter.io/quick_actions";
		protected static boolean isInitialized = false;

[quick_actions] Android support only calling initialize once #4204

[quick_actions] Android support only calling initialize once #4204

Uh oh!

Conversation

monmanuela commented Jul 29, 2021

Pre-launch Checklist

Uh oh!

stuartmorgan-g 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!

monmanuela Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

stuartmorgan-g commented Jul 29, 2021

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!

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!

monmanuela commented Jul 30, 2021

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

monmanuela commented Aug 17, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

monmanuela Jul 30, 2021 •

edited

Loading