Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@matthew-carroll
Copy link
Contributor

@matthew-carroll matthew-carroll commented Oct 1, 2019

Rework lifecycle reference provided to plugins to force plugins to use another plugin to dereference a Lifecycle object.

Plugin side:
flutter/plugins#2129

* concerned about which Android Component is currently holding the {@link FlutterEngine}.
* TODO(mattcarroll): add info about ActivityAware and ServiceAware for plugins that care.
*/
class FlutterPluginBinding implements LifecycleOwner {
Copy link

Choose a reason for hiding this comment

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

does it also need to remove the imports?

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 can definitely clean things up, but does this PR and the other PR look like they accomplish the goal? Mostly I'm wondering if this allows us to achieve the desired decoupling in plugins...

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 took another look at this class and there is a package private constructor at the bottom that takes in an actual Lifecycle. So unless we make further changes, we still need the import. Do you think there's any issue with what we have here?

Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM

* {@link Lifecycle} in a way that makes it easier for Flutter and the Flutter plugin ecosystem to
* handle breaking changes in Lifecycle libraries.
*/
public class HiddenLifecycleReference {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious - would it work if we make this a private class, and keep the exact same code duplicated as a private class in the lifecycle plugin (and in the plugin cast the Object to it's copy of HiddenLifecycleReference) ?

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 not sure, but I'd recommend against it. I think the answer to that question will always be determined by the details of Dalvik, Art, and any future bytecode system. If this were Objective-C then we might be able to do what you're saying because those dispatch tables can use strings as method identifiers, but the closest thing that Java has is reflection, which I think would be more trouble than it's worth.

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

lgtm

@amirh
Copy link
Contributor

amirh commented Oct 3, 2019

Shall we be updating FlutterEngine as well? e.g it's still possible for a plugin to do binding.getFlutterEngine().getLifecycle() without depending on the pub flutter_android_lifecycle package?

@matthew-carroll
Copy link
Contributor Author

For the record, this PR cannot be merged until we have merged and published the flutter_android_lifecycle plugin:
flutter/plugins#2129

@matthew-carroll
Copy link
Contributor Author

@amirh @blasten I just pushed an @Keep annotation to facilitate reflection in flutter_android_lifecycle.

@matthew-carroll
Copy link
Contributor Author

@amirh @mklim please feel free to hit merge on this whenever it works for the plugin team.

…e another plugin to dereference a Lifecycle object.
…ecycle package can access via reflection instead of compile-time references.
@matthew-carroll matthew-carroll force-pushed the rework_plugin_lifecycle_reference branch from ac1098f to e46e58a Compare October 11, 2019 23:24
@matthew-carroll matthew-carroll force-pushed the rework_plugin_lifecycle_reference branch from e46e58a to 90e5353 Compare October 11, 2019 23:26
* <p>
* <strong>DO NOT USE THIS CLASS IN AN APP OR A PLUGIN.</strong>
* <p>
* This class is used by the flutter_android_lifecycle package to provide access to a
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to update this to flutter_plugin_android_lifecycle

@amirh
Copy link
Contributor

amirh commented Oct 17, 2019

https://pub.dev/packages/flutter_plugin_android_lifecycle published, feel free to land this (after updating the comment with the final plugin name)

@matthew-carroll
Copy link
Contributor Author

Closing in favor of #13402

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants