Add plugin for Android lifecycle in embedding #2168

amirh · 2019-10-08T22:40:30Z

Based off @matthew-carroll's #2129

Changes in this PR:

Change the iOS plugin to ObjC.
Add e2e tests.
Don't crash the sample app if the lifecycle API isn't available (so it passes on master)
Cleanup unneeded files.

Original description

Add plugin for Android lifecycle in embedding.

Embedding side:
flutter/engine#12715

packages/flutter_android_lifecycle/CHANGELOG.md

packages/flutter_android_lifecycle/LICENSE

packages/flutter_android_lifecycle/android/build.gradle

mklim · 2019-10-09T00:10:43Z

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

+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+public class FlutterLifecycleAdapter {


nit: Needs documentation.

https://google.github.io/styleguide/javaguide.html#s7.3-javadoc-where-required

Just a nit, but this is still needed for this class to comply with Google Java style.

packages/flutter_android_lifecycle/example/README.md

...oid/app/src/main/java/io/flutter/plugins/flutter_android_lifecycle_example/MainActivity.java

packages/flutter_android_lifecycle/example/ios/Runner/AppDelegate.h

packages/flutter_android_lifecycle/lib/flutter_android_lifecycle.dart

packages/flutter_android_lifecycle/README.md

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

mklim

LGTM. Just a couple minor nits still.

mklim · 2019-10-10T21:57:41Z

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

+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+public class FlutterLifecycleAdapter {


Just a nit, but this is still needed for this class to comply with Google Java style.

...oid/app/src/main/java/io/flutter/plugins/flutter_android_lifecycle_example/MainActivity.java

matthew-carroll · 2019-10-11T20:27:06Z

packages/flutter_android_lifecycle/README.md

+in the plugin's binding.
+
+The purpose of having this plugin instead of exposing an Android `Lifecycle` object in the engine's
+Android embedder plugins API is to force plugins to have a pub constraint that signifies the


FYI, the term "embedder" refers to the C++ code. "Embedding" is the name given to the Java/Obj-C layers.
https://github.com/flutter/flutter/wiki/Glossary

amirh · 2019-10-16T17:40:37Z

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

+
+  @Nullable private Lifecycle lifecycle = null;
+
+  public FlutterLifecycleAdapter(@NonNull Object reference) {


@mklim @matthew-carroll

Currently the usage of this API looks like:

FlutterLifecycleAdapter adapter = new FlutterLifecycleAdapter(flutterPluginBinding.getLifecycle()); Lifecycle lifecycle = adapter.getLifecycle();

What do you think about making the getLifecycle method static instead, I see how usually we want to avoid static methods for testability, but in this case I don't see how this is particularly useful especially without us adding some FlutterLifecycleAdapterFactory method which will potentially allow tests to replace the concrete implementation of FlutterLifecycleAdapter.

Second question - what do yo think about making the adapter be the one that calls getLifecycle?

If we do both of these we may end up with the call site looking like:

Lifecycle lifecycle = FlutterLifecycleAdapter.getLifecycle(flutterPluginBinding);

What do you think?

I avoid any static call that I don't absolutely need, but it's up to you. Seems like you can also achieve the second goal whether you go static or not.

I avoid any static call that I don't absolutely need, but it's up to you.

I'm looking for feedback 😄 do you see any issue with using static here?

Seems like you can also achieve the second goal whether you go static or not.

Yes, that's why I separated them to discuss separately, what is your opinion on that?

I think the second suggestion is fine.

My feedback on the static suggestion is that I never make something static if I don't have to. I would personally not make it static. But I'm also not going to block this PR if you choose to do so. I don't see anything that will break by making it static. Then again, I could say the same thing for many instance methods that aren't static.

This is totally subjective so I think it's up to you.

My personal opinion on static is that it's preferable if the method is truly immutable, doesn't require any kind of state or precondition, and truly doesn't have any kind of side effects. Using static in that way is a soft indicator that a method is purely functional. Good examples are Arrays#asList or TextUtils#isEmpty. A "bad" counter example of static is our own FlutterJNI, which has a bunch of "static" methods that don't depend on any member variables but do depend on there being this magic global c library loaded in the program's global runtime. The danger in methods like that is first that it's extremely easy to call them out of sync, and then second that they're really hard to work around when testing.

The generic problem with any static method is that it can't be mocked in unit tests. I generally prefer testing real behavior to using mocks whenever possible so that's not a huge concern to me but it can be a gigantic headache in the moment when the method really does need to be avoided in a test context (again, like FlutterJNI). Static also prevents subclasses from overriding the method, but that's a non-issue for me because I generally prefer composition to inheritance.

I think in this specific case it would be fine to make this static, because it's basically just returning the object that's passed into it. In a test context if you really needed to mock anything you would be returning a mock from the input parameter. I again prefer static interfaces because they're a soft signal that the function doesn't have silent side effects so I'd be happy to use one here, but it's totally subjective.

updated to static, I think there's an upside (less cluttered call site) and no downside.

amirh · 2019-10-16T22:45:49Z

Will land on green

amirh · 2019-10-16T22:51:53Z

Actually I'll wait for another LGTM from @matthew-carroll and @mklim as the public API changed

matthew-carroll · 2019-10-16T22:57:19Z

LGTM

mklim

LGTM

This plugin just landed in: #2168 The flutter_android_lifecycle package name is already taken, renaming to flutter_plugin_android_lifecycle.

…r#2205) This plugin just landed in: flutter#2168 The flutter_android_lifecycle package name is already taken, renaming to flutter_plugin_android_lifecycle.

amirh added 7 commits October 8, 2019 09:54

plugin template

a24c43b

xcodeproj auto update

ad027b0

use new pubspec schema

b6058a0

implementation

4fc704b

add e2e test

b9724c9

remove unused Dart unit tests

ba87bfe

format

e748eb1

googlebot added the cla: yes label Oct 8, 2019

amirh mentioned this pull request Oct 8, 2019

Add plugin for Android lifecycle in embedding. #2129

Closed

mklim self-assigned this Oct 8, 2019

mklim self-requested a review October 8, 2019 22:54

amirh added 2 commits October 8, 2019 16:16

remove the example app's widget test

137188f

remove unused import

c47c388

mklim reviewed Oct 9, 2019

View reviewed changes

rajbheda5 approved these changes Oct 9, 2019

View reviewed changes

review fixes

80fa536

mklim approved these changes Oct 10, 2019

View reviewed changes

matthew-carroll reviewed Oct 11, 2019

View reviewed changes

matthew-carroll approved these changes Oct 11, 2019

View reviewed changes

conditionally include androidx gradle dep

88c8461

amirh commented Oct 16, 2019

View reviewed changes

amirh added 2 commits October 16, 2019 15:39

more cleanups

22941f2

update readme

b52fed6

mklim approved these changes Oct 16, 2019

View reviewed changes

amirh merged commit 8251326 into flutter:master Oct 16, 2019

amirh mentioned this pull request Oct 17, 2019

s/flutter_android_lifecycle/flutter_plugin_android_lifecycle/ #2205

Merged

amirh added a commit that referenced this pull request Oct 17, 2019

s/flutter_android_lifecycle/flutter_plugin_android_lifecycle/ (#2205)

ec57790

This plugin just landed in: #2168 The flutter_android_lifecycle package name is already taken, renaming to flutter_plugin_android_lifecycle.

mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019

Add plugin for Android lifecycle in embedding (flutter#2168)

76a5726

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019

Add plugin for Android lifecycle in embedding (flutter#2168)

6276730


		@Nullable private Lifecycle lifecycle = null;

		public FlutterLifecycleAdapter(@NonNull Object reference) {

Add plugin for Android lifecycle in embedding #2168

Add plugin for Android lifecycle in embedding #2168

Uh oh!

Conversation

amirh commented Oct 8, 2019

Original description

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

amirh Oct 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh commented Oct 16, 2019

Uh oh!

amirh commented Oct 16, 2019

Uh oh!

matthew-carroll commented Oct 16, 2019

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amirh Oct 16, 2019 •

edited

Loading