feat: Add Clustering composable, maps-compose-utils library #258

DSteve595 · 2023-01-24T04:48:48Z

This adds a Compose-friendly API for marker clustering. I'd appreciate heavy scrutiny on it, since it's got some hacks and ugly bits!

MapClusteringActivity has been updated to show an example of how it can be used. Notably, the bug with Marker (and other) click handlers no longer working, as described in #140 (comment), is fixed! It's unfortunately done by working around some unwanted behavior of maps-utils, where it clobbers the click listeners that MapApplier previously set. To facilitate that, I made some changes to how MapApplier dispatches input, namely a new InputHandler composable that can be added to arbitrarily handle all unhandled input. This solves the problem for clustering since ClusterManager wants to receive input events directly, and the current implementation throws away the marker click events since we haven't added corresponding MarkerNodes for them.

To match the structure of maps-utils (which contains the non-Compose clustering utilities), this also adds a new artifact, maps-compose-utils, in which the Clustering composable lives.

To do before merging:

Update readme to mention Clustering instead of MapEffect
Update kdoc in MapEffect

Fixes #44

DSteve595 · 2023-01-24T14:24:05Z

Open question: Do we still need MapEffect? Clustering is the only use case I'm aware of for it (though there certainly could be more), and it introduces lots of footguns

StephenVinouze · 2023-01-25T08:06:22Z

Except if I've missed it, I don't see a way to customize the cluster icons. That's critical for branding and one of the pain points when using maps-compose with the former util library since you need to shape your marker and cluster icons with the View system and turn them into bitmaps (preventing us from using our Composables).

DSteve595 · 2023-01-25T15:05:43Z

Correct, that isn't accounted for in this PR. Will explore options.

ColtonIdle · 2023-01-26T20:02:48Z

Adding "true" clustering support with compose-maps would be great. I don't mind the MapEffect to be honest since it works, but something more "native" is def fine with me. But to @StephenVinouze point of customizing the marker icons and cluster icons would be like a 100% need.

Good work getting it this far @DSteve595

arriolac

Have you considered implementing Clustering as a ComposeNode<ClusteringNode, MapApplier>? The advantage of implementing it this way is that you can update underlying state in the same frame that recomposition happens. With LaunchedEffect, properties won't be updated until the next frame which means you might lose the click events

maps-compose-utils/src/main/java/com/google/maps/android/compose/clustering/Clustering.kt

arriolac · 2023-01-26T22:02:26Z

maps-compose-utils/src/main/java/com/google/maps/android/compose/clustering/Clustering.kt

+@Composable
+public fun <T : ClusterItem> Clustering(
+    items: Collection<T>,
+    cameraPositionState: CameraPositionState,


Suggestion/idea: how about making this a part of the MapApplier so that you can access it implicitly?

Was wondering if that'd be prudent. I think I'll go for it.

Leaning towards a CompositionLocal (not publicly providable), to avoid giving MapApplier even more responsibilities.

Actually, a public Composable property seems better, since then I can annotate it with @GoogleMapComposable.

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

DSteve595 · 2023-01-26T22:17:13Z

@arriolac

Have you considered implementing Clustering as a ComposeNode<ClusteringNode, MapApplier>? The advantage of implementing it this way is that you can update underlying state in the same frame that recomposition happens. With LaunchedEffect, properties won't be updated until the next frame which means you might lose the click events

I wanted to directly reference MapApplier for a few things, but since this is technically in another library (maps-compose-utils), I don't have access to it (it's internal). Forced to eat our own MapEffect dogfood.

Can you give an example of how a click event might be lost? I'm assuming you're referring to the LaunchedEffect that sets the click listeners. As I understand it, the user would have to click a marker within a single choreographer frame after an onClick lambda is changed. I don't think that's even possible, since clicks take multiple frames of input to process.

arriolac · 2023-01-26T22:24:48Z

@arriolac

Have you considered implementing Clustering as a ComposeNode<ClusteringNode, MapApplier>? The advantage of implementing it this way is that you can update underlying state in the same frame that recomposition happens. With LaunchedEffect, properties won't be updated until the next frame which means you might lose the click events

I wanted to directly reference MapApplier for a few things, but since this is technically in another library (maps-compose-utils), I don't have access to it (it's internal). Forced to eat our own MapEffect dogfood.

Can you give an example of how a click event might be lost? I'm assuming you're referring to the LaunchedEffect that sets the click listeners. As I understand it, the user would have to click a marker within a single choreographer frame after an onClick lambda is changed. I don't think that's even possible, since clicks take multiple frames of input to process.

Ahh, that's right, this is a different module altogether.

I was thinking an event can be lost in this scenario:

state triggers recomposition and you update the click handler
click event occurs
new handler hasn't been applied yet since the block inside LaunchedEffect hasn't been executed yet

It's an unlikely edge case I thought worth calling out though it would definitely have to be validated if that's even possible. Using ComposeNode would rule this out entirely but I'm hesitant about changing the scope of MapApplier for this use case.

DSteve595 · 2023-02-02T01:16:31Z

I updated the PR with new args to Clustering:

clusterContent: (@Composable (Cluster) -> Unit))?
clusterItemContent: (@Composable (T) -> Unit))?

This lets you render custom cluster icons using composables.
This also contains two other PRs, #261 and #262, which enable some of the functionality needed.

DSteve595 · 2023-02-02T14:52:20Z

@arriolac I've been trying to come up with a test case that repros missing click handler events, but I've been coming up dry. I might be missing a case you're envisioning. If you find the time, would you be able to come up with one?
I'm not considering it a blocker currently because the API itself should be fine, we can always patch the implementation.

arriolac · 2023-02-02T23:41:50Z

@arriolac I've been trying to come up with a test case that repros missing click handler events, but I've been coming up dry. I might be missing a case you're envisioning. If you find the time, would you be able to come up with one? I'm not considering it a blocker currently because the API itself should be fine, we can always patch the implementation.

Agreed that it shouldn't be blocking as the underlying implementation can change. An alternative to using LaunchedEffect in https://github.com/googlemaps/android-maps-compose/pull/258/files#diff-73d069b3c67a66f9bbaf70ba78d487309d39454a28940f02898176dabd6e6414R73 that doesn't have the downsides I mentioned is to use SideEffect as the lambda inside will be invoked in the same frame as the recomposition.

DSteve595 · 2023-02-14T17:15:22Z

Note: Exploring possibilities for removing the no-animation restriction, since that also would break async image loaders

DSteve595 · 2023-02-16T00:07:31Z

With some invalidation hacks, marker/cluster icons are now rendered continuously! This importantly fixes async image loaders (like Glide), and also allows for animations.

wangela · 2023-02-16T20:15:48Z

Noting here some decisions we have made in this PR:

Add a maps-compose-utils library even though it only contains the one utility for now, Marker Clustering.
Manage the code for the maps-compose-utils library in this repo but distribute it as a separate dependency through Maven, similar to maps-compose-widgets, to be close to the code for the related composables.
Keep documentation about the Experimental MapEffect, in case direct control of the map is needed for non-clustering functionality. Remove documentation only when MapEffect is removed (RFC: Remove experimental MapEffect composable #267).

# [2.11.0](v2.10.0...v2.11.0) (2023-02-16) ### Features * Add Marker Clustering composable, maps-compose-utils library ([#258](#258)) ([7f00a72](7f00a72))

googlemaps-bot · 2023-02-16T20:32:28Z

🎉 This PR is included in version 2.11.0 🎉

The release is available on:

v2.11.0
GitHub release

Your semantic-release bot 📦🚀

ColtonIdle · 2023-02-18T01:54:42Z

Great job @DSteve595 Can't wait to use this!

Add Clustering composable, maps-compose-utils library

eeebe1d

DSteve595 requested review from arriolac and wangela January 24, 2023 04:48

DSteve595 changed the title ~~Add Clustering composable, maps-compose-utils library~~ feat: Add Clustering composable, maps-compose-utils library Jan 24, 2023

Fix missing newlines at end

03f644e

wangela removed the request for review from arriolac January 25, 2023 00:54

wangela mentioned this pull request Jan 25, 2023

Add support for clustering #44

Closed

brezinajn mentioned this pull request Jan 25, 2023

Relax generic requirement on ClusterItem on clustering related interfaces googlemaps/android-maps-utils#779

Open

arriolac reviewed Jan 26, 2023

View reviewed changes

DSteve595 mentioned this pull request Feb 2, 2023

feat: Add public currentCameraPositionState property #262

Merged

DSteve595 mentioned this pull request Feb 7, 2023

Define custom marker images with Composables #41

Closed

DSteve595 force-pushed the clustering branch from 971a705 to 27024b4 Compare February 13, 2023 16:43

DSteve595 marked this pull request as ready for review February 13, 2023 16:43

DSteve595 mentioned this pull request Feb 13, 2023

Maps clustering: DefaultClusterRenderer crashes when setting an icon #257

Closed

DSteve595 marked this pull request as draft February 14, 2023 17:15

DSteve595 added 5 commits February 15, 2023 19:04

Merge remote-tracking branch 'upstream/main' into clustering

8ad994b

Add composable param for rendering cluster content

449a2fa

Add cluster item content

bbccc8a

Reuse single ComposeView for rendering clusters

9355411

Remove max content size, fix wrong content size after re-layouts

e09b163

DSteve595 added 3 commits February 15, 2023 19:04

Set ClusterManager listeners in SideEffect

a401079

Inline cameraPositionState

3083256

Render content continuously

af4085d

DSteve595 force-pushed the clustering branch from cae783a to af4085d Compare February 16, 2023 00:06

DSteve595 marked this pull request as ready for review February 16, 2023 00:07

DSteve595 added 2 commits February 15, 2023 19:17

Update readme with clustering

7f74f33

Re-add MapEffect to readme

8259f41

wangela approved these changes Feb 16, 2023

View reviewed changes

blunderbuss-gcf bot assigned wangela Feb 16, 2023

wangela mentioned this pull request Feb 16, 2023

RFC: Remove experimental MapEffect composable #267

Open

wangela merged commit 7f00a72 into googlemaps:main Feb 16, 2023

googlemaps-bot pushed a commit that referenced this pull request Feb 16, 2023

chore(release): 2.11.0 [skip ci]

6983e47

# [2.11.0](v2.10.0...v2.11.0) (2023-02-16) ### Features * Add Marker Clustering composable, maps-compose-utils library ([#258](#258)) ([7f00a72](7f00a72))

googlemaps-bot added the released label Feb 16, 2023

DSteve595 deleted the clustering branch February 18, 2023 02:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Clustering composable, maps-compose-utils library #258

feat: Add Clustering composable, maps-compose-utils library #258

DSteve595 commented Jan 24, 2023 •

edited

Loading

DSteve595 commented Jan 24, 2023

StephenVinouze commented Jan 25, 2023

DSteve595 commented Jan 25, 2023

ColtonIdle commented Jan 26, 2023

arriolac left a comment

arriolac Jan 26, 2023

DSteve595 Jan 26, 2023

DSteve595 Feb 2, 2023 •

edited

Loading

DSteve595 Feb 2, 2023

DSteve595 commented Jan 26, 2023 •

edited

Loading

arriolac commented Jan 26, 2023

DSteve595 commented Feb 2, 2023 •

edited

Loading

DSteve595 commented Feb 2, 2023 •

edited

Loading

arriolac commented Feb 2, 2023

DSteve595 commented Feb 14, 2023

DSteve595 commented Feb 16, 2023

wangela commented Feb 16, 2023

googlemaps-bot commented Feb 16, 2023

ColtonIdle commented Feb 18, 2023

feat: Add Clustering composable, maps-compose-utils library #258

feat: Add Clustering composable, maps-compose-utils library #258

Conversation

DSteve595 commented Jan 24, 2023 • edited Loading

DSteve595 commented Jan 24, 2023

StephenVinouze commented Jan 25, 2023

DSteve595 commented Jan 25, 2023

ColtonIdle commented Jan 26, 2023

arriolac left a comment

Choose a reason for hiding this comment

arriolac Jan 26, 2023

Choose a reason for hiding this comment

DSteve595 Jan 26, 2023

Choose a reason for hiding this comment

DSteve595 Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

DSteve595 Feb 2, 2023

Choose a reason for hiding this comment

DSteve595 commented Jan 26, 2023 • edited Loading

arriolac commented Jan 26, 2023

DSteve595 commented Feb 2, 2023 • edited Loading

DSteve595 commented Feb 2, 2023 • edited Loading

arriolac commented Feb 2, 2023

DSteve595 commented Feb 14, 2023

DSteve595 commented Feb 16, 2023

wangela commented Feb 16, 2023

googlemaps-bot commented Feb 16, 2023

ColtonIdle commented Feb 18, 2023

DSteve595 commented Jan 24, 2023 •

edited

Loading

DSteve595 Feb 2, 2023 •

edited

Loading

DSteve595 commented Jan 26, 2023 •

edited

Loading

DSteve595 commented Feb 2, 2023 •

edited

Loading

DSteve595 commented Feb 2, 2023 •

edited

Loading