Rough proposed new API surface for marker dragging

[bubenheimer]

Fixes #149 🦕

This is primarily intended at this point to share the changed API surface I have in mind. Underlying rationale is in linked issue.

[arriolac]

Some questions on your rough design. I'm hoping the addition of an event lambda specific for drag events will resolve the main issue you're encountering with the current design.

I would expect a design closer to this:

@Composable
fun Marker(
  // ...
+ onDragEvent: ((DragState) -> Unit)? = null,
)

public class MarkerState(
   position: LatLng = LatLng(0.0, 0.0)
) {
   public var position: LatLng by mutableStateOf(position)

- public var dragState: DragState by mutableStateOf(DragState.END)
+ public var dragging: Boolean by mutableStateOf(false)
}

The reason the position still has to be encapsulated within MarkerState is because the Maps SDK is the owner of the marker's position, which is not the Compose way of doing this. This solution is a workaround that behavior (this is similar to why we have CameraPositionState). Let me know if this makes sense and if I'm missing something.

[arriolac]

What's the reason for removing this param? I would expect this to still be here.

[javadude]

I was thinking the same thing - we still need it to indicate if the user can drag a marker

[bubenheimer]

Thanks, having DragState available via an event handler will be useful.

In #149 I laid out in detail why I find it both necessary and feasible to also eliminate position from MarkerState. In summary, it is true that the Maps SDK owns the marker's position, but closely aligning the Compose API design with that ownership model is barely usable except in toy apps. I linked several issues caused by this design that came up since I created #149, and I am certain that this pattern will repeat until the design changes.

I agree that it is not ideal and means taking a small risk to turn the ownership model on its head, but the current approach is just too awful to use, every time I look at my workarounds there I get a stomach ache. A design change here could be confined to an experimental playground somehow. It is the lesser evil from a usability perspective.

I'd likely try to run my own fork if it cannot be resolved within the main project.

CameraPositionState is very different because there is only one camera. Hoisting it into a State object does not have a great impact on usability.

[bubenheimer]

When position is removed from MarkerState, the only way to keep track of user-changed marker position is via the onDragEvent callback. It would be rather odd to have a draggable marker without wanting to be informed about position changes via the callback, and even in that unusual case the callback can just be.an empty lambda. So the draggable parameter ends up just duplicating the presence of the callback.

[bubenheimer]

I do think that onDragEvent should receive the current Marker position as a parameter in any case, as I suggested in my original PR. Otherwise the onDragEvent lambda would need to rely on an implicit promise that MarkerState is timely updated to the position tied to the corresponding drag event. This is obscure on the API consumer's side, and would seem error-prone on the API implementation side.

[arriolac]

This is definitely the Compose way of representing a marker's position. However, since this is an interop library, the issue with this design is that the marker's position is owned by the Maps SDK. So when you drag the marker to a new location, this value is no longer correct. This is why MarkerState was introduced and needed as a workaround to the internal behavior of the Maps SDK.

[bubenheimer]

Please see my comments in #149 regarding how to keep marker position up-to-date.

[arriolac]

Moving position here would definitely match the mental model of Compose though I think changing the behavior of what the Maps SDK is doing under the hood to create a better Compose API might create more problems down the line.

I see your comment below about reworking a solution. My 2c of a summary of proposed changes that would solve your particular use case is:

• Add onDrag: (LatLng, DragEvent) -> lambda to Marker
• Deprecate MarkerState.dragState
• Add MarkerState.isDragging

[bubenheimer]

Thank you for the input, @arriolac I'm OOO right now, but should have some time to get back into this in 2 weeks

…

On Wed, Oct 19, 2022, 22:54 Chris Arriola ***@***.***> wrote: ***@***.**** commented on this pull request. Some questions on your rough design. I'm hoping the addition of an event lambda specific for drag events will resolve the main issue you're encountering with the current design. I would expect a design closer to this: @composable fun Marker( // ...+ onDragEvent: ((DragState) -> Unit)? = null, ) public class MarkerState( position: LatLng = LatLng(0.0, 0.0) ) { public var position: LatLng by mutableStateOf(position) - public var dragState: DragState by mutableStateOf(DragState.END)+ public var dragging: Boolean by mutableStateOf(false) } The reason the position still has to be encapsulated within MarkerState is because the Maps SDK is the owner of the marker's position, which is not the Compose way of doing this. This solution is a workaround that behavior (this is similar to why we have CameraPositionState). Let me know if this makes sense and if I'm missing something. ------------------------------ In maps-compose/src/main/java/com/google/maps/android/compose/Marker.kt <#150 (comment)> : > state: MarkerState = rememberMarkerState(), alpha: Float = 1.0f, anchor: Offset = Offset(0.5f, 1.0f), - draggable: Boolean = false, What's the reason for removing this param? I would expect this to still be here. ------------------------------ In maps-compose/src/main/java/com/google/maps/android/compose/Marker.kt <#150 (comment)> : > @@ -148,10 +114,10 @@ public fun rememberMarkerState( @composable @GoogleMapComposable public fun Marker( + position: LatLng, This is definitely the Compose way of representing a marker's position. However, since this is an interop library, the issue with this design is that the marker's position is owned by the Maps SDK. So when you drag the marker to a new location, this value is no longer correct. This is why MarkerState was introduced and needed as a workaround to the internal behavior of the Maps SDK. — Reply to this email directly, view it on GitHub <#150 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFIE57PY7RGNHIG34EYB7LWEBNZDANCNFSM5Y5ATBRA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[javadude]

maybe just onDrag(...)?

[bubenheimer]

Not really opposed to it, except for English semantics, which are debatable. To me onDrag sounds like a singular complete drag action to a new position. Marker dragging is a sequence of events, at least 2, usually many more. onDragEvent carries a less succinct meaning to me, it could mean anything. onDragSequenceEvent would be better from that perspective, but is verbose.

[bubenheimer]

After thinking about it some more I will take another shot at reworking my code under the assumption of MarkerState continuing to encapsulate the marker position. I expect to have an update in a few days.