Bump Jackson 2.12.4 → 2.13.5 (highest version allowed by the minSdk 21 ceiling)#1568
Open
bmander wants to merge 43 commits into
Open
Bump Jackson 2.12.4 → 2.13.5 (highest version allowed by the minSdk 21 ceiling)#1568bmander wants to merge 43 commits into
bmander wants to merge 43 commits into
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.
…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.
- 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.
2.13.5 is the hard ceiling while minSdk stays at 21: per Jackson's release wiki, 2.13 is the last branch supporting Android SDK 21+. Jackson 2.14+ requires SDK 26 and crashes on API 21-25 devices at ObjectMapper init (NoClassDefFoundError: java.lang.BootstrapMethodError; see square/retrofit#4131), and core library desugaring does not help since java.lang.invoke is not in desugar_jdk_libs. The bump still captures the known databind CVE fixes missing from 2.12.4 (CVE-2020-36518, CVE-2022-42003/42004) and uniformly lifts the stale transitive Jackson requests (2.5.3/2.7.3/2.9.x) to 2.13.5. Also replaces APIs deprecated within Jackson 2.x: - MappingJsonFactory.createJsonGenerator() -> createGenerator() - ObjectMapper.setVisibilityChecker() -> setVisibility() - ObjectMapper.reader(Class) -> readerFor(Class)
📝 WalkthroughWalkthroughAdds trip extrapolation math and data flow, polling/fetching, an in-memory trip store, Google Maps trip map with vehicle overlays/controllers, UI activities/fragments, utilities/resources, and extensive unit/instrumentation tests. Updates build config/dependencies and minor app wiring. ChangesTrip map rendering and extrapolation pipeline
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripMapFragment.kt (1)
171-180: ⚡ Quick winConsider exception handling for overlay creation coroutine.
If
TripMapOverlayFactory.createorensureShapewithin it throws an unexpected exception (e.g., network timeout wrapping), the coroutine will crash silently. Consider wrapping with a try-catch or usingCoroutineExceptionHandlerto gracefully handle failures and notify viamapCallback.♻️ Suggested improvement
viewLifecycleOwner.lifecycleScope.launch { - val result = - TripMapOverlayFactory.create( - map, requireContext(), tripId, selectedStopId, response) + val result = try { + TripMapOverlayFactory.create( + map, requireContext(), tripId, selectedStopId, response) + } catch (e: Exception) { + Log.e(TAG, "Overlay creation threw unexpected exception", e) + mapCallback?.onTripMapActivationFailed(true) + return@launch + } when (result) {🤖 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/tripmap/TripMapFragment.kt` around lines 171 - 180, Wrap the launch block that calls TripMapOverlayFactory.create in a try-catch (or install a CoroutineExceptionHandler on viewLifecycleOwner.lifecycleScope) to catch any unexpected exceptions from TripMapOverlayFactory.create (and its internal ensureShape) and forward failures to mapCallback; specifically, catch Throwable and call mapCallback?.onTripMapActivationFailed(...) with an appropriate non-retryable flag or translated reason if result creation throws instead of returning TripMapOverlayResult.Failed, while preserving the existing onOverlaysReady(result.overlays) handling for the Ready case.onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripStore.kt (1)
33-52: ⚡ Quick winEnforce the main-thread-only contract in code, not just comments.
Line 33 states main-thread-only access, but
lookupTripState()/update()do not enforce it. If any off-main caller slips in, the get-transform-put path can lose updates.💡 Proposed guard
+import android.os.Looper @@ object TripStore { @@ + private fun checkMainThread() { + check(Looper.myLooper() == Looper.getMainLooper()) { + "TripStore must be accessed on the main thread" + } + } + `@JvmStatic` - fun lookupTripState(tripId: String?): TripState? = tripId?.let { trips.get(it) } + fun lookupTripState(tripId: String?): TripState? { + checkMainThread() + return tripId?.let { trips.get(it) } + } @@ private inline fun update(tripId: String, transform: (TripState) -> TripState) { + checkMainThread() trips.put(tripId, transform(trips.get(tripId) ?: TripState.empty(tripId))) }🤖 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/TripStore.kt` around lines 33 - 52, TripStore currently documents "main thread only" but does not enforce it; add a runtime guard at the entry points to prevent off-main-thread access: in lookupTripState(tripId: String?) and update(tripId: String, transform: (TripState) -> TripState) (and any other public methods that touch trips/LruCache) assert the current thread is the main thread (e.g., require(Looper.getMainLooper().isCurrentThread) or similar) or annotate with `@MainThread` plus a runtime check to throw early if violated; this ensures the get-transform-put path on trips (the LruCache) cannot be accessed from background threads and prevents lost updates.onebusaway-android/src/main/java/org/onebusaway/android/util/UIUtils.java (1)
2168-2178: ⚡ Quick winUse localized resources/plurals for elapsed-time text.
Hardcoded
"sec/min ago"strings won’t localize correctly and miss singular/plural handling.🤖 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/util/UIUtils.java` around lines 2168 - 2178, The current formatElapsedTime(long elapsedMs) in UIUtils uses hardcoded "sec/min ago" strings which break localization and plural handling; change the method to accept a Context (e.g., formatElapsedTime(Context ctx, long elapsedMs)) and replace concatenated literals with calls to Resources.getQuantityString(...) for seconds and minutes (and a localized "ago" or include "ago" in the plural strings) to produce properly localized singular/plural forms; update callers of UIUtils.formatElapsedTime to pass a Context and ensure the plural string resources (e.g., plural_elapsed_seconds, plural_elapsed_minutes, or combined minute/second variants) are added to values/strings.xml.onebusaway-android/src/main/res/layout/route_debug_list_item.xml (1)
24-29: ⚡ Quick winAdd
android:paddingLeftfor consistency with existing RTL support pattern.The codebase consistently pairs
paddingStartwithpaddingLeftfor RTL support (seetrip_details_head.xmllines 28-29, 41-42). Line 29 should include both attributes.♻️ Proposed fix to maintain RTL support pattern
<TextView android:id="@+id/debug_vehicle_id" android:layout_width="wrap_content" android:layout_height="wrap_content" android:textSize="13sp" + android:paddingLeft="8dp" android:paddingStart="8dp" />🤖 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/res/layout/route_debug_list_item.xml` around lines 24 - 29, The TextView with id debug_vehicle_id in route_debug_list_item.xml uses android:paddingStart but lacks android:paddingLeft; add android:paddingLeft="8dp" alongside android:paddingStart to follow the project's RTL attribute pairing (consistent with patterns like trip_details_head.xml) so old platforms still get the intended left padding.
🤖 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/google/java/org/onebusaway/android/map/googlemapsv2/BaseMapFragment.java`:
- Around line 1039-1041: The current call to mLineOverlay.clear() only drops
references and leaves existing Polyline objects rendered on the map; update the
clear branch to first iterate the stored polylines/overlays (the collection held
by mLineOverlay), call remove() (or equivalent map removal) on each Polyline
instance to remove them from the GoogleMap, and only then call
mLineOverlay.clear() (or clear the collection) so both the map and the overlay
container are cleaned; look for mLineOverlay.clear() in BaseMapFragment and
remove each polyline via its remove() method before clearing the collection.
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripVehicleOverlay.kt`:
- Around line 175-179: The elapsed time calculation uses (now - updateTime)
directly which can be negative if device clock lags the server; clamp the
elapsed milliseconds to >= 0 before computing ageSec and before passing to
UIUtils.formatElapsedTime. Replace uses of (now - updateTime) in the
TripVehicleOverlay logic (the block that updates ageSec/lastAgeLabelSec,
existing.snippet and related block around lines with ageSec and the similar
block at 185-187) by first doing elapsedMs = max(0, now - updateTime), then
compute ageSec = elapsedMs / 1000 and pass elapsedMs to
UIUtils.formatElapsedTime; keep dataReceivedInfoShown and
existing.showInfoWindow() behavior unchanged.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/Pollers.kt`:
- Around line 81-85: The local receipt timestamp is sampled before the network
call which biases staleness calculations; update the code so localTimeMs is
captured immediately after the call() returns. Specifically, in the block that
invokes ObaTripDetailsRequest.newRequest(ctx, tripId).call() (and the analogous
second occurrence around the 202–209 region), move the
System.currentTimeMillis() call to the line immediately following the call()
result and use that post-call localTimeMs for all subsequent
staleness/extrapolation calculations.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/math/prob/ProbDistribution.kt`:
- Around line 65-67: The bisect routine's bracket/refine loops compare f(x) to
target without guarding against non-finite results, so NaN from f can make
comparisons behave incorrectly and produce a finite quantile; update the logic
in bisect to explicitly check Double.isFinite(f(lo)), Double.isFinite(f(hi)) and
Double.isFinite(f(mid)) (or !f(...).isFinite()) wherever f(...) is used in the
while loops and return Double.NaN immediately if any of those evaluations are
non-finite (also when incrementing iter and checking BRACKET_MAX_ITER), ensuring
the bracket expansion (hi *= 2) and the refine loop both bail out with
Double.NaN on non-finite CDF values rather than continuing comparisons that can
yield wrong finite results.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/io/elements/ObaTripSchedule.java`:
- Around line 191-213: In findSegmentStartIndex, add an early validation that
rejects non-finite distanceAlongTrip (NaN or infinite) before any range checks:
use Double.isFinite(distanceAlongTrip) (or check Double.isNaN/Double.isInfinite)
and throw an IllegalArgumentException (or IndexOutOfBoundsException per project
convention) with a clear message like "Non-finite distanceAlongTrip" so
NaN/infinities cannot bypass the range checks and return the last segment index
incorrectly; place this check at the top of the method before inspecting
stopTimes and the first/last distance comparisons.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/TrajectoryGraphView.kt`:
- Around line 62-67: TrajectoryGraphView currently sets currentTime =
System.currentTimeMillis() which is in device-local clock and causes the "now"
line to be offset from server-domain times; instead compute a server-domain
currentTime by applying the server/local offset from a HistoryEntry (use
HistoryEntry.serverTimeMs and HistoryEntry.localTimeMs to derive offset =
serverTimeMs - localTimeMs) and update TrajectoryGraphView.currentTime by adding
that offset to System.currentTimeMillis() (or cache the offset when history is
loaded/updated and recalc currentTime whenever history changes). Ensure all
places that read currentTime (e.g., drawing/deviation calculations) use this
translated server-domain value so the red "now" line aligns with server times.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/dataview/VehicleLocationDataActivity.kt`:
- Around line 72-79: The start helper in VehicleLocationDataActivity currently
always starts an activity with the provided Context, which can crash if the
Context is not an Activity; update VehicleLocationDataActivity.start(...) to
detect if context is an instance of android.app.Activity and only omit
Intent.FLAG_ACTIVITY_NEW_TASK in that case, otherwise add
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) before calling
context.startActivity(...); keep the same Intent extras (EXTRA_TRIP_ID,
EXTRA_VEHICLE_ID, EXTRA_STOP_ID) and behavior but ensure the flag is applied
when needed to avoid launch-time crashes from non-Activity contexts.
In `@onebusaway-android/src/main/java/org/onebusaway/android/util/Polyline.kt`:
- Around line 28-42: The public points property is only a shallow list copy so
callers can still mutate shared Location instances and break interpolation;
change the initialization to produce a deep copy of each Location (e.g., build
points using points.map { Location(it) }.toList() or equivalent copy-constructor
per Location) and use that copied list for cumulativeDistances computation so
geometry and cached distances remain consistent (referencing the points property
and cumulativeDistances in Polyline.kt).
In
`@onebusaway-android/src/maplibre/java/org/onebusaway/android/map/maplibre/tripmap/TripMapFragment.kt`:
- Line 56: The padding call in TripMapFragment.kt uses raw pixels
(setPadding(32, 32, 32, 32)), which will scale poorly across densities; change
it to density-independent pixels by converting 32 dp to pixels before calling
setPadding (e.g., use the view/context resources density or
TypedValue/requireContext().resources.getDimensionPixelSize with a dimen
resource) so the placeholder view’s spacing is consistent; update the call site
where setPadding is invoked in TripMapFragment (the placeholder view setup) to
use the dp->px conversion or reference a dimen resource instead of the hardcoded
32.
---
Nitpick comments:
In
`@onebusaway-android/src/google/java/org/onebusaway/android/map/googlemapsv2/tripmap/TripMapFragment.kt`:
- Around line 171-180: Wrap the launch block that calls
TripMapOverlayFactory.create in a try-catch (or install a
CoroutineExceptionHandler on viewLifecycleOwner.lifecycleScope) to catch any
unexpected exceptions from TripMapOverlayFactory.create (and its internal
ensureShape) and forward failures to mapCallback; specifically, catch Throwable
and call mapCallback?.onTripMapActivationFailed(...) with an appropriate
non-retryable flag or translated reason if result creation throws instead of
returning TripMapOverlayResult.Failed, while preserving the existing
onOverlaysReady(result.overlays) handling for the Ready case.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/extrapolation/data/TripStore.kt`:
- Around line 33-52: TripStore currently documents "main thread only" but does
not enforce it; add a runtime guard at the entry points to prevent
off-main-thread access: in lookupTripState(tripId: String?) and update(tripId:
String, transform: (TripState) -> TripState) (and any other public methods that
touch trips/LruCache) assert the current thread is the main thread (e.g.,
require(Looper.getMainLooper().isCurrentThread) or similar) or annotate with
`@MainThread` plus a runtime check to throw early if violated; this ensures the
get-transform-put path on trips (the LruCache) cannot be accessed from
background threads and prevents lost updates.
In `@onebusaway-android/src/main/java/org/onebusaway/android/util/UIUtils.java`:
- Around line 2168-2178: The current formatElapsedTime(long elapsedMs) in
UIUtils uses hardcoded "sec/min ago" strings which break localization and plural
handling; change the method to accept a Context (e.g., formatElapsedTime(Context
ctx, long elapsedMs)) and replace concatenated literals with calls to
Resources.getQuantityString(...) for seconds and minutes (and a localized "ago"
or include "ago" in the plural strings) to produce properly localized
singular/plural forms; update callers of UIUtils.formatElapsedTime to pass a
Context and ensure the plural string resources (e.g., plural_elapsed_seconds,
plural_elapsed_minutes, or combined minute/second variants) are added to
values/strings.xml.
In `@onebusaway-android/src/main/res/layout/route_debug_list_item.xml`:
- Around line 24-29: The TextView with id debug_vehicle_id in
route_debug_list_item.xml uses android:paddingStart but lacks
android:paddingLeft; add android:paddingLeft="8dp" alongside
android:paddingStart to follow the project's RTL attribute pairing (consistent
with patterns like trip_details_head.xml) so old platforms still get the
intended left padding.
🪄 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: 7673c64a-da66-479e-8f27-56688b6d5861
📒 Files selected for processing (93)
.gitignoreonebusaway-android/build.gradleonebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/AdaptersTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/extrapolation/test/InterpolateAlongPolylineTest.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/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/directions/util/JacksonConfig.javaonebusaway-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/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/JacksonSerializer.javaonebusaway-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/SingleFlight.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/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/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
Comment on lines
1039
to
1041
| if (clear) { | ||
| mLineOverlay.clear(); | ||
| } |
There was a problem hiding this comment.
Clear path drops references without removing old polylines from the map.
mLineOverlay.clear() at Line 1040 forgets existing polylines but leaves them rendered, so later cleanup can’t remove them.
Suggested fix
if (mMap != null) {
if (clear) {
- mLineOverlay.clear();
+ removeRouteOverlay();
}
int totalPoints = 0;
for (ObaShape s : shapes) {
totalPoints += addArrowPolyline(s.getPoints(), lineOverlayColor);
}🤖 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 1039 - 1041, The current call to mLineOverlay.clear() only drops
references and leaves existing Polyline objects rendered on the map; update the
clear branch to first iterate the stored polylines/overlays (the collection held
by mLineOverlay), call remove() (or equivalent map removal) on each Polyline
instance to remove them from the GoogleMap, and only then call
mLineOverlay.clear() (or clear the collection) so both the map and the overlay
container are cleaned; look for mLineOverlay.clear() in BaseMapFragment and
remove each polyline via its remove() method before clearing the collection.
Comment on lines
+175
to
+179
| val ageSec = (now - updateTime) / 1000 | ||
| if (ageSec != lastAgeLabelSec) { | ||
| lastAgeLabelSec = ageSec | ||
| existing.snippet = UIUtils.formatElapsedTime(now - updateTime) | ||
| if (dataReceivedInfoShown) existing.showInfoWindow() |
There was a problem hiding this comment.
Clamp negative elapsed time before building age labels.
On Line 175/178 and Line 185/187, now - updateTime can go negative when device time lags server time, which leads to incorrect age text and gating. Clamp elapsed milliseconds to >= 0 before computing ageSec and formatting.
Proposed fix
- val ageSec = (now - updateTime) / 1000
+ val elapsedMs = (now - updateTime).coerceAtLeast(0L)
+ val ageSec = elapsedMs / 1000
if (ageSec != lastAgeLabelSec) {
lastAgeLabelSec = ageSec
- existing.snippet = UIUtils.formatElapsedTime(now - updateTime)
+ existing.snippet = UIUtils.formatElapsedTime(elapsedMs)
if (dataReceivedInfoShown) existing.showInfoWindow()
}
...
- lastAgeLabelSec = if (updateTime > 0) (now - updateTime) / 1000 else -1L
+ lastAgeLabelSec = if (updateTime > 0) ((now - updateTime).coerceAtLeast(0L) / 1000) else -1L
- val label = if (updateTime > 0) UIUtils.formatElapsedTime(now - updateTime) else ""
+ val label =
+ if (updateTime > 0) UIUtils.formatElapsedTime((now - updateTime).coerceAtLeast(0L)) else ""Also applies to: 185-187
🤖 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/tripmap/TripVehicleOverlay.kt`
around lines 175 - 179, The elapsed time calculation uses (now - updateTime)
directly which can be negative if device clock lags the server; clamp the
elapsed milliseconds to >= 0 before computing ageSec and before passing to
UIUtils.formatElapsedTime. Replace uses of (now - updateTime) in the
TripVehicleOverlay logic (the block that updates ageSec/lastAgeLabelSec,
existing.snippet and related block around lines with ageSec and the similar
block at 185-187) by first doing elapsedMs = max(0, now - updateTime), then
compute ageSec = elapsedMs / 1000 and pass elapsedMs to
UIUtils.formatElapsedTime; keep dataReceivedInfoShown and
existing.showInfoWindow() behavior unchanged.
Comment on lines
+81
to
+85
| val localTimeMs = System.currentTimeMillis() | ||
| val response = | ||
| withContext(Dispatchers.IO) { | ||
| ObaTripDetailsRequest.newRequest(ctx, tripId).call() | ||
| } |
There was a problem hiding this comment.
Capture local receipt time after the network call.
Line 81 and Line 202 currently sample localTimeMs before the request executes. That inflates perceived staleness by request latency and biases the server↔local clock mapping used downstream for extrapolation. Capture local time immediately after call() returns.
💡 Proposed fix
private suspend fun fetchAndRecordTripDetails(ctx: Context, tripId: String): ObaTripDetailsResponse? =
try {
- val localTimeMs = System.currentTimeMillis()
val response =
withContext(Dispatchers.IO) {
ObaTripDetailsRequest.newRequest(ctx, tripId).call()
}
+ val localTimeMs = System.currentTimeMillis()
if (response.code == ObaApi.OBA_OK) {
putTripDetailsResponse(tripId, response.status?.activeTripId, response)
putSchedule(tripId, response.schedule)
putServiceDate(tripId, response.status?.serviceDate ?: 0)
response.toObservations().forEach { record(it, localTimeMs) }
@@
private suspend fun CoroutineScope.fetchAndRecordTripsForRoute(
ctx: Context,
routeId: String
): ObaTripsForRouteResponse? =
try {
- val localTimeMs = System.currentTimeMillis()
val response =
withContext(Dispatchers.IO) {
ObaTripsForRouteRequest.Builder(ctx, routeId)
.setIncludeStatus(true)
.build()
.call()
}
+ val localTimeMs = System.currentTimeMillis()
if (response.code == ObaApi.OBA_OK) {
response.toObservations().forEach { record(it, localTimeMs) }
prefetchSchedulesAndShapes(response)Also applies to: 202-209
🤖 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/Pollers.kt`
around lines 81 - 85, The local receipt timestamp is sampled before the network
call which biases staleness calculations; update the code so localTimeMs is
captured immediately after the call() returns. Specifically, in the block that
invokes ObaTripDetailsRequest.newRequest(ctx, tripId).call() (and the analogous
second occurrence around the 202–209 region), move the
System.currentTimeMillis() call to the line immediately following the call()
result and use that post-call localTimeMs for all subsequent
staleness/extrapolation calculations.
Comment on lines
+65
to
+67
| while (f(hi) < target) { | ||
| hi *= 2 | ||
| if (++iter >= BRACKET_MAX_ITER || !hi.isFinite()) return Double.NaN |
There was a problem hiding this comment.
Handle non-finite f(x) results in bisect to avoid false finite quantiles.
On Line 65 and Line 74, NaN < target evaluates to false, so pathological/degenerate CDFs can drive the refine loop toward zero and return a finite value instead of Double.NaN, contradicting the contract documented in Line 32–34 and Line 50–53.
Suggested fix
internal fun bisect(f: (Double) -> Double, target: Double, initialHi: Double): Double {
var hi = if (initialHi > 0 && initialHi.isFinite()) initialHi else 1.0
+ var fHi = f(hi)
+ if (!fHi.isFinite()) return Double.NaN
var iter = 0
- while (f(hi) < target) {
+ while (fHi < target) {
hi *= 2
- if (++iter >= BRACKET_MAX_ITER || !hi.isFinite()) return Double.NaN
+ if (++iter >= BRACKET_MAX_ITER || !hi.isFinite()) return Double.NaN
+ fHi = f(hi)
+ if (!fHi.isFinite()) return Double.NaN
}
var lo = 0.0
iter = 0
while (hi - lo > BISECT_REL_TOL * (hi + lo) && ++iter < REFINE_MAX_ITER) {
val mid = (lo + hi) / 2
- if (f(mid) < target) lo = mid else hi = mid
+ val fMid = f(mid)
+ if (!fMid.isFinite()) return Double.NaN
+ if (fMid < target) lo = mid else hi = mid
}
return (lo + hi) / 2
}Also applies to: 72-75
🤖 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/math/prob/ProbDistribution.kt`
around lines 65 - 67, The bisect routine's bracket/refine loops compare f(x) to
target without guarding against non-finite results, so NaN from f can make
comparisons behave incorrectly and produce a finite quantile; update the logic
in bisect to explicitly check Double.isFinite(f(lo)), Double.isFinite(f(hi)) and
Double.isFinite(f(mid)) (or !f(...).isFinite()) wherever f(...) is used in the
while loops and return Double.NaN immediately if any of those evaluations are
non-finite (also when incrementing iter and checking BRACKET_MAX_ITER), ensuring
the bracket expansion (hi *= 2) and the refine loop both bail out with
Double.NaN on non-finite CDF values rather than continuing comparisons that can
yield wrong finite results.
Comment on lines
+191
to
+213
| public int findSegmentStartIndex(double distanceAlongTrip) { | ||
| if (stopTimes == null || stopTimes.length < 2) { | ||
| throw new IndexOutOfBoundsException("Fewer than 2 stop times"); | ||
| } | ||
|
|
||
| if (distanceAlongTrip < stopTimes[0].distanceAlongTrip) { | ||
| throw new IndexOutOfBoundsException("Distance is before first stop"); | ||
| } | ||
|
|
||
| if (distanceAlongTrip > stopTimes[stopTimes.length - 1].distanceAlongTrip) { | ||
| throw new IndexOutOfBoundsException("Distance is after last stop"); | ||
| } | ||
|
|
||
| for (int i = 0; i < stopTimes.length - 1; i++) { | ||
| if (stopTimes[i].distanceAlongTrip <= distanceAlongTrip && | ||
| distanceAlongTrip < stopTimes[i + 1].distanceAlongTrip) { | ||
| return i; | ||
| } | ||
| } | ||
|
|
||
| // At exactly the last stop's distance | ||
| return stopTimes.length - 2; | ||
| } |
There was a problem hiding this comment.
Reject non-finite distanceAlongTrip before segment lookup.
On Line 196–206, NaN and infinities can bypass the range checks and then fall through to Line 212, incorrectly returning the last segment index. This should fail fast to avoid wrong speed/position calculations downstream.
Suggested fix
public int findSegmentStartIndex(double distanceAlongTrip) {
if (stopTimes == null || stopTimes.length < 2) {
throw new IndexOutOfBoundsException("Fewer than 2 stop times");
}
+ if (!Double.isFinite(distanceAlongTrip)) {
+ throw new IndexOutOfBoundsException("Distance is not finite");
+ }
if (distanceAlongTrip < stopTimes[0].distanceAlongTrip) {
throw new IndexOutOfBoundsException("Distance is before first stop");
}📝 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.
Suggested change
| public int findSegmentStartIndex(double distanceAlongTrip) { | |
| if (stopTimes == null || stopTimes.length < 2) { | |
| throw new IndexOutOfBoundsException("Fewer than 2 stop times"); | |
| } | |
| if (distanceAlongTrip < stopTimes[0].distanceAlongTrip) { | |
| throw new IndexOutOfBoundsException("Distance is before first stop"); | |
| } | |
| if (distanceAlongTrip > stopTimes[stopTimes.length - 1].distanceAlongTrip) { | |
| throw new IndexOutOfBoundsException("Distance is after last stop"); | |
| } | |
| for (int i = 0; i < stopTimes.length - 1; i++) { | |
| if (stopTimes[i].distanceAlongTrip <= distanceAlongTrip && | |
| distanceAlongTrip < stopTimes[i + 1].distanceAlongTrip) { | |
| return i; | |
| } | |
| } | |
| // At exactly the last stop's distance | |
| return stopTimes.length - 2; | |
| } | |
| public int findSegmentStartIndex(double distanceAlongTrip) { | |
| if (stopTimes == null || stopTimes.length < 2) { | |
| throw new IndexOutOfBoundsException("Fewer than 2 stop times"); | |
| } | |
| if (!Double.isFinite(distanceAlongTrip)) { | |
| throw new IndexOutOfBoundsException("Distance is not finite"); | |
| } | |
| if (distanceAlongTrip < stopTimes[0].distanceAlongTrip) { | |
| throw new IndexOutOfBoundsException("Distance is before first stop"); | |
| } | |
| if (distanceAlongTrip > stopTimes[stopTimes.length - 1].distanceAlongTrip) { | |
| throw new IndexOutOfBoundsException("Distance is after last stop"); | |
| } | |
| for (int i = 0; i < stopTimes.length - 1; i++) { | |
| if (stopTimes[i].distanceAlongTrip <= distanceAlongTrip && | |
| distanceAlongTrip < stopTimes[i + 1].distanceAlongTrip) { | |
| return i; | |
| } | |
| } | |
| // At exactly the last stop's distance | |
| return stopTimes.length - 2; | |
| } |
🤖 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/io/elements/ObaTripSchedule.java`
around lines 191 - 213, In findSegmentStartIndex, add an early validation that
rejects non-finite distanceAlongTrip (NaN or infinite) before any range checks:
use Double.isFinite(distanceAlongTrip) (or check Double.isNaN/Double.isInfinite)
and throw an IllegalArgumentException (or IndexOutOfBoundsException per project
convention) with a clear message like "Non-finite distanceAlongTrip" so
NaN/infinities cannot bypass the range checks and return the last segment index
incorrectly; place this check at the top of the method before inspecting
stopTimes and the first/last distance comparisons.
Comment on lines
+62
to
+67
| // TODO: clock-domain mismatch. currentTime is local clock (System.currentTimeMillis), | ||
| // but the time axis is laid out against server-clock values — history[].lastUpdateTime | ||
| // and serviceDate + stopTime*1000. Any client/server clock skew shifts the red "now" | ||
| // line away from the latest trajectory dot. Fix by translating currentTime into server | ||
| // clock via HistoryEntry's serverTimeMs/localTimeMs offset. | ||
| private var currentTime = System.currentTimeMillis() |
There was a problem hiding this comment.
Resolve the clock-domain mismatch before using this view for timing/deviation judgments.
currentTime uses device local time while plotted status/schedule times are server-domain values, so the red "now" line and computed deviation can be systematically wrong under clock skew.
🤖 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, TrajectoryGraphView currently sets currentTime =
System.currentTimeMillis() which is in device-local clock and causes the "now"
line to be offset from server-domain times; instead compute a server-domain
currentTime by applying the server/local offset from a HistoryEntry (use
HistoryEntry.serverTimeMs and HistoryEntry.localTimeMs to derive offset =
serverTimeMs - localTimeMs) and update TrajectoryGraphView.currentTime by adding
that offset to System.currentTimeMillis() (or cache the offset when history is
loaded/updated and recalc currentTime whenever history changes). Ensure all
places that read currentTime (e.g., drawing/deviation calculations) use this
translated server-domain value so the red "now" line aligns with server times.
Comment on lines
+72
to
+79
| fun start(context: Context, tripId: String?, vehicleId: String?, stopId: String?) { | ||
| context.startActivity( | ||
| Intent(context, VehicleLocationDataActivity::class.java).apply { | ||
| putExtra(EXTRA_TRIP_ID, tripId) | ||
| putExtra(EXTRA_VEHICLE_ID, vehicleId) | ||
| putExtra(EXTRA_STOP_ID, stopId) | ||
| } | ||
| ) |
There was a problem hiding this comment.
Guard start() for non-Activity contexts to avoid launch-time crashes.
This helper accepts Context, but launching without FLAG_ACTIVITY_NEW_TASK can crash when called from application/service contexts.
Proposed fix
`@JvmStatic`
fun start(context: Context, tripId: String?, vehicleId: String?, stopId: String?) {
- context.startActivity(
- Intent(context, VehicleLocationDataActivity::class.java).apply {
- putExtra(EXTRA_TRIP_ID, tripId)
- putExtra(EXTRA_VEHICLE_ID, vehicleId)
- putExtra(EXTRA_STOP_ID, stopId)
- }
- )
+ val intent = Intent(context, VehicleLocationDataActivity::class.java).apply {
+ putExtra(EXTRA_TRIP_ID, tripId)
+ putExtra(EXTRA_VEHICLE_ID, vehicleId)
+ putExtra(EXTRA_STOP_ID, stopId)
+ if (context !is android.app.Activity) {
+ addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
+ }
+ }
+ context.startActivity(intent)
}🤖 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/VehicleLocationDataActivity.kt`
around lines 72 - 79, The start helper in VehicleLocationDataActivity currently
always starts an activity with the provided Context, which can crash if the
Context is not an Activity; update VehicleLocationDataActivity.start(...) to
detect if context is an instance of android.app.Activity and only omit
Intent.FLAG_ACTIVITY_NEW_TASK in that case, otherwise add
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) before calling
context.startActivity(...); keep the same Intent extras (EXTRA_TRIP_ID,
EXTRA_VEHICLE_ID, EXTRA_STOP_ID) and behavior but ensure the flag is applied
when needed to avoid launch-time crashes from non-Activity contexts.
Comment on lines
+28
to
+42
| /** Owned copy, so a caller mutating its list can't desync it from [cumulativeDistances]. */ | ||
| val points: List<Location> = points.toList() | ||
|
|
||
| private val cumulativeDistances: DoubleArray = | ||
| points | ||
| .zipWithNext { prev, cur -> | ||
| LocationUtils.haversineDistance( | ||
| prev.latitude, | ||
| prev.longitude, | ||
| cur.latitude, | ||
| cur.longitude | ||
| ) | ||
| } | ||
| .runningFold(0.0) { acc, dist -> acc + dist } | ||
| .toDoubleArray() |
There was a problem hiding this comment.
Make points a deep copy to keep geometry and cached distances consistent.
points.toList() only copies the list container; callers can still mutate shared Location instances and invalidate interpolation math.
Proposed fix
- /** Owned copy, so a caller mutating its list can't desync it from [cumulativeDistances]. */
- val points: List<Location> = points.toList()
+ /** Owned deep copy to keep coordinates stable against caller-side Location mutation. */
+ val points: List<Location> = points.map { Location(it) }🤖 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/util/Polyline.kt`
around lines 28 - 42, The public points property is only a shallow list copy so
callers can still mutate shared Location instances and break interpolation;
change the initialization to produce a deep copy of each Location (e.g., build
points using points.map { Location(it) }.toList() or equivalent copy-constructor
per Location) and use that copied list for cumulativeDistances computation so
geometry and cached distances remain consistent (referencing the points property
and cumulativeDistances in Polyline.kt).
| ): View { | ||
| return TextView(requireContext()).apply { | ||
| text = getString(R.string.trip_map_not_supported) | ||
| setPadding(32, 32, 32, 32) |
There was a problem hiding this comment.
Use density-independent padding for the placeholder view.
setPadding(32, 32, 32, 32) is raw px, so spacing will vary noticeably by screen density. Use dp-to-px conversion (or a dimen resource) to keep consistent UI spacing.
Suggested fix
+import kotlin.math.roundToInt
...
return TextView(requireContext()).apply {
text = getString(R.string.trip_map_not_supported)
- setPadding(32, 32, 32, 32)
+ val paddingPx = (16 * resources.displayMetrics.density).roundToInt()
+ setPadding(paddingPx, paddingPx, paddingPx, paddingPx)
}📝 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.
Suggested change
| setPadding(32, 32, 32, 32) | |
| val paddingPx = (16 * resources.displayMetrics.density).roundToInt() | |
| setPadding(paddingPx, paddingPx, paddingPx, paddingPx) |
🤖 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/tripmap/TripMapFragment.kt`
at line 56, The padding call in TripMapFragment.kt uses raw pixels
(setPadding(32, 32, 32, 32)), which will scale poorly across densities; change
it to density-independent pixels by converting 32 dp to pixels before calling
setPadding (e.g., use the view/context resources density or
TypedValue/requireContext().resources.getDimensionPixelSize with a dimen
resource) so the placeholder view’s spacing is consistent; update the call site
where setPadding is invoked in TripMapFragment (the placeholder view setup) to
use the dp->px conversion or reference a dimen resource instead of the hardcoded
32.
Member
|
@bmander oops, looks like this needs to be rebased against |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Stacked on #1554 (
animated-markers) — only the final commit (Bump Jackson 2.12.4 -> 2.13.5 and clear deprecated Jackson API usage) is new here; the rest of the diff disappears once #1554 merges.Summary
Bumps
jackson-core/jackson-annotations/jackson-databindfrom 2.12.4 (mid-2021) to 2.13.5, and clears the Jackson APIs deprecated within 2.x:MappingJsonFactory.createJsonGenerator()→createGenerator()(JacksonSerializer.java)ObjectMapper.setVisibilityChecker()→setVisibility()(JacksonSerializer.java)ObjectMapper.reader(Class)→readerFor(Class)(JacksonConfig.java)Why 2.13.5 and not the latest 2.x
2.13.5 is the highest Jackson version allowable on this project's SDK ceiling. Per Jackson's release wiki, 2.13 is the last branch supporting Android SDK 21+ — Jackson 2.14+ requires SDK 26 and crashes on API 21–25 devices at
ObjectMapperinstantiation (NoClassDefFoundError: java.lang.BootstrapMethodError; see square/retrofit#4131). Core library desugaring does not help, sincejava.lang.invokeis not indesugar_jdk_libs. AsminSdk 21is a deliberate project decision for broad device support, any further Jackson advance is gated on that decision. A comment inbuild.gradlenow documents this so the pin isn't bumped past the ceiling accidentally.Even so, the bump captures the known databind security fixes missing from 2.12.4 — CVE-2020-36518 (deep-nesting DoS) and CVE-2022-42003/42004 (resource exhaustion) — and uniformly lifts the stale transitive Jackson requests (2.5.3/2.7.3/2.9.x via
geojson-jacksonand others) to 2.13.5.Test plan
./gradlew compileObaGoogleDebugSources— clean build./gradlew testObaGoogleDebugUnitTest— all JVM unit tests passJacksonTest,ObaArrivalInfoTest) — 18/18 passed on a Pixel 7 Pro (Android 16)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes