Animated vehicle markers with speed-based extrapolation#1554
Conversation
- Add distanceAlongTrip to ObaTripSchedule.StopTime, with findNextStopIndex, findSegmentStartIndex, and getStartTime helpers - Expose getLastKnownDistanceAlongTrip on ObaTripStatus - Add ObaRoute.isGradeSeparated() for route-type classification - Add Kotlin extensions: ObaTripStatus.isLocationRealtime and ObaTripSchedule.speedAtDistance - Add LocationUtils.haversineDistance matching the OBA server formula - Add UIUtils.formatElapsedTime for human-readable durations - Add JVM test dependency and tests for schedule queries and haversine
- Remove pre-ICS/HC/GB animation code paths from AnimationUtil; the minSdk is 21 so only the ObjectAnimator path is needed. Add optional duration and completion-callback parameters. - Extract polyline-arrow stamp creation from BaseMapFragment into StampedPolylineFactory so it can be reused by other map overlays.
Probability distributions for modeling vehicle speed uncertainty: - ProbDistribution interface with pdf, cdf, quantile, and bisect - GammaDistribution, DiracDistribution, GammaMixtureDistribution - AffineTransformDistribution for unit conversion (m/s to mph) - FrozenDistribution for caching expensive quantile computations Polyline class for distance-based interpolation along transit routes with O(log n) lookup via precomputed cumulative distances. Includes JVM tests for all distributions and an instrumented test for polyline interpolation against real geographic coordinates.
- Extrapolator base class and ExtrapolationResult sealed class - GammaExtrapolator: models vehicle speed as an H34 two-gamma mixture distribution, producing a probability distribution over distance - ScheduleReplayExtrapolator: uses GTFS schedule segment speeds when no real-time data is available - Trip: per-trip data holder that selects the appropriate strategy based on available vehicle history and schedule data Includes JVM tests for gamma model numerics, schedule replay behavior, and Trip gating/recording logic.
- TripDataManager: singleton cache for per-trip data (vehicle history, schedule, shape) with SharedFlow-based change notification - RoutePoller / TripPoller: coroutine-based polling replacing the old Handler/Loader mechanism, with rate-limiting across concurrent trips - ThrottledFrameLoop: Choreographer wrapper that throttles frame callbacks to a target FPS for animation loops - Add kotlinx-coroutines dependency - Instrumented test for the speed estimation integration
Refactor VehicleOverlay into focused components: - VehicleMapController: manages marker lifecycle, position extrapolation, and per-frame animation using ThrottledFrameLoop - VehicleMarkerState: per-vehicle view model holding marker refs, extrapolator, and animation state - VehicleIconFactory: marker icon bitmap cache with direction support - VehicleInfoWindowAdapter: info window rendering with ETA snippets - MapIconUtils: shared icon utilities Replace RouteMapController's Loader-based vehicle refresh with coroutine-based RoutePoller integrated with TripDataManager. Add selectVehicle to MapModeController for programmatic vehicle selection by trip ID.
- TripMapFragment: Google Maps fragment showing a single trip's route, vehicle position, and extrapolated distance probability distribution - TripRouteOverlay: stamped polyline for the route shape - TripVehicleOverlay: animated vehicle marker with data-received indicator - DistanceEstimateOverlay: renders the speed distribution as colored polyline segments along the route - TripExtrapolationController: coordinates frame-by-frame extrapolation - TripMapRendererFactory: constructs overlays from trip data - TripMapFragmentFactory: reflective instantiation via build config - MapLibre stub for the non-Google flavor - TripMapCallback interface for activity-fragment communication
Integrate the speed estimation pipeline into the user-facing flow: - TripDetailsActivity: toggle between list and map view; routes trip selection to TripDataManager so both views share live data - TripDetailsListFragment: feed trip details responses into the data manager; display speed info in the header - ArrivalsListHeader: render speed when available - Rename trip_details menu to trip_details_activity with new toggle and data view entries Add developer data views for debugging the speed estimation: - VehicleLocationDataActivity: tabbed view with graph + table - TrajectoryGraphView: trajectory chart with schedule overlay - GraphViewport: shared transform logic for trajectory rendering Register the new activity in AndroidManifest and add supporting layouts, drawables, strings, and styles.
aaronbrethorst
left a comment
There was a problem hiding this comment.
Brandon — this is a remarkable piece of work. A 9k-line PR could easily be a mess, and this one is the opposite: the math layer is tested to a standard you usually see in dedicated libraries (the H34 mixture worked-example, the PDF-integrates-to-one assertions, BisectTest's timeout guards on every degenerate input), the 888-line VehicleOverlay decomposes into well-bounded units, per-frame allocation discipline is real (subPolylineMapInto, the scratch Location, reused arrays in DistanceEstimateOverlay), and the commit-per-layer organization made the review tractable. The choreographer-driven main-thread invariant is a great design call — it lets Trip hold plain mutable fields and lets you skip a synchronization story you'd otherwise need to write and test.
Two themes pulled me back from a clean merge:
- Global singleton + multi-screen state.
TripDataManager.INSTANCEis shared across the route map, the trip details screen, and the data view. A few code paths assume they own it (e.g.RouteMapController.clearCurrentState()wipes the whole registry; in-flight fetches don't get cancelled). Under realistic use, screens will step on each other. - Lifecycle gaps. The choreographer loop and the pollers can outlive their host (fragment hidden via
FragmentManager, async map-ready arriving post-pause, fetch results resurrecting cleared trips).
Those are the real ones. The rest are tractable cleanups.
I'm going to ask for changes before this merges — the global-state corruption in particular has a clear path to user-visible weirdness (stale markers, animations referencing dead Trip objects). Once those land, this is shipping-quality.
Critical Issues (3 found)
1. RouteMapController.clearCurrentState() wipes the global trip registry
onebusaway-android/src/main/java/org/onebusaway/android/map/RouteMapController.java:150—TripDataManager.INSTANCE.clearAll()runs every time the user switches routes on the home map. ButTripDataManageris a singleton shared withTripDetailsActivity,TripMapFragment, andVehicleLocationDataActivity. If any of those screens is in the back stack with aTripreference, thatTripis now orphaned — the nextgetOrCreateTrip(tripId)call creates a new one, butVehicleMarkerState.tripis not refreshed inVehicleMapController.updateVehicle()(VehicleMapController.java:149-160), so the per-frame extrapolation loop reads from the now-stale instance.- Fix: stop calling
clearAll()here. TheMAX_TRACKED_TRIPS = 100LRU eviction will handle this naturally, or do a targeted eviction of trips matching the previous route. Also refreshvehicle.tripto the currentgetOrCreateTrip(tripId)inVehicleMapController.updateVehicle.
2. clearAll() doesn't cancel in-flight fetches; ghost trips can resurrect
onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripDataManager.kt:370-375together withensureFetched(305-333).clearAll()empties the registry and the pending sets, but tasks already running onfetchExecutorcontinue. When they complete,mainHandler.post { onSuccess(...) }callsgetOrCreateTrip(tripId).schedule = ...or... .polyline = ..., re-creating the trip you just cleared with only a schedule/shape and no status/history. Combined with #1, this is a recipe for partially-populated phantom trips persisting across user actions.- Fix: track an epoch counter and have the
onSuccessblock check it, or maintain a cancellation set that pending tasks consult before applying results.
3. Trip.kt:39 documents a synchronization scheme that doesn't exist
- The KDoc on
TripreadsThread safety: all mutable state is accessed under TripDataManager's @Synchronized lock.There is no@Synchronizedanywhere inTripDataManager—grepfinds only this Trip.kt comment. The real invariant ("main thread only") is documented onTripDataManager.kt:40-45, and the two file-level docs are now contradicting each other. - This isn't just a stale comment — it's actively misleading. A maintainer who reads
Trip.ktfirst will reasonably conclude they can callTripmethods from a background thread (since "the lock handles it"), which would race against everything else. Fix the docstring to reference the main-thread contract.
Important Issues (9 found)
4. TripMapFragment can start its poller and frame loop while paused
TripMapFragment.kt:172-182—onOverlaysReadyunconditionally starts both the extrapolation controller and a newTripDetailsPoller. It's invoked viaTripMapOverlayFactory.create, which can fire either inline (cached shape) or viaTripDataManager.ensureShape's main-thread callback after a network fetch. Either path can resolve afteronPauseran — the user backgrounds the app while the map is still loading, or rotates mid-fetch.- Net effect: poller and choreographer run while the fragment is paused. Add an
isResumed()guard, or setactivated = truehere and letonResume()start the moving parts.
5. onMapReady doesn't guard against post-detach invocation
TripMapFragment.kt:103-114.getMapAsync(this)is async;onMapReadycan fire afteronDestroyView. The firstrequireContext()(line 108) or therequireArguments()insidetripId(line 72) will throwIllegalStateExceptionin that window. Same exposure inactivate()at line 145.- Early-return if
!isAdded || view == null.
6. VehicleOverlay frame loop keeps running when the host fragment is hidden
VehicleOverlay.java:149-155andThrottledFrameLoop.kt. The loop only stops onclear()or whenmActivity.isDestroyed().HomeActivityhides the map fragment viaFragmentManager.hide()when the user navigates to starred stops/etc; the activity isn't destroyed and the choreographer keeps firingupdatePositions()on markers that aren't visible.- Plumb a stop into
BaseMapFragment.onPause/onHidden.
7. Trip.getOrCreateExtrapolator() silently locks in the wrong strategy
Trip.kt:142-152. Strategy selection runs once and is cached for the trip's lifetime. IfrouteTypeis null whenextrapolate()is first called — which happens wheneverrecordStatusruns beforerecordTripsForRouteResponsepopulates the route metadata — the trip is permanently bound toGammaExtrapolatoreven if a later poll learns it's actually a rail line. Subway/rail extrapolation silently degrades to the bus model for trips with the wrong race ordering.- Either invalidate the cached extrapolator when
routeTypechanges, defer construction untilrouteTypeis known, or document the invariant ongetOrCreateExtrapolatorandTrip.kt:36. (Today the class-level KDoc onTrip.kt:36-37also claims a third "schedule-only fallback" strategy that doesn't exist —getOrCreateExtrapolatoronly ever returns Gamma or ScheduleReplay.)
8. VehicleMapController.updatePositions catches RuntimeException — too broad
VehicleMapController.java:317-324. The comment says this is forrequire()failures in the gamma model, butRuntimeExceptionis wide enough to swallow NPE from a race withclear(),ConcurrentModificationExceptionifmStatesis mutated mid-iteration, andIllegalStateExceptionfrom the Maps API on a removed marker. At 20fps, a structural bug would surface asWlog spam and degraded animation — quietly enough that nobody would notice in production.- Narrow this to
IllegalArgumentException(which is whatrequire()throws) and let the rest propagate. Same pattern inTripExtrapolationController.kt:43-52.
9. Pollers.kt swallows everything and retries forever at full cadence
Pollers.kt:65-67, 115-117, 145-147.catch (e: Exception)is broad enough to swallow Jackson parse errors,IllegalStateExceptionfromApplication.get(), NPEs — anything that isn't anError. Combined with the unconditional 10sdelay(), a persistent failure (bus parked in a tunnel; misconfigured server; broken response shape) means six requests per minute, for the entire lifetime of the screen, with the user staring at a spinner that never resolves.- Narrow the catch to
IOExceptionplus a separate clause for parse/state errors, add exponential backoff with a cap, and surface failures back to the UI.TripDetailsListFragment.java:218,281-282,514-523has no escape hatch today — if every poll fails, the loading spinner shows indefinitely.fetchTripDetailsOnce(lines 129-149) explicitly documents itself as "does not notify callers on success or failure"; pull-to-refresh failures vanish silently.
10. AffineTransformDistribution accepts negative or zero scale silently
AffineTransformDistribution.kt:22-38. Forscale < 0,pdfreturns 0 (invalid: should be a flipped distribution),cdfbecomes a step atoffset, andquantileis non-monotone inp— violating theProbDistributioncontract. Forscale == 0, you get a degenerate point mass that doesn't integrate to 1. Today the only caller (GammaExtrapolator.kt:57) gets a non-negative scale becauseTrip.kt:131guardsdtMs < 0, but the type itself doesn't enforce its contract.- Add
require(scale > 0)(or accept0and explicitly route to aDiracDistribution(offset)). The other distributions in this package guard their preconditions and are exemplary; bring this one up to that bar.
11. Trip exposes all state via public var; parallel-array invariants live in convention
Trip.kt:41-72.history,fetchTimes,localFetchTimesareval mutableListOf<…>()— the reference is final but the contents are publicly mutable.schedule,serviceDate,polyline,routeType,lastActiveTripId,tripDetailsResponseare all publicvar. The three lists must stay index-aligned (truncated together, grown together) and onlyrecordStatusis a well-formed producer — but that contract is implicit. Any caller can break it.- At minimum, make the three lists
private valand exposeList<>views (or useHistorySnapshotexclusively). Make the cache fields settable only via package-private setters or via the manager.
12. HistorySnapshot is undermined by per-field accessors
TripDataManager.kt:159-187. The whole point ofHistorySnapshotis that two separategetHistory()/getFetchTimes()calls can see different sizes if a poll lands between them. But the per-field accessors (getHistory,getFetchTimes,getLocalFetchTimes,getHistorySize) are still public and several are unused by production code (only used in tests).- Either delete the per-field accessors and force callers to use
HistorySnapshot, or document the snapshot type as transitional in a way that doesn't outlive its label.
Suggestions (8 found)
13. onTripMapActivationFailed collapses six failure modes into one generic toast
TripDetailsActivity.java:240-246andTripMapFragment.kt:145-170.TripMapOverlayFactoryenumerates five distinct reasons (MISSING_SCHEDULE,MISSING_REFERENCES,TRIP_NOT_IN_REFERENCES,MISSING_SHAPE_ID,SHAPE_FETCH_FAILED) plus the "no cached details" case inactivate(). All six get the same toast: "Trip map data is not available yet." ForSHAPE_FETCH_FAILEDthe right answer is probably a retry, not falling back to list view. Plumb the reason throughonTripMapActivationFailed(reason)so distinct problems get distinct UX.
14. Stale test class name SpeedEstimatorTest
onebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/SpeedEstimatorTest.kt:35-37. KDoc claims "Tests for the speed estimation framework classes" but noSpeedEstimatorclass exists; the tests actually exerciseTripDataManagerand the extrapolation result types. Rename to something likeTripDataManagerExtrapolationTestand update the KDoc.
15. Polyline.kt:23-25 complexity claim overstates subPolyline
interpolateandbearingAtare O(log n);subPolylineInto/subPolylineMapIntoare O(log n + k) where k is the number of vertices in the requested range. The KDoc says "O(log n) via binary search" forsubPolyline— true for the lookups, false for the walk. Fix the comment or add a separate claim.
16. TripDataManager.changes emission contract is overstated
TripDataManager.kt:70. The KDoc says "Emits Unit whenever any mutation method runs" butnotifyChanged()is gated on actual state changes — duplicaterecordStatus, no-opputSchedule(null, …), zero-serviceDateputServiceDate, etc., all run without emitting. Adjust to "whenever a mutation method actually changes registry state."
17. TripDataManager.ensureFetched failure retry behavior needs at least a log bump
TripDataManager.kt:296-333. The "no data" path logs atD(line 328). A persistently failing schedule fetch means a route's gamma extrapolator silently returnsMissingScheduleforever, the user sees a stuck marker, and there's nothing inadb logcataboveDto point at it. Bump toWand consider a per-trip failure marker so debug overlays can surface it. Same theme as #9.
18. Polyline leaks its internal point list
Polyline.kt:26. Takes aList<Location>and exposes it as publicval points. Callers passing aMutableList<Location>(orLocationinstances they mutate) can desynccumulativeDistancesfrompointssilently. Defensive-copy in the constructor (points.toList()) and requiresize >= 2viarequire().
19. MainScope().launch in pollers is fire-and-forget
Pollers.kt:50, 100, 135. Each poller creates its own scope; nothing ties them to a lifecycle. The "caller owns lifecycle" contract is fine when honored, but it's also exactly the kind of thing #4 above slipped through. Consider attaching toviewLifecycleOwner.lifecycleScope(or making the poller aDefaultLifecycleObserver) so this is structurally enforced.
20. TripDataManager LRU keepalive comment lists too few methods
TripDataManager.kt:56-59calls outrecordStatus / getOrCreateTrip / isScheduleCachedbut every read that goes throughgetTrip()promotes the entry (getHistory,getAnchor,getSchedule,getPolyline, etc., all promote). A future maintainer who callsgetAnchoron a long-unobserved trip will be surprised to find they just kept it alive. Just say "every access viagetTrip/getOrCreateTrip."
Strengths
- Probability/math layer is the strongest part of this PR.
GammaDistribution,DiracDistribution, andFrozenDistributionuse constructorrequire()checks to make illegal states unrepresentable. The H34 worked-example inGammaSpeedModelTest:73-78pins down a real-world number;BisectTest's@Test(timeout = 1000)on every degenerate input prevents future infinite-loop regressions;extremely low speed does not crash(line 201-210) locks in the mixture fallback. - The
VehicleMapController.java:362-367reference-equality trick onanchorto detect "fresh data" is the natural signal and avoids epsilon comparisons. Comment explains the why, which is the right level. - Per-frame allocation discipline in
DistanceEstimateOverlay(reusedDoubleArray,latLngBuffer,scratchLocation) andPolyline.subPolylineMapIntois exactly right for a 20Hz loop. - The commit organization made review tractable — eight commits, each one mappable to a layer. Don't squash if you don't have to.
- Defensive numerics in
bisect(iteration caps, NaN/Inf guards, documented fallback semantics) are model. - The sealed
ExtrapolationResultcarries enough debug fidelity that future log messages and crash dumps will tell you why extrapolation didn't run, even if every caller currently treats it as binary. That's an investment that pays back.
Recommended Action
- Fix the global-state corruption: stop the indiscriminate
clearAll()inRouteMapController(#1), guard fetch results against post-clear resurrection (#2), and updateTrip.kt:39to match reality (#3). - Tighten the fragment/poller lifecycles: guard
onOverlaysReady/onMapReadyagainst post-pause invocation (#4, #5), and stop the choreographer loop on hide/pause (#6). - Fix the extrapolator strategy lock-in (#7) and narrow the
RuntimeExceptioncatches (#8). - Make the poller failure path observable: narrow exceptions, add backoff, surface to UI (#9). Either fix
fetchTripDetailsOnce's silent contract or document it loudly. - Tighten the types:
AffineTransformDistributioninvariants (#10),Tripfield visibility (#11),HistorySnapshotexclusivity (#12). - Suggestions are independent — pick them up as you go.
Verdict: request changes.
The architecture is right. The math is solid. The remaining issues are the kind that surface in production rather than CI, and they're all addressable without restructuring anything you've built.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds extrapolation math, TripState/TripStore, polling/fetchers, map overlays/controllers, trip-map UI and debug views, resources, and extensive unit/instrumentation tests. ChangesTrip map, extrapolation, and overlay integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.java (1)
1030-1038:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear path drops references without removing existing polylines from the map.
At Line [1033], calling
mLineOverlay.clear()before removing existing polylines leaves old route lines rendered and unreachable for cleanup.🛠️ Proposed fix
`@Override` public void setRouteOverlay(int lineOverlayColor, ObaShape[] shapes, boolean clear) { if (mMap != null) { if (clear) { + for (Polyline polyline : mLineOverlay) { + polyline.remove(); + } mLineOverlay.clear(); } int totalPoints = 0; for (ObaShape s : shapes) { totalPoints += addArrowPolyline(s.getPoints(), lineOverlayColor); } Log.d(TAG, "Total points for route polylines = " + totalPoints); } }Also applies to: 1043-1047
🤖 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/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.java` around lines 1030 - 1038, The clear branch in setRouteOverlay currently calls mLineOverlay.clear() which drops references but leaves existing Polyline objects on the map; instead, iterate over mLineOverlay and call remove() on each Polyline (or map overlay object) to remove them from mMap, then clear the collection. Update both occurrences where clear is handled (in setRouteOverlay and the similar block around the 1043-1047 region), and ensure you null-check mLineOverlay and each element before calling remove().
🧹 Nitpick comments (5)
onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Trip.kt (1)
38-40: 💤 Low valueThread safety comment is slightly misleading.
The comment mentions
@Synchronized lockbutTripDataManageruses main-thread-only enforcement, not@Synchronized. Consider updating for accuracy.📝 Suggested fix
/** * All data for a single tracked trip: vehicle history, extrapolation anchor, schedule, polyline, * and route metadata. Provides [extrapolate] which selects the appropriate strategy (gamma, * schedule replay, or schedule-only fallback) based on the data available. * - * Thread safety: all mutable state is accessed under TripDataManager's `@Synchronized` lock. + * Thread safety: all mutable state is accessed on the main thread only via TripDataManager. */🤖 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/extrapolation/data/Trip.kt` around lines 38 - 40, Update the thread-safety comment in Trip (the class header block) to accurately reflect TripDataManager's synchronization strategy: replace the reference to TripDataManager's "`@Synchronized` lock" with a statement that mutable state is accessed under TripDataManager's main-thread-only enforcement (or the actual mechanism used by TripDataManager), and mention TripDataManager by name so readers can find the enforcement policy.onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/ScheduleReplayExtrapolator.kt (1)
65-70: 💤 Low valueConsider adding a brief comment explaining the exception handling.
The
IndexOutOfBoundsExceptionis intentionally caught and converted tonull(signaling out-of-bounds distance), but the static analysis tool flagged this as a swallowed exception. A short inline comment would clarify the intent and suppress the warning.📝 Suggested comment
val segIdx = try { schedule.findSegmentStartIndex(startDist) } catch (e: IndexOutOfBoundsException) { + // startDist outside schedule bounds → no valid extrapolation return null }🤖 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/extrapolation/ScheduleReplayExtrapolator.kt` around lines 65 - 70, Add a short inline comment next to the try/catch around schedule.findSegmentStartIndex(startDist) (where segIdx is assigned) explaining that IndexOutOfBoundsException is intentionally caught and converted to a null return to signal that startDist is out-of-bounds; mention that this is deliberate to suppress static-analysis warnings about swallowing the exception and that no further handling is required. Ensure the comment references schedule.findSegmentStartIndex and the return null behavior so future readers and analyzers understand the intent.onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripDataManager.kt (1)
46-49: ⚡ Quick winConsider adding main-thread assertion in debug builds.
The class documents "main thread only" but doesn't enforce it. A debug-mode assertion would catch violations early during development.
🛡️ Suggested addition
object TripDataManager { private const val TAG = "TripDataManager" private const val MAX_TRACKED_TRIPS = 100 + + private fun assertMainThread() { + if (BuildConfig.DEBUG && Looper.myLooper() != Looper.getMainLooper()) { + throw IllegalStateException("TripDataManager must be accessed from main thread") + } + }Then call
assertMainThread()at the start of public methods likerecordStatus,getOrCreateTrip, etc.🤖 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/extrapolation/data/TripDataManager.kt` around lines 46 - 49, TripDataManager is documented as "main thread only" but doesn't enforce it; add a debug-only assertion to catch misuse by creating a private assertMainThread() helper (use Looper.getMainLooper() vs Looper.myLooper() or BuildConfig.DEBUG/Assertions) and call assertMainThread() at the start of all public entry points such as recordStatus(...), getOrCreateTrip(...), removeTrip(...), and any other public methods in object TripDataManager so violations fail fast in debug builds while being a no-op in release.onebusaway-android/src/main/java/org/onebusaway/android/ui/SettingsActivity.java (1)
645-645: ⚡ Quick winFix AOSP indentation in
AdvancedSettingsFragment.The field declaration and
if (BuildConfig.USE_FIXED_REGION)block should keep the same indentation level as surrounding members/statements.Suggested fix
- private FirebaseAnalytics mFirebaseAnalytics; + private FirebaseAnalytics mFirebaseAnalytics; ... -if (BuildConfig.USE_FIXED_REGION) { + if (BuildConfig.USE_FIXED_REGION) {As per coding guidelines,
**/*.{java,kt,kts}: Use AOSP code style.Also applies to: 677-677
🤖 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/SettingsActivity.java` at line 645, The indentation of the FirebaseAnalytics field and the conditional using BuildConfig.USE_FIXED_REGION inside AdvancedSettingsFragment is incorrect per AOSP style; locate the declaration "private FirebaseAnalytics mFirebaseAnalytics;" and the "if (BuildConfig.USE_FIXED_REGION)" block inside class AdvancedSettingsFragment and adjust them to match the surrounding member/member-block indentation level (align with other fields/methods in the fragment) so they use the same indenting as adjacent members.onebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/TrajectoryGraphView.kt (1)
62-67: ⚡ Quick winClock-domain mismatch documented.
The TODO correctly identifies that client/server clock skew will shift the "now" line away from the latest trajectory point. The suggested fix (translating via Trip's fetchTimes/localFetchTimes offset) is appropriate.
Would you like me to open a tracking issue for this, or do you plan to address it in a follow-up PR?
🤖 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/dataview/TrajectoryGraphView.kt` around lines 62 - 67, currentTime is set from System.currentTimeMillis(), causing a client/server clock-domain mismatch with trajectory timestamps (history[].lastUpdateTime, serviceDate + stopTime*1000); fix by converting local currentTime into server-clock using the Trip fetch offset: compute offset = Trip.fetchTimes - Trip.localFetchTimes (or equivalent) and apply that offset when setting/using currentTime in TrajectoryGraphView so the "now" line aligns with server timestamps; update usages of currentTime in drawing/layout to use the adjusted time and ensure the offset is fetched from the Trip instance passed into this view (refer to currentTime, Trip, fetchTimes, localFetchTimes, history[].lastUpdateTime, serviceDate, stopTime).
🤖 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.
Inline comments:
In
`@onebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/SpeedEstimatorTest.kt`:
- Around line 109-118: The test currently calls history.toMutableList().clear()
which clears only a copied list and cannot detect whether getHistory() returns a
defensive copy; change the mutation to operate on the actual returned list
(e.g., remove the toMutableList() and call dm.getHistory("trip1").clear() or
cast the returned value to MutableList and call clear() like
(dm.getHistory("trip1") as MutableList).clear()) so the test will fail if
getHistory() exposes its internal collection; update the
testTrackerDefensiveCopy function to mutate the returned list directly and then
assert dm.getHistory("trip1").size remains 1.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripExtrapolationController.kt`:
- Around line 59-63: When shapeData.interpolate(medianDist) returns null, the
previous vehicle marker is left visible; change the conditional in
TripExtrapolationController so that if loc is null you explicitly hide the
marker (call vehicleOverlay.hideVehiclePosition() or equivalent) instead of
skipping updateVehiclePosition, and still call
vehicleOverlay.updateEstimateOverlays(distribution). Update the block around
vehicleOverlay.updateVehiclePosition(loc, trip.anchor, now) to add an else
branch that hides/clears the stale vehicle position.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripMapFragment.kt`:
- Around line 159-170: TripMapOverlayFactory.create(...) callbacks (the success
handler onOverlaysReady and the failure lambda that calls
mapCallback?.onTripMapActivationFailed()) must be guarded against stale fragment
state: add an activation token (e.g., a generation counter or UUID field on
TripMapFragment) that you increment/renew when starting a new activation and
when the fragment/view is paused/destroyed, then capture the current token when
calling TripMapOverlayFactory.create and early-return from the success and
failure lambdas if the captured token no longer matches the fragment’s current
token; also cancel or increment the token before initiating a new create to drop
prior callbacks.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripVehicleOverlay.kt`:
- Around line 79-87: activate currently only constructs estimateOverlay when
vehiclePosition is non-null, preventing later updateEstimateOverlays() from
working; change activate(vehiclePosition: LatLng?) so estimateOverlay is always
instantiated (assign DistanceEstimateOverlay(shapeData, overlayColor) to
estimateOverlay) and call its create(map, context, POLYLINE_WIDTH_PX,
vehiclePosition) only if vehiclePosition != null (or otherwise defer creation
steps until updateEstimateOverlays() supplies a position). Reference: function
activate, variable estimateOverlay, class DistanceEstimateOverlay and its
create(map, context, POLYLINE_WIDTH_PX, vehiclePosition), and
updateEstimateOverlays.
- Around line 161-174: The early return on unchanged lastDataReceivedUpdateTime
prevents refreshing the snippet/InfoWindow; change the logic in
TripVehicleOverlay (around
lastDataReceivedUpdateTime/updateTime/dataReceivedMarker handling) so that when
updateTime == lastDataReceivedUpdateTime you still update
dataReceivedMarker.snippet = label and call marker.showInfoWindow() if
dataReceivedInfoShown, but only skip updating marker.position (and other heavy
updates) when there is truly no new AVL data; in short, remove the early-return
that exits before snippet refresh and instead conditionally update position when
newData is true while always refreshing the snippet/InfoWindow for
dataReceivedMarker.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleInfoWindowAdapter.java`:
- Around line 135-147: The elapsed time computed for the "last updated" label
can be negative; clamp elapsedSec to zero before computing minutes/seconds and
formatting. Inside VehicleInfoWindowAdapter (where
elapsedSec/elapsedMin/secMod60 are computed and lastUpdated is formed), replace
the raw elapsedSec calculation with a non-negative value (e.g., elapsedSec =
max(0, TimeUnit.MILLISECONDS.toSeconds(now - status.getLastUpdateTime()))) then
recompute elapsedMin and secMod60 from that clamped elapsedSec so
mSource.isExtrapolating(marker) and views.lastUpdatedView.setText(lastUpdated)
use a non-negative interval.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMapController.java`:
- Around line 136-146: The marker returned by mMap.addMarker(...) can be null;
guard its result before calling methods or assigning fields to avoid NPEs. In
the addVehicle code path (where Marker m is created) and in
showDataReceivedMarker(), check if the returned Marker is null and handle it
(e.g., log a warning via the existing logger/Log, skip calling m.setTag(...),
skip putting into mStates, and avoid assigning to
VehicleMarkerState.vehicleMarker) instead of dereferencing; update uses of
VehicleMarkerState.vehicleMarker, m.setTag, and mStates.put(tripId, ...) to only
run when m != null so the app won't crash when addMarker returns null.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleOverlay.java`:
- Around line 87-91: After populating the controller in updateVehicles, avoid
starting the frame loop when there are no active vehicle markers; instead check
the controller for active markers (e.g. add/ use a method like
hasActiveVehicleMarkers(), getActiveVehicleCount(), or check the marker
collection on mController after mController.populate(...)) and call
mFrameLoop.start() only if count > 0, otherwise call mFrameLoop.stop(); apply
the same change to the other updateVehicles occurrence referenced (lines
149-155) so the loop isn't started when there are zero vehicles.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/FrozenDistribution.kt`:
- Around line 45-47: The quantile method currently returns the literal 0.0 when
p <= 0.0 which is wrong for distributions that don't start at 0; change the
early-return in quantile(p: Double) to return the precomputed lowest quantile
from the table (e.g., table[0] or table.first()) instead of 0.0 so the method
consistently uses the distribution's bounds (leave the p >= 1.0 case returning
table.last()).
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/TripDetailsActivity.java`:
- Around line 219-221: The fragment transaction in TripDetailsActivity uses
add(...) which can stack fragments; change the transaction to use
replace(R.id.fragment_container, tripDetListFrag, TripDetailsListFragment.TAG)
via getSupportFragmentManager().beginTransaction() and then commit() so the
existing fragment in fragment_container is replaced rather than added on top;
locate the call that currently uses add(...) and swap to replace(...) (keeping
TripDetailsListFragment.TAG and the same commit strategy).
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/TripDetailsListFragment.java`:
- Around line 279-283: When loading cached trip details (e.g. result of
mDataManager.getTripDetails(mTripId)) the UI path currently calls
setTripDetails(...) but does not notify the TripDataCallback, so the activity
may miss initial cached data and keep state (like the map toggle) stale; update
setTripDetails(...) to invoke the same TripDataCallback notification used by
updateVehiclePosition() (call the method that notifies the TripDataCallback or
invoke the callback handler directly after setting data) so cached loads trigger
the same callback flow, and mirror this change for the other cached-load sites
referenced (the other occurrences around the 292-305 and 520-522 blocks) to
ensure consistent behavior.
- Around line 405-411: In setUpHeader(), ensure
updateLocationDataMenuState(status) is invoked before any early returns so the
activity menu visibility stays in sync; specifically, call
updateLocationDataMenuState(status) right before the block that returns when
status == null and likewise before the inactive-trip early-return branches (the
ones around the existing checks at lines ~425-430 and ~511-512), or restructure
setUpHeader() to call updateLocationDataMenuState(status) once unconditionally
near the top after computing status so all return paths use the updated state.
In
`@onebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/MapLibreMapFragment.java`:
- Around line 903-906: The MapLibreMapFragment currently no-ops in
selectVehicle(String) which breaks the fragment contract; either implement
selection or explicitly opt-out so callers/UI can hide the affordance. Fix by
replacing the silent Log.w in MapLibreMapFragment.selectVehicle to a clear
unsupported response (e.g., throw UnsupportedOperationException or call a
fragment-level callback like onSelectionUnsupported) and add a corresponding
capability flag/method (e.g., supportsVehicleSelection() or
isVehicleSelectionSupported) that UI code can query to hide/disable trip/vehicle
selection controls; update callers to check that flag before invoking
selectVehicle.
In
`@onebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/tripmap/TripMapFragment.kt`:
- Around line 53-55: The TextView in TripMapFragment.kt currently hardcodes the
user-facing message ("Trip map is not yet available for MapLibre builds."); move
this string into resources and use the string resource instead: add a new string
resource name (e.g. trip_map_unavailable_maplibre) in strings.xml and replace
the hardcoded text in the TextView creation inside TripMapFragment (the return
TextView(requireContext()).apply { ... } block) with a call to
getString(R.string.trip_map_unavailable_maplibre) so the message is localizable
and centrally managed.
In
`@onebusaway-android/src/test/java/org/onebusaway/android/extrapolation/data/TripTest.kt`:
- Around line 90-99: The test for the boundary case uses an off-by-one value;
update the assertion input so Trip.extrapolate is called with exactly localTime
+ MAX_HORIZON_MS (i.e., localTime + 900_000L) instead of localTime + 900_001L to
verify the exact horizon behavior; locate the test method `extrapolate returns
Stale at exactly max horizon` in TripTest and change the call that computes dtMs
to use the exact MAX_HORIZON_MS boundary when invoking `trip.extrapolate`.
---
Outside diff comments:
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.java`:
- Around line 1030-1038: The clear branch in setRouteOverlay currently calls
mLineOverlay.clear() which drops references but leaves existing Polyline objects
on the map; instead, iterate over mLineOverlay and call remove() on each
Polyline (or map overlay object) to remove them from mMap, then clear the
collection. Update both occurrences where clear is handled (in setRouteOverlay
and the similar block around the 1043-1047 region), and ensure you null-check
mLineOverlay and each element before calling remove().
---
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Trip.kt`:
- Around line 38-40: Update the thread-safety comment in Trip (the class header
block) to accurately reflect TripDataManager's synchronization strategy: replace
the reference to TripDataManager's "`@Synchronized` lock" with a statement that
mutable state is accessed under TripDataManager's main-thread-only enforcement
(or the actual mechanism used by TripDataManager), and mention TripDataManager
by name so readers can find the enforcement policy.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripDataManager.kt`:
- Around line 46-49: TripDataManager is documented as "main thread only" but
doesn't enforce it; add a debug-only assertion to catch misuse by creating a
private assertMainThread() helper (use Looper.getMainLooper() vs
Looper.myLooper() or BuildConfig.DEBUG/Assertions) and call assertMainThread()
at the start of all public entry points such as recordStatus(...),
getOrCreateTrip(...), removeTrip(...), and any other public methods in object
TripDataManager so violations fail fast in debug builds while being a no-op in
release.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/ScheduleReplayExtrapolator.kt`:
- Around line 65-70: Add a short inline comment next to the try/catch around
schedule.findSegmentStartIndex(startDist) (where segIdx is assigned) explaining
that IndexOutOfBoundsException is intentionally caught and converted to a null
return to signal that startDist is out-of-bounds; mention that this is
deliberate to suppress static-analysis warnings about swallowing the exception
and that no further handling is required. Ensure the comment references
schedule.findSegmentStartIndex and the return null behavior so future readers
and analyzers understand the intent.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/TrajectoryGraphView.kt`:
- Around line 62-67: currentTime is set from System.currentTimeMillis(), causing
a client/server clock-domain mismatch with trajectory timestamps
(history[].lastUpdateTime, serviceDate + stopTime*1000); fix by converting local
currentTime into server-clock using the Trip fetch offset: compute offset =
Trip.fetchTimes - Trip.localFetchTimes (or equivalent) and apply that offset
when setting/using currentTime in TrajectoryGraphView so the "now" line aligns
with server timestamps; update usages of currentTime in drawing/layout to use
the adjusted time and ensure the offset is fetched from the Trip instance passed
into this view (refer to currentTime, Trip, fetchTimes, localFetchTimes,
history[].lastUpdateTime, serviceDate, stopTime).
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/SettingsActivity.java`:
- Line 645: The indentation of the FirebaseAnalytics field and the conditional
using BuildConfig.USE_FIXED_REGION inside AdvancedSettingsFragment is incorrect
per AOSP style; locate the declaration "private FirebaseAnalytics
mFirebaseAnalytics;" and the "if (BuildConfig.USE_FIXED_REGION)" block inside
class AdvancedSettingsFragment and adjust them to match the surrounding
member/member-block indentation level (align with other fields/methods in the
fragment) so they use the same indenting as adjacent members.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63b8f6e8-3941-434f-9b9a-f0b152661edd
📒 Files selected for processing (84)
.gitignoreonebusaway-android/build.gradleonebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/InterpolateAlongPolylineTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/SpeedEstimatorTest.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/AnimationUtil.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/MapHelpV2.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/MapIconUtils.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/StampedPolylineFactory.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/ThrottledFrameLoop.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleIconFactory.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleIconParams.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleInfoWindowAdapter.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMapController.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMarkerState.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleOverlay.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/DistanceEstimateOverlay.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripExtrapolationController.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripMapFragment.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripMapRendererFactory.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripRouteOverlay.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripVehicleOverlay.ktonebusaway-android/src/main/AndroidManifest.xmlonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/Extrapolator.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/GammaExtrapolator.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/ScheduleReplayExtrapolator.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Pollers.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Trip.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripDataManager.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/AffineTransformDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/DiracDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/FrozenDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/GammaDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/GammaMixtureDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/ProbDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/io/elements/ObaElementExtensions.ktonebusaway-android/src/main/java/org/onebusaway/android/io/elements/ObaRoute.javaonebusaway-android/src/main/java/org/onebusaway/android/io/elements/ObaTripSchedule.javaonebusaway-android/src/main/java/org/onebusaway/android/io/elements/ObaTripStatus.javaonebusaway-android/src/main/java/org/onebusaway/android/io/elements/ObaTripStatusElement.javaonebusaway-android/src/main/java/org/onebusaway/android/map/MapModeController.javaonebusaway-android/src/main/java/org/onebusaway/android/map/MapParams.javaonebusaway-android/src/main/java/org/onebusaway/android/map/RouteMapController.javaonebusaway-android/src/main/java/org/onebusaway/android/map/TripMapFragmentFactory.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListHeader.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/SettingsActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripDetailsActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripDetailsListFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripMapCallback.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/GraphViewport.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/TrajectoryGraphView.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/VehicleLocationDataActivity.ktonebusaway-android/src/main/java/org/onebusaway/android/util/LocationUtils.javaonebusaway-android/src/main/java/org/onebusaway/android/util/Polyline.ktonebusaway-android/src/main/java/org/onebusaway/android/util/UIUtils.javaonebusaway-android/src/main/res/drawable/ic_fast_estimate.xmlonebusaway-android/src/main/res/drawable/ic_list_white.xmlonebusaway-android/src/main/res/drawable/ic_signal_indicator.xmlonebusaway-android/src/main/res/drawable/ic_vehicle_position.xmlonebusaway-android/src/main/res/layout/activity_trip_details.xmlonebusaway-android/src/main/res/layout/activity_vehicle_location_data.xmlonebusaway-android/src/main/res/layout/route_debug.xmlonebusaway-android/src/main/res/layout/route_debug_list_item.xmlonebusaway-android/src/main/res/layout/trip_details.xmlonebusaway-android/src/main/res/layout/trip_details_head.xmlonebusaway-android/src/main/res/layout/vehicle_debug.xmlonebusaway-android/src/main/res/layout/vehicle_info_window.xmlonebusaway-android/src/main/res/menu/trip_details_activity.xmlonebusaway-android/src/main/res/raw/light_map.jsononebusaway-android/src/main/res/values/donottranslate.xmlonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/main/res/values/styles.xmlonebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/MapLibreMapFragment.javaonebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/tripmap/TripMapFragment.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/GammaSpeedModelTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/ScheduleReplayExtrapolatorTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/data/TripTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/AffineTransformDistributionTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/BisectTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/DiracDistributionTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/FrozenDistributionTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/GammaDistributionTest.ktonebusaway-android/src/test/java/org/onebusaway/android/io/elements/ObaTripScheduleTest.ktonebusaway-android/src/test/java/org/onebusaway/android/util/HaversineDistanceTest.kt
💤 Files with no reviewable changes (1)
- onebusaway-android/src/main/res/layout/trip_details.xml
| fun testTrackerDefensiveCopy() { | ||
| val status = createStatus("v1", "trip1", 47.0, -122.0, 100.0, 100.0, 5000.0, 1000L) | ||
| dm.recordStatus(status, System.currentTimeMillis(), System.currentTimeMillis()) | ||
|
|
||
| val history = dm.getHistory("trip1") | ||
| history.toMutableList().clear() // Modifying a copy | ||
|
|
||
| // Internal history should be unaffected | ||
| assertEquals(1, dm.getHistory("trip1").size) | ||
| } |
There was a problem hiding this comment.
Defensive-copy assertion is currently ineffective.
At Line [114], history.toMutableList().clear() only clears a brand-new copy, so this test can’t detect a non-defensive getHistory() implementation.
💡 Proposed test fix
`@Test`
fun testTrackerDefensiveCopy() {
val status = createStatus("v1", "trip1", 47.0, -122.0, 100.0, 100.0, 5000.0, 1000L)
dm.recordStatus(status, System.currentTimeMillis(), System.currentTimeMillis())
- val history = dm.getHistory("trip1")
- history.toMutableList().clear() // Modifying a copy
-
- // Internal history should be unaffected
- assertEquals(1, dm.getHistory("trip1").size)
+ val snapshot = dm.getHistory("trip1")
+ val status2 = createStatus("v1", "trip1", 47.001, -122.0, 200.0, 200.0, 5000.0, 2000L)
+ dm.recordStatus(status2, System.currentTimeMillis(), System.currentTimeMillis())
+
+ // Snapshot should not be a live view.
+ assertEquals(1, snapshot.size)
+ assertEquals(2, dm.getHistory("trip1").size)
}📝 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 testTrackerDefensiveCopy() { | |
| val status = createStatus("v1", "trip1", 47.0, -122.0, 100.0, 100.0, 5000.0, 1000L) | |
| dm.recordStatus(status, System.currentTimeMillis(), System.currentTimeMillis()) | |
| val history = dm.getHistory("trip1") | |
| history.toMutableList().clear() // Modifying a copy | |
| // Internal history should be unaffected | |
| assertEquals(1, dm.getHistory("trip1").size) | |
| } | |
| fun testTrackerDefensiveCopy() { | |
| val status = createStatus("v1", "trip1", 47.0, -122.0, 100.0, 100.0, 5000.0, 1000L) | |
| dm.recordStatus(status, System.currentTimeMillis(), System.currentTimeMillis()) | |
| val snapshot = dm.getHistory("trip1") | |
| val status2 = createStatus("v1", "trip1", 47.001, -122.0, 200.0, 200.0, 5000.0, 2000L) | |
| dm.recordStatus(status2, System.currentTimeMillis(), System.currentTimeMillis()) | |
| // Snapshot should not be a live view. | |
| assertEquals(1, snapshot.size) | |
| assertEquals(2, dm.getHistory("trip1").size) | |
| } |
🤖 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/extrapolation/test/SpeedEstimatorTest.kt`
around lines 109 - 118, The test currently calls history.toMutableList().clear()
which clears only a copied list and cannot detect whether getHistory() returns a
defensive copy; change the mutation to operate on the actual returned list
(e.g., remove the toMutableList() and call dm.getHistory("trip1").clear() or
cast the returned value to MutableList and call clear() like
(dm.getHistory("trip1") as MutableList).clear()) so the test will fail if
getHistory() exposes its internal collection; update the
testTrackerDefensiveCopy function to mutate the returned list directly and then
assert dm.getHistory("trip1").size remains 1.
| @Override | ||
| public void selectVehicle(String tripId) { | ||
| Log.w(TAG, "selectVehicle() is not yet implemented for MapLibre"); | ||
| } |
There was a problem hiding this comment.
Implement selectVehicle() behavior or disable the entry path for MapLibre.
Line 904 currently makes vehicle selection a silent no-op, so MapLibre users can trigger an action that does nothing. This breaks the map fragment contract for trip/vehicle selection flows.
A safe short-term mitigation is to gate/hide the selection affordance for MapLibre until selection is implemented.
🤖 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/maplibre/java/org/onebusaway/android/map/maplibre/MapLibreMapFragment.java`
around lines 903 - 906, The MapLibreMapFragment currently no-ops in
selectVehicle(String) which breaks the fragment contract; either implement
selection or explicitly opt-out so callers/UI can hide the affordance. Fix by
replacing the silent Log.w in MapLibreMapFragment.selectVehicle to a clear
unsupported response (e.g., throw UnsupportedOperationException or call a
fragment-level callback like onSelectionUnsupported) and add a corresponding
capability flag/method (e.g., supportsVehicleSelection() or
isVehicleSelectionSupported) that UI code can query to hide/disable trip/vehicle
selection controls; update callers to check that flag before invoking
selectVehicle.
| fun `extrapolate returns Stale at exactly max horizon`() { | ||
| val trip = Trip("trip1") | ||
| val serverTime = 100_000L | ||
| val localTime = 100_000L | ||
| trip.recordStatus( | ||
| status(distanceAlongTrip = 500.0, lastUpdateTime = serverTime), | ||
| serverTimeMs = serverTime, localTimeMs = localTime) | ||
| // dtMs == MAX_HORIZON_MS should still be stale (> comparison) | ||
| val result = trip.extrapolate(localTime + 900_001L) | ||
| assertTrue(result is ExtrapolationResult.Stale) |
There was a problem hiding this comment.
Boundary-case test doesn’t currently test the exact horizon.
Line 98 uses localTime + 900_001L, so this repeats the “exceeds horizon” case instead of validating the exact MAX_HORIZON_MS boundary.
✅ Minimal fix
- val result = trip.extrapolate(localTime + 900_001L)
+ val result = trip.extrapolate(localTime + 900_000L)🤖 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/test/java/org/onebusaway/android/extrapolation/data/TripTest.kt`
around lines 90 - 99, The test for the boundary case uses an off-by-one value;
update the assertion input so Trip.extrapolate is called with exactly localTime
+ MAX_HORIZON_MS (i.e., localTime + 900_000L) instead of localTime + 900_001L to
verify the exact horizon behavior; locate the test method `extrapolate returns
Stale at exactly max horizon` in TripTest and change the call that computes dtMs
to use the exact MAX_HORIZON_MS boundary when invoking `trip.extrapolate`.
…rip identity Restructure the trip data layer to make stale-Trip-reference bugs unrepresentable and to separate state from fetch orchestration: - TripStore: pure synchronous main-thread state. Trip instances are permanent (one per tripId for the life of the process), so holding a Trip reference across data updates is always safe. Memory is bounded by evicting trip *payload* (Trip.clearData) via a composed android.util.LruCache, never the instance itself. - TripFetcher: all async machinery (executor, in-flight dedup, ensureSchedule/ensureShape). The schedule/shape backfill that previously fired as a side effect inside recordTripsForRouteResponse is now an explicit step in RoutePoller. - Drop TripDataManager.clearAll() from RouteMapController route switches: the registry is shared with trip details, trip map, and location data screens, and wiping it orphaned their Trip references. - Extract forEachActiveTrip so TripStore recording and TripFetcher backfills walk trips-for-route responses identically. - Rename lastActiveTripId to vehicleActiveTripId and document it (the trip the vehicle most recently reported as active); delete the uncalled putLastActiveTripId.
Now that Trip identity is permanent, holding a Trip reference is always safe — so the store no longer needs to mirror Trip's property surface with tripId-keyed delegating accessors. New contract: read the Trip directly; write through the Store. - Delete ~13 pass-through read accessors, the HistorySnapshot copy class, and dead members (putRouteType, putShape, getFetchTimes, getLocalFetchTimes, getRouteType). - Delete the dormant changes SharedFlow and notifyChanged plumbing — its only collector died with DataViewActivity (f8727ce) — along with the changed-boolean bookkeeping that existed only to gate it. - Consumers acquire a Trip and read fields: TripDetailsListFragment holds mPolledTrip; VehicleLocationDataActivity holds its trip (lateinit, acquired in onCreate) and builds the debug table/graph from it directly. - Establish and document the acquisition rule: getOrCreateTrip is for acquiring a reference to hold or write to; getTrip is for transient reads and never creates shells as a side effect. - Document the serviceDate 0-means-unknown sentinel on Trip.
Unify the trip data layer's two network sources on one async idiom and document their split: Pollers are the time-driven half (recurring, lifecycle-bound re-fetch of volatile vehicle status), TripFetcher the demand-driven half (one-shot memoization of immutable schedules/shapes plus explicit refreshes). - Replace TripFetcher's thread pool + Handler with MainScope + withContext(Dispatchers.IO), preserving the old 2-thread rate limit explicitly via a Semaphore so route-poll backfills can't fan out into dozens of simultaneous API requests. - Move fetchTripDetailsOnce from Pollers.kt to TripFetcher — it's demand-driven, not a poller; drop the now-unneeded @file:JvmName. - Add doc headers on both files stating the volatile-vs-immutable contract.
…dedup Redesign the demand-driven fetch side of the trip data layer around purity and structured concurrency: - Fetchers.kt (was TripFetcher.kt): two pure top-level suspend functions, fetchTripSchedule(tripId) and fetchShape(shapeId), with no TripStore knowledge. Shape fetches are now keyed by shapeId, so trips sharing a shape share one fetch. Rate limiting is a property of the dispatcher (Dispatchers.IO.limitedParallelism) rather than semaphore logic. - Extract SingleFlight<K, V> (util): generic keyed single-flight so concurrent callers share one in-flight execution and slow networks can't pile up duplicate requests. Replaces the callback-and-pending- set scheme, whose in-flight path silently dropped a second caller's callbacks. - Hydration moves to call sites: Pollers.kt (now the store-hydrating side) gains the route poller's prefetch backfill — cancelled with the poller — and reclaims fetchTripDetailsOnce on a shared one-shot scope; TripMapOverlayFactory does check-fetch-put explicitly. - TripMapOverlayFactory.create() becomes suspend returning overlays or null, replacing onReady/onError callbacks; TripMapFragment awaits it in viewLifecycleOwner.lifecycleScope, so destroying the view cancels activation instead of firing callbacks at a dead map. The data package now splits as: Trip (domain), TripStore (identity and retention), Pollers.kt (hydrating sources), Fetchers.kt (pure sources).
Apply the Fetchers.kt treatment to the store: - Extract ShellRegistry<K, V> (util): the generic permanent-identity / bounded-payload-retention mechanism (one instance per key forever; capacity eviction clears a value's payload, never its identity). TripStore keeps only policy — cap, what clearing means — and domain recording. - Dissolve object TripStore into top-level functions with file-private state behind a @file:JvmName("TripStore") facade; Java callers drop .INSTANCE and held store fields. - Rename for bare-call clarity: getTrip -> lookupTrip (transient-read intent; also avoids shadow confusion with response.getTrip inside forEachActiveTrip), clearAll -> clearAllTrips.
Replace the shared mutable Trip object with TripState, an immutable data class published through a per-trip MutableStateFlow. Consumers hold the permanent StateFlow and read one consistent snapshot per frame/tick (a single volatile read); all writes are pure folds (recorded/withServiceDate/ withRouteType) producing new snapshots, so stale held references are unrepresentable. The three parallel history lists merge into one List<HistoryEntry>; extrapolators bind to a snapshot, making instance identity the fit-cache invalidation signal.
With trip data immutable and nothing collecting the per-trip StateFlows (every consumer polls .value per frame/tick), both the permanent-shell/ evictable-payload machinery and the flow wrapper are vestigial. TripStore becomes a textbook LruCache<String, TripState>; identity is the tripId key, and consumers hold the id and look up the current snapshot per frame/tick. Lookups and writes both promote, so an actively displayed or polled trip is never the eviction victim. The lookup returns the same instance until new data is recorded, preserving the per-snapshot extrapolator fit cache and the anchor reference-equality animation trigger.
Reuse the polled snapshot in TripDetailsListFragment.updateVehiclePosition instead of a second same-key lookup (past the active-trip guard the two keys are equal), and move lastAnimatedAnchor initialization into the VehicleMarkerState constructor so the type owns the field's invariant.
…terface Call sites guarding against non-finite medians/quantiles explained the guard by naming the implementation (bisect on a pathological CDF) — an abstraction leak, since the contract was documented only on the internal bisect() helper. Document the degenerate-case NaN contract on ProbDistribution.quantile()/median() and reword the call-site comments in interface-level terms.
updateDirectionIcon now operates purely at the vehicle/icon level, taking the direction bearing directly (NaN = unavailable) instead of reaching into the polyline. This dissolves the halfWindAt helper and its -1 sentinel, which existed only to mix the two domains.
The function updates more than the position — it also updates the direction icon and animation-anchor bookkeeping, all facets of the vehicle marker. The new name matches the file's existing remove/destroy/selectVehicleMarker idiom.
TripStore had grown response-shape knowledge: three record functions that each knew how to pick trip IDs and folds out of a different OBA response type. Split that seam on a standard object: - Adapters.kt (new): TripObservation — what one response said about one trip's vehicle — plus pure toObservations() extensions on ObaTripDetailsResponse and ObaTripsForRouteResponse. No store access. - TripStore.kt: response-blind. Writes are record(observation) plus dumb setters; the polled-trip writeback is unfused from recording as putTripDetailsResponse, with the adapter side deriving the active trip ID. update() stays private, so all folds remain enumerated here. - TripState.observed(): the one fold that consumes an observation. - Pollers.kt: composition of writeback + observations lives in a shared fetchAndRecordTripDetails, deduplicating the near-identical bodies of TripDetailsPoller and fetchTripDetailsOnce. - TripDetailsListFragment.setTripDetails is display-only: it was re-recording responses it had just read out of the store (a vestige of the old loader feed), churning snapshot identity for no new data. - Drop recordStatus (no production callers) and the two null-guard tests whose cases TripObservation now makes unrepresentable at compile time.
recorded/observed read as predicates or past events rather than
copy-producing folds, and record (store fn) / recorded / observed /
TripObservation made two overlapping verb families. Rename:
- TripState.recorded -> withStatus, observed -> withObservation,
joining the existing withServiceDate/withRouteType participles; the
chain now reads record(obs) -> withObservation -> withStatus.
- update()'s lambda param fold -> transform ("fold" promises reduce
semantics this single-step remapper doesn't have), and the header's
get-fold-put -> get-transform-put.
Doc fixes from the same review: the TripState class KDoc now names
withObservation (the fold the store actually drives) instead of the
stale [recorded], and the TripStore header's "know nothing about API
response shapes" claim carves out putTripDetailsResponse's opaque
cached response.
The details adapter always emitted routeType=null even though the response refs can resolve it (trip -> routeId -> route.type), exactly as the trips-for-route adapter already does. A trip hydrated only by TripDetailsPoller — e.g. opened from arrivals, where no RoutePoller runs — therefore never got a routeType, and grade-separated trips fell through to GammaExtrapolator instead of ScheduleReplayExtrapolator. Degrades safely: if the refs lack the trip or route, withRouteType(null) is the usual first-writer-wins no-op.
The writeback in fetchAndRecordTripDetails was gated on a non-null status, so an OBA_OK response for a schedule-only trip (vehicle not reporting) never populated TripState.tripDetailsResponse — and the trip details list, which renders exclusively from the cached response, showed nothing, even though its header has an explicit status == null "show schedule info only" branch. The map activation gate suffered the same way despite a status-tolerant renderer. Now every OBA_OK response is cached, with vehicleActiveTripId derived as status?.activeTripId. That also means a vehicle that stops reporting resets vehicleActiveTripId to null instead of going stale; both consumers already treat null as "unknown".
catch (e: Exception) also catches kotlinx's CancellationException, so stopping a poller mid-fetch logged a spurious "Failed to fetch" error, and cancellation only worked because the delay() outside the try re-threw it — one refactor away from a poller that spins forever. Rethrow cancellation ahead of the generic catch in both fetchAndRecordTripDetails and RoutePoller's loop.
UI code was still writing to the trip store from two places, both re-recording data derived from the poller's own cached response: TripMapOverlayFactory.cacheResponseData and the schedule/serviceDate writeback in TripDetailsListFragment.updateLocationDataMenuState. The root cause was that fetchAndRecordTripDetails cached the response but never extracted its schedule. - fetchAndRecordTripDetails now records everything the response carries (writeback + schedule + serviceDate + observation), making it the single hydration point for details responses. - The shape fetch-and-record composition moves into the data layer as ensureShape, shared by the route poller's backfill and the trip map's on-demand activation. - Both UI writeback sites are deleted; the TripStore header now states the earned invariant: every production writer lives in the data package, UI code only reads.
- Guard schedule writes with a first-writer-wins TripState.withSchedule: putSchedule now runs on every 10s poll tick with a freshly deserialized schedule, and the unguarded copy() churned snapshot identity — discarding the lazily fitted extrapolator — each tick. Schedule is an immutable resource (Fetchers.kt), so first writer wins, matching withRouteType. - Collapse the duplicated trip->route->type refs-walk in both toObservations() adapters into one ObaResponseWithRefs.routeTypeForTrip. - Drop TripObservation's default args; production adapters always passed both fields, and the defaults invited silently omitting them. - Drop the backfill guard that re-encoded ensureShape's internal already-cached check. - Restate the TripStore header contract with the putTripDetailsResponse exception as part of the rule, then slim the whole header to the things the code can't say: one-way data flow, retention rationale, main-thread-only writes. - Credit in-flight fetch dedup to the fetcher, not ensureShape, in the trip map doc.
The trip store was a set of top-level functions over a file-private LruCache, with a @file:JvmName facade standing in for a real name. Wrap them in `object TripStore` with @JvmStatic members instead: the state and its API now live inside one named declaration. Deliberately minimal: Java call sites are byte-identical, Kotlin call bodies are untouched (object members import by name), and the store remains the single process-wide instance — so clearAllTrips and the test reset stay.
The response adapters' guards lost their regression coverage when the store-level null tests were deleted (the guards moved into toObservations, which nothing exercised). Test the adapters at their own layer, against the mock API fixtures: - details response: field mapping including refs-resolved routeType, plus the null-status and null-activeTripId skip guards (the latter via a new fixture: the HART details response with activeTripId removed from its status) - trips-for-route response: one observation per active trip keyed by the vehicle's active trip, and the no-statuses guard Verified on device: 5/5 pass.
Two review gaps, both "async result lands after the lifecycle moved on" (PR OneBusAway#1554 review, items 4 and 5): - getMapAsync could deliver after onDestroyView, crashing on requireContext(). The map now arrives as a value awaited inside viewLifecycleOwner.lifecycleScope (a suspendCancellableCoroutine bridge over getMapAsync), so destroying the view cancels the await and the configuration path is unreachable for a dead fragment. The OnMapReadyCallback implementation is gone. - onOverlaysReady could start the frame loop and TripDetailsPoller while paused (overlay fetch resolving after backgrounding), leaving a 10s poll cadence running until the user came back. The moving parts now start only when resumed; otherwise onResume starts them on return. Cleanups that fell out: the start sequence is one startMovingParts() shared by both gates, and two fields are gone — activated (always equal to extrapolationController != null) and map (written once, read once, immediately) — with the GoogleMap flowing as a parameter instead.
Pure formatting: blank lines between endpoint groups, the no_active_trip trip-details entry moved from the tail of the map to sit with its siblings, and a trailing newline at EOF. Key set and values are unchanged (verified programmatically, 73 entries).
The frame loop now reconciles against two live inputs - marker count and host visibility (resumed and not hidden) - instead of running until the activity is destroyed. BaseMapFragment answers visibility through the new Controller.isShown() and nudges the overlay on resume/hidden-change, so animation stops while the map is off screen and resumes with it.
PR OneBusAway#1554 review item 7 worried that the extrapolation strategy was locked in for a trip's lifetime when routeType arrived after the first extrapolate() call. The TripState refactor resolved that by design — the extrapolator is a lazy body property per immutable snapshot, so withRouteType's copy() re-derives the strategy — but nothing tested it. Three tests pin the behavior: - the regression itself: history + schedule without routeType extrapolates via gamma; the same data after withRouteType(SUBWAY) goes through schedule replay - a negative control: TYPE_BUS keeps the gamma strategy, so the flip above is the routeType, not the copy - first-writer-wins: neither a different value nor null overwrites an existing routeType (assertSame, no snapshot churn) The strategies are told apart from the outside via an asymmetry in their inputs: gamma needs the status's scheduledDistanceAlongTrip to resolve a speed even when a schedule is present (the test status omits it → MissingSchedule), while schedule replay needs only the schedule itself (→ Success). Schedule fixture built by reflection, mirroring ScheduleReplayExtrapolatorTest.
Both frame loops caught RuntimeException around extrapolation, wide enough to swallow NPEs, Maps-API state errors, and iteration bugs as 20Hz warn-spam — invisible in production (PR OneBusAway#1554 review item 8). The comments always described the narrow intent: require() failures in the gamma model on a degenerate schedule, which throw exactly IllegalArgumentException. Catch that, keep the graceful per-frame fallback (raw position on the route map, skipped frame on the trip map), and let everything else propagate loudly.
The request layer never throws — network and parse failures come back as error-coded responses — and the pollers recorded only OK responses into the trip store, so a failing server was perfectly silent: no log, no store change, and TripDetailsListFragment's spinner never resolved (PR OneBusAway#1554 review item 9). - TripDetailsPoller gains an optional ResponseCallback (mirroring RoutePoller) that delivers every completed response, errors included. The trip details list renders failures with the ArrivalsListFragment pattern: network-aware error text (UIUtils.getStopErrorString) when no data has loaded yet, a generic_comm_error_toast when cached data is already showing. Error responses still never enter the store, whose consumers assume usability. - Both poll loops back off exponentially on consecutive failures (10s doubling to an 80s cap, reset on success) via a pure nextPollDelayMs helper, unit-tested in PollersTest. Failed polls now log at W. - Callback delivery moved outside the fetch try/catch in both pollers, so a bug in UI callback code propagates loudly instead of being swallowed every tick (the item-8 convention). RoutePoller stays OK-only delivery — its consumer renders vehicles, not errors. - fetchTripDetailsOnce and its never-cancelled one-shot scope are gone: the refresh menu now delegates to the fragment, which restarts its poller — an immediate fetch whose result flows through the same callback, with backoff reset. Refresh failures are finally visible.
Six findings from the post-commit review of b221dfd: - Toast once per failure streak instead of on every failed poll: with backoff, the old shape nagged a flaky connection every 20-80s indefinitely. mPollFailureNotified resets on success, on explicit refresh, and on resume — the moments a user deserves fresh feedback. - Flush pending fragment transactions before the refresh menu's findFragmentByTag lookup, closing the tiny window where a tap during the map/list swap was silently dropped. - Extract fetchAndRecordTripsForRoute as the trips-for-route sibling of fetchAndRecordTripDetails: the two poll loops are now symmetric, and the hoisted nullable var and !! assertion are gone. - Extract applyTripDetails, shared by the poller callback and the position tick, and document why the tick must keep reading the store: other screens' pollers (trip map, location data view) hydrate it for the same trip without a callback here. - Correct the late-delivery comment: stop()'s cancellation provably prevents post-stop delivery; the lifecycle guard is defensive, not a fix for a real race.
…, 15, 17, 18 - AffineTransformDistribution rejects negative scale (require >= 0) and documents that zero scale is a point mass at the offset — dt = 0 is a legitimate query at exactly the anchor time, so it stays accepted; only the flipped-distribution regime is unrepresentable now. New test pins the rejection. (item 10) - Polyline defensively copies its point list so a caller mutating its list can't desync points from the precomputed cumulative distances. The reviewer's companion require(size >= 2) is deliberately omitted: the degenerate empty/single-point tolerance is tested, documented behavior (InterpolateAlongPolylineTest). (item 18) - Polyline's KDoc no longer overstates subPolyline as O(log n); it is O(log n + k) in the vertices returned. (item 15) - Schedule and shape fetches log at W when they resolve to null — error-coded responses never throw, so these failures were previously invisible above the request layer's internals. (item 17) - SpeedEstimatorTest renamed to TripStoreTest with an accurate KDoc; no SpeedEstimator class ever existed and the tests exercise TripStore. Its ineffective defensive-copy test (cleared a copy, asserted nothing) is replaced by a snapshot-isolation test, the property that actually holds in the immutable TripState design. (item 14 + CodeRabbit inline)
All six activation failure modes collapsed into one generic toast at the TripMapCallback boundary, even though the factory already enumerated and logged them (PR OneBusAway#1554 review item 13). A network blip during the shape fetch read identically to a trip whose data can never support the map. The factory now returns a sealed TripMapOverlayResult instead of flattening to null, each TripMapOverlayFailure carries a retryable flag (only SHAPE_FETCH_FAILED qualifies), and the fragment's own no-cached-details case counts as retryable — a timing problem, not a data problem. onTripMapActivationFailed(retryable) lets the activity toast "try again and it might work" (existing generic_comm_error string) for transient failures and keep the permanent wording for the rest; re-toggling the map retries the fetch. No new strings.
- Trip map: a frame whose median interpolation fails now hides the vehicle marker and estimate overlays instead of leaving a stale position, matching the non-finite-median branch. - TripVehicleOverlay creates the estimate overlay even when the initial vehicle position is null (trip not started yet) — previously every later updateEstimateOverlays call was a permanent no-op. Anchored at the shape start until the first frame repositions or hides it. - The data-received marker's age label now tracks the clock between fixes, gated on the elapsed seconds (a long compare) so the label string is built at most once per second despite the 20Hz frame loop. - Vehicle info window clamps elapsed time at zero against client/server clock drift. - VehicleMapController handles GoogleMap.addMarker()'s @nullable return instead of registering a marker-less state. - FrozenDistribution.quantile(p <= 0) returns the table's first entry (the source's support minimum) instead of literal 0.0, which was wrong for shifted sources like AffineTransformDistribution; test added with a shifted source to make the difference observable. - TripDetailsActivity's onActivityResult fragment recovery uses replace() — the container may hold the map fragment. - The onResume cached-data path goes through applyTripDetails, so the activity's TripDataCallback also learns about cached loads instead of waiting up to a poll interval for the map toggle to enable. - setUpHeader updates the location-data menu state before its early returns (no status / inactive trip / schedule-only), where it could previously go stale; updateLocationDataMenuState is now null-safe for the schedule-only path, treating a missing status as "no active vehicle" via its existing clear-and-notify branch. - The MapLibre trip map stub's placeholder text moves to a string resource (trip_map_not_supported). - TripStateTest's "exactly max horizon" test now tests 900_000 ms, the actual boundary: the stale check is strict, so the boundary query is allowed through to the extrapolator. Not changed: MapLibre's selectVehicle() already logs its not-implemented state, and gating the affordance per-flavor isn't worth it for a stub; the zero-vehicle frame loop and the ineffective defensive-copy test were already fixed in earlier commits.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMapController.java (1)
208-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nullable marker creation in
showDataReceivedMarker().
mMap.addMarker(...)can return null, andm.setTag(vehicle)will crash in that case.Suggested fix
Marker m = mMap.addMarker(new MarkerOptions() .position(MapHelpV2.makeLatLng(loc)) .icon(getOrCreateDataReceivedIcon()) .title(mContext.getString(R.string.marker_most_recent_data)) .anchor(0.5f, 0.5f) .flat(true) .zIndex(DATA_RECEIVED_MARKER_Z_INDEX)); + if (m == null) { + Log.w(TAG, "Failed to add data-received marker for trip " + vehicle.tripId); + return; + } vehicle.dataReceivedMarker = m; vehicle.dataReceivedFixTime = anchor.getLastUpdateTime(); m.setTag(vehicle);For com.google.android.gms:play-services-maps 19.0.0, is GoogleMap.addMarker(MarkerOptions) annotated `@Nullable`, and when can it return null?🤖 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/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMapController.java` around lines 208 - 218, In the showDataReceivedMarker() method, the call to mMap.addMarker() can return null, but the code immediately calls m.setTag(vehicle) without null checking, which will crash if the marker creation fails. Add a null check on the returned Marker m after the mMap.addMarker() call and only execute the subsequent assignments (vehicle.dataReceivedMarker, vehicle.dataReceivedFixTime, and m.setTag(vehicle)) if m is not null.
🧹 Nitpick comments (2)
onebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/AdaptersTest.kt (1)
66-73: Region isolation acrossAdaptersTestmethods already looks safe.
detailsResponseWithoutStatusYieldsNoObservations()doesn’t set a region, but each test runs underObaTestCase/ObaMock, which snapshots and restoresApplication’scurrentRegion/custom API URL in@Before/@After. Also,ObaTestCase.before()resets the default Puget SoundcustomApiUrlfor every test invocation, so execution order shouldn’t matter even though other tests inAdaptersTest.ktcallApplication.get().setCurrentRegion(...). Optional: set the intended region/custom API explicitly in this test for clarity.🤖 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/extrapolation/test/AdaptersTest.kt` around lines 66 - 73, The test detailsResponseWithoutStatusYieldsNoObservations in AdaptersTest.kt relies on implicit region state; to make it explicit and robust, set the intended region/custom API before building the ObaTripDetailsRequest (for example call Application.get().setCurrentRegion(...) or reset the customApiUrl to the Puget Sound fixture) and/or restore it after the test; update the test to explicitly configure the region/API used by ObaTripDetailsRequest.Builder(getTargetContext(), "1_18196913") so the request uses the expected fixture regardless of external test ordering.onebusaway-android/src/test/java/org/onebusaway/android/extrapolation/data/TripStateTest.kt (1)
572-591: 💤 Low valueNon-portable
sun.misc.Unsafeusage in test fixture.The fallback to
sun.misc.Unsafefor creating aLocationinstance works but is not portable across all JVM implementations (e.g., may not exist on GraalVM native-image or future JDK versions). Since this is test-only code and the primaryLocation("test")constructor should work on standard CI environments with android.jar stubs, this is acceptable but worth noting.The swallowed exception (flagged by detekt) is intentional here since the stub throwing is expected behavior, and the comment explains the rationale adequately.
🤖 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/test/java/org/onebusaway/android/extrapolation/data/TripStateTest.kt` around lines 572 - 591, The fallback uses non-portable sun.misc.Unsafe to create FAKE_LOCATION which may fail on some JVMs; replace this with a JVM-portable test double instead (e.g., create a Mockito/MockK mock or Robolectric Location stub) so the companion object’s FAKE_LOCATION initialization does not rely on sun.misc.Unsafe or swallowed RuntimeException; update the companion object (FAKE_LOCATION) to return the mock/stub Location instance and remove the Unsafe reflection block.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@onebusaway-android/src/androidTest/res/raw/urimap.json`:
- Line 76: The new trip-details fixture maps only the
"/api/where/trip-details/Hillsborough%20Area%20Regional%20Transit_1389962_no_active_trip.json"
key to "trip_details_hart_1389962_no_active_trip" but is missing the duplicate
alias for the "/api/api/where/..." route; add an entry mapping
"/api/api/where/trip-details/Hillsborough%20Area%20Regional%20Transit_1389962_no_active_trip.json"
to the same "trip_details_hart_1389962_no_active_trip" fixture (matching the
adjacent HART mapping pattern) so callers hitting /api/api/... will use the same
fixture.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripExtrapolationController.kt`:
- Around line 44-55: The early returns after checking lookupTripState(tripId)
and state.polyline and inside the catch for state.extrapolate(now) leave stale
overlays visible; before each return, call the routine that clears/hides the
trip overlays (e.g. hideOverlaysForTrip(tripId) or
removeTripOverlay/tripOverlayManager.hide for this trip) and ensure that such a
method exists and is safe to call when state is null; also call it in the catch
block (alongside logging via TAG) so any failed extrapolation also clears UI
before returning.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripState.kt`:
- Around line 110-115: The method TripState.withStatus currently returns early
when serverTimeMs <= 0 despite KDoc claiming 0 should use the local clock;
change the implementation (in TripState.withStatus) to remove the early return
for serverTimeMs and instead compute an effectiveServerTime (use serverTimeMs if
> 0, otherwise use localTimeMs or System.currentTimeMillis() as the local-clock
fallback), then proceed with the existing status update logic (still bail only
if status.distanceAlongTrip == null) and update the KDoc for serverTimeMs to
document this fallback behavior.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/util/SingleFlight.kt`:
- Around line 45-49: The catch in SingleFlight (where block() is invoked)
currently catches all Exceptions and turns cancellations into a null result;
update the error handling to preserve coroutine cancellation by rethrowing
CancellationException instead of swallowing it. Concretely, in the code that
calls block() (inside the SingleFlight flow/deferred creation for the given
key), detect CancellationException (either via an explicit catch for
CancellationException or by checking `if (e is CancellationException) throw e`)
and rethrow it, then handle/log other Exceptions as before so only
non-cancellation errors are converted to null and logged.
---
Duplicate comments:
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMapController.java`:
- Around line 208-218: In the showDataReceivedMarker() method, the call to
mMap.addMarker() can return null, but the code immediately calls
m.setTag(vehicle) without null checking, which will crash if the marker creation
fails. Add a null check on the returned Marker m after the mMap.addMarker() call
and only execute the subsequent assignments (vehicle.dataReceivedMarker,
vehicle.dataReceivedFixTime, and m.setTag(vehicle)) if m is not null.
---
Nitpick comments:
In
`@onebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/AdaptersTest.kt`:
- Around line 66-73: The test detailsResponseWithoutStatusYieldsNoObservations
in AdaptersTest.kt relies on implicit region state; to make it explicit and
robust, set the intended region/custom API before building the
ObaTripDetailsRequest (for example call Application.get().setCurrentRegion(...)
or reset the customApiUrl to the Puget Sound fixture) and/or restore it after
the test; update the test to explicitly configure the region/API used by
ObaTripDetailsRequest.Builder(getTargetContext(), "1_18196913") so the request
uses the expected fixture regardless of external test ordering.
In
`@onebusaway-android/src/test/java/org/onebusaway/android/extrapolation/data/TripStateTest.kt`:
- Around line 572-591: The fallback uses non-portable sun.misc.Unsafe to create
FAKE_LOCATION which may fail on some JVMs; replace this with a JVM-portable test
double instead (e.g., create a Mockito/MockK mock or Robolectric Location stub)
so the companion object’s FAKE_LOCATION initialization does not rely on
sun.misc.Unsafe or swallowed RuntimeException; update the companion object
(FAKE_LOCATION) to return the mock/stub Location instance and remove the Unsafe
reflection block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bace521c-f211-4252-ad91-5c0e372082bb
📒 Files selected for processing (41)
.gitignoreonebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/AdaptersTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/TripStoreTest.ktonebusaway-android/src/androidTest/res/raw/trip_details_hart_1389962_no_active_trip.jsononebusaway-android/src/androidTest/res/raw/urimap.jsononebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleInfoWindowAdapter.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMapController.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleMarkerState.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleOverlay.javaonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/DistanceEstimateOverlay.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripExtrapolationController.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripMapFragment.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripMapRendererFactory.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripRouteOverlay.ktonebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripVehicleOverlay.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/Extrapolator.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/GammaExtrapolator.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/ScheduleReplayExtrapolator.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Adapters.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Fetchers.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Pollers.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripState.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripStore.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/AffineTransformDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/FrozenDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/ProbDistribution.ktonebusaway-android/src/main/java/org/onebusaway/android/map/RouteMapController.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripDetailsActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripDetailsListFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/TripMapCallback.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/TrajectoryGraphView.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/VehicleLocationDataActivity.ktonebusaway-android/src/main/java/org/onebusaway/android/util/Polyline.ktonebusaway-android/src/main/java/org/onebusaway/android/util/SingleFlight.ktonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/tripmap/TripMapFragment.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/data/PollersTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/data/TripStateTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/AffineTransformDistributionTest.ktonebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/FrozenDistributionTest.kt
🚧 Files skipped from review as they are similar to previous changes (22)
- .gitignore
- onebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/tripmap/TripMapFragment.kt
- onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/Extrapolator.kt
- onebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/AffineTransformDistributionTest.kt
- onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/FrozenDistribution.kt
- onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleInfoWindowAdapter.java
- onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/AffineTransformDistribution.kt
- onebusaway-android/src/test/java/org/onebusaway/android/extrapolation/math/prob/FrozenDistributionTest.kt
- onebusaway-android/src/main/res/values/strings.xml
- onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/ProbDistribution.kt
- onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/GammaExtrapolator.kt
- onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripRouteOverlay.kt
- onebusaway-android/src/main/java/org/onebusaway/android/util/Polyline.kt
- onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.java
- onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/ScheduleReplayExtrapolator.kt
- onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/VehicleOverlay.java
- onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripVehicleOverlay.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/TripDetailsActivity.java
- onebusaway-android/src/main/java/org/onebusaway/android/map/RouteMapController.java
- onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/DistanceEstimateOverlay.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/VehicleLocationDataActivity.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/TrajectoryGraphView.kt
- urimap.json: add the /api/api alias for the no_active_trip fixture, matching its sibling HART trip-details entries (the Tampa region's base URL produces the doubled path). - TripExtrapolationController: every failed-frame exit now hides the vehicle marker and estimate overlays — the state-missing/shape-missing early returns and the degenerate-schedule catch previously left a prior frame's marker on screen, unlike the other failure paths. The thrice-duplicated hide pair is now one hideFrameOverlays(), and the non-finite-median and null-interpolation branches collapse into a single failed-frame branch. - TripState.withStatus KDoc: the "0 to use local clock" fallback never existed — entries without a server time are skipped (and tested as such); the doc now says so. - SingleFlight rethrows CancellationException instead of masking a cancelled scope as a logged null result, matching the convention everywhere else on this branch.
|
That's amazing !, can you send me apk to test that out ? ( currently my machine have problem with android studio ) |
|
Sure! Here's a debug APK built from this branch (commit 0bb532f): 📦 onebusaway-animated-markers-debug.apk (25 MB) (Release page: https://github.com/bmander/onebusaway-android/releases/tag/animated-markers-test-1) A couple of install notes:
To see the animated markers in action, open a map with active vehicles (or tap a vehicle → trip details) and watch the markers move between AVL updates. Would love to hear how it feels on a real device! |
aaronbrethorst
left a comment
There was a problem hiding this comment.
This looks terrific. Amazing work, @bmander
Summary
Adds real-time vehicle marker animation on the map between AVL data updates, using a probabilistic speed model to extrapolate position. Vehicle markers now move smoothly along the route polyline at their estimated speed, rather than teleporting when new data arrives every ~15s. Also adds a new trip details map view that visualizes the full probability distribution over the vehicle's predicted location.
How to review
This PR is organized into 8 logical commits. Each commit builds on the previous and is independently meaningful. To review this efficiently, step through the commits in order via the "Commits" tab:
distanceAlongTrip,lastKnownDistanceAlongTrip) and adds helpers (haversine distance matching the OBA server formula, segment lookups, elapsed-time formatting).BaseMapFragmentinto a reusable factory.ProbDistributioninterface with Gamma, Dirac, frozen, and mixture implementations;Polylineclass for O(log n) distance-based interpolation. JVM-testable.Extrapolatorthat picks the appropriate strategy (real-time or schedule replay) based on available data.RoutePoller/TripPollerreplacing the old Loader/Handler mechanism, and a Choreographer wrapper for throttled frame animation.VehicleOverlayintoVehicleMapController,VehicleMarkerState,VehicleIconFactory, andVehicleInfoWindowAdapter. Wires per-frame extrapolation into the route map.TripMapFragmentshowing a single trip's route, animated vehicle, and a colored polyline segment visualizing the probability distribution over vehicle location.TripDetailsActivitymap/list toggle, developer data views (trajectory graph + table), and supporting layouts/strings/styles.Test plan
./gradlew test— JVM unit tests pass (distributions, haversine, trip gating, schedule replay, gamma model)./gradlew assembleObaGoogleDebug— compiles clean./gradlew installObaGoogleDebug— installs and launches on device./gradlew assembleObaMaplibreDebug)Notes
Implements #1555
Summary by CodeRabbit
New Features
Improvements