From 59ae3094df335565c72e505e418bdf229897fb87 Mon Sep 17 00:00:00 2001 From: Gino Miceli <228050+gino-m@users.noreply.github.com> Date: Fri, 22 Nov 2024 14:46:43 -0500 Subject: [PATCH] Prevent map from freezing while syncing changes to remote survey and LOIs (#2865) * EntityState -> EntityDeletionState * Minor renames * Rename `state` in `SubmissionEntity` * Missed renames * More renames * Inline function * Keep feature manager in sync on clear * Stop fetching jobs on main thread * Optimize map updates --- .../firebase/schema/JobCollectionReference.kt | 18 ++++++------------ .../firebase/schema/SurveyDocumentReference.kt | 9 +++++---- .../repository/LocationOfInterestRepository.kt | 5 ----- .../ground/ui/common/BaseMapViewModel.kt | 3 ++- .../HomeScreenMapContainerViewModel.kt | 11 ++++++++++- .../ground/ui/map/gms/GoogleMapsFragment.kt | 1 + 6 files changed, 24 insertions(+), 23 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt index f7f111066b..4a7ca00470 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt @@ -19,20 +19,14 @@ package com.google.android.ground.persistence.remote.firebase.schema import com.google.android.ground.model.job.Job import com.google.android.ground.persistence.remote.firebase.base.FluentCollectionReference import com.google.firebase.firestore.CollectionReference -import kotlinx.coroutines.channels.awaitClose -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.callbackFlow +import kotlinx.coroutines.tasks.await +import kotlinx.coroutines.withContext class JobCollectionReference internal constructor(ref: CollectionReference) : FluentCollectionReference(ref) { - fun get(): Flow> = callbackFlow { - reference() - .get() - .addOnSuccessListener { trySend(it.documents.map { doc -> JobConverter.toJob(doc) }) } - .addOnFailureListener { trySend(listOf()) } - - awaitClose { - // Cannot cancel or detach listeners here. + suspend fun get(): List = + withContext(ioDispatcher) { + val docs = reference().get().await() + docs.map { doc -> JobConverter.toJob(doc) } } - } } diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt index 0174671238..283659c0eb 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt @@ -19,7 +19,6 @@ package com.google.android.ground.persistence.remote.firebase.schema import com.google.android.ground.model.Survey import com.google.android.ground.persistence.remote.firebase.base.FluentDocumentReference import com.google.firebase.firestore.DocumentReference -import kotlinx.coroutines.flow.firstOrNull import kotlinx.coroutines.tasks.await private const val LOIS = "lois" @@ -36,8 +35,10 @@ class SurveyDocumentReference internal constructor(ref: DocumentReference) : private fun jobs() = JobCollectionReference(reference().collection(JOBS)) suspend fun get(): Survey { - val document = reference().get().await() - val jobs = jobs().get().firstOrNull() ?: listOf() - return SurveyConverter.toSurvey(document, jobs) + val surveyDoc = reference().get().await() + // TODO(https://github.com/google/ground-android/issues/2864): Move jobs fetch to outside this + // DocumentReference class. + val jobs = jobs().get() + return SurveyConverter.toSurvey(surveyDoc, jobs) } } diff --git a/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt b/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt index defe4a8a53..9249fea434 100644 --- a/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/LocationOfInterestRepository.kt @@ -35,7 +35,6 @@ import javax.inject.Inject import javax.inject.Singleton import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.firstOrNull import kotlinx.coroutines.flow.map import timber.log.Timber @@ -144,10 +143,6 @@ constructor( fun getValidLois(survey: Survey): Flow> = localLoiStore.getValidLois(survey) - /** Returns a list of geometries associated with the given [Survey]. */ - suspend fun getAllGeometries(survey: Survey): List = - getValidLois(survey).first().map { it.geometry } - /** Returns a flow of all [LocationOfInterest] within the map bounds (viewport). */ fun getWithinBounds(survey: Survey, bounds: Bounds): Flow> = getValidLois(survey) diff --git a/ground/src/main/java/com/google/android/ground/ui/common/BaseMapViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/common/BaseMapViewModel.kt index 2504d1ede5..b238edc1a7 100644 --- a/ground/src/main/java/com/google/android/ground/ui/common/BaseMapViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/common/BaseMapViewModel.kt @@ -55,6 +55,7 @@ import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.filterNotNull +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.merge @@ -258,7 +259,7 @@ constructor( } // Compute the default viewport which includes all LOIs in the given survey. - val geometries = locationOfInterestRepository.getAllGeometries(survey) + val geometries = locationOfInterestRepository.getValidLois(survey).first().map { it.geometry } return geometries.toBounds()?.let { NewCameraPositionViaBounds(bounds = it, padding = 100) } } diff --git a/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt index 8af6139816..45b57dcde9 100644 --- a/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt @@ -38,14 +38,18 @@ import com.google.android.ground.ui.map.Feature import com.google.android.ground.ui.map.FeatureType import com.google.android.ground.ui.map.isLocationOfInterest import javax.inject.Inject +import kotlin.time.Duration.Companion.milliseconds import kotlinx.collections.immutable.toPersistentSet import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.debounce +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flatMapLatest @@ -127,12 +131,17 @@ internal constructor( // TODO: Since we depend on survey stream from repo anyway, this transformation can be moved // into the repository. + @OptIn(FlowPreview::class) mapLoiFeatures = activeSurvey.flatMapLatest { if (it == null) flowOf(setOf()) else getLocationOfInterestFeatures(it) - .combine(selectedLoiIdFlow, this::updatedLoiSelectedStates) + .debounce(1000.milliseconds) + .distinctUntilChanged() + .combine(selectedLoiIdFlow) { loiFeatures, selectedLoiId -> + updatedLoiSelectedStates(loiFeatures, selectedLoiId) + } } isZoomedInFlow = diff --git a/ground/src/main/java/com/google/android/ground/ui/map/gms/GoogleMapsFragment.kt b/ground/src/main/java/com/google/android/ground/ui/map/gms/GoogleMapsFragment.kt index 73e027db41..0668fa622c 100644 --- a/ground/src/main/java/com/google/android/ground/ui/map/gms/GoogleMapsFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/map/gms/GoogleMapsFragment.kt @@ -304,6 +304,7 @@ class GoogleMapsFragment : SupportMapFragment(), MapFragment { override fun clear() { map.clear() + featureManager.setFeatures(emptySet()) } companion object {