[Refactor] Simplify home screen features and cards update logic (#1857)

* Move MapConfig to VM * Update viewport when map camera is moved * Store last and current camera position in base view model * Create a new method refreshMapFeatures and move logic for updating map features into it * Refactor loisWithinMapBoundsAtVisibleZoomLevel * Refactor suggestLoiJobs * Cleanup unused Flowable * Refresh LOIs on camera update * Remove duplicate call to notify that map has been moved. * Move adding tile sources for offline area selector to fragment * Add MapConfig for disabling map gestures * Rename var * Apply suggestions * Apply suggestions * Fix merge conflicts --------- Co-authored-by: Gino Miceli <228050+gino-m@users.noreply.github.com>
google · Sep 11, 2023 · 779c72e · 779c72e
1 parent 59e4c9f
commit 779c72e
Show file tree

Hide file tree

Showing 12 changed files with 125 additions and 132 deletions.
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
@@ -165,13 +165,9 @@ constructor(
   /** Returns a flowable of all [LocationOfInterest] within the map bounds (viewport). */
   fun getWithinBoundsOnceAndStream(
     survey: Survey,
-    cameraBoundUpdates: Flowable<Bounds>
+    bounds: Bounds
   ): Flowable<List<LocationOfInterest>> =
-    cameraBoundUpdates
-      .switchMap { bounds ->
-        getLocationsOfInterestOnceAndStream(survey).map { lois ->
-          lois.filter { bounds.contains(it.geometry) }
-        }
-      }
+    getLocationsOfInterestOnceAndStream(survey)
+      .map { lois -> lois.filter { bounds.contains(it.geometry) } }
       .distinctUntilChanged()
 }
diff --git a/ground/src/main/java/com/google/android/ground/ui/common/AbstractMapContainerFragment.kt b/ground/src/main/java/com/google/android/ground/ui/common/AbstractMapContainerFragment.kt
@@ -75,22 +75,30 @@ abstract class AbstractMapContainerFragment : AbstractFragment() {
   }
 
   private fun applyMapConfig(map: Map) {
-    val config = getMapConfig()
+    val viewModel = getMapViewModel()
+    val config = viewModel.mapConfig
 
-    // Map Type
+    // Map type
     if (config.overrideMapType != null) {
       map.mapType = config.overrideMapType
     } else {
-      getMapViewModel().mapType.observe(viewLifecycleOwner) { map.mapType = it }
+      viewModel.mapType.observe(viewLifecycleOwner) { map.mapType = it }
     }
 
     // Tile overlays.
-    if (getMapConfig().showOfflineTileOverlays) {
-      getMapViewModel().offlineTileSources.observe(viewLifecycleOwner) {
+    if (config.showOfflineTileOverlays) {
+      viewModel.offlineTileSources.observe(viewLifecycleOwner) {
         map.clearTileOverlays()
         it.forEach(map::addTileOverlay)
       }
     }
+
+    // Map gestures
+    if (config.disableGestures) {
+      map.disableGestures()
+    } else {
+      map.enableGestures()
+    }
   }
 
   /** Opens a dialog for selecting a [MapType] for the basemap layer. */
@@ -141,10 +149,6 @@ abstract class AbstractMapContainerFragment : AbstractFragment() {
     } else {
       error("Must have either target or bounds set")
     }
-
-    // Manually notify that the camera has moved as `map.cameraMovedEvents` only returns
-    // an event when the map is moved by the user (REASON_GESTURE).
-    onMapCameraMoved(newPosition)
   }
 
   /** Called when the map camera is moved by the user or due to current location/survey changes. */
@@ -153,17 +157,8 @@ abstract class AbstractMapContainerFragment : AbstractFragment() {
   }
 
   /** Called when the map is attached to the fragment. */
-  protected abstract fun onMapReady(map: Map)
+  protected open fun onMapReady(map: Map) {}
 
   /** Provides an implementation of [BaseMapViewModel]. */
   protected abstract fun getMapViewModel(): BaseMapViewModel
-
-  // TODO: Should this be moved to BaseMapViewModel?
-  /** Configuration to enable/disable base map features. */
-  protected open fun getMapConfig(): MapConfig = DEFAULT_MAP_CONFIG
-
-  companion object {
-    private val DEFAULT_MAP_CONFIG: MapConfig =
-      MapConfig(showOfflineTileOverlays = true, overrideMapType = null)
-  }
 }
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
@@ -37,15 +37,10 @@ import com.google.android.ground.system.LocationManager
 import com.google.android.ground.system.PermissionDeniedException
 import com.google.android.ground.system.PermissionsManager
 import com.google.android.ground.system.SettingsManager
-import com.google.android.ground.ui.map.Bounds
 import com.google.android.ground.ui.map.CameraPosition
 import com.google.android.ground.ui.map.MapType
 import com.google.android.ground.ui.map.gms.GmsExt.toBounds
 import com.google.android.ground.ui.map.gms.toCoordinates
-import io.reactivex.BackpressureStrategy
-import io.reactivex.Flowable
-import io.reactivex.subjects.PublishSubject
-import io.reactivex.subjects.Subject
 import javax.inject.Inject
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.flow.Flow
@@ -75,12 +70,6 @@ constructor(
 ) : AbstractViewModel() {
 
   private val _cameraPosition = MutableStateFlow<CameraPosition?>(null)
-  private val cameraZoomSubject: @Hot Subject<Float> = PublishSubject.create()
-  val cameraZoomUpdates: Flowable<Float> = cameraZoomSubject.toFlowable(BackpressureStrategy.LATEST)
-
-  private val cameraBoundsSubject: @Hot Subject<Bounds> = PublishSubject.create()
-  val cameraBoundUpdates: Flowable<Bounds> =
-    cameraBoundsSubject.toFlowable(BackpressureStrategy.LATEST)
 
   val locationLock: MutableStateFlow<Result<Boolean>> =
     MutableStateFlow(Result.success(mapStateRepository.isLocationLockEnabled))
@@ -115,6 +104,16 @@ constructor(
 
   val offlineTileSources: LiveData<List<TileSource>>
 
+  /** Configuration to enable/disable base map features. */
+  open val mapConfig: MapConfig = DEFAULT_MAP_CONFIG
+
+  /** Current camera position. */
+  val currentCameraPosition: CameraPosition? = _cameraPosition.value
+
+  /** Last camera position. */
+  var lastCameraPosition: CameraPosition? = null
+    private set
+
   init {
     mapType = mapStateRepository.mapTypeFlowable.toLiveData()
     offlineTileSources =
@@ -230,14 +229,12 @@ constructor(
   }
 
   private fun updatePosition(cameraPosition: CameraPosition) {
+    lastCameraPosition = _cameraPosition.value
     _cameraPosition.value = cameraPosition
   }
 
   /** Called when the map camera is moved. */
-  open fun onMapCameraMoved(newCameraPosition: CameraPosition) {
-    newCameraPosition.zoomLevel?.let { cameraZoomSubject.onNext(it) }
-    newCameraPosition.bounds?.let { cameraBoundsSubject.onNext(it) }
-  }
+  open fun onMapCameraMoved(newCameraPosition: CameraPosition) {}
 
   companion object {
     private val LOCATION_LOCK_ICON_TINT_ENABLED = R.color.md_theme_primary
@@ -246,5 +243,7 @@ constructor(
     // TODO(#1789): Consider adding another icon for representing "GPS disabled" state.
     private val LOCATION_LOCK_ICON_ENABLED = R.drawable.ic_gps_lock
     private val LOCATION_LOCK_ICON_DISABLED = R.drawable.ic_gps_lock_not_fixed
+
+    private val DEFAULT_MAP_CONFIG: MapConfig = MapConfig(showOfflineTileOverlays = true)
   }
 }
diff --git a/ground/src/main/java/com/google/android/ground/ui/common/MapConfig.kt b/ground/src/main/java/com/google/android/ground/ui/common/MapConfig.kt
@@ -18,4 +18,8 @@ package com.google.android.ground.ui.common
 import com.google.android.ground.ui.map.MapType
 
 /** Configuration to apply on the rendered base map. */
-data class MapConfig(val showOfflineTileOverlays: Boolean, val overrideMapType: MapType?)
+data class MapConfig(
+  val showOfflineTileOverlays: Boolean,
+  val overrideMapType: MapType? = null,
+  val disableGestures: Boolean = false
+)
diff --git a/...ain/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragment.kt b/...ain/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerFragment.kt
@@ -19,7 +19,6 @@ import android.os.Bundle
 import android.view.LayoutInflater
 import android.view.View
 import android.view.ViewGroup
-import androidx.lifecycle.asFlow
 import androidx.lifecycle.lifecycleScope
 import androidx.recyclerview.widget.LinearLayoutManager
 import androidx.recyclerview.widget.PagerSnapHelper
@@ -87,8 +86,7 @@ class HomeScreenMapContainerFragment : Hilt_HomeScreenMapContainerFragment() {
     }
 
     lifecycleScope.launch {
-      mapContainerViewModel.loisWithinMapBoundsAtVisibleZoomLevel
-        .asFlow()
+      mapContainerViewModel.loisInViewport
         .combine(mapContainerViewModel.suggestLoiJobs) { lois, jobs ->
           val loiCards = lois.map { MapCardUiData.LoiCardUiData(it) }
           val jobCards = jobs.map { MapCardUiData.SuggestLoiCardUiData(it) }
@@ -176,7 +174,7 @@ class HomeScreenMapContainerFragment : Hilt_HomeScreenMapContainerFragment() {
   override fun onMapReady(map: Map) {
     // Observe events emitted by the ViewModel.
     viewLifecycleOwner.lifecycleScope.launch {
-      mapContainerViewModel.mapLocationOfInterestFeatures.collect { map.renderFeatures(it) }
+      mapContainerViewModel.mapLoiFeatures.collect { map.renderFeatures(it) }
     }
 
     homeScreenViewModel.bottomSheetState.observe(this) { state: BottomSheetState ->

diff --git a/...in/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt b/...in/java/com/google/android/ground/ui/home/mapcontainer/HomeScreenMapContainerViewModel.kt
@@ -15,12 +15,11 @@
  */
 package com.google.android.ground.ui.home.mapcontainer
 
-import androidx.lifecycle.LiveData
-import androidx.lifecycle.toLiveData
 import androidx.lifecycle.viewModelScope
 import com.google.android.ground.Config.CLUSTERING_ZOOM_THRESHOLD
 import com.google.android.ground.Config.ZOOM_LEVEL_THRESHOLD
 import com.google.android.ground.coroutines.IoDispatcher
+import com.google.android.ground.model.Survey
 import com.google.android.ground.model.geometry.Point
 import com.google.android.ground.model.job.Job
 import com.google.android.ground.model.locationofinterest.LocationOfInterest
@@ -37,20 +36,17 @@ import com.google.android.ground.ui.common.BaseMapViewModel
 import com.google.android.ground.ui.common.SharedViewModel
 import com.google.android.ground.ui.map.CameraPosition
 import com.google.android.ground.ui.map.Feature
-import io.reactivex.Flowable
 import io.reactivex.Observable
 import io.reactivex.subjects.PublishSubject
 import io.reactivex.subjects.Subject
 import javax.inject.Inject
 import kotlinx.coroutines.CoroutineDispatcher
-import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.SharingStarted
 import kotlinx.coroutines.flow.StateFlow
-import kotlinx.coroutines.flow.combine
-import kotlinx.coroutines.flow.distinctUntilChanged
 import kotlinx.coroutines.flow.emitAll
 import kotlinx.coroutines.flow.stateIn
-import kotlinx.coroutines.flow.transform
+import kotlinx.coroutines.launch
 import kotlinx.coroutines.reactive.asFlow
 import timber.log.Timber
 
@@ -78,72 +74,87 @@ internal constructor(
     ioDispatcher
   ) {
 
-  val mapLocationOfInterestFeatures: StateFlow<Set<Feature>>
+  private var _survey: Survey? = null
 
-  private var lastCameraPosition: CameraPosition? = null
+  private val _mapLoiFeatures: MutableStateFlow<Set<Feature>> = MutableStateFlow(setOf())
+  val mapLoiFeatures: StateFlow<Set<Feature>> =
+    _mapLoiFeatures.stateIn(viewModelScope, SharingStarted.Lazily, setOf())
 
   /* UI Clicks */
   private val zoomThresholdCrossed: @Hot Subject<Nil> = PublishSubject.create()
 
   /**
-   * List of [LocationOfInterest] for the active survey that are present within the map bounds and
+   * List of [LocationOfInterest] for the active survey that are present within the viewport and
    * zoom level is clustering threshold or higher.
    */
-  val loisWithinMapBoundsAtVisibleZoomLevel: LiveData<List<LocationOfInterest>>
+  private val _loisInViewport: MutableStateFlow<List<LocationOfInterest>> =
+    MutableStateFlow(listOf())
+  val loisInViewport: StateFlow<List<LocationOfInterest>> =
+    _loisInViewport.stateIn(viewModelScope, SharingStarted.Lazily, listOf())
 
-  val suggestLoiJobs: Flow<List<Job>>
+  private val _suggestLoiJobs: MutableStateFlow<List<Job>> = MutableStateFlow(listOf())
+  val suggestLoiJobs: StateFlow<List<Job>> =
+    _suggestLoiJobs.stateIn(viewModelScope, SharingStarted.Lazily, listOf())
 
   init {
     // THIS SHOULD NOT BE CALLED ON CONFIG CHANGE
     // TODO: Clear location of interest markers when survey is deactivated.
     // TODO: Since we depend on survey stream from repo anyway, this transformation can be moved
     //  into the repository.
 
-    // LOIs that are persisted to the local and remote dbs.
-    mapLocationOfInterestFeatures =
-      surveyRepository.activeSurveyFlow
-        .transform { survey ->
-          if (survey == null) {
-            emit(setOf())
-          } else {
-            emitAll(locationOfInterestRepository.findLocationsOfInterestFeatures(survey))
-          }
-        }
-        .distinctUntilChanged()
-        .stateIn(viewModelScope, SharingStarted.Lazily, setOf())
-
-    loisWithinMapBoundsAtVisibleZoomLevel =
-      surveyRepository.activeSurveyFlowable
-        .switchMap { survey ->
-          cameraZoomUpdates.switchMap { zoomLevel ->
-            if (zoomLevel >= CLUSTERING_ZOOM_THRESHOLD && survey.isPresent)
-              locationOfInterestRepository.getWithinBoundsOnceAndStream(
-                survey.get(),
-                cameraBoundUpdates
-              )
-            else Flowable.just(listOf())
-          }
-        }
-        .toLiveData()
-
-    suggestLoiJobs =
-      surveyRepository.activeSurveyFlow
-        .combine(cameraZoomUpdates.asFlow()) { survey, zoomLevel ->
-          if (zoomLevel < CLUSTERING_ZOOM_THRESHOLD) {
-            listOf()
-          } else {
-            survey?.jobs?.filter { job -> job.suggestLoiTaskType != null }?.toList() ?: listOf()
-          }
-        }
-        .distinctUntilChanged()
+    viewModelScope.launch {
+      surveyRepository.activeSurveyFlow.collect {
+        _survey = it
+        refreshMapFeaturesAndCards(it)
+      }
+    }
+  }
+
+  private suspend fun refreshMapFeaturesAndCards(survey: Survey?) {
+    updateMapFeatures(survey)
+    updateLoisAndJobs(survey, currentCameraPosition)
+  }
+
+  private suspend fun updateLoisAndJobs(survey: Survey?, cameraPosition: CameraPosition?) {
+    updateMapLois(survey, cameraPosition)
+    updateSuggestLoiJobs(survey, cameraPosition)
+  }
+
+  private suspend fun updateMapFeatures(survey: Survey?) {
+    if (survey == null) {
+      _mapLoiFeatures.value = setOf()
+    } else {
+      // LOIs that are persisted to the local and remote dbs.
+      _mapLoiFeatures.emitAll(locationOfInterestRepository.findLocationsOfInterestFeatures(survey))
+    }
+  }
+
+  private suspend fun updateMapLois(survey: Survey?, cameraPosition: CameraPosition?) {
+    val bounds = cameraPosition?.bounds
+    if (bounds == null || survey == null || cameraPosition.isBelowClusteringZoomThreshold()) {
+      _loisInViewport.value = listOf()
+    } else {
+      _loisInViewport.emitAll(
+        locationOfInterestRepository.getWithinBoundsOnceAndStream(survey, bounds).asFlow()
+      )
+    }
+  }
+
+  private fun updateSuggestLoiJobs(survey: Survey?, cameraPosition: CameraPosition?) {
+    if (survey == null || cameraPosition.isBelowClusteringZoomThreshold()) {
+      _suggestLoiJobs.value = listOf()
+    } else {
+      _suggestLoiJobs.value = survey.jobs.filter { it.suggestLoiTaskType != null }.toList()
+    }
   }
 
   override fun onMapCameraMoved(newCameraPosition: CameraPosition) {
     super.onMapCameraMoved(newCameraPosition)
     Timber.d("Setting position to $newCameraPosition")
     onZoomChange(lastCameraPosition?.zoomLevel, newCameraPosition.zoomLevel)
     mapStateRepository.setCameraPosition(newCameraPosition)
-    lastCameraPosition = newCameraPosition
+
+    viewModelScope.launch { updateLoisAndJobs(_survey, newCameraPosition) }
   }
 
   private fun onZoomChange(oldZoomLevel: Float?, newZoomLevel: Float?) {
@@ -170,4 +181,7 @@ internal constructor(
   }
 
   fun getZoomThresholdCrossed(): Observable<Nil> = zoomThresholdCrossed
+
+  private fun CameraPosition?.isBelowClusteringZoomThreshold() =
+    this?.zoomLevel?.let { it < CLUSTERING_ZOOM_THRESHOLD } ?: true
 }
diff --git a/ground/src/main/java/com/google/android/ground/ui/offlinebasemap/OfflineAreaListAdapter.kt b/ground/src/main/java/com/google/android/ground/ui/offlinebasemap/OfflineAreaListAdapter.kt
@@ -39,7 +39,7 @@ internal class OfflineAreaListAdapter(private val navigator: Navigator) :
     }
 
     override fun onClick(v: View) {
-      if (areas.size > 0) {
+      if (areas.isNotEmpty()) {
         val id = areas[adapterPosition].id
         navigator.navigate(OfflineAreasFragmentDirections.viewOfflineArea(id))
       }

diff --git a/.../java/com/google/android/ground/ui/offlinebasemap/selector/OfflineAreaSelectorFragment.kt b/.../java/com/google/android/ground/ui/offlinebasemap/selector/OfflineAreaSelectorFragment.kt
@@ -24,9 +24,7 @@ import com.google.android.ground.databinding.OfflineBaseMapSelectorFragBinding
 import com.google.android.ground.ui.common.AbstractMapContainerFragment
 import com.google.android.ground.ui.common.BaseMapViewModel
 import com.google.android.ground.ui.common.EphemeralPopups
-import com.google.android.ground.ui.common.MapConfig
 import com.google.android.ground.ui.map.Map
-import com.google.android.ground.ui.map.MapType
 import dagger.hilt.android.AndroidEntryPoint
 import javax.inject.Inject
 
@@ -60,10 +58,9 @@ class OfflineAreaSelectorFragment : Hilt_OfflineAreaSelectorFragment() {
     return binding.root
   }
 
-  override fun onMapReady(map: Map) = viewModel.onMapReady(map)
+  override fun onMapReady(map: Map) {
+    viewModel.remoteTileSources.forEach { map.addTileOverlay(it) }
+  }
 
   override fun getMapViewModel(): BaseMapViewModel = viewModel
-
-  override fun getMapConfig(): MapConfig =
-    super.getMapConfig().copy(showOfflineTileOverlays = false, overrideMapType = MapType.ROAD)
 }