Including user location in map zoom when available#3569
Including user location in map zoom when available#3569OrangeAndGreen wants to merge 3 commits intomasterfrom
Conversation
…n (one-time, minimal delay). Changed EntityMapActivity to include user's location when zooming the map (if available). Also simplified logic for checking whether bounds builder has added points.
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis change introduces location-based functionality to the entity map feature. The EntityMapActivity now retrieves the current user location during map initialization and uses it to center the map view. A new callback-based location retrieval method was added to CommCareFusedLocationController to support this flow. Method signatures in EntityMapActivity were refactored from returning boolean values to void, with side effects now consistently applied. The location controller factory was updated to accept nullable listener parameters, allowing optional location tracking. Sequence Diagram(s)sequenceDiagram
participant EntityMapActivity
participant CommCareLocationControllerFactory
participant CommCareFusedLocationController
participant LocationClient as Location APIs
EntityMapActivity->>CommCareLocationControllerFactory: getLocationController(context, listener?)
CommCareLocationControllerFactory-->>EntityMapActivity: returns CommCareLocationController
EntityMapActivity->>CommCareFusedLocationController: getCurrentLocation(callback)
CommCareFusedLocationController->>CommCareFusedLocationController: Check permission
alt Permission Granted
CommCareFusedLocationController->>LocationClient: Request current location (HIGH_ACCURACY)
LocationClient-->>CommCareFusedLocationController: Location received
CommCareFusedLocationController->>CommCareFusedLocationController: Invoke callback.onResult(location)
else Permission Denied
CommCareFusedLocationController->>CommCareFusedLocationController: Invoke callback.onResult(null)
end
CommCareFusedLocationController-->>EntityMapActivity: Location via callback
EntityMapActivity->>EntityMapActivity: addMarker()
EntityMapActivity->>EntityMapActivity: addBoundaryPolygon()
EntityMapActivity->>EntityMapActivity: addGeoPoints()
EntityMapActivity->>EntityMapActivity: proceedWithLocation(boundsBuilder, location)
EntityMapActivity->>EntityMapActivity: Build bounds and reposition map camera
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🧹 Nitpick comments (1)
app/src/org/commcare/gis/EntityMapActivity.java (1)
212-220: Location retrieval is implementation-coupled to fused controller.At Line 214, the
instanceof CommCareFusedLocationControllerdowncast means provider-controller paths always fall back toproceedWithLocation(builder, null). Consider moving one-shot location retrieval intoCommCareLocationControllerso both implementations can participate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/gis/EntityMapActivity.java` around lines 212 - 220, The code currently checks for and downcasts to CommCareFusedLocationController (using instanceof) and only calls getCurrentLocation on that implementation, causing other CommCareLocationController implementations to always fall back to proceedWithLocation(builder, null); move the one-shot location retrieval API into the CommCareLocationController interface (e.g., add a method like getCurrentLocation(Consumer<Location> callback) to CommCareLocationController), implement it in CommCareFusedLocationController and any provider-based controllers, then replace the instanceof block in EntityMapActivity with a call to locationController.getCurrentLocation(userLocation -> proceedWithLocation(builder, userLocation)) so all controller types can supply a location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/location/CommCareFusedLocationController.kt`:
- Around line 67-69: The getCurrentLocation method currently gates execution
with isLocationPermissionGranted(mContext) which requires both ACCESS_FINE and
ACCESS_COARSE; change this to allow either permission so
FusedLocationProviderClient.getCurrentLocation can run when only one is granted:
either call a new helper (e.g., isAnyLocationPermissionGranted(context) or
modify isLocationPermissionGranted to accept a flag) and use that in
CommCareFusedLocationController.getCurrentLocation, ensuring the permission
check uses OR logic (fine || coarse) rather than AND before invoking
mFusedLocationClient.getCurrentLocation in getCurrentLocation.
---
Nitpick comments:
In `@app/src/org/commcare/gis/EntityMapActivity.java`:
- Around line 212-220: The code currently checks for and downcasts to
CommCareFusedLocationController (using instanceof) and only calls
getCurrentLocation on that implementation, causing other
CommCareLocationController implementations to always fall back to
proceedWithLocation(builder, null); move the one-shot location retrieval API
into the CommCareLocationController interface (e.g., add a method like
getCurrentLocation(Consumer<Location> callback) to CommCareLocationController),
implement it in CommCareFusedLocationController and any provider-based
controllers, then replace the instanceof block in EntityMapActivity with a call
to locationController.getCurrentLocation(userLocation ->
proceedWithLocation(builder, userLocation)) so all controller types can supply a
location.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/gis/EntityMapActivity.javaapp/src/org/commcare/location/CommCareFusedLocationController.ktapp/src/org/commcare/location/CommCareLocationControllerFactory.kt
| fun getCurrentLocation(callback: CurrentLocationCallback) { | ||
| if (isLocationPermissionGranted(mContext)) { | ||
| mFusedLocationClient.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY, null) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the relevant files
fd "CommCareLocationController" --type f
fd "CommCareFusedLocationController" --type fRepository: dimagi/commcare-android
Length of output: 258
🌐 Web query:
Android FusedLocationProviderClient.getCurrentLocation permission requirements ACCESS_FINE_LOCATION ACCESS_COARSE_LOCATION
💡 Result:
FusedLocationProviderClient.getCurrentLocation(...) requires that your app holds location permission at runtime: either ACCESS_COARSE_LOCATION or ACCESS_FINE_LOCATION (no permission → no location result). [1]
Practical requirements:
- Manifest: declare at least
ACCESS_COARSE_LOCATION; addACCESS_FINE_LOCATIONonly if you need precise. [3] - Runtime (Android 12 / API 31+): don’t request
ACCESS_FINE_LOCATIONby itself. Request bothACCESS_FINE_LOCATIONandACCESS_COARSE_LOCATIONin the same runtime permission request, or the system may ignore the fine-location request and log: “ACCESS_FINE_LOCATION must be requested with ACCESS_COARSE_LOCATION.” [2] - If the user chooses Approximate, you’ll effectively have only
ACCESS_COARSE_LOCATIONeven if you requested fine. [2][3]
Sources: [1] https://developers.google.com/android/reference/com/google/android/gms/location/FusedLocationProviderClient [2] https://developer.android.com/develop/sensors-and-location/location/permissions/runtime [3] https://developer.android.com/develop/sensors-and-location/location/permissions
🏁 Script executed:
cat -n app/src/org/commcare/location/CommCareLocationController.ktRepository: dimagi/commcare-android
Length of output: 2325
🏁 Script executed:
cat -n app/src/org/commcare/location/CommCareFusedLocationController.ktRepository: dimagi/commcare-android
Length of output: 6573
Permission gate is stricter than needed for one-shot location.
At Line 68, isLocationPermissionGranted(mContext) requires both ACCESS_FINE_LOCATION and ACCESS_COARSE_LOCATION (see CommCareLocationController.kt Line 27-30), but FusedLocationProviderClient.getCurrentLocation() requires only either permission. This causes getCurrentLocation() to return null even when the app holds one valid location permission and could still resolve a location, particularly on Android 12+ where users may grant only approximate location.
Relax the permission check to allow either permission:
Suggested fix
`@SuppressLint`("MissingPermission")
fun getCurrentLocation(callback: CurrentLocationCallback) {
- if (isLocationPermissionGranted(mContext)) {
+ val context = mContext ?: run {
+ callback.onResult(null)
+ return
+ }
+ val hasLocationPermission =
+ androidx.core.content.ContextCompat.checkSelfPermission(
+ context,
+ android.Manifest.permission.ACCESS_FINE_LOCATION,
+ ) == android.content.pm.PackageManager.PERMISSION_GRANTED ||
+ androidx.core.content.ContextCompat.checkSelfPermission(
+ context,
+ android.Manifest.permission.ACCESS_COARSE_LOCATION,
+ ) == android.content.pm.PackageManager.PERMISSION_GRANTED
+
+ if (hasLocationPermission) {
mFusedLocationClient.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY, null)
.addOnSuccessListener { location -> callback.onResult(location) }
.addOnFailureListener { callback.onResult(null) }
} else {
callback.onResult(null)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun getCurrentLocation(callback: CurrentLocationCallback) { | |
| if (isLocationPermissionGranted(mContext)) { | |
| mFusedLocationClient.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY, null) | |
| `@SuppressLint`("MissingPermission") | |
| fun getCurrentLocation(callback: CurrentLocationCallback) { | |
| val context = mContext ?: run { | |
| callback.onResult(null) | |
| return | |
| } | |
| val hasLocationPermission = | |
| androidx.core.content.ContextCompat.checkSelfPermission( | |
| context, | |
| android.Manifest.permission.ACCESS_FINE_LOCATION, | |
| ) == android.content.pm.PackageManager.PERMISSION_GRANTED || | |
| androidx.core.content.ContextCompat.checkSelfPermission( | |
| context, | |
| android.Manifest.permission.ACCESS_COARSE_LOCATION, | |
| ) == android.content.pm.PackageManager.PERMISSION_GRANTED | |
| if (hasLocationPermission) { | |
| mFusedLocationClient.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY, null) | |
| .addOnSuccessListener { location -> callback.onResult(location) } | |
| .addOnFailureListener { callback.onResult(null) } | |
| } else { | |
| callback.onResult(null) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/location/CommCareFusedLocationController.kt` around
lines 67 - 69, The getCurrentLocation method currently gates execution with
isLocationPermissionGranted(mContext) which requires both ACCESS_FINE and
ACCESS_COARSE; change this to allow either permission so
FusedLocationProviderClient.getCurrentLocation can run when only one is granted:
either call a new helper (e.g., isAnyLocationPermissionGranted(context) or
modify isLocationPermissionGranted to accept a flag) and use that in
CommCareFusedLocationController.getCurrentLocation, ensuring the permission
check uses OR logic (fine || coarse) rather than AND before invoking
mFusedLocationClient.getCurrentLocation in getCurrentLocation.
| companion object { | ||
| @JvmStatic | ||
| fun getLocationController(context: Context, mListener: CommCareLocationListener): CommCareLocationController { | ||
| fun getLocationController(context: Context, mListener: CommCareLocationListener?): CommCareLocationController { |
There was a problem hiding this comment.
@OrangeAndGreen I don't think we should allow null listeners, as without listeners, location controllers are of no use.
There was a problem hiding this comment.
Yeah, the no-action callbacks for the listener seemed awkward and I saw the constructors and implementations for both classes were ready for null listeners so thought it might be simpler to just make the listener nullable in the factory method also.
But instead, I created a LocationHelper class to provide a simpler way for callers to get a one-time location, and included a no-action listener there. I changed the factory back to requiring a non-null listener.
Noting though that not providing a listener does make sense if you're not going to use the controller for streaming, but only for a one-time request. When calling getCurrentLocation on either the fused or provider controllers, they take a different callback that's completely unrelated to the listener passed in to the constructor.
1897909
| fun interface CurrentLocationCallback { | ||
| fun onResult(location: Location?) | ||
| } | ||
|
|
||
| @SuppressLint("MissingPermission") | ||
| fun getCurrentLocation(callback: CurrentLocationCallback) { | ||
| if (isLocationPermissionGranted(mContext)) { | ||
| mFusedLocationClient.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY, null) | ||
| .addOnSuccessListener { location -> callback.onResult(location) } | ||
| .addOnFailureListener { callback.onResult(null) } | ||
| } else { | ||
| callback.onResult(null) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@OrangeAndGreen This will break the current architecture used for the fused and provider location controllers. For getting the current location and that too one time, you can add another interface method getCurrentLocation here and implement this method. It already has the interface method getLocation but this doesn't guarantee the latest or non-null.
There was a problem hiding this comment.
Ah, sure enough, somehow I missed the available getCurrentLocation call in the provider controller before, but it's there.
I added the matching implementation in that class and added the function to the interface definition so it can be called genericly. 1897909
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
I don't think I have anything else to add here. Looking good!
…ontroller. Added getCurrentLocation to CommCareLocationController interface. Created LocationHelper to wrap no-action callback and request for one-time location.
| fun interface CurrentLocationCallback { | ||
| fun onResult(location: Location?) | ||
| } |
There was a problem hiding this comment.
@OrangeAndGreen This seems bit odd to define interface inside interface. Can't we use existing CommCareLocationListener only?
There was a problem hiding this comment.
Yeah, that extra callback interface was a bit awkward. I'm not crazy about reusing the CommCareLocationListener though since it has a good bit more than we need for this functionality and could be misleading to callers.
As a third option, I reworked the new code to return a Task<Location?> so we don't need to use either of those callbacks: 0c96fc70c96fc7
There was a problem hiding this comment.
This looks good way to proceed. Ideally, we should have BaseLocationListener activity with full fledge implementation, which can be re-usable. But seems ok for this implementation.
I have only concern as stated here
…ated CurrentLocationCallback).
| LocationHelper.getCurrentLocation(this).addOnCompleteListener(task -> | ||
| proceedWithLocation(builder, task.isSuccessful() ? task.getResult() : null)); |
There was a problem hiding this comment.
The only concern here is that this might leak whenever activity gets destroyed before getting the result. Activity should cancel the task if results are not received in onDestroy.
3 participants
https://dimagi.atlassian.net/browse/CCCT-2142
Product Description
When the user goes to the Entity Map page, the map will now include their location when auto-zooming (in case it isn't already within the bounds of the provided case data points).
(screenshot pending)
Technical Summary
Implemented a one-time location request in CommCareFusedLocationController.
Calling the new function from EntityMapActivity when loading the page and map.
If the location is unavailable for any reason, the map zooms without it (based on case data points).
I also refactored the "added" logic that was used to ensure the bounds builder had at least one coordinate before attempting to build the bounds. It turns out it's just as good to see if any of the numMarkers/Polygons/Geopoints variables are >0, since anytime the helper methods returned true (i.e. "yes I added") they were also incrementing one of those counters. The logic is a bit cleaner now and more friendly to the async calls.
Feature Flag
None
Safety Assurance
Safety story
I tested with location services enabled and disabled.
When enabled, my location was included in the zoom.
When disabled, my location was not included in the zoom.
Automated test coverage
None
QA Plan
Test the map zooming when entering the Entity Map page, with and without location enabled on the device.
Zooming should occur either way.
When location is enabled, the user's location should be factored into the zoom calculation so the user is included.