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

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c2daa2
Rework lifecycle reference provided to plugins to force plugins to us…
matthew-carroll Oct 1, 2019
6c046b9
PR Updates.
matthew-carroll Oct 1, 2019
a47fc8b
PR Updates.
matthew-carroll Oct 2, 2019
2cfc39a
Add @Keep to HiddenLifecycleReference so that the flutter_android_lif…
matthew-carroll Oct 7, 2019
90e5353
Fixed rebase merge mistake.
matthew-carroll Oct 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,7 @@ FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugin
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugins/contentprovider/ContentProviderAware.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugins/contentprovider/ContentProviderControlSurface.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugins/contentprovider/ContentProviderPluginBinding.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugins/lifecycle/HiddenLifecycleReference.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugins/service/ServiceAware.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugins/service/ServiceControlSurface.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/plugins/service/ServicePluginBinding.java
Expand Down
1 change: 1 addition & 0 deletions shell/platform/android/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ android_java_sources = [
"io/flutter/embedding/engine/plugins/contentprovider/ContentProviderAware.java",
"io/flutter/embedding/engine/plugins/contentprovider/ContentProviderControlSurface.java",
"io/flutter/embedding/engine/plugins/contentprovider/ContentProviderPluginBinding.java",
"io/flutter/embedding/engine/plugins/lifecycle/HiddenLifecycleReference.java",
"io/flutter/embedding/engine/plugins/service/ServiceAware.java",
"io/flutter/embedding/engine/plugins/service/ServiceControlSurface.java",
"io/flutter/embedding/engine/plugins/service/ServicePluginBinding.java",
Expand Down
13 changes: 6 additions & 7 deletions shell/platform/android/io/flutter/embedding/engine/plugins/FlutterPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
package io.flutter.embedding.engine.plugins;

import android.arch.lifecycle.Lifecycle;
import android.arch.lifecycle.LifecycleOwner;
import android.content.Context;
import android.support.annotation.NonNull;

import io.flutter.embedding.engine.FlutterEngine;
import io.flutter.embedding.engine.plugins.lifecycle.HiddenLifecycleReference;

/**
* Interface to be implemented by all Flutter plugins.
Expand Down Expand Up @@ -93,10 +93,10 @@ public interface FlutterPlugin {
* 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

@blasten blasten Oct 1, 2019

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

@matthew-carroll matthew-carroll Oct 1, 2019

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

@matthew-carroll matthew-carroll Oct 1, 2019

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?

class FlutterPluginBinding {
private final Context applicationContext;
private final FlutterEngine flutterEngine;
private final Lifecycle lifecycle;
private final HiddenLifecycleReference hiddenLifecycleReference;

public FlutterPluginBinding(
@NonNull Context applicationContext,
Expand All @@ -105,7 +105,7 @@ public FlutterPluginBinding(
) {
this.applicationContext = applicationContext;
this.flutterEngine = flutterEngine;
this.lifecycle = lifecycle;
this.hiddenLifecycleReference = new HiddenLifecycleReference(lifecycle);
}

@NonNull
Expand All @@ -118,10 +118,9 @@ public FlutterEngine getFlutterEngine() {
return flutterEngine;
}

@Override
@NonNull
public Lifecycle getLifecycle() {
return lifecycle;
public Object getLifecycle() {
return hiddenLifecycleReference;
}
}
}
33 changes: 33 additions & 0 deletions ...tform/android/io/flutter/embedding/engine/plugins/lifecycle/HiddenLifecycleReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package io.flutter.embedding.engine.plugins.lifecycle;

import android.arch.lifecycle.Lifecycle;
import android.support.annotation.Keep;
import android.support.annotation.NonNull;

/**
* An {@code Object} that can be used to obtain a {@link Lifecycle} reference.
* <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

@amirh amirh Oct 17, 2019

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

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

@amirh amirh Oct 1, 2019

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

@matthew-carroll matthew-carroll Oct 2, 2019

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.

@NonNull
private final Lifecycle lifecycle;

public HiddenLifecycleReference(@NonNull Lifecycle lifecycle) {
this.lifecycle = lifecycle;
}

@NonNull
public Lifecycle getLifecycle() {
return lifecycle;
}
}