From 38109fb928c201cebd88a3ea0f893612f6e4b14f Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Sat, 16 Nov 2024 08:54:40 -0500 Subject: [PATCH 1/3] Fix typo --- .../ground/persistence/remote/RemotePersistenceModule.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/RemotePersistenceModule.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/RemotePersistenceModule.kt index 94aa3c84db..4dc4488dbf 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/RemotePersistenceModule.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/RemotePersistenceModule.kt @@ -70,6 +70,6 @@ abstract class RemotePersistenceModule { return FirebaseStorage.getInstance().reference } - @Provides @Singleton fun firebaseFuntions(): FirebaseFunctions = Firebase.functions + @Provides @Singleton fun firebaseFunctions(): FirebaseFunctions = Firebase.functions } } From 43f966712e1f45a9daab7646e42d415dfb3002ce Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Sat, 16 Nov 2024 10:44:42 -0500 Subject: [PATCH 2/3] Fix "FirebaseFirestore has already been started" sync failures --- .../firebase/FirebaseFirestoreProvider.kt | 3 ++ .../remote/firebase/FirestoreUuidGenerator.kt | 12 +++---- .../persistence/uuid/OfflineUuidGenerator.kt | 2 +- .../ground/repository/UserMediaRepository.kt | 6 ++-- .../datacollection/DataCollectionViewModel.kt | 2 +- .../tasks/photo/PhotoTaskFragment.kt | 9 ++++-- .../tasks/photo/PhotoTaskViewModel.kt | 2 +- .../tasks/point/DropPinTaskViewModel.kt | 13 +++++--- .../tasks/polygon/DrawAreaTaskViewModel.kt | 32 ++++++++++--------- .../persistence/uuid/FakeUuidGenerator.kt | 2 +- 10 files changed, 47 insertions(+), 36 deletions(-) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseFirestoreProvider.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseFirestoreProvider.kt index c75abeaf64..fbafb8beb5 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseFirestoreProvider.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseFirestoreProvider.kt @@ -26,6 +26,9 @@ import javax.inject.Singleton @Singleton class FirebaseFirestoreProvider @Inject constructor(settings: FirebaseFirestoreSettings) : AsyncSingletonProvider({ + // WARNING: `FirebaseFirestore.getInstance()` should only be called here and nowhere + // else since settings can only be set on first call. Inject FirebaseFirestore instead + // of calling `getInstance()` directly. FirebaseFirestore.getInstance().apply { setFirestoreSettings(this, settings) FirebaseFirestore.setLoggingEnabled(Config.FIRESTORE_LOGGING_ENABLED) diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreUuidGenerator.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreUuidGenerator.kt index 87f4af71a2..d721a90c33 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreUuidGenerator.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreUuidGenerator.kt @@ -16,15 +16,15 @@ package com.google.android.ground.persistence.remote.firebase import com.google.android.ground.persistence.uuid.OfflineUuidGenerator -import com.google.firebase.firestore.FirebaseFirestore import javax.inject.Inject -class FirestoreUuidGenerator @Inject constructor() : OfflineUuidGenerator { +class FirestoreUuidGenerator +@Inject +constructor(private val firebaseFirestoreProvider: FirebaseFirestoreProvider) : + OfflineUuidGenerator { - // TODO: Check if this be replaced with the underlying implementation directly Util.autoId() - // Also remove Fake implementation if the dependency on firebase init is removed. - override fun generateUuid(): String = - FirebaseFirestore.getInstance().collection(ID_COLLECTION).document().id + override suspend fun generateUuid(): String = + firebaseFirestoreProvider.get().collection(ID_COLLECTION).document().id companion object { const val ID_COLLECTION = "/ids" diff --git a/ground/src/main/java/com/google/android/ground/persistence/uuid/OfflineUuidGenerator.kt b/ground/src/main/java/com/google/android/ground/persistence/uuid/OfflineUuidGenerator.kt index 00ad1071b4..458571d44d 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/uuid/OfflineUuidGenerator.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/uuid/OfflineUuidGenerator.kt @@ -25,5 +25,5 @@ interface OfflineUuidGenerator { * Implementations should ensure that the probability of collision is so small to be considered * insignificant. */ - fun generateUuid(): String + suspend fun generateUuid(): String } diff --git a/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt b/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt index 00b98f7312..794328c777 100644 --- a/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt +++ b/ground/src/main/java/com/google/android/ground/repository/UserMediaRepository.kt @@ -50,10 +50,10 @@ constructor( private val rootDir: File? get() = context.getExternalFilesDir(Environment.DIRECTORY_PICTURES) - private fun createImageFilename(fieldId: String): String = + private suspend fun createImageFilename(fieldId: String): String = fieldId + "-" + uuidGenerator.generateUuid() + Config.PHOTO_EXT - fun createImageFile(fieldId: String): File = File(rootDir, createImageFilename(fieldId)) + suspend fun createImageFile(fieldId: String): File = File(rootDir, createImageFilename(fieldId)) /** * Creates a new file from bitmap and saves under external app directory. @@ -61,7 +61,7 @@ constructor( * @throws IOException If path is not accessible or error occurs while saving file */ @Throws(IOException::class) - fun savePhoto(bitmap: Bitmap, fieldId: String): File = + suspend fun savePhoto(bitmap: Bitmap, fieldId: String): File = createImageFile(fieldId).apply { FileOutputStream(this).use { fos -> bitmap.compress(Bitmap.CompressFormat.JPEG, 100, fos) } Timber.d("Photo saved %s : %b", path, exists()) diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt index 0c5d3f38e3..48486e92a8 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt @@ -265,8 +265,8 @@ internal constructor( /** Persists the changes locally and enqueues a worker to sync with remote datastore. */ private fun saveChanges(deltas: List) { - val collectionId = offlineUuidGenerator.generateUuid() externalScope.launch(ioDispatcher) { + val collectionId = offlineUuidGenerator.generateUuid() submitDataUseCase.invoke(loiId, job, surveyId, deltas, customLoiName, collectionId) } } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt index ac97e774b5..ca7f844161 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt @@ -33,6 +33,7 @@ import androidx.compose.runtime.remember import androidx.compose.ui.platform.ComposeView import androidx.compose.ui.res.stringResource import androidx.core.content.FileProvider +import androidx.lifecycle.lifecycleScope import com.google.android.ground.BuildConfig import com.google.android.ground.R import com.google.android.ground.coroutines.ApplicationScope @@ -66,7 +67,7 @@ class PhotoTaskFragment : AbstractTaskFragment() { // Registers a callback to execute after a user captures a photo from the on-device camera. private var capturePhotoLauncher: ActivityResultLauncher = registerForActivityResult(ActivityResultContracts.TakePicture()) { result: Boolean -> - if (result) viewModel.savePhotoTaskData(capturedPhotoUri) + externalScope.launch { if (result) viewModel.savePhotoTaskData(capturedPhotoUri) } } private var hasRequestedPermissionsOnResume = false @@ -169,10 +170,12 @@ class PhotoTaskFragment : AbstractTaskFragment() { } fun onTakePhoto() { - obtainCapturePhotoPermissions { launchPhotoCapture(viewModel.task.id) } + obtainCapturePhotoPermissions { + lifecycleScope.launch { launchPhotoCapture(viewModel.task.id) } + } } - private fun launchPhotoCapture(taskId: String) { + private suspend fun launchPhotoCapture(taskId: String) { try { val photoFile = userMediaRepository.createImageFile(taskId) capturedPhotoUri = diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt index fec3802dd1..f82a5f0b42 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt @@ -67,7 +67,7 @@ constructor( * Saves photo data stored on an on-device URI in Ground-associated storage and prepares it for * inclusion in a data collection submission. */ - fun savePhotoTaskData(uri: Uri) { + suspend fun savePhotoTaskData(uri: Uri) { val currentTask = taskWaitingForPhoto requireNotNull(currentTask) { "Photo captured but no task waiting for the result" } diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropPinTaskViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropPinTaskViewModel.kt index 63a515b244..4a27ca553a 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropPinTaskViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/point/DropPinTaskViewModel.kt @@ -16,6 +16,7 @@ package com.google.android.ground.ui.datacollection.tasks.point import androidx.lifecycle.MutableLiveData +import androidx.lifecycle.viewModelScope import com.google.android.ground.model.geometry.Point import com.google.android.ground.model.job.Job import com.google.android.ground.model.job.getDefaultColor @@ -29,6 +30,7 @@ import com.google.android.ground.ui.map.CameraPosition import com.google.android.ground.ui.map.Feature import com.google.android.ground.ui.map.FeatureType import javax.inject.Inject +import kotlinx.coroutines.launch class DropPinTaskViewModel @Inject @@ -66,13 +68,14 @@ constructor( dropMarker(point) } - private fun dropMarker(point: Point) { - val feature = createFeature(point) - features.postValue(setOf(feature)) - } + private fun dropMarker(point: Point) = + viewModelScope.launch { + val feature = createFeature(point) + features.postValue(setOf(feature)) + } /** Creates a new map [Feature] representing the point placed by the user. */ - private fun createFeature(point: Point): Feature = + private suspend fun createFeature(point: Point): Feature = Feature( id = uuidGenerator.generateUuid(), type = FeatureType.USER_POINT.ordinal, diff --git a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/polygon/DrawAreaTaskViewModel.kt b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/polygon/DrawAreaTaskViewModel.kt index 181d5d6f91..12cc634293 100644 --- a/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/polygon/DrawAreaTaskViewModel.kt +++ b/ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/polygon/DrawAreaTaskViewModel.kt @@ -39,6 +39,7 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.launch import timber.log.Timber @SharedViewModel @@ -178,21 +179,22 @@ internal constructor( } /** Updates the [Feature] drawn on map based on the value of [vertices]. */ - private fun refreshMap() { - _draftArea.value = - if (vertices.isEmpty()) { - null - } else { - Feature( - id = uuidGenerator.generateUuid(), - type = FeatureType.USER_POLYGON.ordinal, - geometry = LineString(vertices), - style = Feature.Style(strokeColor, Feature.VertexStyle.CIRCLE), - clusterable = false, - selected = true, - ) - } - } + private fun refreshMap() = + viewModelScope.launch { + _draftArea.value = + if (vertices.isEmpty()) { + null + } else { + Feature( + id = uuidGenerator.generateUuid(), + type = FeatureType.USER_POLYGON.ordinal, + geometry = LineString(vertices), + style = Feature.Style(strokeColor, Feature.VertexStyle.CIRCLE), + clusterable = false, + selected = true, + ) + } + } override fun validate(task: Task, taskData: TaskData?): Int? { // Invalid response for draw area task. diff --git a/sharedTest/src/main/kotlin/com/sharedtest/persistence/uuid/FakeUuidGenerator.kt b/sharedTest/src/main/kotlin/com/sharedtest/persistence/uuid/FakeUuidGenerator.kt index 6f33601d26..cbcace1e1b 100644 --- a/sharedTest/src/main/kotlin/com/sharedtest/persistence/uuid/FakeUuidGenerator.kt +++ b/sharedTest/src/main/kotlin/com/sharedtest/persistence/uuid/FakeUuidGenerator.kt @@ -19,5 +19,5 @@ import com.google.android.ground.persistence.uuid.OfflineUuidGenerator import javax.inject.Inject class FakeUuidGenerator @Inject internal constructor() : OfflineUuidGenerator { - override fun generateUuid(): String = "TEST UUID" + override suspend fun generateUuid(): String = "TEST UUID" } From 2514fb02c614a307ef6ca4df245dc9d7c425aed1 Mon Sep 17 00:00:00 2001 From: Gino Miceli Date: Sat, 16 Nov 2024 11:09:07 -0500 Subject: [PATCH 3/3] Fix tests --- .../android/ground/persistence/sync/MediaUploadWorkerTest.kt | 2 +- .../ground/ui/datacollection/DataCollectionFragmentTest.kt | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt b/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt index 2063059e14..15df16a234 100644 --- a/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt +++ b/ground/src/test/java/com/google/android/ground/persistence/sync/MediaUploadWorkerTest.kt @@ -184,7 +184,7 @@ class MediaUploadWorkerTest : BaseHiltTest() { private suspend fun addSubmissionMutationToLocalStorage(status: Mutation.SyncStatus) = localSubmissionStore.applyAndEnqueue(createSubmissionMutation().copy(syncStatus = status)) - private fun createSubmissionMutation(photoName: String? = null): SubmissionMutation { + private suspend fun createSubmissionMutation(photoName: String? = null): SubmissionMutation { val photo = userMediaRepository.savePhoto( Bitmap.createBitmap(1, 1, Bitmap.Config.HARDWARE), diff --git a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt index f08a112fac..692fcc9c4a 100644 --- a/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt +++ b/ground/src/test/java/com/google/android/ground/ui/datacollection/DataCollectionFragmentTest.kt @@ -43,6 +43,7 @@ import dagger.hilt.android.testing.HiltAndroidTest import javax.inject.Inject import kotlinx.collections.immutable.persistentListOf import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.advanceUntilIdle import org.junit.Test import org.junit.runner.RunWith @@ -68,7 +69,7 @@ class DataCollectionFragmentTest : BaseHiltTest() { @Inject lateinit var uuidGenerator: OfflineUuidGenerator lateinit var collectionId: String - override fun setUp() { + override fun setUp() = runBlocking { super.setUp() collectionId = uuidGenerator.generateUuid()