Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis change refactors location handling across multiple components by centralizing location processing logic. A new Sequence Diagram(s)sequenceDiagram
participant Controller as LocationController<br/>(FusedOrProvider)
participant Handler as CommCareLocationController
participant Prefs as LocationPreferences
participant Listener as CommCareLocationListener
Controller->>Handler: onLocationReceived(newLocation, listener, lambda)
alt isFirstLocation
Handler->>Prefs: updateLastLocation(newLocation)
Handler->>Listener: onLocationResult(newLocation)
Handler->>Prefs: setLastAcceptedLocationTimestamp(now)
else isDifferentLocation
Handler->>Prefs: updateLastLocation(newLocation)
Handler->>Listener: onLocationResult(newLocation)
Handler->>Prefs: setLastAcceptedLocationTimestamp(now)
Handler->>Handler: logStaleLocationIfGpsTimeDrifted()
else SameLocation
alt thresholdElapsed
Handler->>Handler: discardLocation(error)
else thresholdNotElapsed
Handler->>Listener: onLocationResult(newLocation)
Handler->>Prefs: setLastAcceptedLocationTimestamp(now)
end
end
Handler->>Controller: lambda(location) → mCurrentLocation = it
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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/location/CommCareLocationController.kt (1)
55-60: Consider simplifyingdiscardLocationlogging.Using
Logger.exception()with a fabricatedExceptionfor informational logging is unconventional. If this is purely for telemetry/logging purposes rather than indicating an actual error state, consider using a standard log method instead, or add a comment clarifying why exception-style logging is used here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/location/CommCareLocationController.kt` around lines 55 - 60, The discardLocation function currently calls Logger.exception with a fabricated Exception which is misleading; replace this with a non-exception log call (e.g., Logger.info or Logger.warn) and log the same message and details (use GeoUtils.locationToString(location) and location.accuracy) or, if exception-style telemetry is intentional, add a clarifying comment above discardLocation explaining why an Exception is being created for telemetry; update references in discardLocation and remove the fabricated Exception usage.
🤖 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/CommCareLocationController.kt`:
- Around line 84-115: onLocationReceived performs a non-atomic read-modify-write
on LocationPreferences (reads lastLocationString/lastAcceptedTimestamp then
calls updateLastLocation/acceptLocation), which can race when multiple
controllers call it concurrently; protect this critical section by serializing
access (e.g., add a private lock object or make the method synchronized) around
the logic that reads LocationPreferences and calls
updateLastLocation/acceptLocation/discardLocation so the checks in
isFirstLocation/isDifferentLocation and subsequent updates execute atomically;
ensure the same lock is used by any other code paths that modify
LocationPreferences so state stays consistent.
---
Nitpick comments:
In `@app/src/org/commcare/location/CommCareLocationController.kt`:
- Around line 55-60: The discardLocation function currently calls
Logger.exception with a fabricated Exception which is misleading; replace this
with a non-exception log call (e.g., Logger.info or Logger.warn) and log the
same message and details (use GeoUtils.locationToString(location) and
location.accuracy) or, if exception-style telemetry is intentional, add a
clarifying comment above discardLocation explaining why an Exception is being
created for telemetry; update references in discardLocation and remove the
fabricated Exception usage.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/org/commcare/activities/GeoPointActivity.javaapp/src/org/commcare/android/javarosa/PollSensorAction.javaapp/src/org/commcare/location/CommCareFusedLocationController.ktapp/src/org/commcare/location/CommCareLocationController.ktapp/src/org/commcare/location/CommCareProviderLocationController.ktapp/src/org/commcare/preferences/LocationPreferences.kt
💤 Files with no reviewable changes (2)
- app/src/org/commcare/activities/GeoPointActivity.java
- app/src/org/commcare/android/javarosa/PollSensorAction.java
| private fun isFirstLocation(lastLocationString: String?): Boolean = lastLocationString.isNullOrEmpty() | ||
|
|
||
| private fun isDifferentLocation( | ||
| newLocation: Location, | ||
| lastLocationString: String, | ||
| ): Boolean = GeoUtils.locationToString(newLocation) != lastLocationString |
There was a problem hiding this comment.
These two functions are only being used in one place currently. Unless we plan on reusing them in the future, can we convert these to local boolean variables instead?
| private fun discardLocation(location: Location) { | ||
| Logger.exception( | ||
| "Discarding stale repeated location", | ||
| Exception("Discarding stale repeated location ${GeoUtils.locationToString(location)} with accuracy ${location.accuracy}"), | ||
| ) | ||
| } | ||
|
|
||
| fun getStaleLocationException(location: Location): Throwable = | ||
| private fun getStaleLocationException( | ||
| location: Location, | ||
| currentDeviceTime: Long, | ||
| ): Throwable = | ||
| Exception( | ||
| "Stale location with accuracy ${location.accuracy}" + | ||
| " with time ${location.time}" + " and current device time ${System.currentTimeMillis()}", | ||
| " with time ${location.time}" + " and current device time $currentDeviceTime", | ||
| ) |
There was a problem hiding this comment.
Similar concern with these two functions as well. Guess this brings up a more general question - how modular do we want this class to be really? Understandably, all these functions make onLocationReceived() a bit easier to read, but there is a trade-off with having a lot of functions doing very simple tasks that could be reduced to single lines and/or local variables
There was a problem hiding this comment.
@conroy-ricketts Yeah, here more important part was code readability and also performing each task separately as defined here in proposal.
There was a problem hiding this comment.
Cool that sounds good to me if we want to put more emphasis on code readability 👍
| ): Boolean = GeoUtils.locationToString(newLocation) != lastLocationString | ||
|
|
||
| private fun updateLastLocation(location: Location) { | ||
| LocationPreferences.setLastAcceptedLocation(GeoUtils.locationToString(location)) |
There was a problem hiding this comment.
we need to have the location timestamp as part of the location string we are saving here as well.
| fun onLocationReceived( | ||
| newLocation: Location, | ||
| listener: CommCareLocationListener?, | ||
| setCurrentLocation: (Location) -> Unit, |
There was a problem hiding this comment.
think we can put the mCurrentLocation from the implementations in this class itself and get rid of the callback here.
| Logger.exception( | ||
| "Received a stale location", | ||
| getStaleLocationException(location, currentDeviceTime), |
There was a problem hiding this comment.
we should log the actual drift in mins here as well to easily know more about the drift range if this happens.
3 participants
CCCT-2196
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-2196
The application is avoiding the stale location as per the proposal.
Also, a new preference file
LocationPreferenceshas been created instead of using the existing one, as it was crowded and kept the location-specific methods inLocationPreferences.Safety Assurance
Safety story
Labels and Review