Add save, share, and reuse trip plans feature (#1110)

[diveshpatil9104]

Summary

Implements Save, Share & Reuse Trip Plans (closes #1110).

Users can now:

• Save a planned trip with a custom name and optional favorite flag
• Share trip details (route, times, duration) via any app (Messages, Email, etc.)
• View saved trips from the navigation drawer
• Re-open a saved trip to view its itinerary
• Favorite trips with a star toggle
• Delete trips via long-press with confirmation

What Changed

• New menu actions in Trip Plan screen: "Save trip" and "Share trip"
• Save dialog with trip name input and favorite checkbox
• Share intent formats trip details (depart/arrive times respecting 24h setting, duration, route)
• Saved Trips screen accessible from nav drawer, with RecyclerView list and empty state
• Room database migration (v2 → v3) adding saved_trips table
• New files: SavedTripEntity, SavedTripDao, SavedTripsManager, ItineraryJsonConverter, SavedTripsActivity, SavedTripsFragment, 4 layout XMLs

Architecture

Follows existing codebase patterns:

• Room Entity/DAO/Manager pattern (mirrors RecentStopsManager)
• Gson for Itinerary serialization/deserialization
• Nav drawer integration matching existing items
• include_toolbar + 0dp/weight layout pattern
• White-label safe (%1$s + getString(R.string.app_name))

Screen Recording

trip.save.mp4

Test Plan

• Save a trip with custom name — appears in Saved Trips list
• Save a trip as favorite — star icon filled
• Share a trip — share sheet shows formatted trip text
• Open Saved Trips from nav drawer — list displays correctly
• Tap saved trip — re-opens in Trip Plan screen
• Toggle favorite — star updates immediately
• Long-press delete — confirmation dialog, trip removed
• No trip planned → "Save trip" shows "No trip to save" toast
• Empty state — "No saved trips" message when list is empty

[aaronbrethorst]

Hey Divesh, the overall design of this feature is solid -- the Room Entity/DAO/Manager pattern mirrors existing code, the nav drawer integration is clean, and the save dialog with default naming is a nice touch. There are a few issues that need to be resolved before we can merge, though.

Critical Issues (1 found)

1. Re-opened saved trip doesn't pass from/to addresses to TripPlanActivity

SavedTripsFragment.java:112-115

When the user taps a saved trip, the intent only contains the itinerary -- it doesn't pass the from/to addresses. This means TripPlanActivity's TripRequestBuilder has no origin or destination, so the trip plan form shows empty from/to fields. The user can view the results panel, but they can't re-plan the trip, and the context of where the trip goes is lost from the planning screen.

The SavedTripEntity already stores fromAddress, toAddress, fromLat, fromLon, toLat, toLon -- they just need to be wired through to the intent:

private void onTripClicked(SavedTripEntity trip) {
    // ... existing itinerary deserialization ...

    Intent intent = new Intent(requireContext(), TripPlanActivity.class);
    intent.putExtra(OTPConstants.ITINERARIES, itineraries);
    intent.putExtra(OTPConstants.INTENT_SOURCE, OTPConstants.Source.ACTIVITY);

    // Pass from/to so TripPlanFragment can display and re-plan
    CustomAddress from = new CustomAddress(Locale.getDefault());
    from.setAddressLine(0, trip.getFromAddress());
    from.setLatitude(trip.getFromLat());
    from.setLongitude(trip.getFromLon());
    intent.putExtra(OTPConstants.FROM_ADDRESS, from);

    CustomAddress to = new CustomAddress(Locale.getDefault());
    to.setAddressLine(0, trip.getToAddress());
    to.setLatitude(trip.getToLat());
    to.setLongitude(trip.getToLon());
    intent.putExtra(OTPConstants.TO_ADDRESS, to);

    startActivity(intent);
}

You'll need to verify the exact OTPConstants keys and how TripRequestBuilder picks them up -- check how the notification source path handles this in TripPlanActivity.onResume().

Important Issues (3 found)

1. getAdapterPosition() is deprecated

SavedTripsFragment.java:214, 225

RecyclerView.ViewHolder.getAdapterPosition() was deprecated in RecyclerView 1.2.0. Replace with getBindingAdapterPosition() in both the long-click and favorite-click handlers.

2. confirmDelete captures stale position in dialog callback

SavedTripsFragment.java:127-148

The position parameter is captured in the dialog's positive button lambda. If the fragment goes through a pause/resume cycle while the dialog is visible (e.g., user briefly switches apps), loadTrips() runs in onResume and rebuilds mTrips. The captured position is now stale -- mTrips.remove(position) could remove the wrong trip or throw IndexOutOfBoundsException.

Fix by looking up the trip by ID instead of relying on position:

.setPositiveButton(android.R.string.ok, (dialog, which) -> {
    SavedTripsManager.deleteTrip(requireContext(), trip);
    int currentPos = mTrips.indexOf(trip);
    if (currentPos >= 0) {
        mTrips.remove(currentPos);
        mAdapter.notifyItemRemoved(currentPos);
    }
    updateEmptyState();
    // ...
})

Note: this relies on SavedTripEntity having proper equals() (which it does, since it's a Kotlin data class).

3. onFavoriteClicked manually reconstructs entity with 11 positional args

SavedTripsFragment.java:120-123

This is fragile -- if SavedTripEntity's constructor order changes, this will silently pass wrong values with no compiler error (e.g., swapping fromLat and fromLon). Since the entity is a Kotlin data class, call the Kotlin-generated copy() method instead. If calling from Java is awkward, create a small Kotlin helper:

// In SavedTripEntity.kt
fun withToggledFavorite(): SavedTripEntity = copy(favorite = !favorite)

Then in Java:

mTrips.set(position, trip.withToggledFavorite());

Suggestions (2 found)

1. Nav drawer icon reuses "Plan a trip" icon

NavigationDrawerFragment.java:150

"Saved Trips" uses ic_maps_directions, identical to "Plan a trip". Users scanning the drawer will see two items with the same icon side by side. Consider a distinct icon -- a bookmark or save drawable would differentiate them.

2. Share text omits the date

TripPlanActivity.java:574-587

The share text includes departure and arrival times (e.g., "Depart: 3:45 PM") but not the date. If someone shares a trip planned for tomorrow, the recipient has no idea which day. Include the date in the formatted time, e.g., "Depart: Mar 15, 3:45 PM".

Strengths

• Clean architecture following existing Entity/DAO/Manager patterns
• Proper database migration (v2 -> v3) with schema export
• White-label safe strings using %1$s placeholders
• Save dialog defaults to "From -> To" naming with select-all for easy editing
• Share formatting respects 24-hour time setting
• Itinerary JSON deserialization handles errors gracefully
• Nav drawer item correctly gated behind OTP availability check

Recommended Action

1. Fix the from/to address passthrough (Critical #1) -- this is the core "reuse" part of "save, share, and reuse"
2. Fix the deprecated API, stale position, and fragile constructor (Important #1-3)
3. Consider the suggestions

[diveshpatil9104]

Critical 1 — Fixed. onTripClicked now builds CustomAddress objects from the saved from/to data, sets them via TripRequestBuilder, and passes them through the intent using Source.NOTIFICATION path so
TripPlanActivity.onResume() copies them into the builder bundle. From/to fields now populate correctly on re-open.

Important 1 — getAdapterPosition(): getBindingAdapterPosition() is not available in this project RecyclerView version (pre-1.2.0), so keeping getAdapterPosition() for now. Can be updated when the
dependency is bumped.

Important 2 — Fixed. confirmDelete no longer captures a stale position. Now uses mTrips.indexOf(trip) with a bounds check at deletion time, relying on the data class equals().

Important 3 — Fixed. Added withToggledFavorite() helper to SavedTripEntity.kt using Kotlin copy(). Java call site is now just trip.withToggledFavorite().

Suggestion 1 — Fixed. Changed nav drawer icon from ic_maps_directions to ic_drawer_star to differentiate from Plan a Trip.

Suggestion 2 — Fixed. Share text time format now includes the date: MMM d, HH:mm (e.g., Mar 15, 15:45).

[aaronbrethorst]

Hey Divesh, thanks for addressing the feedback from the first review -- the from/to address passthrough, the stale position fix, and the withToggledFavorite() helper all look correct. A few things remain before we can merge.

Critical Issues (0 found)

None -- the from/to address passthrough is working correctly now.

Important Issues (2 found)

1. getAdapterPosition() is still deprecated -- and getBindingAdapterPosition() IS available

SavedTripsFragment.java:240

In the previous review you mentioned that getBindingAdapterPosition() isn't available because the project uses a pre-1.2.0 RecyclerView. However, Gradle dependency resolution shows RecyclerView 1.3.1 (pulled in transitively by material:1.12.0), so getBindingAdapterPosition() is available. Please replace:

int pos = getAdapterPosition();

with:

int pos = getBindingAdapterPosition();

2. Fire-and-forget coroutines in SavedTripsManager silently swallow exceptions

SavedTripsManager.kt:41, 97, 110

saveTrip(), toggleFavorite(), and deleteTrip() launch coroutines with CoroutineScope(Dispatchers.IO).launch { ... } and have no exception handler. If the DAO call throws (e.g., a SQLite constraint violation), the exception is silently swallowed -- the user sees "Trip saved" toast but the trip isn't actually saved.

At minimum, wrap the DAO calls in a try/catch and log the error. Better yet, consider using a shared CoroutineExceptionHandler:

private val exceptionHandler = CoroutineExceptionHandler { _, throwable ->
    Log.e(TAG, "Database operation failed", throwable)
}

// Then in each launch:
CoroutineScope(Dispatchers.IO + exceptionHandler).launch {
    dao.insert(trip)
    callback?.invoke(id)
}

This also applies to toggleFavorite and deleteTrip, where a failed write means the UI is now out of sync with the database.

Suggestions (1 found)

1. saveTripToDatabase still uses positional constructor for SavedTripEntity

TripPlanActivity.java:547-550

The same fragility issue flagged for onFavoriteClicked (now fixed via withToggledFavorite()) still exists here -- SavedTripEntity is constructed with 11 positional args. If the entity's constructor order changes, this call site will silently pass wrong values. Consider using Kotlin named arguments (from a Kotlin call site) or creating a builder/factory method on SavedTripEntity.

This isn't blocking, but it's worth tracking as a follow-up.

Verification of Previous Feedback

Previous Item	Status
Critical #1: from/to address passthrough	Fixed -- onTripClicked now builds CustomAddress objects and uses TripRequestBuilder
Important #1: deprecated getAdapterPosition()	Not fixed -- author believed RecyclerView was pre-1.2.0, but resolved version is 1.3.1
Important #2: stale position in confirmDelete	Fixed -- now uses mTrips.indexOf(trip) with bounds check
Important #3: fragile positional constructor in onFavoriteClicked	Fixed -- uses withToggledFavorite() helper
Suggestion #1: duplicate nav drawer icon	Fixed -- changed to ic_drawer_star
Suggestion #2: share text omits date	Fixed -- format is now MMM d, HH:mm / MMM d, hh:mm a

Strengths

• All critical and most important items from the first review were addressed correctly
• The TripRequestBuilder + Source.NOTIFICATION approach for re-opening trips is clean
• withToggledFavorite() is a nice, idiomatic Kotlin helper
• The indexOf(trip) fix for stale position is correct since SavedTripEntity is a data class with proper equals()
• Nav drawer icon change differentiates Saved Trips from Plan a Trip

Recommended Action

1. Replace getAdapterPosition() with getBindingAdapterPosition() (Important #1)
2. Add exception handling to fire-and-forget coroutines (Important #2)
3. Consider the positional constructor suggestion as a follow-up

[diveshpatil9104]

Thanks Aaron! issues addressed:

Important 1 — I ran ./gradlew :onebusaway-android:dependencies --configuration obaGoogleDebugCompileClasspath | grep recyclerview and the resolved version is 1.1.0 (not 1.3.1). The 1.3.1 version from material:1.12.0 is not being pulled in transitively Gradle resolves to 1.1.0. getBindingAdapterPosition() was added in 1.2.0, so it does not compile. Keeping getAdapterPosition() for now. Happy to bump the RecyclerView dependency in a follow-up PR if you'd like.

Important 2 — Fixed. Added a shared CoroutineExceptionHandler with Log.e() to all three fire-and-forget coroutines (saveTrip, toggleFavorite, deleteTrip). Database failures are now logged instead of silently swallowed.

Suggestion 1 — Noted for follow-up. Will track the positional constructor cleanup separately.

[diveshpatil9104]

My bad on the RecyclerView version 🥲 you were right! Added explicit recyclerview:1.3.1 to build.gradle and switched to getBindingAdapterPosition(). All done.