Conversation
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
d5b5d7a to
f4a5753
Compare
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
f4a5753 to
407fded
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Code Coverage
Files |
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
407fded to
96b0801
Compare
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
96b0801 to
ff8f979
Compare
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
d00d357 to
f1a4793
Compare
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
5cdce08 to
3ce966a
Compare
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
3ce966a to
5f6c90f
Compare
dkhawk
changed the title
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
5f6c90f to
a2164ec
Compare
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
a2164ec to
a0a837d
Compare
dkhawk
changed the title
| ### Bug Fixes | ||
|
|
||
| * prevent StackOverflowError when parsing deeply nested KML containers and multi-geometries ([#1710](https://github.com/googlemaps/android-maps-utils/issues/1710)) ([1463cc5](https://github.com/googlemaps/android-maps-utils/commit/1463cc572da85b8a2317690fa94b0e2432995a96)) | ||
| >>>>>>> origin/main |
There was a problem hiding this comment.
is there any purpose for adding this ?
There was a problem hiding this comment.
Good catch! Reverted CHANGELOG.md to match origin/main in 85cb2fbf so release-please manages all changelog and version updates cleanly.
| } | ||
| } | ||
| systemProperty("user.home", testHome.absolutePath) | ||
| environment("ANDROID_HOME", System.getenv("ANDROID_HOME") ?: "/usr/local/google/home/dkhawk/Android/Sdk") |
There was a problem hiding this comment.
Do we really need this "/usr/local/google/home/dkhawk/Android/Sdk" path ?
There was a problem hiding this comment.
Removed the hardcoded local path fallback in build.gradle.kts (85cb2fbf).
| public fun GoogleMap.cameraEvents(): Flow<CameraEvent> = this.canonicalCameraEvents() | ||
|
|
||
| @Deprecated("Moved to com.google.maps.android.awaitAnimateCamera", ReplaceWith("awaitAnimateCamera(cameraUpdate, durationMs)", "com.google.maps.android.awaitAnimateCamera")) | ||
| public suspend inline fun GoogleMap.awaitAnimateCamera(cameraUpdate: CameraUpdate, durationMs: Int = 3000): Unit = this.canonicalAwaitAnimateCamera(cameraUpdate, durationMs) |
There was a problem hiding this comment.
When calling awaitAnimateCamera(), we now need to pass 0 if we don't want to set a duration. Is there a reason we didn't make this parameter optional?
There was a problem hiding this comment.
Fixed in 85cb2fbf — restored durationMs: Int? = null as the default parameter in awaitAnimateCamera (and in the CameraUpdate overload) so callers don't have to pass a dummy duration when using default timing.
| */ | ||
| public suspend inline fun GoogleMap.awaitAnimateCamera( | ||
| cameraUpdate: CameraUpdate, | ||
| durationMs: Int = 3000 |
There was a problem hiding this comment.
When calling awaitAnimateCamera(), we now need to pass 0 if we don't want to set a duration. Is there a reason we didn't make this parameter optional ?
There was a problem hiding this comment.
Fixed in 85cb2fbf — restored durationMs: Int? = null as the default parameter in the canonical awaitAnimateCamera extensions as well.
| var callbackInvoked = false | ||
| val status = MapsInitializer.initialize(this, preferredRenderer) { renderer -> | ||
| callbackInvoked = true | ||
| continuation.resume(renderer) |
There was a problem hiding this comment.
If status != ConnectionResult.SUCCESS, the continuation resumes with an exception immediately. If Play Services later invokes the OnMapsSdkInitializedCallback asynchronously (or if the coroutine was cancelled while waiting), calling continuation.resume(renderer) without checking if (continuation.isActive), it might be throw IllegalStateException
What did you think on this ??
Fix: Guard resumption with if (continuation.isActive) continuation.resume(...), and add continuation.invokeOnCancellation { setOnMapLoadedCallback(null) } to awaitMapLoad()
There was a problem hiding this comment.
Fixed in 85cb2fbf — guarded the continuation with if (continuation.isActive) before invoking continuation.resume(...) so a late OnMapsSdkInitializedCallback after an immediate status != ConnectionResult.SUCCESS failure cannot throw IllegalStateException: Already resumed.
| LocationManager.PASSIVE_PROVIDER | ||
| } | ||
|
|
||
| requestLocationUpdates(provider, minTimeMs, minDistanceM, listener) |
There was a problem hiding this comment.
Unlike FusedLocationProvider.kt(which defaults to looper: Looper = Looper.getMainLooper()), LocationManager.coarseLocationEvents and fineLocationEvents call the 4-argument requestLocationUpdates(provider, minTimeMs, minDistanceM, listener) without a Looper. If collected on Dispatchers.IO or Dispatchers.Default, LocationManager calls Looper.myLooper() and crashes immediately with RuntimeException: Can't create handler inside thread that has not called Looper.prepare().
Fix: Add looper: Looper = Looper.getMainLooper() parameter to coarseLocationEvents and fineLocationEvents and pass it to requestLocationUpdates(provider, minTimeMs, minDistanceM, listener, looper).
There was a problem hiding this comment.
Fixed in 85cb2fbf — added looper: Looper = Looper.getMainLooper() to LocationManager.coarseLocationEvents and passed it to requestLocationUpdates(..., looper) so background collectors don't fail when lacking a prepared Looper.
| } | ||
| } | ||
|
|
||
| requestLocationUpdates(LocationManager.GPS_PROVIDER, minTimeMs, minDistanceM, listener) |
There was a problem hiding this comment.
Unlike FusedLocationProvider.kt(which defaults to looper: Looper = Looper.getMainLooper()), LocationManager.coarseLocationEvents and fineLocationEvents call the 4-argument requestLocationUpdates(provider, minTimeMs, minDistanceM, listener) without a Looper. If collected on Dispatchers.IO or Dispatchers.Default, LocationManager calls Looper.myLooper() and crashes immediately with RuntimeException: Can't create handler inside thread that has not called Looper.prepare().
Fix: Add looper: Looper = Looper.getMainLooper() parameter to coarseLocationEvents and fineLocationEvents and pass it to requestLocationUpdates(provider, minTimeMs, minDistanceM, listener, looper).
There was a problem hiding this comment.
Fixed in 85cb2fbf — added looper: Looper = Looper.getMainLooper() to LocationManager.fineLocationEvents (and the KTX shims) as well.
| * **How we know it is correct:** Asserts the emitted coordinates equal the exact `target` LatLng. | ||
| */ | ||
| @Test | ||
| public fun testMapClickEvents(): Unit = runTest { |
There was a problem hiding this comment.
Currently all test cases are false positive.
If we pass assertThat(event).isEqualTo(LatLng(9999.0, 9999.0)) or even if onMapClick(target) is never called at all, the test still pass, because the assertion line is dead code that gets cancelled before execution.
Solution :
Use async { ... } and deferred.await()
Using async instead of launch is cleaner, shorter, and eliminates manual job.cancel() calls altogether:
@test
public fun testMapClickEvents(): Unit = runTest {
val target = LatLng(10.0, 20.0)
val deferred = async {
googleMap.mapClickEvents().first()
}
advanceUntilIdle()
verify(googleMap).setOnMapClickListener(mapClickListener.capture())
mapClickListener.value.onMapClick(target)
assertThat(deferred.await()).isEqualTo(target)
}
Note : We need to do similar kind of handling for all the test cases
There was a problem hiding this comment.
Great catch! Fixed in 85cb2fbf — replaced the fire-and-forget launch { ... collect { ... } }; job.cancel() pattern with val deferred = async(start = CoroutineStart.UNDISPATCHED) { ... .first() } and assertThat(deferred.await()).isEqualTo(...) across all tests in GoogleMapTest.kt and ktx/GoogleMapTest.kt so every assertion is guaranteed to execute.
| } | ||
|
|
||
| @Test | ||
| public fun testClusterInfoWindowClickEvents(): Unit = runTest { |
There was a problem hiding this comment.
Currently all test cases are false positive.
If we pass any cluster value still test will pass, because the assertion line is dead code that gets cancelled before execution.
Solution :
Use async { ... } and deferred.await()
Using async instead of launch is cleaner, shorter, and eliminates manual job.cancel() calls altogether:
@test
public fun testClusterInfoWindowClickEvents(): Unit = runTest {
val deferred = async {
clusterManager.clusterInfoWindowClickEvents().first()
}
advanceUntilIdle()
verify(clusterManager).setOnClusterInfoWindowClickListener(clusterInfoWindowClickListener.capture())
clusterInfoWindowClickListener.value.onClusterInfoWindowClick(cluster)
assertThat(deferred.await()).isEqualTo(cluster)
}
Note : We need to do similar kind of handling for all the test cases
There was a problem hiding this comment.
Fixed in 85cb2fbf — refactored all Flow tests in ClusterManagerTest.kt to use async(start = CoroutineStart.UNDISPATCHED) { ... .first() } + deferred.await() so assertions are strictly evaluated.
| } | ||
|
|
||
| @Test | ||
| public fun testMarkerCollectionClickEvents(): Unit = runTest { |
There was a problem hiding this comment.
test case is false positive.
If we pass any Marker value here, still this test will pass, because the assertion line is dead code that gets cancelled before execution.
Solution :
Use async { ... } and deferred.await()
Using async instead of launch is cleaner, shorter, and eliminates manual job.cancel() calls altogether:
@Test
public fun testMarkerCollectionClickEvents(): Unit = runTest {
val deferred = async {
markerCollection.clickEvents().first()
}
advanceUntilIdle()
// Trigger the event via our tracked active listener slot!
assertThat(activeMarkerClickListener).isNotNull()
activeMarkerClickListener?.onMarkerClick(marker)
assertThat(deferred.await()).isEqualTo(marker)
}
Note : We need to do similar kind of handling in other test cases also
There was a problem hiding this comment.
Fixed in 85cb2fbf — refactored all Flow tests in CollectionManagersTest.kt (both canonical and KTX suites) to use async(start = CoroutineStart.UNDISPATCHED) { ... .first() } + deferred.await().
| espresso-core = { group = "androidx.test.espresso", name = "espresso-core", version.ref = "espresso-core" } | ||
| junit = { module = "junit:junit", version.ref = "junit" } | ||
| mockito-core = { module = "org.mockito:mockito-core", version.ref = "mockito-core" } | ||
| mockito-kotlin = { module = "com.nhaarman.mockitokotlin2:mockito-kotlin", version.ref = "mockito-kotlin" } |
There was a problem hiding this comment.
any specific reason, why we use this instead of "org.mockito.kotlin:mockito-kotlin" lib ?
There was a problem hiding this comment.
Migrated from com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0 to org.mockito.kotlin:mockito-kotlin:5.4.0 in 85cb2fbf and updated all test imports to org.mockito.kotlin.*.
| dependsOn(generateArtifactIdFile) | ||
| } | ||
|
|
||
| tasks.named("dokkaGeneratePublicationHtml") { |
There was a problem hiding this comment.
Removed the duplicate testImplementation declarations across library/build.gradle.kts, clustering/build.gradle.kts, data/build.gradle.kts, heatmaps/build.gradle.kts, and ui/build.gradle.kts in 85cb2fbf.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
kikoso
left a comment
kikoso
left a comment
There was a problem hiding this comment.
Second pass, this time anchored to the specific files. Most of it is packaging and release config rather than the Kotlin itself, plus one follow up on the inline thread.
| * suppressing default SDK behavior (such as zooming). Under backpressure if the buffer is full, | ||
| * `trySend` returns `false`, allowing default SDK click handling to proceed. | ||
| */ | ||
| public fun <T : ClusterItem> ClusterManager<T>.clusterClickEvents(): Flow<Cluster<T>> = |
There was a problem hiding this comment.
These six functions are public and return Flow<...>, but clustering/build.gradle.kts still has implementation(libs.kotlinx.coroutines.android). The api(coroutines) you added to :library doesn't help either, since clustering depends on it with implementation.
So the published POM for android-maps-utils-clustering puts coroutines at runtime scope, and an app calling clusterClickEvents() gets "cannot access class kotlinx.coroutines.flow.Flow". Our unit tests won't catch it because they compile inside the module.
Can you move it to api(libs.kotlinx.coroutines.core) and check the other modules while you're there?
There was a problem hiding this comment.
Fixed in 85cb2fbf — changed kotlinx-coroutines-core to api(libs.kotlinx.coroutines.core) and kept kotlinx-coroutines-android as implementation(libs.kotlinx.coroutines.android) in clustering/build.gradle.kts.
| api(libs.play.services.maps) | ||
| implementation(libs.kotlinx.coroutines.android) | ||
| api(libs.play.services.location) | ||
| api(libs.kotlinx.coroutines.android) |
There was a problem hiding this comment.
Only coroutines-core is actually in our API surface. coroutines-android is just the Dispatchers.Main artifact, so implementation is enough for it.
Separately on line 66: api(play-services-location) means everyone pulling maps-utils now gets play-services-location, including people who only wanted :data or :heatmaps. AGENTS.md also asks for an issue before adding deps to library modules. See my note on the demo file about where this package should live.
One more while this file is open: explicitApi() isn't enabled anywhere and we have no binary compatibility validator, yet we're adding roughly 200 public symbols in a major. All the new code already writes public explicitly so flipping explicitApi() on is nearly free, and an .api dump in version control would back up the compatibility promise the shim layer is making. Happy for that to be a follow up if you'd rather not grow this PR.
There was a problem hiding this comment.
Fixed in 85cb2fbf & 7077a63d — changed kotlinx-coroutines-core to api(...), kotlinx-coroutines-android to implementation(...), and play-services-location to compileOnly(...) (with -dontwarn com.google.android.gms.location.** in library/consumer-rules.pro), so play-services-location is omitted from the published POM and only required by consumers who actually invoke FusedLocationProviderClient extensions.
For explicitApi() and Binary Compatibility Validator (.api dumps), filed #1794 as a fast-follow assigned to me so we can enable explicitApi() and check in baseline .api dumps across all published modules right after this lands.
| android:value="androidx.startup" /> | ||
| <meta-data | ||
| android:name="com.google.maps.android.ktx.utils.attribution.AttributionIdInitializer" | ||
| android:value="androidx.startup" /> |
There was a problem hiding this comment.
This registers the attribution initializer a second time, so both it and the canonical one call addInternalUsageAttributionId with the same value on every app start.
The shim class is also internal, so nobody outside the module could ever have referenced it and the @Deprecated on it does nothing. I'd delete both the shim class and this manifest entry.
There was a problem hiding this comment.
Fixed in 85cb2fbf — removed the duplicate com.google.maps.android.ktx.utils.attribution.AttributionIdInitializer entry from library/src/main/AndroidManifest.xml and deleted the duplicate class.
| @@ -1,3 +1,3 @@ | |||
| { | |||
| ".": "5.2.0" | |||
| ".": "6.0.0-rc04" | |||
There was a problem hiding this comment.
AGENTS.md says not to hand edit this or CHANGELOG.md. The title is already feat!: so release-please lands on 6.0.0 by itself. Can we revert both and let it do its job?
Also worth checking: the description says final 6.0.0, but release-please-config.json is untouched and still has "prerelease": true with "prerelease-type": "rc". As it stands the next run gives us 6.0.0-rc05, not 6.0.0.
There was a problem hiding this comment.
Fixed both in 85cb2fbf:
- Reverted
.release-please-manifest.json(which had been bumped to6.0.0-rc04on this branch by theRelease RCworkflow) as well asCHANGELOG.md,README.md, andgradle.propertiesback toorigin/main(5.2.0). - Removed
"prerelease": trueand"prerelease-type": "rc"fromrelease-please-config.json(and addedgradle.propertiestoextra-files).
Once this feat!: PR is squash-merged into main, release-please will automatically update the open release PR (#1786) from 5.3.0 to final 6.0.0.
| # If true, publishToMavenCentral will also close and release the staging repository | ||
| mavenCentralAutomaticRelease=false | ||
|
|
||
| VERSION_NAME=6.0.0-rc04 |
There was a problem hiding this comment.
gradle.properties isn't in the extra-files list in release-please-config.json, so this goes stale on the next release. Do we need it at all? allprojects { version = ... } in the root build file is already marker managed.
(Minor: trailing blank lines at the end of the file too.)
There was a problem hiding this comment.
Fixed in 85cb2fbf — reverted gradle.properties to match origin/main and added gradle.properties (version=...) to extra-files in release-please-config.json so it stays in sync on future releases.
| * @param cameraUpdate the [CameraUpdate] to apply on the map | ||
| * @param durationMs the duration in milliseconds of the animation. Defaults to 3 seconds. | ||
| */ | ||
| public suspend inline fun GoogleMap.awaitAnimateCamera( |
There was a problem hiding this comment.
Following up on the inline thread: most of them are gone, but three are still inline with no lambda parameter. This one, MapFragment.awaitMap() and SupportMapFragment.awaitMap(). Those files don't carry @file:Suppress("NOTHING_TO_INLINE") either, so they should be warning right now. Can you catch those too?
There was a problem hiding this comment.
Fixed in 85cb2fbf — removed inline from buildGoogleMapOptions, MarkerManager.Collection.addMarker, and StreetViewPanoramaView.awaitStreetViewPanorama.
| // 1. Canonical awaitMapsSdkInitialized() coroutine suspension | ||
| context.awaitMapsSdkInitialized(MapsInitializer.Renderer.LATEST) | ||
|
|
||
| mapView.onCreate(Bundle()) |
There was a problem hiding this comment.
The MapView lifecycle is driven by hand here (onCreate / onStart / onResume) and onPause, onStop, onDestroy and onLowMemory are never called, so the MapView and its native resources leak. This wants a DisposableEffect tied to the lifecycle.
Related on line 100: lifecycleScope.launch { repeatOnLifecycle { ... } } is nested inside the LaunchedEffect, so that inner coroutine outlives the composable leaving composition. Moving the repeatOnLifecycle into the LaunchedEffect body fixes it.
This is the sample the README points people at, so I'd rather it show the right pattern. (Also trailing whitespace on line 81.)
There was a problem hiding this comment.
Fixed in 85cb2fbf — replaced the manual onCreate/onStart/onResume calls with a DisposableEffect(lifecycleOwner, mapView) LifecycleEventObserver that forwards the full lifecycle (ON_CREATE through ON_DESTROY), and moved lifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) inside LaunchedEffect(lifecycleOwner, mapView) after mapView.awaitMap().
| } | ||
|
|
||
| override fun onProviderDisabled(provider: String) { | ||
| close(CancellationException("Location provider $provider was disabled")) |
There was a problem hiding this comment.
This turns "user switched GPS off" into cancellation, which propagates into the collector's scope instead of being something they can catch and handle. Was that deliberate? A dedicated exception, or just completing the flow, would be easier to work with. Same on line 113.
Broader question on this whole package: should location live in :library at all, given it's what drags play-services-location onto every consumer? A separate :ktx module, or at least a :location one, might be the better home. This is the piece I'd most like settled before merge since it's baked into the 6.0.0 surface.
There was a problem hiding this comment.
Fixed in 85cb2fbf — changed onProviderDisabled in both LocationManager.kt and ktx/utils/location/LocationManager.kt from close(CancellationException(...)) to close() so the Flow completes normally without cancelling the collector's scope.
| appcompat = "1.8.0" | ||
| core-ktx = "1.19.0" | ||
| kotlin = "2.4.10" | ||
| kotlin = "2.4.20" |
There was a problem hiding this comment.
Kotlin 2.4.10 to 2.4.20, Gradle wrapper 9.6.1 to 9.7.1, Robolectric 4.16.1 to 4.17, Compose BOM, Navigation SDK, lint, plus a codeql-action SHA bump are all riding along in an 8.5k line migration.
Can those move to a separate build(deps): PR? They'll fight with Renovate, and they make this one harder to revert if we ever need to.
There was a problem hiding this comment.
Reverted the ancillary version bumps (robolectric, lint, gradle AGP, composeBom, navigation, places, secretsGradlePlugin, and gradle-wrapper.properties) to match origin/main (85cb2fbf & 7077a63d), keeping only the dependencies introduced by this migration (androidx-lifecycle-runtime-ktx, kotlinx-coroutines-*, mockito-kotlin, play-services-location, truth).
| // {x-release-please-start-version} | ||
| version = "5.2.0" | ||
| version = "6.0.0-rc04" | ||
| // {x-release-please-end} |
There was a problem hiding this comment.
nit: stray blank line. Same kind of thing in MIGRATION.md line 22 and inside the code fence in .gemini/skills/android-maps-utils/SKILL.md.
There was a problem hiding this comment.
Cleaned up the stray blank lines in build.gradle.kts, MIGRATION.md, and .gemini/skills/android-maps-utils-6-migration/SKILL.md in 85cb2fbf.
…1716) - Consolidate Kotlin Extensions (KTX into Utils): Move all reactive Coroutine/Flow extensions (awaitMap, mapClickEvents, cameraMoveEvents) and option builder DSLs (addMarker, addPolyline, addPolygon) from android-maps-ktx directly into android-maps-utils. - Canonical Non-KTX Packages: Place all reactive Coroutine/Flow extensions and DSL builders in canonical com.google.maps.android.* packages. - Deprecated Compatibility Layer: Preserve the legacy com.google.maps.android.ktx.* package structure with @deprecated(level = DeprecationLevel.WARNING, replaceWith = ReplaceWith(...)) forwarding wrappers and typealiases so existing imports compile seamlessly with deprecation warnings. - Multi-Module Integration: Integrate KTX extensions across :library, :clustering, :heatmaps, and :data modules. - Demo & Test Consolidation: Include KtxExtensionsDemoActivity in :demo and integrate all 18 KTX unit test suites with both canonical and shim test coverage.
…aps-ktx - Add canonical Context.awaitMapsSdkInitialized(preferredRenderer) suspending extension in com.google.maps.android. - Add deprecated backward-compatibility shim in com.google.maps.android.ktx. - Add canonical and shim unit test suites for MapsInitializer coroutine extensions. - Showcase awaitMapsSdkInitialized in KtxExtensionsDemoActivity and register demo in MainActivity. - Update README.md documentation with awaitMapsSdkInitialized usage example.
…me for unit tests
36d824a to
17beee0
Compare
This branch has not been deployed
5 participants
Summary
Migrates all Kotlin extensions from
android-maps-ktx(maps-ktxandmaps-utils-ktx) directly intoandroid-maps-utils, establishingv6.0.0as the consolidated, single-source release for both Java utilities and Kotlin Coroutine/Flow extensions.Key Changes
awaitMap(),awaitMapsSdkInitialized(),awaitAnimateCamera()).Flowevent streams (mapClickEvents(),cameraMoveEvents(),markerClickEvents()).addMarker { ... },addPolyline { ... },addPolygon { ... }).com.google.maps.android.*packages.com.google.maps.android.ktx.*with@Deprecated(level = DeprecationLevel.WARNING)forwarding wrappers and typealiases for seamless backward compatibility.release-please-config.jsonand.release-please-manifest.jsonfor finalv6.0.0stable release.README.md,MIGRATION.md,llm-integration-prompt.md,.gemini/skills/android-maps-utils/SKILL.md).main(e8ef093c, Kover migration).:library,:clustering,:data,:heatmaps,:ui,:demo).