android-maps-compose Change the confusing `parameter` position to a clear name. #637

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:

[x] 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
[x] Ensure the tests and linter pass
[x] Code coverage does not decrease (if any source code was changed)
[x] 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.)

Oct 16 '24 05:10 JSpiner

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.

Oct 16 '24 05:10 google-cla[bot]

I resolved the conflict.

Nov 04 '24 09:11 JSpiner

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.

Dec 02 '24 20:12 kikoso

@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)
}

Dec 14 '24 08:12 JSpiner

@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 https://github.com/googlemaps/android-maps-compose/issues/637#issuecomment-2415820993

Dec 14 '24 12:12 bubenheimer

@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 17f38572dc5ed425885e9492c7f5064d0ce78904

Dec 14 '24 13:12 JSpiner

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

Dec 16 '24 15:12 kikoso

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?