tlserver / flutter_map_location_marker

A flutter map plugin for displaying device current location.
https://pub.dev/packages/flutter_map_location_marker
BSD 3-Clause "New" or "Revised" License
98 stars 83 forks source link

`Geolocator.getPositionStream` steals control of position update configuration. #15

Closed comatory closed 2 years ago

comatory commented 2 years ago

I believe this is not maybe an issue but more like architectural decision on the part of the author.

By creating its own instance of position stream via Geolocator.getPositionStream, the plugin basically removes ability for developer to change configuration of the stream.

Please see https://github.com/Baseflow/flutter-geolocator/issues/939#issuecomment-101199093 for issues related to this.

unfortunately the problem here is with the native implementation on the platforms (specifically on iOS and macOS) which will cancel any existing location stream when you open another. Meaning it is impossible to listen to multiple location streams (within the sandbox of the App) at the same time.

So it means whoever makes getPositionStream call first wins since the cached stream is returned. You could say that I could create instance of the plugin only after I created my own getPositionStream which is fine. However if I want to change the stream configuration (by providing distanceFilter for example), first all listeners have to be canceled. That is not possible since the plugin doesn't expose the stream subscription (_positionStreamSubscription).

I think one solution would be to create optional position argument to the plugin. Whenever the position would be changed, the plugin could detect it in didUpdateWidget method and move the pin on the map appropriately. This would allow me to retain full control over position stream and use the plugin basically only as a presentational widget with some extra features (like camera centering).

If position argument would be present, the plugin wouldn't register listener to Geolocator.getPositionStream. The default behaviour could stay the same for people who want to use the plugin easily.

comatory commented 2 years ago

I ended up creating a PR which adds optional argument of Position type. It avoids the problem of multiple position streams thus you can safely use Geolocator within your own code.

There's an example page in the sample application where I demonstrate how you can change the stream during runtime and how it affects the rendering of the marker while still allowing to listen to the changes in your own app with the ability to recreate the stream any time.

tlserver commented 2 years ago

I just uploaded version 3.0.0 which can accept streams from application as the source. Hope that solve this problem.

comatory commented 2 years ago

I just uploaded version 3.0.0 which can accept streams from application as the source. Hope that solve this problem.

Yes this should fix #15 and #18 for people on this new version but not on v2. For v2 you could accept https://github.com/tlserver/flutter_map_location_marker/pull/16 which is backwards compatible and only extends the functionality.

My opinion is that the positionStream could be Stream<Position>, not sure why you're creating your own class LocationMarkerPosition, it's kind of redundant[^1] in my opinion. But that's just a nitpick, I'm glad you improved ergonomics

[^1]: Since the plugin depends on geolocator anyway, you could have used Position class. I just apply stream transformer that converts between the two and it seems like extra step. I know there are ppl who might not even use geolocator in which case it'd be better for the plugin to not rely on it at all.

tlserver commented 2 years ago

If I expose the streams to API, I think it should accept data from other plugin. It may confuse people if I still using Position in geolocator or CompassEvent in flutter_compass, because it is too many unused field. There is always a class LocationMarkerDataStreamFactory to help converting. I am considering to split the class LocationMarkerDataStreamFactory to separate package to make this does not depend on geolocator anymore. So that, the only widget of this package will be a presentation widget only.

tlserver commented 2 years ago

Come from #14, Would you please explain more about the different of this issue & #13? I think #14 should solve this but seem it does not.

If I understand correctly, you want to change the LocationSettings while you have your own geolocator's position stream outside this package. Assume #14 is applied, is it possible to first cancel all the geolocator's position stream outside the package, then change the locationSettings in LocationMarkerPlugin and subscribe again to the new position stream outside the package?

I just want to make the issue clear. You say canceling all listeners of the position stream is not possible. Isn't that #14 create a chance to re-create the position stream by replacing the locationSettings in LocationMarkerPlugin?

Sorry for not accepting #16 yet and thank a lot for helping improving this package.

comatory commented 2 years ago

On Wed Feb 2, 2022 at 12:32 AM CET, 6y wrote:

If I expose the streams to API, I think it should accept data from other plugin. It may confuse people if I still using Position in geolocator or CompassEvent in flutter_compass, because it is too many unused field. There is always a class LocationMarkerDataStreamFactory to help converting. I am considering to split the class LocationMarkerDataStreamFactory to separate package to make this does not depend on geolocator anymore. So that, the only widget of this package will be a presentation widget only.

Yeah that sounds good!

comatory commented 2 years ago

If I understand correctly, you want to change the LocationSettings while you have your own geolocator's position stream outside this package.

Yes

Assume #14 is applied, is it possible to first cancel all the geolocator's position stream outside the package, then change the locationSettings in LocationMarkerPlugin and subscribe again to the new position stream outside the package?

I don't think that's the way it works. Geolocator.getPositionStream always returns single position stream, that's a decision the authors of the plugin made to make it consistent between platforms.

Even if I cancel the stream in my app, your plugin is still using it, I think it'd have to be other way around, cancel stream in the plugin and then recreating it in application. When I tried doing what you proposed it did not work for me.

It doesn't matter now that much though, with v3 you added the ability to provide stream from outside so it fixes this problem.

tlserver commented 2 years ago

Ok, then I close this