Keep stop-focus color when a vehicle is selected (#1893 regression)#1899
Merged
Conversation
#1893 routed every vehicle selection through publishMapPresentation(), whose new selected-trip branch always drew the exact trip in the route's GTFS color (currentRouteColor()) over a generic direction underlay. Inside stop focus, where the emphasized route is drawn in its adjacency color, tapping a vehicle therefore reverted the route to its GTFS hue and layered the trip over the generic route-direction geometry — reading as standalone route mode. Resolve the selected trip's color from the adjacency palette (routeColors[selected.routeDirection]) when the route is emphasized in stop focus, falling back to the GTFS color only in whole-route mode where no adjacency entry exists. In that emphasized case also drop the generic direction underlay: the adjacency-colored trip already reads on its own, and the underlay is what made it look like route mode. Whole-route standalone selection is unchanged (GTFS trip + underlay). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSelected trip rendering in stop focus now prefers route-direction adjacency colors and suppresses the selected-direction underlay when adjacency coloring is available, while retaining GTFS color and underlay fallbacks. ChangesSelected trip rendering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 16, 2026
bmander
added a commit
that referenced
this pull request
Jul 16, 2026
) (#1916) publishMapPresentation() decided "am I in stop focus with this route emphasized?" by checking whether an adjacency-color lookup happened to return non-null — a proxy for the real stopFocusSession state one line up. An opposite-direction selected vehicle inside stop focus (its direction absent from the focused stop's trip set) would silently fall back to GTFS color + the generic underlay, the exact regression #1899 fixed. Extract selectedTripStyle() into RouteViewGeometry.kt: the underlay decision now keys off stopFocusActive directly, while the color still falls back to the GTFS color on an adjacency-color miss. Table-tested across all four (stopFocusActive x adjacency-color-present) combinations. Also drops the always-empty badges computation on the selected-trip path. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
bmander
added a commit
that referenced
this pull request
Jul 17, 2026
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>
bmander
added a commit
that referenced
this pull request
Jul 17, 2026
…) (#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>
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
1 participant
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.
Problem
After #1893 ("Focus vehicles on exact trip geometry and stops"), tapping a vehicle while a route is emphasized inside stop focus reverted the route line to its GTFS color and layered the exact trip over a generic route-direction underlay — so it read as if the map had dropped into standalone route mode. The intuitive behavior is that the route keeps its stop-focus (adjacency) color and does not print the generic route-direction geometry.
Root cause
#1893 added
showSelectionPresentation(tripId)to the selection collector, so every vehicle tap now callspublishMapPresentation(), whose new selected-trip branch unconditionally:currentRouteColor()(the route's GTFS color), andselectedDirectionUnderlay(...)(the whole direction shape, also GTFS-colored) beneath it.In whole-route standalone mode that's correct (there's no adjacency color). But inside stop focus the emphasized route is drawn from the adjacency palette (
_focusedRouteColors), so selecting a vehicle visibly swapped the adjacency-colored route for a GTFS trip over a generic underlay.Fix
In
RouteMapController.publishMapPresentation(), resolve the selected trip's color from the adjacency palette (routeColors[selected.routeDirection]) when the route is emphasized in stop focus, falling back to the GTFS color only in whole-route mode where no adjacency entry exists. In that emphasized case, also drop the generic direction underlay — the adjacency-colored trip already reads on its own, and the underlay is what made it look like route mode.As a small bonus this skips the O(n) underlay geometry build in the stop-focus case.
Testing
./gradlew :onebusaway-android:compileObaGoogleDebugKotlin -PwarningsAsErrors=true— clean (CI warnings gate).obaGoogleDebugon device for on-device confirmation of the rendering.🤖 Generated with Claude Code
Summary by CodeRabbit