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

[google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin #3213

Merged
amirh merged 14 commits into from
Oct 29, 2020
Merged

[google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin #3213

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c977679
GoogleMapController is now uniformly driven by implementing DefaultLi…
math1man Oct 27, 2020
e934a18
Update CHANGELOG.md and version
math1man Oct 27, 2020
9bdc386
Merge remote-tracking branch 'origin/master'
math1man Oct 27, 2020
96cf26b
Fix formatting issues
math1man Oct 28, 2020
62476dc
Update GoogleMapControllerTest
math1man Oct 28, 2020
a8e02a4
Merge remote-tracking branch 'origin/master'
math1man Oct 28, 2020
b920973
Remove extraneous import
math1man Oct 28, 2020
ea016b8
Update GoogleMapsPlugin.ProxyLifecycleProvider to use handleLifecycle…
math1man Oct 28, 2020
d6c9014
Merge remote-tracking branch 'origin/master'
math1man Oct 28, 2020
5f479f0
Update version from 1.1.0 to 1.0.5
math1man Oct 28, 2020
2c182a9
Merge remote-tracking branch 'origin/master'
math1man Oct 28, 2020
039e3ce
Add javadoc for ProxyLifecycleProvider and move initial ActivityLifec…
math1man Oct 28, 2020
85e592e
Merge remote-tracking branch 'origin/master'
math1man Oct 28, 2020
df2345d
Remove LifecycleProvider as an interface of GoogleMapsPlugin and inst…
math1man Oct 28, 2020
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
Prev Previous commit
Remove LifecycleProvider as an interface of GoogleMapsPlugin and inst… 
…ead create an anonymous inner class.
  • Loading branch information
@math1man
math1man committed Oct 28, 2020
commit df2345da8d2d55e74a87524c7bb36142b43f5ca6
20 changes: 12 additions & 8 deletions ...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapsPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
* the map. A Texture drawn using GoogleMap bitmap snapshots can then be shown instead of the
* overlay.
*/
public class GoogleMapsPlugin implements FlutterPlugin, ActivityAware, LifecycleProvider {
public class GoogleMapsPlugin implements FlutterPlugin, ActivityAware {

@Nullable private Lifecycle lifecycle;

Expand Down Expand Up @@ -69,7 +69,17 @@ public GoogleMapsPlugin() {}
public void onAttachedToEngine(FlutterPluginBinding binding) {
binding
.getPlatformViewRegistry()
.registerViewFactory(VIEW_TYPE, new GoogleMapFactory(binding.getBinaryMessenger(), this));
.registerViewFactory(
VIEW_TYPE,
new GoogleMapFactory(
binding.getBinaryMessenger(),
new LifecycleProvider() {
Copy link
Contributor

@amirh amirh Oct 28, 2020

Choose a reason for hiding this comment

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

Is this going to keep an implicit reference to the outer class?

Copy link
Contributor Author

@math1man math1man Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It actually keeps an explicit reference to the outer class, because lifecycle is a field in the outer class. I don't think that's avoidable because only the plugin itself can implement ActivityAware.

Copy link
Contributor

@amirh amirh Oct 28, 2020

Choose a reason for hiding this comment

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

I'm probably missing something obvious... what fails if:

GoogleMapsPlugin holds a reference to the provider (an implementation with a lifecycle field) and sets the lifecycle when attached/detached to the activity, and we pass that provider reference to GoogleMapFactory, won't we avoid a reference from GoogleMapFactory to GoogleMapsPlugin in this case?

Copy link
Contributor Author

@math1man math1man Oct 28, 2020

Choose a reason for hiding this comment

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

I could do that, but it doesn't really matter and ends up being more boiler plate. My assumption is that the scope of GoogleMapsPlugin is greater than the scope of GoogleMapFactory, so it doesn't matter if GoogleMapFactory transitively captures the GooglemapsPlugin instance. If that assumption is violated, all sorts of worse things happen than this particular leak.

I guess my question is what problem are you trying to solve?

Copy link
Contributor

@amirh amirh Oct 28, 2020

Choose a reason for hiding this comment

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

You are correct, thanks for pushing back and keeping the code simple!

@Nullable
@Override
public Lifecycle getLifecycle() {
return lifecycle;
}
}));
}

@Override
Expand Down Expand Up @@ -97,12 +107,6 @@ public void onDetachedFromActivityForConfigChanges() {
onDetachedFromActivity();
}

@Nullable
@Override
public Lifecycle getLifecycle() {
return lifecycle;
}

/**
* This class provides a {@link LifecycleOwner} for the activity driven by {@link
* ActivityLifecycleCallbacks}.
Expand Down