diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index c89f51015..6927a26a7 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -7,8 +7,10 @@ What is this PR for? iOS - [ ] If you added any user-facing strings on iOS, are they included in Localizable.xcstrings? - [ ] Add temporary machine translations, marked "Needs Review" + android - [ ] All user-facing strings added to strings resource +- [ ] Expensive calculations are run in `withContext(Dispatchers.Default)` where possible ### Testing diff --git a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/HomeMapView.kt b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/HomeMapView.kt index 107bcd65a..fcc0c3d64 100644 --- a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/HomeMapView.kt +++ b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/HomeMapView.kt @@ -12,6 +12,7 @@ import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -63,6 +64,7 @@ import com.mbta.tid.mbta_app.model.response.StopMapResponse import io.github.dellisd.spatialk.geojson.Position import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.flow.map +import kotlinx.coroutines.launch @OptIn(MapboxExperimental::class, ExperimentalPermissionsApi::class) @Composable @@ -82,6 +84,7 @@ fun HomeMapView( var nearbyTransitSelectingLocation by nearbyTransitSelectingLocationState val previousNavEntry: NavBackStackEntry? = rememberPrevious(current = currentNavEntry) + val coroutineScope = rememberCoroutineScope() val layerManager = remember { LazyObjectQueue() } var selectedStop by remember { mutableStateOf(null) } @@ -128,7 +131,7 @@ fun HomeMapView( viewportProvider.animateTo(stop.position.toMapbox()) } - fun updateDisplayedRoutesBasedOnStop() { + suspend fun updateDisplayedRoutesBasedOnStop() { val globalResponse = globalResponse ?: return val railRouteShapes = railRouteShapes ?: return val stopMapData = stopMapData ?: return @@ -159,19 +162,19 @@ fun HomeMapView( } } - fun refreshRouteLineSource() { + suspend fun refreshRouteLineSource() { val routeData = railRouteLineData ?: return layerManager.run { updateRouteSourceData(RouteFeaturesBuilder.buildCollection(routeData).toMapbox()) } } - fun refreshStopSource() { + suspend fun refreshStopSource() { val sourceData = stopSourceData ?: return layerManager.run { updateStopSourceData(sourceData) } } - fun handleNearbyNavRestoration() { + suspend fun handleNearbyNavRestoration() { if ( previousNavEntry?.destination?.route?.contains("NearbyTransit") == true && currentNavEntry?.destination?.route?.contains("StopDetails") == true @@ -186,7 +189,7 @@ fun HomeMapView( } } - fun handleNavChange() { + suspend fun handleNavChange() { handleNearbyNavRestoration() val stopId = currentNavEntry?.arguments?.getString("stopId") if (stopId == null) { @@ -211,8 +214,10 @@ fun HomeMapView( mapEvents = MapEvents( onStyleLoaded = { - layerManager.run { - addLayers(if (isDarkMode) ColorPalette.dark else ColorPalette.light) + coroutineScope.launch { + layerManager.run { + addLayers(if (isDarkMode) ColorPalette.dark else ColorPalette.light) + } } }, onCameraChanged = { viewportProvider.updateCameraState(it.cameraState) } @@ -299,8 +304,9 @@ fun HomeMapView( } MapEffect { map -> - if (layerManager.`object` == null) - layerManager.`object` = MapLayerManager(map.mapboxMap, context) + if (layerManager.`object` == null) { + layerManager.setObject(MapLayerManager(map.mapboxMap, context)) + } } if ( diff --git a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapLayerManager.kt b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapLayerManager.kt index 143c909ea..63f5a9485 100644 --- a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapLayerManager.kt +++ b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapLayerManager.kt @@ -18,6 +18,8 @@ import com.mbta.tid.mbta_app.map.RouteLayerGenerator import com.mbta.tid.mbta_app.map.StopFeaturesBuilder import com.mbta.tid.mbta_app.map.StopIcons import com.mbta.tid.mbta_app.map.StopLayerGenerator +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext class MapLayerManager(val map: MapboxMap, context: Context) { init { @@ -28,23 +30,27 @@ class MapLayerManager(val map: MapboxMap, context: Context) { } } - fun addSource(source: GeoJsonSource) { - this.map.addSource(source) + suspend fun addSource(source: GeoJsonSource) { + withContext(Dispatchers.Main) { map.addSource(source) } } - fun addLayers(colorPalette: ColorPalette) { + suspend fun addLayers(colorPalette: ColorPalette) { val layers: List = - RouteLayerGenerator.createAllRouteLayers(colorPalette).map { it.toMapbox() } + - StopLayerGenerator.createStopLayers(colorPalette).map { it.toMapbox() } - for (layer in layers) { - if (map.styleLayerExists(checkNotNull(layer.layerId))) { - // Skip attempting to add layer if it already exists - continue + withContext(Dispatchers.Default) { + RouteLayerGenerator.createAllRouteLayers(colorPalette).map { it.toMapbox() } + + StopLayerGenerator.createStopLayers(colorPalette).map { it.toMapbox() } } - if (map.styleLayerExists("puck")) { - map.addLayerBelow(layer, below = "puck") - } else { - map.addLayer(layer) + withContext(Dispatchers.Main) { + for (layer in layers) { + if (map.styleLayerExists(checkNotNull(layer.layerId))) { + // Skip attempting to add layer if it already exists + continue + } + if (map.styleLayerExists("puck")) { + map.addLayerBelow(layer, below = "puck") + } else { + map.addLayer(layer) + } } } } @@ -55,24 +61,27 @@ class MapLayerManager(val map: MapboxMap, context: Context) { } } - private fun updateSourceData(sourceId: String, data: FeatureCollection) { - if (map.styleSourceExists(sourceId)) { - map.setStyleGeoJSONSourceData( - sourceId, - "", - GeoJSONSourceData(checkNotNull(data.features())) - ) + private suspend fun updateSourceData(sourceId: String, data: FeatureCollection) { + // styleSourceExists is not thread safe, but setStyleGeoJSONSourceData is + if (withContext(Dispatchers.Main) { map.styleSourceExists(sourceId) }) { + withContext(Dispatchers.Default) { + map.setStyleGeoJSONSourceData( + sourceId, + "", + GeoJSONSourceData(checkNotNull(data.features())) + ) + } } else { val source = GeoJsonSource.Builder(sourceId).featureCollection(data).build() addSource(source) } } - fun updateRouteSourceData(routeData: FeatureCollection) { + suspend fun updateRouteSourceData(routeData: FeatureCollection) { updateSourceData(RouteFeaturesBuilder.routeSourceId, routeData) } - fun updateStopSourceData(stopData: FeatureCollection) { + suspend fun updateStopSourceData(stopData: FeatureCollection) { updateSourceData(StopFeaturesBuilder.stopSourceId, stopData) } } diff --git a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapViewModel.kt b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapViewModel.kt index 2e062bb7f..0ca91590f 100644 --- a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapViewModel.kt +++ b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/map/MapViewModel.kt @@ -1,11 +1,9 @@ package com.mbta.tid.mbta_app.android.map import androidx.compose.runtime.Composable -import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModelProvider @@ -13,6 +11,7 @@ import androidx.lifecycle.viewModelScope import com.mapbox.common.HttpServiceFactory import com.mapbox.common.MapboxOptions import com.mapbox.geojson.FeatureCollection +import com.mbta.tid.mbta_app.android.util.rememberSuspend import com.mbta.tid.mbta_app.dependencyInjection.UsecaseDI import com.mbta.tid.mbta_app.map.RouteFeaturesBuilder import com.mbta.tid.mbta_app.map.RouteLineData @@ -35,6 +34,7 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import kotlinx.datetime.Clock import kotlinx.datetime.Instant import org.koin.core.component.KoinComponent @@ -103,18 +103,16 @@ open class MapViewModel( } override suspend fun globalMapData(now: Instant): GlobalMapData? = - globalResponse.first()?.let { - GlobalMapData(it, GlobalMapData.getAlertsByStop(it, alertsData, now)) + withContext(Dispatchers.Default) { + globalResponse.first()?.let { + GlobalMapData(it, GlobalMapData.getAlertsByStop(it, alertsData, now)) + } } @Composable override fun rememberGlobalMapData(now: Instant): GlobalMapData? { - var result by remember { mutableStateOf(null) } val globalResponse by this.globalResponse.collectAsState(initial = null) - LaunchedEffect(this.alertsData, globalResponse, now) { - launch(Dispatchers.IO) { result = globalMapData(now) } - } - return result + return rememberSuspend(this.alertsData, globalResponse, now) { globalMapData(now) } } override suspend fun refreshRouteLineData(now: Instant) { diff --git a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/nearbyTransit/NearbyTransitView.kt b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/nearbyTransit/NearbyTransitView.kt index 03be401db..0eadc2d7c 100644 --- a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/nearbyTransit/NearbyTransitView.kt +++ b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/nearbyTransit/NearbyTransitView.kt @@ -22,6 +22,7 @@ import com.mbta.tid.mbta_app.android.state.getNearby import com.mbta.tid.mbta_app.android.state.getSchedule import com.mbta.tid.mbta_app.android.state.subscribeToPredictions import com.mbta.tid.mbta_app.android.util.managePinnedRoutes +import com.mbta.tid.mbta_app.android.util.rememberSuspend import com.mbta.tid.mbta_app.android.util.timer import com.mbta.tid.mbta_app.model.Coordinate import com.mbta.tid.mbta_app.model.NearbyStaticData @@ -32,6 +33,8 @@ import com.mbta.tid.mbta_app.model.response.GlobalResponse import com.mbta.tid.mbta_app.model.withRealtimeInfo import io.github.dellisd.spatialk.geojson.Position import kotlin.time.Duration.Companion.seconds +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext @Composable fun NearbyTransitView( @@ -58,7 +61,7 @@ fun NearbyTransitView( val (pinnedRoutes, togglePinnedRoute) = managePinnedRoutes() val nearbyWithRealtimeInfo = - remember( + rememberSuspend( nearby, globalResponse, targetLocation, @@ -68,19 +71,21 @@ fun NearbyTransitView( now, pinnedRoutes ) { - if (targetLocation != null) { - nearby?.withRealtimeInfo( - globalData = globalResponse, - sortByDistanceFrom = targetLocation, - schedules, - predictions, - alertData, - now, - pinnedRoutes.orEmpty(), - useTripHeadsigns = false, - ) - } else { - null + withContext(Dispatchers.Default) { + if (targetLocation != null) { + nearby?.withRealtimeInfo( + globalData = globalResponse, + sortByDistanceFrom = targetLocation, + schedules, + predictions, + alertData, + now, + pinnedRoutes.orEmpty(), + useTripHeadsigns = false, + ) + } else { + null + } } } diff --git a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/pages/StopDetailsPage.kt b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/pages/StopDetailsPage.kt index 36e4b76a6..a26d7923b 100644 --- a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/pages/StopDetailsPage.kt +++ b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/pages/StopDetailsPage.kt @@ -3,7 +3,6 @@ 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.remember import androidx.compose.ui.Modifier import com.mapbox.maps.MapboxExperimental import com.mbta.tid.mbta_app.android.state.getGlobalData @@ -11,12 +10,15 @@ import com.mbta.tid.mbta_app.android.state.getSchedule import com.mbta.tid.mbta_app.android.state.subscribeToPredictions import com.mbta.tid.mbta_app.android.stopDetails.StopDetailsView import com.mbta.tid.mbta_app.android.util.managePinnedRoutes +import com.mbta.tid.mbta_app.android.util.rememberSuspend import com.mbta.tid.mbta_app.android.util.timer import com.mbta.tid.mbta_app.model.Stop import com.mbta.tid.mbta_app.model.StopDetailsDepartures import com.mbta.tid.mbta_app.model.StopDetailsFilter import com.mbta.tid.mbta_app.model.response.AlertsStreamDataResponse import kotlin.time.Duration.Companion.seconds +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext @Composable @ExperimentalMaterial3Api @@ -41,7 +43,7 @@ fun StopDetailsPage( val (pinnedRoutes, togglePinnedRoute) = managePinnedRoutes() val departures = - remember( + rememberSuspend( stop, globalResponse, schedulesResponse, @@ -50,18 +52,20 @@ fun StopDetailsPage( pinnedRoutes, now ) { - if (globalResponse != null) { - StopDetailsDepartures.fromData( - stop, - globalResponse, - schedulesResponse, - predictionsResponse, - alertData, - pinnedRoutes.orEmpty(), - now, - useTripHeadsigns = false, - ) - } else null + withContext(Dispatchers.Default) { + if (globalResponse != null) { + StopDetailsDepartures.fromData( + stop, + globalResponse, + schedulesResponse, + predictionsResponse, + alertData, + pinnedRoutes.orEmpty(), + now, + useTripHeadsigns = false, + ) + } else null + } } LaunchedEffect(departures) { updateDepartures(departures) } diff --git a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/util/LazyObjectQueue.kt b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/util/LazyObjectQueue.kt index 5d7dd99dd..d36bf4a5d 100644 --- a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/util/LazyObjectQueue.kt +++ b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/util/LazyObjectQueue.kt @@ -6,20 +6,22 @@ package com.mbta.tid.mbta_app.android.util */ class LazyObjectQueue { var `object`: T? = null - set(value) { - field = value - if (value != null) { - val oldQueue = queue - queue = mutableListOf() - for (op in oldQueue) { - value.op() - } + private set + + suspend fun setObject(value: T?) { + `object` = value + if (value != null) { + val oldQueue = queue + queue = mutableListOf() + for (op in oldQueue) { + value.op() } } + } - private var queue: MutableList Unit> = mutableListOf() + private var queue: MutableList Unit> = mutableListOf() - fun run(op: T.() -> Unit) { + suspend fun run(op: suspend T.() -> Unit) { val `object` = this.`object` if (`object` != null) { `object`.op() diff --git a/androidApp/src/main/java/com/mbta/tid/mbta_app/android/util/rememberSuspend.kt b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/util/rememberSuspend.kt new file mode 100644 index 000000000..eca533835 --- /dev/null +++ b/androidApp/src/main/java/com/mbta/tid/mbta_app/android/util/rememberSuspend.kt @@ -0,0 +1,25 @@ +package com.mbta.tid.mbta_app.android.util + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisallowComposableCalls +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue + +/** + * Wraps the common pattern of a [remember]ed [mutableStateOf] that is computed with a `suspend + * fun`. + */ +@Composable +inline fun rememberSuspend( + vararg keys: Any?, + crossinline computation: @DisallowComposableCalls suspend () -> T? +): T? { + var state by remember { mutableStateOf(null) } + + LaunchedEffect(*keys) { state = computation() } + + return state +}