Skip to content

Commit 00cdfd1

Browse files
bmanderclaude
bmander
and
claude
authored
RouteMapController: make presentation assembly pure and testable (#1907) (#1920)
* RouteMapController: extract pure presentation assembly (#1907) The controller's publishMapPresentation() merged three presentation modes (selected-vehicle exact trip, plain base route, stop-focus adjacency) via early returns, and the #1899 regression was exactly a missed interaction between two of them (adjacency colour vs. the generic direction underlay) — yet none of that policy had a JVM test. Extract the merge into a pure assembleRouteMapPresentation() in the new RouteMapPresentationPlan.kt, beside RouteViewGeometry.kt: a function of (base polylines/stops, focus session data, focused geometry/stops/routes, palette, mode flags, selected-trip render inputs) -> a RouteMapPresentation render plan. The controller keeps IO/lifecycle and now just resolves the IO-backed pieces (selectedTripRenderInput) and forwards each plan field to the render / stop layers. The one Location-dependent step, the focused-stop projection, enters as a lazy thunk so the assembler stays plain-data and JVM-testable across every branch. Also dedup the nearest-point-across-polylines kernel shared by projectFocusedStops and projectStopsOntoPolylines (moved to the new file) and give both a unit home. Tests: - RouteMapPresentationPlanTest (JVM): the three-mode precedence, the adjacency-colour/underlay interaction (both sides of the #1899 case), the degenerate-selection fall-through, and selection-wins-over-focus. - RouteStopProjectionTest (instrumented): the projection geometry, which snaps against android.location.Location and so can't run in this module's plain-JVM tests. Verified on-device (6/6 pass). No behaviour change: the assembler is a faithful extraction of the prior branching. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Cleanup: laziness, single lookup, file split for the presentation assembler Follow-up polish on the #1907 extraction (no behaviour change): - Restore the laziness the extraction lost. SelectedTripRenderInput built its directionUnderlay (a per-segment RoutePolyline copy) and stopPresentation (a Location stop projection) eagerly on every publish with a selected vehicle, but the assembler discards the underlay whenever the route has an adjacency colour and the stop projection whenever the trip is undrawable. The pre-extraction code computed both only inside the branch that used them. Defer both as thunks (mirroring the existing projectedFocusStops thunk) so each is computed only when its branch is taken. - Collapse the selected-vehicle branch to one adjacency-colour lookup and drop the redundant `selected != null` guard (selectedTrip is non-null only when selected is), restoring the original single-nullable shape. - Drop the now-dead `= { emptyMap() }` default on projectedFocusStops — the sole production caller always supplies it; make the contract explicit. - Move the Location-dependent projection kernels (projectFocusedStops, projectStopsOntoPolylines, the shared nearest-point kernel) out of RouteMapPresentationPlan.kt into a new RouteStopProjection.kt, matching the instrumented RouteStopProjectionTest's name and leaving the assembler file cleanly JVM-pure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Document why whole-route emphasis keeps the base route; pin it with a test Address CodeRabbit review on #1920: the exact-equality showBaseRoute check (it.routeDirection == emphasizedRoute) was flagged as incorrectly keeping the base route in whole-route mode. It is intentional, not a bug: - In whole-route mode emphasizedRoute carries a null direction, so basePolylines is the merged both-directions shape. The single-direction adjacency lines cover only part of it, so the base must stay — dropping it (matching route id alone) would erase the unfocused direction. - The proposed null-direction match would also empty the focused-stop presentation, since routeDirectionsByStopId's exact-match filter drops every concrete-direction trip against a null-direction key. Expand the comment to record this, and add a JVM test pinning that whole-route mode keeps the base route (and base stops) under a same-route focused stop. No behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent a56e5f6 commit 00cdfd1

5 files changed

Lines changed: 778 additions & 113 deletions

File tree

‎onebusaway-android/src/androidTest/java/org/onebusaway/android/map/RouteStopProjectionTest.kt‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
/*
2+
* Copyright (C) 2026 Open Transit Software Foundation
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.onebusaway.android.map
17+
18+
import androidx.test.ext.junit.runners.AndroidJUnit4
19+
import org.junit.Assert.assertEquals
20+
import org.junit.Assert.assertFalse
21+
import org.junit.Assert.assertNull
22+
import org.junit.Assert.assertTrue
23+
import org.junit.Test
24+
import org.junit.runner.RunWith
25+
import org.onebusaway.android.api.adapters.ObaStopElement
26+
import org.onebusaway.android.map.render.GeoPoint
27+
28+
/**
29+
* Instrumented coverage for the route-map stop projection geometry ([projectStopsOntoPolylines] and
30+
* [projectFocusedStops]) — the pure nearest-point-across-polylines kernel and its fall-backs. Runs on a
31+
* device because [org.onebusaway.android.util.Polyline] snaps against `android.location.Location`, which
32+
* this project's plain-JVM unit tests can't construct (the presentation-assembly logic that consumes the
33+
* result is JVM-tested in [RouteMapPresentationPlanTest]).
34+
*/
35+
@RunWith(AndroidJUnit4::class)
36+
class RouteStopProjectionTest {
37+
38+
// A meridian segment: latitude 0, longitude 0..10.
39+
private val meridian = listOf(GeoPoint(0.0, 0.0), GeoPoint(0.0, 10.0))
40+
41+
@Test
42+
fun projectStopsOntoPolylines_snapsEachStopOntoTheLine() {
43+
val stops = listOf(stop("a", lat = 1.0, lon = 5.0), stop("b", lat = -2.0, lon = 8.0))
44+
45+
val projected = projectStopsOntoPolylines(stops, listOf(meridian))
46+
47+
// Each stop drops perpendicularly onto the meridian: same longitude, latitude pulled to 0.
48+
assertClose(GeoPoint(0.0, 5.0), projected.getValue("a"))
49+
assertClose(GeoPoint(0.0, 8.0), projected.getValue("b"))
50+
}
51+
52+
@Test
53+
fun projectStopsOntoPolylines_picksTheNearerCandidateShape() {
54+
val far = listOf(GeoPoint(10.0, 0.0), GeoPoint(10.0, 10.0))
55+
val stops = listOf(stop("a", lat = 1.0, lon = 5.0))
56+
57+
val projected = projectStopsOntoPolylines(stops, listOf(far, meridian))
58+
59+
// The stop at latitude 1 is nearer the meridian (lat 0) than the far line (lat 10).
60+
assertClose(GeoPoint(0.0, 5.0), projected.getValue("a"))
61+
}
62+
63+
@Test
64+
fun projectStopsOntoPolylines_withNoDrawableShapeReturnsEmpty() {
65+
val stops = listOf(stop("a", lat = 1.0, lon = 5.0))
66+
67+
// A single-point "line" is not drawable (< 2 points), so nothing projects.
68+
assertTrue(projectStopsOntoPolylines(stops, listOf(listOf(GeoPoint(0.0, 0.0)))).isEmpty())
69+
assertTrue(projectStopsOntoPolylines(stops, emptyList()).isEmpty())
70+
}
71+
72+
@Test
73+
fun projectFocusedStops_snapsScheduledStopOntoItsTripShape() {
74+
val trips = setOf(focusedTrip("t1", "r1", shapeId = "s1"))
75+
val geometry = FocusedTripGeometry(listOf(shape("s1", "r1", meridian)))
76+
val stops = FocusedTripStops(
77+
stopIdsByTripId = mapOf("t1" to listOf("a")),
78+
stopsById = mapOf("a" to stop("a", lat = 1.0, lon = 5.0)),
79+
)
80+
81+
val projected = projectFocusedStops(trips, geometry, stops)
82+
83+
assertClose(GeoPoint(0.0, 5.0), projected.getValue("a"))
84+
}
85+
86+
@Test
87+
fun projectFocusedStops_picksNearestAcrossEveryTripThatServesTheStop() {
88+
val trips = setOf(
89+
focusedTrip("t1", "r1", shapeId = "s1"),
90+
focusedTrip("t2", "r2", shapeId = "s2"),
91+
)
92+
val geometry = FocusedTripGeometry(
93+
listOf(
94+
shape("s1", "r1", listOf(GeoPoint(10.0, 0.0), GeoPoint(10.0, 10.0))),
95+
shape("s2", "r2", meridian),
96+
)
97+
)
98+
// The stop is served by both trips; it must snap to whichever shape is nearer (the meridian).
99+
val stops = FocusedTripStops(
100+
stopIdsByTripId = mapOf("t1" to listOf("a"), "t2" to listOf("a")),
101+
stopsById = mapOf("a" to stop("a", lat = 1.0, lon = 5.0)),
102+
)
103+
104+
val projected = projectFocusedStops(trips, geometry, stops)
105+
106+
assertClose(GeoPoint(0.0, 5.0), projected.getValue("a"))
107+
}
108+
109+
@Test
110+
fun projectFocusedStops_omitsStopsWithNoDrawableCandidate() {
111+
val trips = setOf(
112+
focusedTrip("t-missing", "r1", shapeId = "s-absent"),
113+
focusedTrip("t-degenerate", "r2", shapeId = "s-degenerate"),
114+
focusedTrip("t-noshape", "r3", shapeId = null),
115+
)
116+
val geometry = FocusedTripGeometry(
117+
// s-absent isn't present at all; s-degenerate has < 2 points.
118+
listOf(shape("s-degenerate", "r2", listOf(GeoPoint(0.0, 0.0))))
119+
)
120+
val stops = FocusedTripStops(
121+
stopIdsByTripId = mapOf(
122+
"t-missing" to listOf("a"),
123+
"t-degenerate" to listOf("b"),
124+
"t-noshape" to listOf("c"),
125+
),
126+
stopsById = mapOf(
127+
"a" to stop("a", lat = 1.0, lon = 5.0),
128+
"b" to stop("b", lat = 1.0, lon = 5.0),
129+
"c" to stop("c", lat = 1.0, lon = 5.0),
130+
),
131+
)
132+
133+
val projected = projectFocusedStops(trips, geometry, stops)
134+
135+
assertTrue(projected.isEmpty())
136+
assertNull(projected["a"])
137+
assertFalse(projected.containsKey("b"))
138+
}
139+
140+
private fun assertClose(expected: GeoPoint, actual: GeoPoint) {
141+
assertEquals(expected.latitude, actual.latitude, 1e-6)
142+
assertEquals(expected.longitude, actual.longitude, 1e-6)
143+
}
144+
145+
private fun stop(id: String, lat: Double, lon: Double) = ObaStopElement(id = id, lat = lat, lon = lon)
146+
147+
private fun shape(shapeId: String, routeId: String, points: List<GeoPoint>) =
148+
FocusedTripShape(shapeId, routeId, routeColor = null, points = points)
149+
150+
private fun focusedTrip(tripId: String, routeId: String, shapeId: String?) =
151+
org.onebusaway.android.models.FocusedTrip(tripId, routeId, shapeId, routeColor = null)
152+
}

‎onebusaway-android/src/main/java/org/onebusaway/android/map/RouteMapController.kt‎

Lines changed: 42 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -126,12 +126,6 @@ class RouteMapController(
126126
private var basePolylines: List<RoutePolyline> = emptyList()
127127
private var baseStopPresentation: RouteStopPresentation? = null
128128

129-
private data class SelectedTripPresentation(
130-
val points: List<GeoPoint>,
131-
val stopIds: List<String>,
132-
val routeDirection: RouteDirectionKey,
133-
)
134-
135129
private data class StopFocusSession(
136130
val stopId: String,
137131
val trips: Set<FocusedTrip>,
@@ -590,73 +584,51 @@ class RouteMapController(
590584
publishMapPresentation()
591585
}
592586

587+
// Assemble the render plan from the current controller state (pure — see
588+
// [assembleRouteMapPresentation]) and forward each field to the render/stop layers. IO/retained
589+
// state is resolved into plain data first ([selectedTripRenderInput]); the mode-merging policy lives
590+
// in the pure function, so this stays a plumb-through.
593591
private fun publishMapPresentation() {
594-
val focus = stopFocusSession
595-
val emphasizedRoute = routeId?.let { RouteDirectionKey(it, presentationDirectionId) }
596-
val routeColors = _focusedRouteColors.value
597-
val selected = selectedTripPresentation()
598-
if (selected != null) {
599-
// See selectedTripStyle: stop focus alone gates the underlay, not whether an adjacency
600-
// color happened to be found for this exact direction (#1902).
601-
val style = selectedTripStyle(focus != null, selected.routeDirection, routeColors, currentRouteColor())
602-
val selectedTrip = selected.points
603-
.takeIf { it.size >= 2 }
604-
?.let { points -> focusedRoutePolyline(style.color, points, directional = true) }
605-
// A selected vehicle shows its exact trip everywhere: the trip line, framed to that trip,
606-
// over its scheduled stops.
607-
if (selectedTrip != null) {
608-
renderState.setRoutePolylines(
609-
polylines = focusedGeometry.toTripFocusedRoutePolylines(
610-
selected.routeDirection,
611-
routeColors,
612-
if (style.includeUnderlay) {
613-
selectedDirectionUnderlay(selected.routeDirection.directionId)
614-
} else emptyList(),
615-
selectedTrip,
616-
),
617-
framingPolylines = listOf(selectedTrip),
618-
routeModeScalesStopsWithZoom = isActive,
619-
)
620-
// A selected trip is only reachable in single-route mode, where emphasizedRoute is
621-
// always non-null too — the badges below are always suppressed here regardless.
622-
renderState.setRouteBadges(emptyList())
623-
stopsController.setRoutePresentation(selectedTripStopPresentation(selected))
624-
return
625-
}
626-
}
627-
val badges = if (emphasizedRoute == null) {
628-
focusedGeometry.toRouteBadges(focusedRoutes, routeColors)
629-
} else emptyList()
630-
if (focus == null) {
631-
renderState.setRoutePolylines(basePolylines, routeModeScalesStopsWithZoom = isActive)
632-
renderState.setRouteBadges(emptyList())
633-
stopsController.setRoutePresentation(baseStopPresentation)
634-
return
635-
}
636-
val showBaseRoute = isActive && emphasizedRoute != null &&
637-
focus.trips.none { it.routeDirection == emphasizedRoute }
638-
val routesByStopId = focusedStops.routeDirectionsByStopId(focus.trips, emphasizedRoute)
592+
val plan = assembleRouteMapPresentation(
593+
isActive = isActive,
594+
emphasizedRoute = routeId?.let { RouteDirectionKey(it, presentationDirectionId) },
595+
basePolylines = basePolylines,
596+
baseStopPresentation = baseStopPresentation,
597+
focusTrips = stopFocusSession?.trips,
598+
focusedGeometry = focusedGeometry,
599+
focusedStops = focusedStops,
600+
focusedRoutes = focusedRoutes,
601+
routeColors = _focusedRouteColors.value,
602+
selected = selectedTripRenderInput(),
603+
projectedFocusStops = {
604+
stopFocusSession?.let { projectFocusedStops(it.trips, focusedGeometry, focusedStops) }.orEmpty()
605+
},
606+
)
639607
renderState.setRoutePolylines(
640-
polylines = focusedGeometry.toRoutePolylines(emphasizedRoute, routeColors) +
641-
if (showBaseRoute) basePolylines else emptyList(),
642-
// Adjacency remains visible underneath route mode, but the active route's fully loaded
643-
// shape alone defines FramingIntent.Route. Using the displayed adjacency lines here would
644-
// fit the union of every route serving the focused stop.
645-
framingPolylines = if (isActive) basePolylines else emptyList(),
646-
routeModeScalesStopsWithZoom = isActive,
608+
polylines = plan.polylines,
609+
framingPolylines = plan.framingPolylines,
610+
routeModeScalesStopsWithZoom = plan.routeModeScalesStopsWithZoom,
647611
)
648-
renderState.setRouteBadges(badges)
649-
stopsController.setRoutePresentation(
650-
if (showBaseRoute) {
651-
baseStopPresentation
652-
} else {
653-
RouteStopPresentation(
654-
stops = routesByStopId.keys.mapNotNull(focusedStops.stopsById::get),
655-
routes = focusedRoutes,
656-
routeDirectionsByStopId = routesByStopId,
657-
projectedPoints = projectFocusedStops(focus.trips, focusedGeometry, focusedStops),
658-
)
659-
}
612+
renderState.setRouteBadges(plan.badges)
613+
stopsController.setRoutePresentation(plan.stopPresentation)
614+
}
615+
616+
/**
617+
* The selected vehicle's render inputs, or null when no vehicle is selected or its exact shape +
618+
* schedule haven't both resolved yet. Resolves the IO-backed pieces ([selectedTripPresentation],
619+
* the GTFS colour fallback, the direction underlay, the projected stop presentation) so the pure
620+
* assembler gets plain data.
621+
*/
622+
private fun selectedTripRenderInput(): SelectedTripRenderInput? {
623+
val selected = selectedTripPresentation() ?: return null
624+
return SelectedTripRenderInput(
625+
presentation = selected,
626+
routeColorFallback = currentRouteColor(),
627+
// Deferred: the assembler resolves the underlay only when selectedTripStyle keeps it (stop
628+
// focus inactive) and the stop projection only for a drawable trip, so neither is computed on
629+
// the branches that skip it.
630+
directionUnderlay = { selectedDirectionUnderlay(selected.routeDirection.directionId) },
631+
stopPresentation = { selectedTripStopPresentation(selected) },
660632
)
661633
}
662634

@@ -700,32 +672,6 @@ class RouteMapController(
700672
)
701673
}
702674

703-
/** Snap each exact scheduled stop to the closest successful shape of a trip that serves it. */
704-
private fun projectFocusedStops(
705-
trips: Set<FocusedTrip>,
706-
geometry: FocusedTripGeometry,
707-
stops: FocusedTripStops,
708-
): Map<String, GeoPoint> {
709-
val tripById = trips.associateBy(FocusedTrip::tripId)
710-
val candidates = LinkedHashMap<String, MutableList<Polyline>>()
711-
for ((tripId, stopIds) in stops.stopIdsByTripId) {
712-
val shapeId = tripById[tripId]?.shapeId ?: continue
713-
val points = geometry.shapes.firstOrNull { it.shapeId == shapeId }?.points ?: continue
714-
if (points.size < 2) continue
715-
val polyline = Polyline(points.map(GeoPoint::toLocation))
716-
stopIds.forEach { candidates.getOrPut(it, ::mutableListOf).add(polyline) }
717-
}
718-
return buildMap {
719-
for ((stopId, shapes) in candidates) {
720-
val stop = stops.stopsById[stopId] ?: continue
721-
val location = stop.location
722-
val point = shapes.mapNotNull { it.nearestPoint(location.latitude, location.longitude) }
723-
.minByOrNull(location::distanceTo)
724-
if (point != null) put(stopId, point.toGeoPoint())
725-
}
726-
}
727-
}
728-
729675
/** Restart the vehicle poll if a route is shown and the poll isn't running (the host's onResume). */
730676
fun onResume() {
731677
if (routeId != null && vehicleJob?.isActive != true) {
@@ -818,23 +764,6 @@ class RouteMapController(
818764
return projectStopsOntoPolylines(stops, route.shapeForDirection(currentDirectionId).polylines)
819765
}
820766

821-
private fun projectStopsOntoPolylines(
822-
stops: List<ObaStop>,
823-
points: List<List<GeoPoint>>,
824-
): Map<String, GeoPoint> {
825-
val shapes = points
826-
.filter { it.size >= 2 }
827-
.map { line -> Polyline(line.map(GeoPoint::toLocation)) }
828-
if (shapes.isEmpty()) return emptyMap()
829-
return stops.associate { stop ->
830-
val loc = stop.location
831-
val nearest = shapes
832-
.mapNotNull { it.nearestPoint(loc.latitude, loc.longitude) }
833-
.minByOrNull { loc.distanceTo(it) }
834-
stop.id to (nearest?.toGeoPoint() ?: loc.toGeoPoint())
835-
}
836-
}
837-
838767
/**
839768
* The drawn lines for [directionId]'s shape: the selected direction's own travel-ordered shape
840769
* (with direction arrows), or the whole-route merged shape drawn undirected when [directionId] is

0 commit comments

Comments
 (0)