Marker dragging: fix loss of drag events and difficulty observing marker positions

[bubenheimer]

android-maps-compose 2.2.1

The current API design of Marker and MarkerState poses significant challenges. This issue is a continuation of comments I made in #10; @arriolac asked to move discussion to a new issue.

1. 'DragState' events are lost, in particular START, but also DRAG, and, theoretically, even END.

This is due to the characteristics of snapshot state in relation to events:

Observable state is a lossy compression of the events that produced that state.

https://developer.android.com/reference/kotlin/androidx/compose/runtime/package-summary#snapshotFlow(kotlin.Function0)

I have a use case where I need to reliably detect START and END to temporarily keep a backup of the old marker location to restore in case the final marker drag location is deemed invalid by business logic.

2. Hoisted state objects are not suitable for representing collections of app marker data.

Hoisted state objects can be a suitable vessel for maintaining UI state. However, they are an anti-pattern for representing data that is ultimately maintained in an app data model. Observing requires using snapshotFlow for each indivdual state object, and updating implies feeding model data back to the state object in some manner.

An app commonly maintains a collection of markers, so a collection of MarkerState objects is needed, one for each marker. Markers may be added or removed, adding significant complexity to the task of keeping separate collections of marker data in sync. Typically one will opt to recreate the entire collection if one element changes in any way.

I will add more detail and proposals in comments below.

Clarification: In my main use case for this feature, markers define the corners of a polyline/polygon, so order is significant, and the marker collection is a list, where updates, inserts, and deletions can occur at any position. Keeping lists of variable size with changing marker content synchronized is a more challenging problem than for example maps of markers where order is not significant; I use the latter in another case, and it is easier to deal with.

[bubenheimer]

Regarding the loss of DragState events: the MarkerState.dragState property needs to be replaced with a boolean MarkerState.dragging property. This is the only thing that can be validly represented as state.

To make DragState events available in a reliable manner, a callback on the Marker class should be made available, which is passed a DragState, and the current marker position. This callback can come directly from the Play Maps SDK dragging callbacks. Additional detail below.

[bubenheimer]

Regarding problems with hoisted state objects: remove position from MarkerState and replace by the standard Compose approach of hoisting state: pass position from the user's data model into the Marker composable as a parameter, and pass back updated position via the DragState callback mentioned above.

I believe it is possible to completely separate the SDK's position changes and app-controlled position changes. I'm assuming that the only time the map SDK impl changes the marker position on its own is dragging. Dragging is reliably demarcated by START and END events.

maps-compose needs to box the SDK's dragging events. Dragging position should not generally be reflected in maps-compose marker position, but only be updated in response to Marker recomposition with a new position value (which would typically be triggered from the marker dragging callback in some indirect way).

To avoid visual artifacts, compose-maps needs to delay effectuating visual changes in Marker's position parameter until drag END (a.k.a. wait until drag END before informing the Maps SDK about whatever position maps-compose has at that point). Essentially maps-compose needs to treat marker position changes from the SDK as tentative, and "restore" or "impose" whatever position it has at the time of drag END.

Typically dragging callbacks will update Marker's position parameter with whatever position is passed as a parameter, via hoisted state or app data model. This is the common way of structuring a compose API with hoisted state (pass in state value as parameter, receive changed values via callback, update parameter via recomposition), so the only thing that needs explanation is the SDK-controlled marker position during a drag, requiring deferred visual updates for the compose-controlled position parameter.

The boolean Marker.draggable parameter should be replaced with the dragging callback lambda, with null passed for dragging disabled.

The above proposal turns Map SDK's marker drag design on its head, replacing it with the default API a compose user would expect, and it can do it very cleanly, I believe.

The only gotcha I can think of is an app's inability to influence the actual marker position on the map while the marker is being dragged. This is generally a bad idea due to visual artifacts, but there is one case I can think of where an app may choose to do it despite artifacts; more in a future comment.

[bubenheimer]

The use case I was alluding to in the last comment is marker dragging and keeping the marker from jumping out of place. When the marker starts dragging, Maps SDK moves the marker up and out of place a good amount to make it visible from underneath a user's "finger". This approach can cause a lot of problems, so in some cases one might choose to manually override the marker position upon each START/DRAG/END event to put the marker back in place. This causes visual artifacts, and is a grossly unsupported hack of map's data structures, but depending on the use case one might choose to try it anyway.

Overriding marker drag position manually in this fashion would not work with the approach I sketched. Marker position set via composition during dragging will take effect only after dragging is finished.

The issues with marker jump on drag were previously brought up with Maps SDK and dismissed. The main issue I saw is this: https://issuetracker.google.com/issues/35824023.

If this is a concern then the best approach may be to open a new issue with Maps SDK. Perhaps it would be addressed with added leverage from @arriolac. The marker jump problem affects one of my use cases as well, where the marker needs precise repositioning upon dragging.

Alternatively, hacky use cases like this could likely be implemented via the new direct access to Maps SDK's GoogleMap from maps-compose.

[bubenheimer]

N.B. I've reviewed the use cases and concerns about prior implementations and API designs from #10 and #11. I believe the API I am proposing in this issue addresses them.

[bubenheimer]

Upon implementing this new API, a public MarkerState object is no longer particularly meaningful and should be refactored. The only remaining stateful element is the dragging boolean, which by itself does not look valuable enough to carry the weight of a whole state object.

[bubenheimer]

Thoughts?

[arriolac]

Thanks @bubenheimer for creating this issue. This is very detailed and helps us improve the API for marker dragging. I agree with a lot of the proposed changes—it will be a breaking change though and I'll label it as such. We may want to release this with any other breaking change requests/issues in the repo.

[bubenheimer]

Thanks @arriolac, makes sense. I made one clarification in the original comment that my primary use case is changing lists of markers, which is a more challenging problem than unordered collections.

[bubenheimer]

I have run a few experiments since my last comment in June, and what I've arrived at conforms with what I described above.

In particular, I still believe MarkerState.position should be hidden because it makes Marker usage needlessly and oddly complex; it only forces users to reinvent the wheel. MarkerState.position does make sense behind the scenes.

MarkerState.position faithfully carries forward the issue of dual sources of truth from the Google Maps Marker API. This is unavoidable. It implies:

1. During Marker drag, updates from Google Maps Marker are the source of truth. These updates are surfaced via drag event callbacks.
2. Outside of Marker drag, updates from app data are the source of truth.

This behavior should be embodied in code by the Maps Compose API, not by the user. At that point MarkerState.position is no longer needed.

[arriolac]

@bubenheimer Are you able to share a minimum viable sample demonstrating the usability issues perhaps in a fork of the sample app? I think this'll give us a bit more color on how the current API is hard to work with in your use case. Thanks for your continued feedback on this issue.

[bubenheimer]

Just had to get a gist ready with a simple wrapper for Marker(). The relevant code is small, and I'd expect not much extra for the API: https://gist.github.com/bubenheimer/fe7580248a7f7f905f05b70ceaeb97ed

NB: I am proposing this as a general approach, regardless of use case. I don't think the exposed MarkerState.position has a real benefit, it just adds complexity and obscurity. For backwards compatibility and potential odd use cases the lower level API could stick around, but most users should use the wrapper and not deal with MarkerState.position

[bubenheimer]

Most importantly, the contract here is that the app's data model is not the sole source of truth, and updates from marker dragging need to be carried forward to the app's data model via a drag event handler. This could be worked around by the more involved approach I described in prior comments, yet that approach does not seem preferable.

[bubenheimer]

For justification of usability issues: it's not obvious how to get this kind of wrapper right, e.g. #198. It took me a bit myself to get the logic just right for updating MarkerState.position with app position. Of course, the wrapper could be made available as sample code instead, but not using the wrapper simply does not really make sense in the first place and makes things confusing, that's why I am saying that the API should take care of it, and the user has one less headache to deal with, or just a lighter one at least.

[bubenheimer]

One potential problem area is races between the 2 sources of truth at the time of switching (start/end of drag), which would require careful coding to know the actual truth. Perhaps not the most common scenario, but needs consideration.

[bubenheimer]

I think addressing these races can be left to the API user. A Marker change driven by the app data model can always be imposed cleanly by going down a different Compose graph subtree via key(), which would delete the old Marker and create a new one. I've also uploaded a new version of the gist, which is capable of deciding the race.

[bubenheimer]

There are small, but significant changes in the latest version of my gist: https://gist.github.com/bubenheimer/fe7580248a7f7f905f05b70ceaeb97ed

In particular: Marker position can now be set from the dragging handler. This is particularly relevant at the end of a drag sequence, where an app may decide to accept, discard, or change the final marker position. The handler can promptly change the marker position in the middle of a drag operation as well, to be on par with existing Maps SDK capabilities. (And without going through recomposition which could cause additional visual artifacts.) Of course, this is not really recommended. It effectively offers all the capabilities of the Maps SDK without needing to expose MarkerState.position directly to composition.

The other gist changes are smaller refinements and bug fixes.

The gist is the essence of what I think the Maps Compose API changes should look like.

[bubenheimer]

I don't believe the semver:major label is accurate anymore. The proposed change is non-breaking.