feat: Add marker drag event callback to Marker composable

[chibatching]

To observe dragged marker object, I added onMakerDrag callback lambda to Marker composable

---

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #10 🦕

[jpoehnelt]

@chibatching Please take a moment to fill out this short survey. Thank you!

This is an automated message, feel free to ignore.

[arriolac]

One minor suggestion. Would you mind also adding usage of this to the sample app to help folks how to use this?

[chibatching]

Ok, I'll add it as soon as possible!

[chibatching]

I made a wrapper class to add Immutable annotation to observe the marker object always.
But I think my class naming is not so good. So please let me know if you have any good ideas.

[chibatching]

I made changes to the sample code and fixed the observing marker state problem.

[arriolac]

I have a concern with how this is currently implemented, and may need to be redesigned due having two sources of truth for the marker's position. I proposed a new design, have a look.

[arriolac]

Hm this is an interesting consequence of this feature. The initially set position state will no longer be accurate when dragged which is already true for the existing version of the library. Having to do the above is not ideal, the Marker composable should handle this for you.

I think a more intuitive design would be to emulate how CameraPositionState works. So something like the following:

class MarkerPositionState(
    position: LatLng = LatLng(0.0f, 0.0f)
) {
    var position: LatLng by mutableStateOf(position)

    var dragState: DragState by mutableStateOf(DragState.END)
        internal set
}

This means updating the position property to accept MarkerPositionState and removal of the markerDragState as the DragState would now be accessed via MarkerPositionState.

Some pros/cons of this:

• pro: position would always be consistent as the marker is dragged
• con: would cause a breaking change
• con: may have some odd behaviors when the same state is passed to multiple marker instances

Other considerations:

• note that only the position is exposed and not the full Marker object. I'm not convinced that it should be exposed unless it is useful for the tag property to be accessed while the Marker is dragged.

[chibatching]

In my use cases, I want to access only positions.
So, it seems good to introduce MarkerPositionState. But

con: may have some odd behaviors when the same state is passed to multiple marker instances

If we want to avoid these odd behaviors, I think using a callback-based approach instead of the state-based is worth considering.
A callback-based approach could solve the problem without breaking changes.

[arriolac]

@chibatching after reading through Compose's API guidelines which states "Prefer stateless and controlled @composable functions", the callback-based approach makes more sense here over what I proposed in #11 (comment) However, I think there are a few behavior and API changes needed. These will be breaking changes and necessitate a major version update.

• markerDragState should be removed from the Marker composable. Instead, it should accept a callback called onMarkerDrag of type (Marker, DragState) -> Unit.
• it is expected that implementations of onMarkerDrag should modify a MutableState<LatLng> position (i.e. the position provided in the Marker.position composable). Failure to do so won't actually drag the marker. This way, the marker position state is fully controlled by the caller of Marker. Note that the existing implementation violates this assumption.

[chibatching]

I fixed conflicts

[arriolac]

If this line was removed markerPositionState will be out-of-sync with the marker's actual position. I don't think there's really anyway around this given how the marker's position is managed by the map. Hoisting a state object as I mentioned in #11 (comment) may be the only appropriate solution here.

[chibatching]

Ah, I'm sorry. I misunderstood and I agree with you. I'll fix it.

[chibatching]

I replaced raw position with position state completely. But should I remain position parameter or make overload?

[arriolac]

An overload would be ideal, but I think it would be a maintenance burden to support polymorphic types for position and positionState. So, I'm in favor of replacing position and markerDragState here.

[arriolac]

@chibatching I've given this a bit more taught and while "prefer stateless and controlled @composable functions" is the recommendation, taking this approach will not behave well in Maps Compose since ultimately it is an interop library of the Maps SDK. And, the Maps SDK internally manages drag state of a marker. So, trying to combat that by designing a callback-based approach as I mentioned in #11 (comment) would cause marker dragging to be jerky. This would definitely be the preferred API though if Maps Compose were a Compose native implementation of Maps SDK.

Given this, the right approach here would be to use hoisted state types as I mentioned in #11 (comment). Apologies for the circuitous decisions around the correct API though I think it's important to weigh the pros/cons here.

Let me know if you think you can update this PR with the following changes. If not, I'm happy to take it from here. Really appreciate your contributions thus far.

[chibatching]

@arriolac Please don't worry about it. I support your deeply considered decision.
I am still new to building applications using Compose, so this was a great learning experience.

I fixed my PR. If you have any problems, please don't hesitate to tell me!

[arriolac]

This PR looks good to me! Since this is a breaking change, please change the base branch to maps-compose-2.0.x. I'll create a SNAPSHOT of 2.0 so folks can try out this change.
I'm considering adding another hoisted state object that contains MarkerPositionState (like MarkerState) which also contains controls for showing/hiding info window before promoting this change to 2.0.

[arriolac]

An overload would be ideal, but I think it would be a maintenance burden to support polymorphic types for position and positionState. So, I'm in favor of replacing position and markerDragState here.

[chibatching]

@arriolac Thank you for your review!!
And also I changed the base branch to maps-compose-2.0.x.

[arriolac]

Just one more modification and then should be good to merge.

[arriolac]

This should implement a rememberSaveable and expose a configurable key similar to rememberCameraPositionState. See the implementation of rememberCameraPositionState as a reference.

[arriolac]

LGTM. Thanks @chibatching

[arriolac]

FYI MarkerPositionState was renamed to MarkerState in #69 since that object now has functionality other than controlling the marker's position (i.e. showing/hiding the info window).