Add LocationCompat utility for graceful GMS fallback on devices without Play Services#469
Merged
torlando-tech merged 4 commits intomainfrom Feb 16, 2026
Merged
Conversation
On devices without Google Play Store (custom ROMs, F-Droid builds), every FusedLocationProviderClient call generated repeated warnings: W GooglePlayServicesUtil: com.lxmf.messenger required the Google Play Store W GoogleApiManager: The service for ...location.k is not available Add LocationCompat utility that checks GMS availability once and logs at Info level. When GMS is unavailable, falls back to Android's built-in LocationManager instead of calling GMS APIs that produce the warnings. Updated all 5 location consumers: - LocationSharingManager (continuous updates) - TelemetryCollectorManager (one-shot location) - MapScreen (continuous updates) - DiscoveredInterfacesScreen (one-shot location) - OfflineMapDownloadScreen (one-shot location) Closes #456 https://claude.ai/code/session_01Bv18BreVCSD2GeUg4bHvG1
app/src/main/java/com/lxmf/messenger/ui/screens/offlinemaps/OfflineMapDownloadScreen.kt
Show resolved
Hide resolved
Contributor
Greptile SummaryThis PR introduces Major changes:
Key implementation details:
Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
Start[App requests location] --> Check{LocationCompat.isPlayServicesAvailable?}
Check -->|Yes - Cached| GMS[Use FusedLocationProviderClient]
Check -->|No - Cached| Platform[Use Android LocationManager]
Check -->|First call| DCL[Double-checked locking]
DCL --> GMSCheck{GoogleApiAvailability check}
GMSCheck -->|SUCCESS| CacheTrue[Cache: available=true]
GMSCheck -->|Failed/Exception| CacheFalse[Cache: available=false]
CacheTrue --> GMS
CacheFalse --> Platform
GMS --> GMSLast[getLastLocation]
GMS --> GMSCurrent[getCurrentLocation with CancellationToken]
GMS --> GMSUpdates[requestLocationUpdates with LocationCallback]
Platform --> PlatformLast[LocationManager.getLastKnownLocation]
Platform --> PlatformCurrent{Android version?}
PlatformCurrent -->|R+| CurrentAPI[LocationManager.getCurrentLocation]
PlatformCurrent -->|Pre-R| SingleUpdate[requestSingleUpdate + 10s timeout]
Platform --> PlatformUpdates[requestLocationUpdates GPS+Network]
PlatformUpdates --> Dedup[Deduplicate within 1s window]
GMSUpdates --> Callback[Location delivered to app]
Dedup --> Callback
CurrentAPI --> Callback
SingleUpdate --> Callback
Last reviewed commit: fa71687 |
|
|
||
| @Volatile | ||
| private var checked = false | ||
| private var available = false |
Contributor
There was a problem hiding this comment.
missing @Volatile annotation
Suggested change
| private var available = false | |
| @Volatile | |
| private var available = false |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/util/LocationCompat.kt
Line: 33:33
Comment:
missing `@Volatile` annotation
```suggestion
@Volatile
private var available = false
```
How can I resolve this? If you propose a fix, please make it concise.Address review findings from Greptile and Sentry AI: 1. Add @volatile to LocationCompat.available field - the double-checked locking pattern requires both `checked` and `available` to be volatile, otherwise the fast-path read can see a stale `available` value. 2. Add 10-second timeout to LocationCompat.getCurrentLocation() on pre-Android 11 devices - requestSingleUpdate() may never invoke its callback if no GPS fix is obtained (poor signal, indoor). Without a timeout, callers like OfflineMapDownloadScreen get permanently stuck showing a loading spinner (isGettingLocation never resets to false). 3. Fix MapScreen LocationListener memory leak - the fallback path called LocationCompat.requestLocationUpdates() but discarded the returned listener, so it was never removed. Now startLocationUpdates() returns the listener, MapScreen stores it in a remembered state var, and a DisposableEffect removes it when the composable leaves composition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use expression body `= Unit` instead of empty block body `{}` for
required LocationListener interface overrides that intentionally
do nothing.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Owner
Author
Comment on lines
+752
to
761
| } else { | ||
| // Fallback to platform LocationManager | ||
| if (continuation.isActive) { | ||
| LocationCompat.getCurrentLocation(context) { location -> | ||
| if (continuation.isActive) { | ||
| continuation.resume(location) | ||
| } | ||
| } catch (e: IllegalArgumentException) { | ||
| // CancellationToken was already cancelled - this can happen in race conditions | ||
| Log.w(TAG, "Location request cancelled before it could start", e) | ||
| if (continuation.isActive) { | ||
| continuation.resume(null) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
No cancellation handling for platform fallback
In the GMS path, invokeOnCancellation cancels the CancellationTokenSource, but the non-GMS fallback path has no cancellation support. If the coroutine is cancelled while LocationCompat.getCurrentLocation is pending (e.g., during the pre-R 10-second timeout), the platform LocationManager listener will remain registered until the timeout fires and self-cleans. This is a minor resource leak during cancellation.
Consider adding invokeOnCancellation to clean up the platform request, or at minimum adding a comment acknowledging this limitation:
Suggested change
| } else { | |
| // Fallback to platform LocationManager | |
| if (continuation.isActive) { | |
| LocationCompat.getCurrentLocation(context) { location -> | |
| if (continuation.isActive) { | |
| continuation.resume(location) | |
| } | |
| } catch (e: IllegalArgumentException) { | |
| // CancellationToken was already cancelled - this can happen in race conditions | |
| Log.w(TAG, "Location request cancelled before it could start", e) | |
| if (continuation.isActive) { | |
| continuation.resume(null) | |
| } | |
| } | |
| } | |
| } else { | |
| // Fallback to platform LocationManager | |
| // Note: cancellation is not forwarded to LocationCompat; the platform | |
| // request will self-clean via its internal 10s timeout if cancelled. | |
| if (continuation.isActive) { | |
| LocationCompat.getCurrentLocation(context) { location -> | |
| if (continuation.isActive) { | |
| continuation.resume(location) | |
| } | |
| } | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/service/TelemetryCollectorManager.kt
Line: 752:761
Comment:
**No cancellation handling for platform fallback**
In the GMS path, `invokeOnCancellation` cancels the `CancellationTokenSource`, but the non-GMS fallback path has no cancellation support. If the coroutine is cancelled while `LocationCompat.getCurrentLocation` is pending (e.g., during the pre-R 10-second timeout), the platform `LocationManager` listener will remain registered until the timeout fires and self-cleans. This is a minor resource leak during cancellation.
Consider adding `invokeOnCancellation` to clean up the platform request, or at minimum adding a comment acknowledging this limitation:
```suggestion
} else {
// Fallback to platform LocationManager
// Note: cancellation is not forwarded to LocationCompat; the platform
// request will self-clean via its internal 10s timeout if cancelled.
if (continuation.isActive) {
LocationCompat.getCurrentLocation(context) { location ->
if (continuation.isActive) {
continuation.resume(location)
}
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+222
to
+244
| // Request from GPS if available | ||
| if (locationManager.isProviderEnabled(LocationManager.GPS_PROVIDER)) { | ||
| locationManager.requestLocationUpdates( | ||
| LocationManager.GPS_PROVIDER, | ||
| intervalMs, | ||
| 0f, | ||
| listener, | ||
| Looper.getMainLooper(), | ||
| ) | ||
| } | ||
|
|
||
| // Also request from network if available (provides faster initial fix) | ||
| if (locationManager.isProviderEnabled(LocationManager.NETWORK_PROVIDER)) { | ||
| locationManager.requestLocationUpdates( | ||
| LocationManager.NETWORK_PROVIDER, | ||
| intervalMs, | ||
| 0f, | ||
| listener, | ||
| Looper.getMainLooper(), | ||
| ) | ||
| } | ||
|
|
||
| return listener |
Contributor
There was a problem hiding this comment.
Duplicate location callbacks from dual provider registration
Registering the same LocationListener with both GPS_PROVIDER and NETWORK_PROVIDER means the onLocation callback can fire twice for the same time period - once from each provider. In LocationSharingManager, this results in duplicate sendLocationToRecipients calls, and in MapScreen it causes redundant updateUserLocation calls.
Consider using a deduplication strategy, such as tracking the last delivered location timestamp and ignoring updates that arrive within a short window:
val listener =
object : LocationListener {
private var lastDeliveredTime = 0L
override fun onLocationChanged(location: Location) {
val now = SystemClock.elapsedRealtime()
if (now - lastDeliveredTime > 1000) {
lastDeliveredTime = now
onLocation(location)
}
}
// ...
}Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/util/LocationCompat.kt
Line: 222:244
Comment:
**Duplicate location callbacks from dual provider registration**
Registering the same `LocationListener` with both `GPS_PROVIDER` and `NETWORK_PROVIDER` means the `onLocation` callback can fire twice for the same time period - once from each provider. In `LocationSharingManager`, this results in duplicate `sendLocationToRecipients` calls, and in `MapScreen` it causes redundant `updateUserLocation` calls.
Consider using a deduplication strategy, such as tracking the last delivered location timestamp and ignoring updates that arrive within a short window:
```kotlin
val listener =
object : LocationListener {
private var lastDeliveredTime = 0L
override fun onLocationChanged(location: Location) {
val now = SystemClock.elapsedRealtime()
if (now - lastDeliveredTime > 1000) {
lastDeliveredTime = now
onLocation(location)
}
}
// ...
}
```
How can I resolve this? If you propose a fix, please make it concise.…eedback Wrap GoogleApiAvailability.isGooglePlayServicesAvailable() in try-catch so it returns false instead of throwing GooglePlayServicesMissingManifestValueException in unit test environments (fixes 25 failing TelemetryCollectorManagerTest tests). Add time-based deduplication to requestLocationUpdates() to prevent duplicate callbacks when both GPS and network providers are registered. Document the platform fallback cancellation limitation in TelemetryCollectorManager. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a new
LocationCompatutility class that provides a graceful fallback mechanism for location services on devices without Google Play Services installed. This resolves repeated warning logs that flood the console on custom ROMs and F-Droid builds when attempting to useFusedLocationProviderClient.Key Changes
New
LocationCompatutility class (app/src/main/java/com/lxmf/messenger/util/LocationCompat.kt):LocationManagergetLastKnownLocation(),getCurrentLocation(),requestLocationUpdates(), andremoveLocationUpdates()getCurrentLocation(), older versions userequestSingleUpdate())Updated
MapScreen.kt:FusedLocationProviderClientonly when Play Services is availableLocationCompatfor location updates when GMS is unavailablestartLocationUpdates()Updated
LocationSharingManager.kt:FusedLocationProviderClientbased on GMS availabilityLocationManagerotherwiseLocationListenerreference for proper cleanupUpdated
TelemetryCollectorManager.kt:FusedLocationProviderClientonly when Play Services is availablegetCurrentLocation()with GMS availability check and fallback toLocationCompatUpdated
OfflineMapDownloadScreen.kt:FusedLocationProviderClientLocationCompat.getCurrentLocation()when GMS is unavailableUpdated
DiscoveredInterfacesScreen.kt:FusedLocationProviderClientLocationCompat.getCurrentLocation()for location retrievalImplementation Details
@SuppressLint("MissingPermission")annotations where permission checks occur at higher levelshttps://claude.ai/code/session_01Bv18BreVCSD2GeUg4bHvG1