Enable location service availability stream on Android #1
Conversation
|
A little confused about the project duplicate flutter-permission-handlers <-> flutter-permission-handler. But this is the project referenced in geolocator, so I thought I'd add the feature in here. |
There was a problem hiding this comment.
@Lootwig, thank you very much for this pull-request. Except from some minor remarks, it looks good. Please have a look at my comments.
...sions/android/src/main/java/com/baseflow/location_permissions/LocationPermissionsPlugin.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void onCancel(Object arguments) { |
There was a problem hiding this comment.
Shouldn't we remove our self as a registered receiver from the active context here? And maybe clean up the eventSink (e.g. set to null)?
There was a problem hiding this comment.
Setting the field to null now.
Unregistering the receiver did not make sense without moving the registry part as well (because there wouldn't be a chance to re-register again with the current implementation), so I moved that part too.
|
Regarding your confusion, you are right it is confusing. The idea is that we split up all permissions that are current packed into the So to solve this problem we will split up all permissions into their own packages. When this is finished we will stop support on the |
| @@ -169,12 +168,20 @@ public void onMethodCall(MethodCall call, Result result) { | |||
|
|
|||
| @Override | |||
| public void onListen(Object arguments, EventSink events) { | |||
| mEventSink = events; | |||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) { | |||
| getActiveContext().registerReceiver(mReceiver, mIntentFilter); | |||
There was a problem hiding this comment.
I think we should save the context in a class member here to make sure we unsubscribe from the same context when we cancel listening to the stream. Depending on the state of the App it is possible that the getActiveContext returns a different value (e.g. depending if the App is active of in the background). Something like:
mContext = getActiveContext();
mContext.registerReceiver(mReceiver, mIntentFilter);
There was a problem hiding this comment.
Wouldn't this introduce a potential memory leak? Sorry if this is a misinformed question, but I was under the impression keeping references to Context instances was bad practice.
There was a problem hiding this comment.
You are right indeed, thank you for letting me know.
According to this article we should use the ApplicationContext which is a singleton so we will always have the same instance.
We can access the ApplicationContext through the mRegistrar.context() method (see also: https://docs.flutter.io/javadoc/io/flutter/plugin/common/PluginRegistry.Registrar.html).
So I think it would be good to replace getActiveContext() with mRegistrar.context()
There was a problem hiding this comment.
Updated the implementation accordingly!
| } | ||
|
|
||
| @Override | ||
| public void onCancel(Object arguments) { | ||
| mEventSink = null; | ||
| if (mEventSink != null) { | ||
| getActiveContext().unregisterReceiver(mReceiver); |
There was a problem hiding this comment.
And here make sure we unsubscribe from the same context:
if (mEventSink != null && mContext != null) {
mContext.unregisterReceiver(mReceiver);
mContext = null;
mEventSink = null;
}
There was a problem hiding this comment.
Resolved by agreeing not to store the context?
There was a problem hiding this comment.
Looks good, thank you @Lootwig appreciate the effort you put in to help out!
|
@mvanbeusekom and @Lootwig thanks for this PR. This is the only way we were able to recognize user opening the android status bar and toggling location services. |
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Access to streamed location service enabled/disabled state.
💥 Does this PR introduce a breaking change?
Nope.
🐛 Recommendations for testing
Only Android side implemented.