Trip planner: map-overlay layout (top-sheet form + bottom-sheet directions)#1872
Merged
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTrip planning now runs as directions mode on the HOME map. HOME hosts planner and results state, renders endpoint picking and itinerary UI, draws itineraries through map directives, persists directions focus, and removes the standalone trip-plan destination. ChangesHOME directions trip-planning flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant HomeScreen
participant TripPlanViewModel
participant TripResultsViewModel
participant HomeViewModel
participant MapViewModel
User->>HomeScreen: Enter directions mode
HomeScreen->>TripPlanViewModel: Set endpoints and planning options
TripPlanViewModel->>TripResultsViewModel: Produce itineraries
HomeScreen->>HomeViewModel: Show selected itinerary
HomeViewModel->>MapViewModel: Dispatch itinerary map directive
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…tions) Restructures the "Plan a trip" flow so the directions map is the constant backdrop instead of the form being a full screen: - The form is now a top sheet that slides down over the map. It drags like a bottom sheet: an AnchoredDraggable state drives its height, and the full form cross-fades with a compact "From -> To" summary bar as it minimizes/maximizes. - The directions are a custom bottom sheet whose drag gesture lives on the handle only, so the directions list scrolls freely inside. It opens to 40% of the screen and collapses to the handle, seated above the nav-bar chrome while its surface reaches the bottom edge (list clearance via scroll content padding). - The map (DirectionsMapViewModel) is mounted for the whole flow and draws the itinerary once a plan completes; the standalone toolbar is folded into the sheet header. TripPlanTopBar/BottomSheetScaffold usage removed; TripResultsSheet/Map reused.
Fold the trip planner into the home map instead of a separate screen, driven by a new CurrentFocus.Directions in the existing focus system: - CurrentFocus.Directions (persisted); HomeViewModel.enterDirections/exitDirections + MapDirective.ShowItinerary/ClearItinerary bridged in MapFeature. - MapViewModel gains a DirectionsMapController session integrated with leaveCurrentView (entering directions drops stop/route focus; exiting restores nearby stops). - The top-chrome search field is replaced by a compact trip-plan form while planning (DirectionsFormCard); a directions results sheet + itinerary render over the map; a plan error / no-route / loading state is now surfaced. - Pick a From/To point on the home map (crosshair + confirm); current-location, date/time, and advanced-settings are wired. - Retire the standalone TRIP_PLAN screen (TripPlanScreen/TripPlanFormSheet/ DirectionsMapViewModel/TripResultsMap); the drawer entry now enters directions focus and the trip-monitor notification opens Home. - Remove the address-book (contacts) picker (#1936 tracks place-intent receiving). TripPlanViewModel/TripResultsViewModel are hosted at HomeActivity scope.
bmander
force-pushed
the
feature/trip-plan-map-overlay
branch
from
July 17, 2026 19:01
6204126 to
4ebf15a
Compare
bmander
marked this pull request as ready for review
July 17, 2026 19:02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanMonitorService.kt (1)
232-244: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftImplement directions restoration before removing the trip-plan route.
The notification carries the request and updated itineraries, but no supplied HOME path consumes them; the comment confirms restoration is deferred. Tapping a trip-change alert therefore opens generic HOME instead of the changed trip. Restore these extras into directions focus before shipping this removal.
🤖 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/directions/realtime/TripPlanMonitorService.kt` around lines 232 - 244, Update the notification tap flow around requestExtras and openIntent so the serialized request and itineraries are restored into the directions screen before removing the trip-plan route. Use the existing directions restoration/navigation path, including the appropriate navigation route extra if required, instead of opening generic HOME without EXTRA_NAV_ROUTE; preserve the notification source metadata.
🤖 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/main/java/org/onebusaway/android/ui/home/directions/DirectionsFeature.kt`:
- Around line 154-160: Update setCurrentLocation so a null lastKnownLocation()
is not always reported as a permission failure. Check the current
location-permission state first and show the permission message only when
permission is denied; otherwise show the appropriate location-unavailable
message before returning.
- Around line 165-203: Update pickTripDate and pickTripTime to perform date/time
reads, mutations, and writes using WallTime at the ViewModel/UI boundary rather
than raw epoch Long values or Calendar arithmetic. Convert WallTime to epoch
milliseconds only when supplying MaterialDatePicker or MaterialTimePicker
values, and wrap picker results back into WallTime before calling
viewModel.setDateTime.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt`:
- Around line 694-702: Update the onConfirm handler for the DirectionsPickTarget
picker so it does not clear pickTarget when mapViewModel.camera.value or its
center is unavailable. Either disable confirmation until a camera center exists
or dismiss the picker only after successfully calling setFrom or setTo, while
preserving the existing endpoint selection behavior.
---
Outside diff comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanMonitorService.kt`:
- Around line 232-244: Update the notification tap flow around requestExtras and
openIntent so the serialized request and itineraries are restored into the
directions screen before removing the trip-plan route. Use the existing
directions restoration/navigation path, including the appropriate navigation
route extra if required, instead of opening generic HOME without
EXTRA_NAV_ROUTE; preserve the notification source metadata.
🪄 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: 6ae0230d-1ae7-4bba-8059-64e5f4ced48b
📒 Files selected for processing (18)
onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanMonitorService.ktonebusaway-android/src/main/java/org/onebusaway/android/map/DirectionsMapViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/map/MapViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/CurrentFocus.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/CurrentFocusPersistence.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeNavHost.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/directions/DirectionsFeature.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/MapFeature.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanDestinations.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanForm.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsScreen.ktonebusaway-android/src/main/res/drawable/baseline_import_contacts_24.xmlonebusaway-android/src/main/res/values/strings.xml
💤 Files with no reviewable changes (4)
- onebusaway-android/src/main/java/org/onebusaway/android/map/DirectionsMapViewModel.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/TripPlanScreen.kt
- onebusaway-android/src/main/res/drawable/baseline_import_contacts_24.xml
- onebusaway-android/src/main/res/values/strings.xml
Comment on lines
+165
to
+203
| private fun pickTripDate(activity: AppCompatActivity, viewModel: TripPlanViewModel) { | ||
| val current = viewModel.formState.value.dateTimeMillis | ||
| val picker = MaterialDatePicker.Builder.datePicker() | ||
| .setTitleText(R.string.trip_plan_date) | ||
| .setSelection(current) | ||
| .build() | ||
| picker.addOnPositiveButtonClickListener { selection -> | ||
| // Read the selection in UTC, exactly as the user saw it (matches the legacy form). | ||
| val utc = Calendar.getInstance(TimeZone.getTimeZone("UTC")).apply { timeInMillis = selection } | ||
| val calendar = Calendar.getInstance().apply { | ||
| timeInMillis = current | ||
| set(Calendar.YEAR, utc.get(Calendar.YEAR)) | ||
| set(Calendar.MONTH, utc.get(Calendar.MONTH)) | ||
| set(Calendar.DAY_OF_MONTH, utc.get(Calendar.DAY_OF_MONTH)) | ||
| } | ||
| viewModel.setDateTime(calendar.timeInMillis) | ||
| } | ||
| picker.show(activity.supportFragmentManager, "DATE_PICKER") | ||
| } | ||
|
|
||
| private fun pickTripTime(activity: AppCompatActivity, viewModel: TripPlanViewModel) { | ||
| val current = viewModel.formState.value.dateTimeMillis | ||
| val calendar = Calendar.getInstance().apply { timeInMillis = current } | ||
| val timeFormat = | ||
| if (DateFormat.is24HourFormat(activity)) TimeFormat.CLOCK_24H else TimeFormat.CLOCK_12H | ||
| val picker = MaterialTimePicker.Builder() | ||
| .setTimeFormat(timeFormat) | ||
| .setHour(calendar.get(Calendar.HOUR_OF_DAY)) | ||
| .setMinute(calendar.get(Calendar.MINUTE)) | ||
| .setTitleText(R.string.trip_plan_time) | ||
| .setTheme(R.style.ThemeOverlay_App_TimePicker) | ||
| .build() | ||
| picker.addOnPositiveButtonClickListener { | ||
| calendar.set(Calendar.HOUR_OF_DAY, picker.hour) | ||
| calendar.set(Calendar.MINUTE, picker.minute) | ||
| viewModel.setDateTime(calendar.timeInMillis) | ||
| } | ||
| picker.show(activity.supportFragmentManager, "TIME_PICKER") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep trip date/time arithmetic in the WallTime domain.
These helpers read, mutate, and write raw epoch Long values through Calendar. Use WallTime at the ViewModel/UI boundary and unwrap only for the Material picker APIs.
As per coding guidelines, “Use ServerTime, WallTime, and ElapsedTime from org.onebusaway.android.time for new time arithmetic instead of raw Long values.”
🤖 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/home/directions/DirectionsFeature.kt`
around lines 165 - 203, Update pickTripDate and pickTripTime to perform
date/time reads, mutations, and writes using WallTime at the ViewModel/UI
boundary rather than raw epoch Long values or Calendar arithmetic. Convert
WallTime to epoch milliseconds only when supplying MaterialDatePicker or
MaterialTimePicker values, and wrap picker results back into WallTime before
calling viewModel.setDateTime.
Source: Coding guidelines
- setCurrentLocation: a null last-known location no longer always reports "no location permission". Check the permission state first via PermissionUtils.hasGrantedAtLeastOnePermission; show the recoverable "trying to get a fix" message when permission is granted but there's no fix yet, and the permission message only when it's actually denied. - DirectionsPickOverlay onConfirm: only clear pickTarget (dismiss the picker) after successfully committing an endpoint. With no map center available, keep the picker open instead of silently losing the selection.
bmander
added a commit
that referenced
this pull request
Jul 18, 2026
#1940) PR #1872 embedded directions into the home map and retired the standalone TripPlanScreen, whose maybeRestoreFromIntent was the only consumer of the trip-plan monitor's "your trip changed" notification intent. With that gone, tapping the alert opened generic HOME instead of the changed trip: the intent still carried the request bundle (copyIntoBundleSimple), the updated itineraries (OTPConstants.ITINERARIES, JSON), and INTENT_SOURCE=NOTIFICATION, but nothing in HOME read them. Re-add the consumer in HomeActivity's launch-intent side-effect path, driving the Activity-scoped VMs instead of the retired screen's: guard on INTENT_SOURCE=NOTIFICATION, JSON-decode the itineraries, rehydrate the request via TripRequestBuilder.initFromBundleSimple, then enterDirections() + TripPlanViewModel.restoreFrom(...) to inject the precomputed itineraries as PlanResult.Success without re-planning. The existing directions render path (results sheet + map draw) reacts with no further changes; the producer side and restoreFrom were already intact. No re-restore guard is needed: the launch-intent channel submits each real intent exactly once (cold onCreate seed, warm onNewIntent), so a config change doesn't re-fire it. params=null (monitor not re-armed from the restore) matches pre-#1872 behavior. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 18, 2026
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.
What
Restructures the Plan a trip flow so the directions map is the constant backdrop of the whole flow, instead of the form being a separate full screen.
AnchoredDraggablestate drives its height, and the full form cross-fades with a compactFrom → Tosummary bar as you minimize/maximize (drag the bottom handle, or tap it). Auto-collapses to the summary once a plan completes.AnchoredDraggablesheet whose drag gesture lives on the handle only, so the directionsLazyColumnscrolls freely inside. Opens to 40% of the screen, collapses to just the handle, seated above the nav-bar chrome while its surface still reaches the bottom edge (last row cleared via list scroll content padding).DirectionsMapViewModelis mounted for the whole flow (base map before a plan, the drawn itinerary after). The standalone trip-plan toolbar is folded into the top sheet's header (back + "report problem").Notes
TripPlanTopBarand theBottomSheetScaffoldusage in the trip-plan container are removed;TripResultsSheet/TripResultsMapare reused unchanged aside from a new optional list bottom-inset.Testing
./gradlew installObaGoogleDebug(verified on a Pixel 7 Pro).Rebased continuation of #1683 (moved from the fork branch to a branch on this repo; rebased onto latest
main). The map-overlay commit was replayed onto currentmain— which meanwhile refactored the same screen (region/report email now viaTripPlanViewModel.otpContactEmail,locationOf/mapPickerCenter(), and a requiredparamsarg onTripResultsSheet); those were reconciled and the strict warnings-as-errors compile passes.🤖 Generated with Claude Code
Summary by CodeRabbit