docs: added recomposition sample

[kikoso]

This PR adds a recomposition sample, to showcase how to remove or add markers using a remember variable and recomposition.

video_2023-10-27_16-56-20.mp4

---

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 #442, #91, #403 🦕

[wangela]

Let's also add to the "Drawing on a map" section of the README about how to update the state of composables that have been added to the map. Can just link to this activity if the code snippet gets too long.

[kikoso]

Let's also add to the "Drawing on a map" section of the README about how to update the state of composables that have been added to the map. Can just link to this activity if the code snippet gets too long.

This has been added, @wangela

[bubenheimer]

@wangela May I ask why these commits were merged without further discussion? I pointed out a problem, yet my comments were ignored. This action does not promote a healthy open-source culture for this project. I will need to assess the relevance of future contributions from my side.

I respect that there may be differences in opinion, but there needs to be discussion to resolve those. Apart from my prior pointers: the API provides a dedicated function rememberMarkerState() to maintain MarkerState. It is unclear why the project documentation and samples would promote a different approach that does not actually remember the state object itself, but continuously recreates it.

I tried to voice my concerns in a helpful, professional manner, but perhaps I was too subtle? I would not go through the effort of commenting unless I see a strong need for changes.

The present action suggests that I and other users of this project will need to find alternative means to restore and maintain sufficient API quality for our own projects in the face of ongoing regressions since @arriolac's departure. For example the present documentation regression, as well as #466 and #347. I appreciate that #347 was reverted after I voiced my concerns, but this PR points in a different direction. The existence of #466 should have rung alarm bells.

The end result of all this is a necessity for private forks. I do not see a viable alternative, unless there are changes to how this project is managed. Seeing #347 gave me a scare (its problems should have been caught in code reviews, and it even required changing a test to make it pass), so I started to review every PR personally in an effort to catch serious regressions early, but now I am finding that my input is ignored. Private forks are expensive to maintain for independent devs like myself, so I wish this could be resolved in a different manner.

[kikoso]

Hi @bubenheimer ,

Thanks for your comment. Regarding this Pull Request, it has been open for a month. I think the expectation was to write any comment or concern here, but there was none. Any issue is open to any feedback, as you can see in #347. If you had any, you could comment at any time given time, and a discussion could have been started. Sorry if this came as ignoring any feedback, it was definitely not the intention.

This PR showcases how to change a marker based on a changing location. I believe what you want to point out is to use the following to update the location:

val singaporeState = rememberMarkerState(position = location)

which makes sense, and it is pointing out how to use the rememberMarkerState() instead. The original PR wanted to showcase a recomposition example, but this makes sense to showcase the usage of rememberMarkerState()`. But there was no comment on this PR that we could have leveraged to start any discussion.

[bubenheimer]

@kikoso I'm not sure what you mean about "no comment" on this PR. I started a code review right here in this PR soon after your initial commit, with 4 comments total. You did not respond to the concerns that I brought up in the context of the review. It's all still up there. Please clarify.

[bubenheimer]

As for whether to use rememberMarkerState() specifically: it's not essential, it just remembers 'MarkerState'.

What I'm saying is that it does not make sense to recreate MarkerState on recomposition, and remember a separate mutableStateOf() for the location. MarkerState is the relevant state object here and needs to be the one to be remembered. MarkerState encapsulates more than just location.

The merged commit demonstrates a bad pattern. I hope it can be unmerged.

[kikoso]

@kikoso I'm not sure what you mean about "no comment" on this PR. I started a code review right here in this PR soon after your initial commit, with 4 comments total. You did not respond to the concerns that I brought up in the context of the review. It's all still up there. Please clarify.

@bubenheimer , there are no comments in this PR. Did you forget to click the send review button? It is easy to miss.

Screen.Recording.2023-12-10.at.14.16.17.mov

[bubenheimer]

MarkerState.position is a var with a public setter and a mutableStateOf behind it, and it is observed for changes by Marker() machinery. So it is not necessary to keep a separate remembered mutableStateOf to track position changes, or to recreate MarkerState on every recomposition.
Consider this instead:

val singaporeState = remember { MarkerState(singapore) }

Then update via:

singaporeState.position = when (randomValue) {
    0 -> singapore
    1 -> singapore2
    2 -> singapore3
    else -> singapore

[bubenheimer]

@wangela @kikoso if this pattern does not work then there has been a regression that needs to be fixed. Various recently filed issues and my own testing suggest that this is the case. The pattern used to work flawlessly and is the way the API is intended to be used; it's described at https://github.com/androidx/androidx/blob/androidx-main/compose/docs/compose-api-guidelines.md#hoisted-state-types and subsequent sections.

Recreating MarkerState on every recomposition is akin to the following, which the IDE will tell you is missing 'remember':

val state = mutableStateOf(1)

[bubenheimer]

@wangela @kikoso FYI: the regression (if it is just a single one) was introduced in 2.14.1

[kikoso]

Hi @bubenheimer ! I can see your point. Here I was trying to showcase how to recompose the map based on the location attribute, but it does introduce more confusion than another thing. Your approach makes sense.

I am updating the docs and PR-ing the changes.

[bubenheimer]

Same concern as in sample

[kikoso]

Changed too.

[bubenheimer]

Thanks @kikoso, I must have missed clicking the button initially, and I think the button may have stopped working later after force push & merge/closure. I've added my prior comments as regular comments now, to show what I had in mind. I seem no longer able to start a review on a closed PR.

My sincere apologies for the noise & waste of time on your end.

Would it be possible to change the sample & docs still? I don't think there is any valid use case where one would not want to retain MarkerState across recompositions, short of bug workarounds perhaps.

That said, there certainly is a common need to sync MarkerState with information that is kept outside Compose. This would be a slightly larger example. An important example, though.

[googlemaps-bot]

🎉 This PR is included in version 4.3.1 🎉

The release is available on:

• v4.3.1
• GitHub release

Your semantic-release bot 📦🚀