chore: Change the confusing parameter `position` to a clear name. #637 #638

JSpiner · 2024-10-16T05:39:19Z

Thank you for opening a Pull Request!

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 #637 🦕

It may be confusing that it is automatically updated by the position parameter. I renamed it to initialPosition to make it clear that it only works with the initial value.
(The compose foundation libraries also add the init prefix to the initial values that go into rememberXXX.)

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.

google-cla · 2024-10-16T05:39:23Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

JSpiner · 2024-11-04T09:12:53Z

I resolved the conflict.

kikoso · 2024-12-02T20:38:45Z

Hi @JSpiner . This is a reasonable proposal. Typically we would like to not drop a breaking change, but to propose a transition time for folks to change.

I think we could offer an alternative constructor to the current one, deprecate the former and leave folks one version to upgrade to the new one. We could eventually pack this PR with another breaking change.

bubenheimer · 2024-12-03T02:13:34Z

maps-compose/src/main/java/com/google/maps/android/compose/Marker.kt

         */
        @StateFactoryMarker
        public operator fun invoke(
-            position: LatLng = LatLng(0.0, 0.0)
-        ): MarkerState = MarkerState(position)
+            initialPosition: LatLng = LatLng(0.0, 0.0)


I don't see the rationale for changing the parameter name here. Makes sense for rememberMarkerState, but not here. If you end up recomposing MarkerState.invoke(position) you will get a new MarkerState object.

It may look tempting because of the "initial marker position" KDoc verbiage, but that's how State objects generally work, including MutableState. It's mutableStateOf(value="bla").

You are right. This part didn't need to be updated.
However, I completely reverted and rewrote this part based on @kikoso 's opinion.
Please review again. 🙇

JSpiner · 2024-12-14T08:53:07Z

@kikoso @bubenheimer

Following @kikoso 's suggestion, I kept the existing rememberMarkerState function without breaking changes.
All existing changes have been reverted and rewritten.

Instead of leaving the existing 'rememberMarkerState' function as is, I added a comment that the state is not updated depending on the parameter.
Added 'rememberUpdatedMarkerState' which updates the state based on the parameter.
I wrote a test code for 'rememberUpdatedMarkerState'.

I think it would be a good idea to deprecate rememberMarkerState like below. I'd like to hear your opinions.

@Composable
@Deprecated(
    message = "Use 'rememberUpdatedMarkerState' instead - It may be confusing to think " +
            "that the state is automatically updated as the position changes, " +
            "so it will be changed.",
    replaceWith = ReplaceWith(
        expression = """
            val markerState = rememberUpdatedMarkerState()
        """
    )
)
public fun rememberMarkerState(
    key: String? = null,
    position: LatLng = LatLng(0.0, 0.0)
): MarkerState = rememberSaveable(key = key, saver = MarkerState.Saver) {
    MarkerState(position)
}

bubenheimer · 2024-12-14T12:04:44Z

@JSpiner I agree with deprecating rememberMarkerState, it's a misleading API. The @ReplaceWith part seems off, though; I think it should give the original implementation of rememberMarkerState, to preserve semantics, and because users need to know where to find the Saver.

I disagree with the changed implementation of rememberUpdatedMarkerState, basing it on rememberSaveable instead of remember. This is invalid for the examples where it is used, and does not align with the rationale from the posts in #637 (comment)

JSpiner · 2024-12-14T13:43:57Z

@bubenheimer

Thanks for comments.

I disagree with the changed implementation of rememberUpdatedMarkerState, basing it on rememberSaveable instead of remember.

Sorry, my mistake. I changed it to use the original implementation.
356d429

I think it should give the original implementation of rememberMarkerState

I apply it. thanks
17f3857

bubenheimer

A maintainer should review this as well.

bubenheimer · 2024-12-16T11:11:27Z

app/src/androidTest/java/com/google/maps/android/compose/GoogleMapViewTests.kt

@@ -281,6 +282,30 @@ class GoogleMapViewTests {
            .performClick()
    }

+    @Test


Does this test actually pass?

yes it is.

When I run the test with the existing rememberMarkerState, the test fails as below.

When I run the test with rememberUpdatedMarkerState, the test succeeds as shown below.

This repo requires approval to run CI, so it cannot be verified, but you can verify it by running the tests locally.

FYI: the key is being read from the secrets in the local repo, so (unless you have them set on your fork) it will not run. Typically we run the tests on a PR copy to make sure they pass.

maps-compose/src/main/java/com/google/maps/android/compose/Marker.kt

kikoso · 2024-12-16T15:50:58Z

Also, @JSpiner : did you have the chance to take a look at the CLA agreement?

googlemaps#637

…o `initialPosition`. googlemaps#637

…meter` to `initialPosition`. googlemaps#637" This reverts commit 565007e.

…arer name. googlemaps#637" This reverts commit 2aa9d4a.

…based on updates to the input parameters.

…tedMarkerState'.

…ker.kt Co-authored-by: Uli Bubenheimer <[email protected]>

JSpiner · 2024-12-17T00:34:38Z

Also, @JSpiner : did you have the chance to take a look at the CLA agreement?

@kikoso I agreed to the CLA agreement. When I looked at the commit logs, my secondary git account name was included and was processed as a non-agreement. I re-organized them by rebasing. Thanks.

When merging @bubenheimer 's suggestion on github, he's name was included in the commit author 5761aaf. Would you also agree to the CLA?

bubenheimer · 2024-12-17T05:16:52Z

When merging @bubenheimer 's suggestion on github, he's name was included in the commit author 5761aaf. Would you also agree to the CLA?

Done

JSpiner · 2024-12-31T05:36:55Z

@kikoso
Hi, CLA validation passed. Can this be merged?

blunderbuss-gcf bot assigned dkhawk Oct 16, 2024

JSpiner force-pushed the main branch 2 times, most recently from 7c63b15 to 34a1a7d Compare October 16, 2024 05:47

JSpiner changed the title ~~Change the confusing parameter position to a clear name. #637~~ chore: Change the confusing parameter position to a clear name. #637 Nov 4, 2024

JSpiner mentioned this pull request Nov 4, 2024

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

Open

JSpiner force-pushed the main branch from 34a1a7d to 565007e Compare November 4, 2024 09:12

bubenheimer reviewed Dec 3, 2024

View reviewed changes

bubenheimer suggested changes Dec 16, 2024

View reviewed changes

JSpiner changed the title ~~chore: Change the confusing parameter position to a clear name. #637~~ chore: Change the confusing parameter position to a clear name. #637 Dec 16, 2024

JSpiner and others added 11 commits December 17, 2024 09:27

feat: Change the position parameter of MarkerState to a clearer name.

820d64e

googlemaps#637

feat: Rename example codes that were using the position parameter t…

97ed22c

…o `initialPosition`. googlemaps#637

Revert "feat: Rename example codes that were using the position `para…

a266768

…meter` to `initialPosition`. googlemaps#637" This reverts commit 565007e.

Revert "feat: Change the position parameter of MarkerState to a cle…

690d094

…arer name. googlemaps#637" This reverts commit 2aa9d4a.

feat: Add 'rememberUpdatedMarkerState' which updates the state value …

2cb2fcc

…based on updates to the input parameters.

chore: Add warning comment at rememberMarkerState.

2fbf536

feat: Modify the example project to use the newly added 'rememberUpda…

c91da9b

…tedMarkerState'.

feat: Add test case for 'testRememberUpdatedMarkerStateBeUpdate'.

7563ea8

feat: remove rememberSavable for rememberUpdatedMarkerState.

bebe715

feat: Deprecate rememberMarkerState function.

20e603e

Update maps-compose/src/main/java/com/google/maps/android/compose/Mar…

5761aaf

…ker.kt Co-authored-by: Uli Bubenheimer <[email protected]>

JSpiner force-pushed the main branch from d1665f1 to 5761aaf Compare December 17, 2024 00:31

Merge branch 'main' into main

e38948b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Change the confusing parameter `position` to a clear name. #637 #638

chore: Change the confusing parameter `position` to a clear name. #637 #638

JSpiner commented Oct 16, 2024 •

edited

Loading

google-cla bot commented Oct 16, 2024

JSpiner commented Nov 4, 2024

kikoso commented Dec 2, 2024

bubenheimer Dec 3, 2024 •

edited

Loading

JSpiner Dec 14, 2024

JSpiner commented Dec 14, 2024

bubenheimer commented Dec 14, 2024

JSpiner commented Dec 14, 2024

bubenheimer left a comment

bubenheimer Dec 16, 2024

JSpiner Dec 16, 2024 •

edited

Loading

kikoso Dec 16, 2024

kikoso commented Dec 16, 2024

JSpiner commented Dec 17, 2024 •

edited

Loading

bubenheimer commented Dec 17, 2024

JSpiner commented Dec 31, 2024

chore: Change the confusing parameter position to a clear name. #637 #638

Are you sure you want to change the base?

chore: Change the confusing parameter position to a clear name. #637 #638

Conversation

JSpiner commented Oct 16, 2024 • edited Loading

google-cla bot commented Oct 16, 2024

JSpiner commented Nov 4, 2024

kikoso commented Dec 2, 2024

bubenheimer Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

JSpiner Dec 14, 2024

Choose a reason for hiding this comment

JSpiner commented Dec 14, 2024

bubenheimer commented Dec 14, 2024

JSpiner commented Dec 14, 2024

bubenheimer left a comment

Choose a reason for hiding this comment

bubenheimer Dec 16, 2024

Choose a reason for hiding this comment

JSpiner Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

kikoso Dec 16, 2024

Choose a reason for hiding this comment

kikoso commented Dec 16, 2024

JSpiner commented Dec 17, 2024 • edited Loading

bubenheimer commented Dec 17, 2024

JSpiner commented Dec 31, 2024

chore: Change the confusing parameter `position` to a clear name. #637 #638

chore: Change the confusing parameter `position` to a clear name. #637 #638

JSpiner commented Oct 16, 2024 •

edited

Loading

bubenheimer Dec 3, 2024 •

edited

Loading

JSpiner Dec 16, 2024 •

edited

Loading

JSpiner commented Dec 17, 2024 •

edited

Loading