[Feature request] Change the confusing markerState parameter `position` to a clear name.

[JSpiner]

MarkerState has rememberMarkerState for use in compose.

@Composable
public fun rememberMarkerState(
    key: String? = null,
    position: LatLng = LatLng(0.0, 0.0)
): MarkerState = rememberSaveable(key = key, saver = MarkerState.Saver) {
    MarkerState(position)
}

// wrong usage example
@Composable
public fun MyGoogleMapScreen(stationPosition: LatLng) {
    val myMarkerState = rembmerMarkerState(position = stationPosition)
}

But looking at the example above, it's easy to get confused. This is because there is a risk of misunderstanding that the value entered as a parameter to remember (position in this case) acts as a key that is automatically reflected when the value changes. I also had a hard time because the position wasn't updated. To avoid this misunderstanding, compose foundation libraries add an inital prefix to rememberXXX functions.

// compose library example
// rememberScrollState from `androidx.compose.foundation`
@Composable
fun rememberScrollState(initial: Int = 0): ScrollState {
    return rememberSaveable(saver = ScrollState.Saver) {
        ScrollState(initial = initial)
    }
}

// rememberLazyListState from `androidx.compose.foundation.lazy`
@Composable
fun rememberLazyListState(
    initialFirstVisibleItemIndex: Int = 0,
    initialFirstVisibleItemScrollOffset: Int = 0
): LazyListState {
    return rememberSaveable(saver = LazyListState.Saver) {
        LazyListState(
            initialFirstVisibleItemIndex,
            initialFirstVisibleItemScrollOffset
        )
    }
}

// rememberLazyGridState from `androidx.compose.foundation.lazy.grid`
@Composable
fun rememberLazyGridState(
    initialFirstVisibleItemIndex: Int = 0,
    initialFirstVisibleItemScrollOffset: Int = 0
): LazyGridState {
    return rememberSaveable(saver = LazyGridState.Saver) {
        LazyGridState(
            initialFirstVisibleItemIndex,
            initialFirstVisibleItemScrollOffset
        )
    }
}

So my suggestion is to change the name from position to initialPosition to reduce confusion. It seems like a simple fix, I'll create a PR. Please review it. Thanks!

[bubenheimer]

Personally I'd be in favor of eliminating rememberMarkerState entirely. I find it misleading and an anti-pattern:

https://dev.to/bubenheimer/effective-map-composables-non-draggable-markers-2b2#:~:text=Be%20aware%20that,recommend%20ignoring%20it.

https://dev.to/bubenheimer/effective-map-composables-draggable-markers-3bea#:~:text=This%20behavior%20is,with%20a%20model.

[dkhawk]

That is an interesting proposition. Admittedly, I have had issues with the way it works as well. Might be worth investigating as a possibility.

On Wed, Oct 16, 2024, 15:14 Uli Bubenheimer @.***> wrote:

Personally I'd be in favor of eliminating rememberMarkerState entirely. I find it misleading and an anti-pattern:

https://dev.to/bubenheimer/effective-map-composables-non-draggable-markers-2b2#:~:text=Be%20aware%20that,recommend%20ignoring%20it .

https://dev.to/bubenheimer/effective-map-composables-draggable-markers-3bea#:~:text=This%20behavior%20is,with%20a%20model .

— Reply to this email directly, view it on GitHub https://github.com/googlemaps/android-maps-compose/issues/637#issuecomment-2415820993, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2GLJ4EXEB2OMQF6IVYXLZ3X73ZAVCNFSM6AAAAABQATZMWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJVHAZDAOJZGM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[JSpiner]

@dkhawk

Yes, it can be confusing. Not only me, but my coworkers are also confused.

https://github.com/googlemaps/android-maps-compose/blob/f7f485414fda392fb7b7ce42cd4d441952b670e1/app/src/main/java/com/google/maps/android/compose/markerexamples/updatingnodragmarkerwithdatamodel/UpdatingNoDragMarkerWithDataModelActivity.kt#L150

@Composable
fun rememberUpdatedMarkerState(position: LatLng): MarkerState =
    // This pattern is equivalent to what rememberUpdatedState() does:
    // rememberUpdatedState() uses MutableState, we use MarkerState.
    // This is more efficient than updating position in an effect,
    // as we avoid an additional recomposition.
    remember { MarkerState(position = position) }.also {
        it.position = position
    }

The example code included in this repository also creates and uses rememberUpdatedMarkerState to solve this problem. I also solved the problem in a similar way. It seems okay to provide this function, but I think the first thing to do is to distinguish between initPosition and position so that there is no confusion.

https://github.com/googlemaps/android-maps-compose/pull/638 Can you review my PR and leave comments?