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
109 stars 98 forks source link

Expose CurrentLocationLayerState.currentPosition publicly #90

Closed Nico04 closed 11 months ago

Nico04 commented 1 year ago

The default constructor of CurrentLocationLayer is quite awesome. I just need to add a button to center the map on the current position when clicked. I could listen to positionStream and store last known position, but this data is already stored in CurrentLocationLayerState.currentPosition, and I would prefer avoid to handle streams myself. So if the state and the property were public, I could access it very easily using a GlobalKey, much simpler and cleaner.

Could you make CurrentLocationLayerState and currentPosition field public please ?

tlserver commented 1 year ago

No, you should avoid getting data from view except rendering information (see FAQ). I think example 2 is exactly what you need, otherwise you can reference to example 8 to subscribe location change.

Nico04 commented 1 year ago

Thanks for your answer. However, example 8 has leaks : if you open the page and then go back, location is still tracked and Flutter still makes redraws (which doesn't happen for default example for instance). So managing our own streams creates leaks & performance issues (at least when copying the example 8) AND is more verbose than a basic field access. Exemple 2 could work for me, but as I need to center ONLY when button is pressed (NOT following), logic will be quite overkill compared to just a field access, don't you think ?

tlserver commented 1 year ago

For leaks, please provide a MRE in new issue, I cannot reproduce the problem you mentioned in example 8. I found that no further redraws happened after page disposed.

It is only 4 lines of code to center the marker. If you get the location from state, you need to handle animation your own.

final _followCurrentLocationStreamController = StreamController<double?>(); // a field

_followCurrentLocationStreamController.close(); // in dispose

followCurrentLocationStream: _followCurrentLocationStreamController.stream, // a config param in CurrentLocationLayer()

_followCurrentLocationStreamController.add(zoom_level_or_null); // add event in onPressed callback
Nico04 commented 1 year ago

I'll open another issue for the error in example 8.

Thanks for the code sample. It works as expected, but because I have a switch that enable / disable the location (by removing widget from list), it throws Unhandled Exception: Bad state: Stream has already been listened to. when re-activating the switch. It's weird because I see in your code you properly call _followCurrentLocationStreamSubscription?.cancel();. I have to dig. Moreover name of the stream, followCurrentLocationStream, is quite misleading : I thought the map will follow the location updates, but it just center the map once (what I needed). Maybe a renaming will help ? Thanks.

tlserver commented 1 year ago

Yes, it was named center but not follow before v5.1.0. However, start from v5.2.0, CurrentLocationLayer() accept two new parameters followScreenPoint and followScreenPointOffset so that the center action is extended. Not only centering the marker but user can define a point on the screen to follow (e.g. usually near bottom for navigation mode).

Actually, the naming of the petermeter / class is always the most difficult part to me so any naming suggestions are welcome. I was hard to find out 4 words to describe 4 different behaviours for the marker motion or map (camera) motion. As you can see in the constructor of CurrentLocationLayer, there are 4 animation duration parameters, which are follow, turn, move and rotate. The 4 corresponding behaviours are:

  1. Moving the marker itself to show device location - move
  2. Rotating the marker itself to show device heading - rotate
  3. Moving the whole map to focus at the marker - follow
  4. Rotating the whole map to align(?) the marker heading - turn(?)

For these 4 behaviours, there are corresponding animation duration, animation curve parameters to config the animation behaviours. Additionally, for (3) and (4), there are corresponding signal stream (perform an action once when receive a signal) and on update trigger (perform an action or not when location or heading update).

FlutterMap v6 introduce a new concept or new name which is map camera. I think it is time to reconsider the names of these 4 behaviours. I think moveMarker, rotateMarker, moveCamera and rotateCamera are clear enough. So that we have:

Some of these names are a bit long, but I think it is acceptable. Any other suggestions?

Oh. I also found that a type error: turnHeadingUpLocationStream should be turnHeadingUpStream.

JaffaKetchup commented 1 year ago

Came here from your link on Discord. Was just wondering whether 'camera' is necessary in every name? And if length isn't too much of an issue, maybe moveMarker, rotateMarker, moveMap, and rotateMap could be an option? Other than that those names seem sensible to me, as long as they are explained in docs!

github-actions[bot] commented 11 months ago

This issue is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Nico04 commented 11 months ago

A naming suggestion, instead of HeadingUp, could be just compass ?

tlserver commented 11 months ago

No, that not this stream really mean. The word up is being with the word turn. turnHeadingUpStream accept a stream which emits "turnUp" events, so that you have a channel to tell the plugin when to rotate the map to follow the heading direction, i.e. setting the MapCamera.rotation to the value of heading. There is another headingStream for receiving heading data.

tlserver commented 11 months ago

Close as v8.0.3 is released