Add plugin for Android lifecycle in embedding. #2129

matthew-carroll · 2019-10-01T02:54:12Z

Add plugin for Android lifecycle in embedding.

Embedding side:
flutter/engine#12715

packages/flutter_android_lifecycle/android/gradle/wrapper/gradle-wrapper.properties

packages/flutter_android_lifecycle/pubspec.yaml

amirh · 2019-10-02T17:31:28Z

packages/flutter_android_lifecycle/pubspec.yaml

+  # adding or updating assets for this project.
+  plugin:
+    androidPackage: com.example.flutter_android_lifecycle
+    pluginClass: FlutterAndroidLifecyclePlugin


Also - make sure the apps using this plugin can build and run for iOS.
Note that we should be soon able to delete all ios code from this plugin once flutter/flutter#39659 is resolved and we migrate to the new pubspec schema, but that will have to wait for the next stable release.

Is there a process for verifying that?

build and run the example app on iOS (or any app the depends on the plugin)

@amirh @blasten I got the following error when attempting to build and run the iOS app. Do either of you happen to know a remedy?

Downloading ios tools... 6.2s Downloading ios-profile tools... 4.4s Downloading ios-release tools... 17.3s Launching lib/main.dart on iPhone Xʀ in debug mode... Running pod install... Running Xcode build... Xcode build done. 30.7s Finished with error: ProcessException: Process exited abnormally: An error was encountered processing the command (domain=NSPOSIXErrorDomain, code=22): Failed to install the requested application The application's Info.plist does not contain CFBundleVersion. Ensure your bundle contains a CFBundleVersion with a valid semantic version number. Command: /usr/bin/xcrun simctl install 0B543D29-BF06-4FF5-951E-2B98933F2403 [redacted]/flutter_plugins/packages/flutter_android_lifecycle/example/build/ios/iphonesimulator/Runner.app

It looks similar to flutter/flutter#34477 cc @jmagman

...le/android/app/src/main/java/com/example/flutter_android_lifecycle_example/MainActivity.java

packages/flutter_android_lifecycle/README.md

packages/flutter_android_lifecycle/LICENSE

amirh · 2019-10-02T17:35:51Z

packages/flutter_android_lifecycle/test/flutter_android_lifecycle_test.dart

+import 'package:flutter_test/flutter_test.dart';
+import 'package:flutter_android_lifecycle/flutter_android_lifecycle.dart';
+
+void main() {


Delete this file

amirh · 2019-10-02T17:37:05Z

packages/flutter_android_lifecycle/lib/flutter_android_lifecycle.dart

+  static const MethodChannel _channel =
+      const MethodChannel('flutter_android_lifecycle');
+
+  static Future<String> get platformVersion async {


I'd definitely remove this, I'm curious if we can publish a pub package with no Dart code (worth trying). If not we should figure out what's the minimal contents we need here (an empty file?)

amirh · 2019-10-02T17:38:05Z

packages/flutter_android_lifecycle/example/test/widget_test.dart

+void main() {
+  testWidgets('Verify Platform version', (WidgetTester tester) async {
+    // Build our app and trigger a frame.
+    await tester.pumpWidget(MyApp());


Actually if we leave just this line (and make the Android code use getLifecycle) we should have our e2e test.

This test is pretty close to being the e2e test you want but it's actually just a unit test that ships with the flutter create plugin template. It will need to call ensureInitialized on a compatible binding to be runnable with FlutterRunner or flutter drive. We could add a TODO to that effect if we want to get it landed.

widget_test.dart will require main.dart, which currently requires flutter_android_lifecycle.dart. Do you know which subset of these files we want to keep?

...oid/src/main/java/io/flutter/embedding/engine/plugins/lifecycle/FlutterLifecycleAdapter.java

matthew-carroll · 2019-10-04T05:18:24Z

@amirh @collinjackson it looks like the largest remaining issue is whether or not we should delete various files. Can you let me know what to keep/delete as soon as possible?

matthew-carroll · 2019-10-04T05:57:13Z

@amirh it looks like we're going to have a chicken/egg problem here with PR merges. This failure is from this PR:

/tmp/cirrus-ci-build/packages/flutter_android_lifecycle/android/src/main/java/io/flutter/embedding/engine/plugins/lifecycle/FlutterLifecycleAdapter.java:14: error: cannot find symbol
    if (!(reference instanceof HiddenLifecycleReference)) {
                               ^
  symbol:   class HiddenLifecycleReference
  location: class FlutterLifecycleAdapter
/tmp/cirrus-ci-build/packages/flutter_android_lifecycle/android/src/main/java/io/flutter/embedding/engine/plugins/lifecycle/FlutterLifecycleAdapter.java:20: error: cannot find symbol
    this.lifecycle = ((HiddenLifecycleReference) reference).getLifecycle();
                       ^
  symbol:   class HiddenLifecycleReference
  location: class FlutterLifecycleAdapter
2 errors
FAILURE: Build failed with an exception.

If we merge the embedding change first, we can solve the above issue, but everyone on master who needs Lifecycle for a plugin will be broken until we merge this. In the opposite direction, we would have to choose to merge with CI failures here, and then merge the embedding once we have published this package.

Thoughts?

packages/flutter_android_lifecycle/README.md

...oid/src/main/java/io/flutter/embedding/engine/plugins/lifecycle/FlutterLifecycleAdapter.java

blasten · 2019-10-04T21:48:09Z

...lutter_android_lifecycle/example/ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json

@@ -0,0 +1,122 @@
+{


nit: I think this ios/ directory can be removed.

Do we know for sure that we can delete it? The android_intent plugin seems to include an /ios directory:
https://github.com/flutter/plugins/tree/master/packages/android_intent

blasten · 2019-10-04T21:49:03Z

packages/flutter_android_lifecycle/ios/.gitignore

@@ -0,0 +1,37 @@
+.idea/


similar comment about the ios/ directory.

blasten · 2019-10-04T21:54:45Z

Consider removing the images from the PR. I know the tool generates a bunch of stuff that isn't really needed in some cases.

matthew-carroll · 2019-10-04T22:00:00Z

@blasten which images?

blasten · 2019-10-04T22:03:55Z

There are a few under flutter_android_lifecycle/example/android/app/src/main/res/....

matthew-carroll · 2019-10-04T22:05:23Z

Those all look to be ic_launcher images. Did you want me to remove all but one? We can't remove them all I don't think, because I believe Android requires an icon launcher image. I'm not sure if Android will allow us to use one for all resolutions or not.

The android_intent plugin seems to contain those same images:
https://github.com/flutter/plugins/tree/master/packages/android_intent/example/android/app/src/main/res

blasten · 2019-10-04T22:41:48Z

@amirh would probably know best, then. I heard that in general we don't want to commit binaries to source trees.

amirh · 2019-10-07T18:49:11Z

...oid/src/main/java/io/flutter/embedding/engine/plugins/lifecycle/FlutterLifecycleAdapter.java

+  @NonNull private final Lifecycle lifecycle;
+
+  public FlutterLifecycleAdapter(@NonNull Object reference) {
+    if (!(reference instanceof HiddenLifecycleReference)) {


It's possible that this is going to be compiled with a Flutter Engine version that does not have the HiddenLifecycleReference (e.g if a migrated plugin is used with today's stable).

I think we should use reflection to access this type? (can we make sure proguard doesn't rename it?)

…ime references.

matthew-carroll · 2019-10-07T21:31:47Z

@amirh @blasten I just pushed a change that uses reflection instead of compile-time references for the hidden lifecycle.

amirh · 2019-10-08T22:41:02Z

Closing in favor of #2168

Add plugin for Android lifecycle in embedding.

4a2513f

matthew-carroll requested review from amirh and blasten October 1, 2019 02:54

googlebot added the cla: yes label Oct 1, 2019

matthew-carroll mentioned this pull request Oct 1, 2019

Rework lifecycle reference provided to plugins to force plugins to use another plugin to dereference a Lifecycle object. flutter/engine#12715

Closed

blasten reviewed Oct 1, 2019

View reviewed changes

packages/flutter_android_lifecycle/android/gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved

PR Updates.

01a8e2d

amirh reviewed Oct 2, 2019

View reviewed changes

...oid/src/main/java/io/flutter/embedding/engine/plugins/lifecycle/FlutterLifecycleAdapter.java Outdated Show resolved Hide resolved

Pr Updates.

5a39303

matthew-carroll added 4 commits October 3, 2019 22:20

Fixed analysis issues.

a1ae510

Changelog and analysis fixes.

506ad06

Formatting fixes.

9568819

Formatting fixes.

600e194

blasten reviewed Oct 4, 2019

View reviewed changes

packages/flutter_android_lifecycle/README.md Outdated Show resolved Hide resolved

blasten reviewed Oct 4, 2019

View reviewed changes

...oid/src/main/java/io/flutter/embedding/engine/plugins/lifecycle/FlutterLifecycleAdapter.java Show resolved Hide resolved

README updates.

1667c11

blasten reviewed Oct 4, 2019

View reviewed changes

packages/flutter_android_lifecycle/ios/.gitignore

@@ -0,0 +1,37 @@

.idea/

Copy link

blasten Oct 4, 2019

Choose a reason for hiding this comment

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

similar comment about the ios/ directory.

amirh reviewed Oct 7, 2019

View reviewed changes

Changed HiddenLifecycleReference to use reflection to avoid compile-t…

bfe981f

…ime references.

amirh mentioned this pull request Oct 8, 2019

Add plugin for Android lifecycle in embedding #2168

Merged

amirh closed this Oct 8, 2019

Add plugin for Android lifecycle in embedding. #2129

Add plugin for Android lifecycle in embedding. #2129

Uh oh!

Conversation

matthew-carroll commented Oct 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

collinjackson Oct 2, 2019 • 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!

Uh oh!

matthew-carroll commented Oct 4, 2019

Uh oh!

matthew-carroll commented Oct 4, 2019

Uh oh!

Uh oh!

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!

blasten commented Oct 4, 2019

Uh oh!

matthew-carroll commented Oct 4, 2019

Uh oh!

blasten commented Oct 4, 2019

Uh oh!

matthew-carroll commented Oct 4, 2019

Uh oh!

blasten commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-carroll commented Oct 7, 2019

Uh oh!

amirh commented Oct 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

collinjackson Oct 2, 2019 •

edited

Loading