Migrate location stack off deprecated GoogleApiClient to FusedLocationProviderClient#1569
Merged
aaronbrethorst merged 1 commit intoJun 13, 2026
Conversation
📝 WalkthroughWalkthroughRemoves GoogleApiClient usage and lifecycle plumbing, refactors Application to a context-only getLastKnownLocation using FusedLocationProviderClient, updates LocationHelper to request fused updates via LocationCallback, and migrates all fragments, activities, controllers, tasks, and tests to the new API. ChangesGoogleApiClient removal and location API modernization
Sequence Diagram(s): sequenceDiagram
participant Caller as UI/Controller
participant App as Application.getLastKnownLocation
participant Fused as FusedLocationProviderClient
participant LM as LocationManager
Caller->>App: request last-known location (Context)
App->>Fused: getLastLocation()
Fused-->>App: fusedLocation (task.isSuccessful())
App->>LM: requestLastKnownFromProviders()
LM-->>App: api-v1 location
App->>App: compare timestamps → return newer Location
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/LocationUtilsTest.java (1)
172-178: ⚡ Quick winTest comment no longer reflects actual behavior.
The comment claims this test runs "without Google Play Services" and "should be a Location API v1 location," but the new
Application.getLastKnownLocation(Context)implementation tries Play Services first when available (seegetLocation2in Application.java). Since the test doesn't disable or mock Play Services, it may actually return a fused location rather than an API v1 location on devices with Play Services.Consider updating the comment to reflect that this test retrieves a last-known location without explicitly requiring API v1, or add a check to skip/modify behavior when Play Services is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/LocationUtilsTest.java` around lines 172 - 178, The test comment for loc = Application.getLastKnownLocation(getTargetContext()) is outdated because Application.getLastKnownLocation(Context) now prefers Play Services (see getLocation2) and may return a fused location; update the test to either (a) change the comment to state it retrieves a last-known location that may come from Play Services/fused provider rather than claiming "without Google Play Services" and "Location API v1", or (b) add a runtime check to detect Play Services availability and skip/adjust the assertion when Play Services is present so the test no longer assumes an API v1 provider; reference Application.getLastKnownLocation and getLocation2 when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/LocationUtilsTest.java`:
- Around line 172-178: The test comment for loc =
Application.getLastKnownLocation(getTargetContext()) is outdated because
Application.getLastKnownLocation(Context) now prefers Play Services (see
getLocation2) and may return a fused location; update the test to either (a)
change the comment to state it retrieves a last-known location that may come
from Play Services/fused provider rather than claiming "without Google Play
Services" and "Location API v1", or (b) add a runtime check to detect Play
Services availability and skip/adjust the assertion when Play Services is
present so the test no longer assumes an API v1 provider; reference
Application.getLastKnownLocation and getLocation2 when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed9a8308-2e2a-4080-b1d1-8282bbd74358
📒 Files selected for processing (21)
onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/LocationUtilsTest.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/app/Application.javaonebusaway-android/src/main/java/org/onebusaway/android/map/BaseMapController.javaonebusaway-android/src/main/java/org/onebusaway/android/map/StopMapController.javaonebusaway-android/src/main/java/org/onebusaway/android/region/ObaRegionsTask.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportProblemFragmentBase.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportStopProblemFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportTripProblemFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/MySearchFragmentBase.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/RegionsFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/SearchResultsFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/survey/activities/SurveyWebViewActivity.ktonebusaway-android/src/main/java/org/onebusaway/android/util/LocationHelper.javaonebusaway-android/src/main/java/org/onebusaway/android/util/LocationUtils.javaonebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/MapLibreMapFragment.java
💤 Files with no reviewable changes (3)
- onebusaway-android/src/main/java/org/onebusaway/android/util/LocationUtils.java
- onebusaway-android/src/main/java/org/onebusaway/android/map/BaseMapController.java
- onebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportProblemFragmentBase.java
…nProviderClient GoogleApiClient was deprecated in 2017 and is removed in current play-services-location majors, blocking Play Services upgrades. In this codebase it was pure ceremony: all real location work already flowed through FusedLocationProviderClient (which needs no connect/disconnect lifecycle), so this change is mostly deletion: - LocationHelper: drop the GoogleApiClient member, ConnectionCallbacks/ OnConnectionFailedListener, and the connect()->onConnected() dance; request fused updates directly, gated only on Play Services availability (preserving the LocationManager fallback for devices without Play Services, e.g. the maplibre flavor). Replace the deprecated LocationRequest.create()/setPriority()/setInterval()/ setFastestInterval() with LocationRequest.Builder + Priority. - Application.getLastKnownLocation()/getLocation2(): drop the GoogleApiClient parameter; the GoogleApiAvailability check alone now gates the fused path. Delete the dead private getLocation() wrapper. - LocationUtils: delete the getGoogleApiClientWithCallbacks() factory and LocationServicesCallback class. - ReportActivity: drop the GoogleApiClient parameter from start() and makeIntent(). - Remove the mGoogleApiClient member and its connect/disconnect lifecycle boilerplate from 10 holder classes (HomeActivity, search/ region/trip-plan/report fragments, map controllers, ObaRegionsTask, SurveyWebViewActivity) and update all getLastKnownLocation call sites. Three bug fixes fall out of the migration: - LocationHelper(Context, int) (used by NavigationService with a 1s interval) never initialized mLocationRequest, so its fused subscription requested updates with a null request. Both constructors now share one initialization path, and unregisterListener() now also tears down fused updates so the working subscription can't leak after navigation ends. - SurveyWebViewActivity crashed in onStop() with UninitializedPropertyAccessException on devices without Play Services, because the lateinit client was only assigned when Play Services was present but disconnect() was called unconditionally. - Application.getLocation2() called Task.getResult() after checking only task.isComplete() - a task that completed with a failure (e.g. a fast-failing getLastLocation() when location permission was revoked) would throw an uncaught RuntimeExecutionException. Now gated on task.isSuccessful(). Previously the isConnected() gate kept null-client callers out of this branch; with that gate gone, all callers reach it, so the check matters more. Verified: obaGoogleDebug and obaMaplibreDebug variants and androidTest sources compile; zero GoogleApiClient references remain in src/.
bmander
force-pushed
the
modernization/fused-location-client
branch
from
03218a5 to
b77e694
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanFragment.java (1)
661-661: ⚡ Quick winUse consistent Context pattern for location retrieval.
Lines 661 and 692 use different Context-passing patterns:
getActivity().getApplicationContext()vsgetContext(). Since both calls retrieve location synchronously for immediate use, the application context vs activity context distinction is unnecessary.Additionally, comparing with the pattern in ReportTripProblemFragment (context snippet 3) which uses
getActivity(), there are now three different patterns across the migrated codebase.For consistency and simplicity, consider using
getContext()in both locations. This eliminates the NPE risk from chaining.getApplicationContext()and aligns with standard Fragment patterns where activity context is sufficient for synchronous operations.♻️ Suggested unification
- Location loc = Application.getLastKnownLocation(getActivity().getApplicationContext()); + Location loc = Application.getLastKnownLocation(getContext());This change makes both callsites use the same pattern and removes the intermediate
getActivity()null-check requirement.Also applies to: 692-692
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanFragment.java` at line 661, In TripPlanFragment replace the calls that pass getActivity().getApplicationContext() into Application.getLastKnownLocation(...) with getContext() (i.e., update the Location loc = Application.getLastKnownLocation(...) call sites to use getContext()), doing the same for the second occurrence; this unifies context usage with other fragments and avoids chaining getActivity(), while keeping the synchronous location retrieval intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanFragment.java`:
- Line 661: In TripPlanFragment replace the calls that pass
getActivity().getApplicationContext() into Application.getLastKnownLocation(...)
with getContext() (i.e., update the Location loc =
Application.getLastKnownLocation(...) call sites to use getContext()), doing the
same for the second occurrence; this unifies context usage with other fragments
and avoids chaining getActivity(), while keeping the synchronous location
retrieval intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 990344e5-4126-4a2a-919b-538547ac8c08
📒 Files selected for processing (21)
onebusaway-android/src/androidTest/java/org/onebusaway/android/util/test/LocationUtilsTest.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/app/Application.javaonebusaway-android/src/main/java/org/onebusaway/android/map/BaseMapController.javaonebusaway-android/src/main/java/org/onebusaway/android/map/StopMapController.javaonebusaway-android/src/main/java/org/onebusaway/android/region/ObaRegionsTask.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportProblemFragmentBase.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportStopProblemFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportTripProblemFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/MySearchFragmentBase.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/RegionsFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/SearchResultsFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/survey/activities/SurveyWebViewActivity.ktonebusaway-android/src/main/java/org/onebusaway/android/util/LocationHelper.javaonebusaway-android/src/main/java/org/onebusaway/android/util/LocationUtils.javaonebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/MapLibreMapFragment.java
💤 Files with no reviewable changes (3)
- onebusaway-android/src/main/java/org/onebusaway/android/util/LocationUtils.java
- onebusaway-android/src/main/java/org/onebusaway/android/map/BaseMapController.java
- onebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportProblemFragmentBase.java
✅ Files skipped from review due to trivial changes (1)
- onebusaway-android/src/main/java/org/onebusaway/android/ui/TripPlanActivity.java
🚧 Files skipped from review as they are similar to previous changes (14)
- onebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.java
- onebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/MapLibreMapFragment.java
- onebusaway-android/src/main/java/org/onebusaway/android/map/StopMapController.java
- onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.java
- onebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportTripProblemFragment.java
- onebusaway-android/src/main/java/org/onebusaway/android/region/ObaRegionsTask.java
- onebusaway-android/src/main/java/org/onebusaway/android/report/ui/ReportActivity.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/MySearchFragmentBase.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/SearchResultsFragment.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/RegionsFragment.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/survey/activities/SurveyWebViewActivity.kt
- onebusaway-android/src/main/java/org/onebusaway/android/app/Application.java
- onebusaway-android/src/main/java/org/onebusaway/android/util/LocationHelper.java
aaronbrethorst
approved these changes
Jun 13, 2026
aaronbrethorst
left a comment
aaronbrethorst
left a comment
Member
There was a problem hiding this comment.
nice work, this looks like a great modernization. I love seeing more deleted LOC than added :)
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.
Removes the deprecated
GoogleApiClient(deprecated 2017, removed in currentplay-services-locationmajors) from the location stack. All real location work already flowed throughFusedLocationProviderClient, so this is mostly deletion (−509/+70 across 21 files):LocationHelperrequests fused updates directly instead of via theconnect()/onConnected()dance; theGoogleApiAvailabilityguard is preserved so devices without Play Services still fall back toLocationManager. DeprecatedLocationRequest.create()chain replaced withLocationRequest.Builder.Application.getLastKnownLocation()andReportActivity.start()drop theirGoogleApiClientparameters; ~10 holder classes lose theirmGoogleApiClientfields and connect/disconnect lifecycle boilerplate.LocationHelper(Context, int)never initializedmLocationRequest(NavigationService's fused subscription requested updates with a null request);SurveyWebViewActivitycrashed inonStop()on devices without Play Services (lateinitclient never assigned);Task.getResult()was called afterisComplete()instead ofisSuccessful(), which crashes on a failed task (e.g. permission revoked).Verified:
obaGoogleDebug+obaMaplibreDebugcompile;LocationUtilsTestpasses on a Pixel 7 Pro; live smoke test confirmed fused registration withHIGH_ACCURACYwhile foregrounded and clean teardown (zero leaked registrations) on backgrounding viadumpsys location.Closes #1570
Summary by CodeRabbit