Added Receiver to listen location service status #3427
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
avazirna and
conroy-ricketts
📝 WalkthroughWalkthroughThis PR extends the CommCare location system by introducing a location service change listener pattern. A new callback Sequence DiagramsequenceDiagram
participant System as Android System
participant Receiver as LocationChangeReceiverBroadcast
participant Controller as CommCareFusedLocationController
participant Listener as CommCareLocationListener<br/>(implementations)
System->>Receiver: PROVIDERS_CHANGED_ACTION broadcast
Receiver->>Controller: locationServicesEnabledGlobally()
alt Location Services Enabled
Controller->>Controller: start location updates
Controller->>Listener: onLocationServiceChange(true)
else Location Services Disabled
Controller->>Controller: stop location updates
Controller->>Listener: onLocationServiceChange(false)
end
Note over Controller,Listener: PersonalIdPhoneFragment clears<br/>location data on false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/org/commcare/activities/GeoPointActivity.java (1)
264-267: Consider handling location service changes in GeoPointActivity.The empty implementation may leave users stuck if location services are disabled while the activity is waiting for a location. Consider notifying the user or triggering the existing
handleNoLocationProviders()flow whenlocationServiceEnabledis false.For example:
@Override public void onLocationServiceChange(boolean locationServiceEnabled) { - + if (!locationServiceEnabled) { + handleNoLocationProviders(); + } }app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
568-575: Consider handling location service re-enablement.The implementation correctly handles the case when location services are disabled. However, when
locationServiceEnabledis true (services re-enabled), the code doesn't automatically request a new location. Users would need to background and resume the app to trigger location acquisition.Consider adding:
@Override public void onLocationServiceChange(boolean locationServiceEnabled) { if (!locationServiceEnabled) { location = null; setLocationToolTip(location); updateContinueButtonState(); + } else { + // Location services re-enabled, attempt to get location + locationController.start(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/org/commcare/activities/GeoPointActivity.java(1 hunks)app/src/org/commcare/android/javarosa/PollSensorController.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(1 hunks)app/src/org/commcare/location/CommCareFusedLocationController.kt(3 hunks)app/src/org/commcare/location/CommCareLocationListener.kt(1 hunks)app/src/org/commcare/utils/GeoUtils.java(1 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/org/commcare/location/CommCareFusedLocationController.kt
[warning] 101-101: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (5)
app/src/org/commcare/location/CommCareLocationListener.kt (1)
22-22: LGTM!The new callback method
onLocationServiceChangeis properly added to the interface to support location service status monitoring. All implementations across the codebase correctly implement this method.app/src/org/commcare/utils/GeoUtils.java (1)
150-150: LGTM!Making
locationServicesEnabledGloballypublic is appropriate to support the new location service change monitoring functionality. The method is already well-implemented with proper exception handling.app/src/org/commcare/android/javarosa/PollSensorController.java (1)
131-134: LGTM!The empty implementation is acceptable for
PollSensorController. This component manages its own location polling lifecycle and doesn't require additional handling for location service changes.app/src/org/commcare/location/CommCareFusedLocationController.kt (2)
101-103: Swallowed exception is acceptable here.The
IllegalArgumentExceptioncatch is appropriate for handling multiple calls tostopLocationServiceChangeReceiver(). The comment correctly explains the scenario.
89-96: Good Android version compatibility handling.The code correctly handles the
RECEIVER_EXPORTEDflag requirement for Android 13 (TIRAMISU) and above.
| private var mContext: Context?, | ||
| private var mListener: CommCareLocationListener?, | ||
| ) : CommCareLocationController { |
There was a problem hiding this comment.
Add null-safety checks in BroadcastReceiver.
The mContext and mListener are nullable and set to null in destroy(), but the LocationChangeReceiverBroadcast may still receive broadcasts if unregisterReceiver() fails silently. This could lead to null pointer exceptions.
Consider adding null checks in the receiver's onReceive method:
inner class LocationChangeReceiverBroadcast : BroadcastReceiver() {
override fun onReceive(
context: Context,
intent: Intent,
) {
+ if (mContext == null || mListener == null) return
if (LocationManager.PROVIDERS_CHANGED_ACTION == intent.getAction()) {
val lm = context.getSystemService(Context.LOCATION_SERVICE) as LocationManager
val locationServiceEnabled = locationServicesEnabledGlobally(lm)
if (locationServiceEnabled) {
start()
} else {
stop()
}
mListener?.onLocationServiceChange(locationServiceEnabled)
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/location/CommCareFusedLocationController.kt around lines
21 to 23, the BroadcastReceiver's onReceive may access nullable fields mContext
and mListener after destroy() clears them; add null-safety guards in onReceive
by checking whether mContext and mListener are null before using them (return
early if null) and optionally log a warning; ensure any calls into mListener or
use of mContext are wrapped in a safe let/if-check so no NPE can occur if
unregisterReceiver failed.
There was a problem hiding this comment.
onReceive is not using mContext, also mListener is checked for null using kotlin ? operator
There was a problem hiding this comment.
@Jignesh-dimagi You're correct on both points:
onReceivedoesn't directly accessmContext- it uses thecontextparameter insteadmListeneruses the safe call operator (?) - preventing NPE on that line
However, there's an indirect concern: onReceive calls start(), which in turn calls restartLocationServiceChangeReceiver() and requestUpdates(). While startLocationServiceChangeReceiver() and stopLocationServiceChangeReceiver() use safe calls (mContext?.registerReceiver(), mContext?.unregisterReceiver()), the requestUpdates() method passes mContext to isLocationPermissionGranted(mContext) where it could potentially cause issues if null.
That said, given that:
stopLocationServiceChangeReceiver()is called indestroy()- The register/unregister methods use safe calls
- The catch block in
stopLocationServiceChangeReceiver()handles the IllegalArgumentException
The risk is quite low. If you're confident that the receiver will be properly unregistered before destroy() completes, the current implementation should be safe. The safe call operators provide adequate null-safety.
🧠 Learnings used
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using NotNull.
There was a problem hiding this comment.
isLocationPermissionGranted is already checking for mContext null and if so it assign application context
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the implementation of isLocationPermissionGranted function
rg -n "fun isLocationPermissionGranted" --type kotlin -A 10
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for isLocationPermissionGranted more broadly (might be Java)
rg -n "isLocationPermissionGranted" -A 10
Length of output: 3135
@Jignesh-dimagi You're absolutely right! I can see that isLocationPermissionGranted uses the Elvis operator to fall back to the application context:
val context = mContext ?: CommCareApplication.instance()This ensures null-safety even if mContext is null after destroy(). Combined with the safe call operators throughout the code and the exception handling in stopLocationServiceChangeReceiver(), the implementation is well-protected against null pointer exceptions.
Thank you for walking through the null-safety measures - the code is properly safeguarded. 👍
🐰✨
🧠 Learnings used
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using NotNull.
Technical Summary
https://dimagi.atlassian.net/browse/QA-8241
Application will listen to location service status and will take necessary action depending upon the status.
QA Plan
Labels and Review