Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add experimental MapEffect composable #140

Merged
merged 3 commits into from
Jun 14, 2022
Merged

feat: Add experimental MapEffect composable #140

merged 3 commits into from
Jun 14, 2022

Conversation

arriolac
Copy link
Contributor

@arriolac arriolac commented Jun 8, 2022

Create MapEffect composable to expose the underlying GoogleMap Maps SDK object to enable extension and usages such as clustering with the utility library.

Example:

@OptIn(MapsComposeExperimentalApi::class)
@Composable
fun GoogleMapClustering(items: List<MyItem>) {
    val cameraPositionState = rememberCameraPositionState {
        position = CameraPosition.fromLatLngZoom(singapore, 10f)
    }
    GoogleMap(
        modifier = Modifier.fillMaxSize(),
        cameraPositionState = cameraPositionState
    ) {
        val context = LocalContext.current
        var clusterManager by remember { mutableStateOf<ClusterManager<MyItem>?>(null) }
        MapEffect(items) { map ->
            clusterManager = ClusterManager<MyItem>(context, map)
            clusterManager?.addItems(items)
        }
        LaunchedEffect(key1 = cameraPositionState.isMoving) {
            if (!cameraPositionState.isMoving) {
                clusterManager?.onCameraIdle()
            }
        }
        MarkerInfoWindow(
            state = rememberMarkerState(position = singapore),
            onClick = {
                // This won't work :(
                Log.d(TAG, "I was clicked $it")
                true
            }
        )
    }
}

Note: this is an experimental API and requires explicit opt in to MapsComposeExperimentalApi.

Fixes #135 🦕

@arriolac arriolac changed the title feat: Create MapEffect feat: Add experimental MapEffect composable Jun 9, 2022
Change-Id: I6612683d4c67d350dd0b66310e98108b564b73e3
Copy link
Contributor

@barbeau barbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arriolac! Some comments in-line.

While there is certainly demand for this feature and this seems like the most likely near-term fix, I also wonder if this is opening a Pandora's box for all kinds of bugs. There are already known gotchas with AMU and using multiple features simultaneously, and it seems like this is piling on top of those. I do like marking this as experimental, largely for this reason. Any other thoughts on mitigating potentially bad experiences is welcome.

README.md Show resolved Hide resolved
app/build.gradle Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ColtonIdle
Copy link

For what it's worth, I love the idea of this being added in an experimental state for the time being. Since I can't use clustering in android-maps-compose, using this library is a non-starter, (which is a shame because this library already includes a bunch of fixes and stuff I've run into using AndroidVIew + google maps without clustering. see: the lifecycle issue created in the issuetracker when you use AndroidVIew + google maps in a tab bar layout)

It seems like knowing that this is a temporary solution would at least allow way more people to adopt this library, and then in the 6months+ it takes to add clustering as a first class citizen we could at least build up to that.

of course. im bias because i tore my hair our trying to get androidView + google maps + tabs + clustering to work so i definitely rather use something that makes no promises, but is kinda supported by the maintainers here + community, vs the frankenstein thing I hacked together. lol

cheers and thanks for considering

…gActivity.kt

Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com>
@arriolac
Copy link
Contributor Author

Thanks @ColtonIdle for providing color to your use case. I thought of this feature precisely to address the issue you faced—it seems to be a really common workflow based on reports I've seen from the community.

Thanks @arriolac! Some comments in-line.

While there is certainly demand for this feature and this seems like the most likely near-term fix, I also wonder if this is opening a Pandora's box for all kinds of bugs. There are already known gotchas with AMU and using multiple features simultaneously, and it seems like this is piling on top of those. I do like marking this as experimental, largely for this reason. Any other thoughts on mitigating potentially bad experiences is welcome.

@barbeau this is definitely not a feature that I originally intended on adding (I think we had a convo about why we choose not to expose the raw GoogleMap before). However, looking at some of the common themes the community is facing (as @ColtonIdle has pointed out), an escape hatch is very much needed for extensibility. My 2c is that as long as we have the right guardrails in place (docs, explicit opt-in with the new MapsComposeExperimentalApi), this solution is much better than developers having to copy/paste parts of Maps Compose code in their projects to get a feature like clustering to work. This is also considering that a Compose version of clustering is yet to be designed and implemented which is no easy feat. Lmk if this makes sense—I'm open to more ideas on how we can integrate this safely for the community.

Change-Id: I84922456c00314ab3a622385accd088980fa0695
Copy link
Contributor

@barbeau barbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arriolac. I agree that all things considered the best way to move forward at this point is with this as an experimental feature.

@wangela wangela merged commit 8ed7eb4 into main Jun 14, 2022
@wangela wangela deleted the chris/clustering branch June 14, 2022 22:17
@newmanw
Copy link

newmanw commented Jun 21, 2022

This is awesome, thanks @arriolac!

Curious about a couple things, not that I am looking for anything specific, just curious.

  1. Are there still plans to implement maps compose clustering?
  2. Currently maps compose is just a wrapper around the maps sdk, will this ever go "compose first"?

googlemaps-bot pushed a commit that referenced this pull request Jun 21, 2022
# [2.3.0](v2.2.1...v2.3.0) (2022-06-21)

### Features

* Add experimental MapEffect composable ([#140](#140)) ([8ed7eb4](8ed7eb4))
@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@arriolac
Copy link
Contributor Author

This is awesome, thanks @arriolac!

Curious about a couple things, not that I am looking for anything specific, just curious.

  1. Are there still plans to implement maps compose clustering?
  2. Currently maps compose is just a wrapper around the maps sdk, will this ever go "compose first"?

Hey @newmanw, thanks for the questions.

  1. This is definitely still desirable. I imagine once that's completed MapEffect should be evaluated for deprecation unless there's another common use case folks want to use it for. You can follow Add support for clustering #44.
  2. Android is pretty invested in Compose so I imagine this could happen. No plans atm as far as I'm aware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose raw GoogleMap for extensibility
6 participants