Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Android): reset nearby transit state on pan/recenter #635

Merged
merged 5 commits into from
Jan 10, 2025
+35 −12
Merged

fix(Android): reset nearby transit state on pan/recenter #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4edcb83
fix(Android): reset nearby transit state on pan/recenter
boringcactus Jan 9, 2025
b461a63
only reset predictions if actually needed
boringcactus Jan 9, 2025
07ba4c5
Merge branch 'main' into mth-android-reset-nearby
boringcactus Jan 9, 2025
27c34a5
expose PredictionsViewModel directly for explicit resets
boringcactus Jan 10, 2025
4d6e2b3
Merge branch 'main' into mth-android-reset-nearby
boringcactus Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions ...pp/src/androidTest/java/com/mbta/tid/mbta_app/android/state/SubscribeToPredictionsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.mbta.tid.mbta_app.android.state

import androidx.activity.ComponentActivity
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
Expand Down Expand Up @@ -52,7 +53,7 @@ class SubscribeToPredictionsTest {

composeTestRule.setContent {
var stopIds by remember { stopIds }
predictions =
val predictionsVM =
subscribeToPredictions(
stopIds,
predictionsRepo,
Expand All @@ -62,6 +63,7 @@ class SubscribeToPredictionsTest {
MockSettingsRepository()
)
)
predictions = predictionsVM.predictionsFlow.collectAsState(initial = null).value
}

composeTestRule.waitUntil { connectProps == listOf("place-a") }
Expand Down Expand Up @@ -102,7 +104,7 @@ class SubscribeToPredictionsTest {
composeTestRule.setContent {
CompositionLocalProvider(LocalLifecycleOwner provides lifecycleOwner) {
var stopIds by remember { stopIds }
predictions =
val predictionsVM =
subscribeToPredictions(
stopIds,
predictionsRepo,
Expand All @@ -112,6 +114,7 @@ class SubscribeToPredictionsTest {
MockSettingsRepository()
)
)
predictions = predictionsVM.predictionsFlow.collectAsState(initial = null).value
}
}

Expand Down Expand Up @@ -145,7 +148,7 @@ class SubscribeToPredictionsTest {
var predictions: PredictionsStreamDataResponse? = null

composeTestRule.setContent {
predictions =
val predictionsVM =
subscribeToPredictions(
emptyList(),
predictionsRepo,
Expand All @@ -155,6 +158,7 @@ class SubscribeToPredictionsTest {
MockSettingsRepository()
)
)
predictions = predictionsVM.predictionsFlow.collectAsState(initial = null).value
}

composeTestRule.waitUntil { predictions != null }
Expand Down
10 changes: 9 additions & 1 deletion androidApp/src/main/java/com/mbta/tid/mbta_app/android/nearbyTransit/NearbyTransitView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
Expand Down Expand Up @@ -65,7 +67,13 @@ fun NearbyTransitView(
val now = timer(updateInterval = 5.seconds)
val stopIds = remember(nearbyVM.nearby) { nearbyVM.nearby?.stopIds()?.toList() }
val schedules = getSchedule(stopIds)
val predictions = subscribeToPredictions(stopIds, errorBannerViewModel = errorBannerViewModel)
val predictionsVM = subscribeToPredictions(stopIds, errorBannerViewModel = errorBannerViewModel)
val predictions by predictionsVM.predictionsFlow.collectAsState(initial = null)
LaunchedEffect(targetLocation == null) {
if (targetLocation == null) {
predictionsVM.reset()
}
}

val (pinnedRoutes, togglePinnedRoute) = managePinnedRoutes()

Expand Down
8 changes: 6 additions & 2 deletions androidApp/src/main/java/com/mbta/tid/mbta_app/android/pages/NearbyTransitPage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import com.mbta.tid.mbta_app.android.map.IMapViewModel
import com.mbta.tid.mbta_app.android.map.MapViewModel
import com.mbta.tid.mbta_app.android.nearbyTransit.NearbyTransitTabViewModel
import com.mbta.tid.mbta_app.android.nearbyTransit.NearbyTransitView
import com.mbta.tid.mbta_app.android.nearbyTransit.NearbyTransitViewModel
import com.mbta.tid.mbta_app.android.nearbyTransit.NoNearbyStopsView
import com.mbta.tid.mbta_app.android.search.SearchBarOverlay
import com.mbta.tid.mbta_app.android.state.subscribeToVehicles
Expand All @@ -64,6 +65,7 @@ import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.FlowPreview
import kotlinx.coroutines.flow.debounce
import kotlinx.coroutines.launch
import org.koin.androidx.compose.koinViewModel
import org.koin.compose.koinInject

@OptIn(ExperimentalMaterial3Api::class)
Expand Down Expand Up @@ -216,6 +218,8 @@ fun NearbyTransitPage(
}
}
composable<SheetRoutes.NearbyTransit> {
// for ViewModel reasons, must be within the `composable` to be the same instance
val nearbyViewModel: NearbyTransitViewModel = koinViewModel()
Comment on lines 220 to +222
Copy link
Member Author

@boringcactus boringcactus Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, I had nearbyViewModel: NearbyTransitViewModel = koinViewModel() as an argument to NearbyTransitPage, but since navigation states can own ViewModels, calling koinViewModel() outside this composable creates a different instance than the one that's created as an argument in NearbyTransitView, but calling it here gives the same instance. This was absolutely miserable to try to debug.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is tricky. To clarify my own understanding, that would be an issue even if we weren't using koin and were instantiating the view models directly, is that right? I haven't been using koinViewModel regularly, and wondering about differences / benefits we get with it aside from test defaults.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this is inherent to the Android ViewModel subsystem and not specific to Koin's integration with it. My understanding is that using koinViewModel lets us pass repositories via Koin into the ViewModel without having to define a custom ViewModelProvider.Factory or deal with CreationExtras.

LaunchedEffect(true) {
if (!navBarVisible) {
showNavBar()
Expand All @@ -234,13 +238,13 @@ fun NearbyTransitPage(
}
LaunchedEffect(nearbyTransit.viewportProvider.isManuallyCentering) {
if (nearbyTransit.viewportProvider.isManuallyCentering) {
// TODO reset view model
nearbyViewModel.reset()
Comment on lines -237 to +241
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love a good TODO -> done

targetLocation = null
}
}
LaunchedEffect(nearbyTransit.viewportProvider.isFollowingPuck) {
if (nearbyTransit.viewportProvider.isFollowingPuck) {
// TODO reset view model
nearbyViewModel.reset()
targetLocation = null
}
}
Expand Down
5 changes: 4 additions & 1 deletion androidApp/src/main/java/com/mbta/tid/mbta_app/android/pages/StopDetailsPage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package com.mbta.tid.mbta_app.android.pages
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.ui.Modifier
import com.mapbox.maps.MapboxExperimental
import com.mbta.tid.mbta_app.android.component.ErrorBannerViewModel
Expand Down Expand Up @@ -36,11 +38,12 @@ fun StopDetailsPage(
) {
val globalResponse = getGlobalData()

val predictionsResponse =
val predictionsVM =
subscribeToPredictions(
stopIds = listOf(stop.id),
errorBannerViewModel = errorBannerViewModel
)
val predictionsResponse by predictionsVM.predictionsFlow.collectAsState(initial = null)

val now = timer(updateInterval = 5.seconds)

Expand Down
4 changes: 3 additions & 1 deletion androidApp/src/main/java/com/mbta/tid/mbta_app/android/phoenix/PhoenixSocketWrapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ value class PhoenixSocketWrapper(private val socket: Socket) : PhoenixSocket {

fun attachLogging() {
socket.onMessage { message -> Log.i("Socket", message.toString()) }
socket.onError { throwable, response -> Log.e("Socket", response.toString(), throwable) }
socket.onError { throwable, response ->
Log.e("Socket", response?.toString() ?: throwable.toString(), throwable)
}
boringcactus marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
10 changes: 6 additions & 4 deletions androidApp/src/main/java/com/mbta/tid/mbta_app/android/state/subscribeToPredictions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.mbta.tid.mbta_app.android.state

import android.util.Log
import androidx.compose.runtime.Composable
import androidx.compose.runtime.collectAsState
import androidx.lifecycle.ViewModel
import androidx.lifecycle.ViewModelProvider
import androidx.lifecycle.compose.LifecycleResumeEffect
Expand All @@ -11,7 +10,6 @@ import com.mbta.tid.mbta_app.android.component.ErrorBannerViewModel
import com.mbta.tid.mbta_app.model.response.ApiResult
import com.mbta.tid.mbta_app.model.response.PredictionsByStopJoinResponse
import com.mbta.tid.mbta_app.model.response.PredictionsByStopMessageResponse
import com.mbta.tid.mbta_app.model.response.PredictionsStreamDataResponse
import com.mbta.tid.mbta_app.repositories.IPredictionsRepository
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.CoroutineScope
Expand Down Expand Up @@ -86,6 +84,10 @@ class PredictionsViewModel(
}
}

fun reset() {
_predictions.value = null
}

fun disconnect() {
predictionsRepository.disconnect()
errorBannerViewModel.loadingWhenPredictionsStale = true
Expand Down Expand Up @@ -119,7 +121,7 @@ fun subscribeToPredictions(
stopIds: List<String>?,
predictionsRepository: IPredictionsRepository = koinInject(),
errorBannerViewModel: ErrorBannerViewModel
): PredictionsStreamDataResponse? {
): PredictionsViewModel {
val viewModel: PredictionsViewModel =
viewModel(
factory = PredictionsViewModel.Factory(predictionsRepository, errorBannerViewModel)
Expand All @@ -133,5 +135,5 @@ fun subscribeToPredictions(

onPauseOrDispose { viewModel.disconnect() }
}
return viewModel.predictionsFlow.collectAsState(initial = null).value
return viewModel
}
Loading