feat(android.NearbyTransitPage): leave / rejoin vehicles channel after backgrounding #579
Conversation
| var railRouteLineData: List<RouteLineData>? by remember { mutableStateOf(null) } | ||
| var stopSourceData: FeatureCollection? by remember { mutableStateOf(null) } |
There was a problem hiding this comment.
Just to prevent crashes while debugging
| var stopDetailsDepartures by rememberSaveable { mutableStateOf<StopDetailsDepartures?>(null) } | ||
| var vehiclesData: List<Vehicle> by remember { mutableStateOf(emptyList()) } | ||
|
|
||
| var stopDetailsDepartures by remember { mutableStateOf<StopDetailsDepartures?>(null) } |
There was a problem hiding this comment.
changed to remember for debugging. Should probably move into the new VM.
| val viewModel: VehiclesViewModel = | ||
| viewModel(factory = VehiclesViewModel.Factory(vehiclesRepository)) |
There was a problem hiding this comment.
I initially tried following the same approach as subscribeToPredictions, but onCleared was never invoked (and from what I can tell, also isn't invoked with subscribeToPredictions.
with that, having a single viewModel that persists across renders and dynamically connecting / disconnecting to the correct channel based on changing topic or lifecycle event seemed more reliable.
If this approach is sound, I'll follow-up with making subscribeToPredictions & subscribeToAlerts consistent.
There was a problem hiding this comment.
I'm definitely up to adopt the ViewModel-centric approach and refactor the two subscribe functions. The only thing I want to suggest is that the ViewModel should abstract away the concept of Channels from the View, e.g. is I'd prefer to avoid lower-level functions like subscribeToVehicleTopic(topic: String) in favor of functions like updateFollowedVehicle(vehicle: Vehicle).
There was a problem hiding this comment.
Good call on naming - I'll switch to something a bit more descriptive like subscribeToVehiclesOnRoute
| @@ -56,11 +56,12 @@ fun ContentView( | |||
| rememberBottomSheetScaffoldState(bottomSheetState = rememberStandardBottomSheetState()) | |||
| var navBarVisible by remember { mutableStateOf(true) } | |||
|
|
|||
| DisposableEffect(null) { | |||
| LifecycleResumeEffect(null) { | |||
| } | ||
| } | ||
| } | ||
|
|
||
| fun handleRouteChange(route: SheetRoutes?) { |
There was a problem hiding this comment.
What do we think about refactoring this a bit so that more logic is in the view model? Maybe even automatically after changing the StopDetailsFilter?
KaylaBrady
force-pushed
the
kb-android-backgrounding
branch
2 times, most recently
from
ce17eab to
40d42df
Compare
KaylaBrady
force-pushed
the
kb-android-backgrounding
branch
from
43aaf33 to
9fa0344
Compare
| import org.koin.compose.koinInject | ||
|
|
||
| class VehiclesViewModel( | ||
| private val vehiclesRepository: IVehiclesRepository, |
There was a problem hiding this comment.
One thing worth mentioning here is that I didn't use the TImerViewModel that was used in subscribeToPredictions & subscribeToAlerts to synchronize updates. My perception was it was taking vehicles a long time to appear on the screen - perhaps because waiting for the 1s synchronized update. @JackVCurtis would be curious to learn more about your findings with using the TimerViewModel though - perhaps it should just have a more frequent rate.
KaylaBrady
force-pushed
the
kb-android-backgrounding
branch
from
6210e40 to
46dc1ed
Compare
KaylaBrady
force-pushed
the
kb-android-backgrounding
branch
from
46dc1ed to
8b36601
Compare
| @@ -82,6 +82,7 @@ dependencies { | |||
| implementation(libs.mapbox.turf) | |||
| implementation(libs.okhttp) | |||
| implementation(libs.playServices.location) | |||
| implementation(libs.androidx.lifecycle.runtime.testing) | |||
There was a problem hiding this comment.
Should this be an androidTestImplementation dependency so it's not available in the main source root (and so it's not included in the dependency list)?
Summary
Ticket: 🤖 | Stability | Leave realtime channels when backgrounding
What is this PR for?
This PR is one slice of the above ticket, focused only on the vehicles channel for early feedback. I plan to extend this approach to the existing
subscribeToPredictionsandsubscribeToAlertschannels.iOS
android
Testing
What testing have you done?