From 03a8289a1398b03f164d12b4b3e1d6d5fb5d73ca Mon Sep 17 00:00:00 2001 From: Onuray Sahin Date: Thu, 26 May 2022 15:45:53 +0300 Subject: [PATCH] Code review fixes. --- .../im/vector/app/core/di/FragmentModule.kt | 6 ++ .../live/map/LocationLiveMapAction.kt | 5 +- .../live/map/LocationLiveMapViewFragment.kt | 57 ++++++++----------- .../live/map/LocationLiveMapViewModel.kt | 24 ++++++-- .../live/map/LocationLiveMapViewState.kt | 6 +- 5 files changed, 56 insertions(+), 42 deletions(-) diff --git a/vector/src/main/java/im/vector/app/core/di/FragmentModule.kt b/vector/src/main/java/im/vector/app/core/di/FragmentModule.kt index 3dba8b797b..b08f4b2e9f 100644 --- a/vector/src/main/java/im/vector/app/core/di/FragmentModule.kt +++ b/vector/src/main/java/im/vector/app/core/di/FragmentModule.kt @@ -64,6 +64,7 @@ import im.vector.app.features.home.room.list.RoomListFragment import im.vector.app.features.home.room.threads.list.views.ThreadListFragment import im.vector.app.features.location.LocationPreviewFragment import im.vector.app.features.location.LocationSharingFragment +import im.vector.app.features.location.live.map.LocationLiveMapViewFragment import im.vector.app.features.login.LoginCaptchaFragment import im.vector.app.features.login.LoginFragment import im.vector.app.features.login.LoginGenericTextInputFormFragment @@ -1005,4 +1006,9 @@ interface FragmentModule { @IntoMap @FragmentKey(LocationPreviewFragment::class) fun bindLocationPreviewFragment(fragment: LocationPreviewFragment): Fragment + + @Binds + @IntoMap + @FragmentKey(LocationLiveMapViewFragment::class) + fun bindLocationLiveMapViewFragment(fragment: LocationLiveMapViewFragment): Fragment } diff --git a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapAction.kt b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapAction.kt index a31e02611f..16cd3badc6 100644 --- a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapAction.kt +++ b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapAction.kt @@ -18,4 +18,7 @@ package im.vector.app.features.location.live.map import im.vector.app.core.platform.VectorViewModelAction -sealed interface LocationLiveMapAction : VectorViewModelAction +sealed class LocationLiveMapAction : VectorViewModelAction { + data class AddMapSymbol(val key: String, val value: Long) : LocationLiveMapAction() + data class RemoveMapSymbol(val key: String) : LocationLiveMapAction() +} diff --git a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewFragment.kt b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewFragment.kt index 46c238ad12..9ade47e321 100644 --- a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewFragment.kt +++ b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewFragment.kt @@ -17,13 +17,10 @@ package im.vector.app.features.location.live.map import android.graphics.drawable.Drawable -import android.os.Bundle import android.view.LayoutInflater -import android.view.View import android.view.ViewGroup import androidx.core.graphics.drawable.toBitmap import androidx.lifecycle.lifecycleScope -import com.airbnb.mvrx.args import com.airbnb.mvrx.fragmentViewModel import com.airbnb.mvrx.withState import com.mapbox.mapboxsdk.geometry.LatLng @@ -36,7 +33,6 @@ import com.mapbox.mapboxsdk.maps.SupportMapFragment import com.mapbox.mapboxsdk.plugins.annotation.SymbolManager import com.mapbox.mapboxsdk.plugins.annotation.SymbolOptions import com.mapbox.mapboxsdk.style.layers.Property -import dagger.hilt.android.AndroidEntryPoint import im.vector.app.R import im.vector.app.core.extensions.addChildFragment import im.vector.app.core.platform.VectorBaseFragment @@ -44,6 +40,7 @@ import im.vector.app.databinding.FragmentSimpleContainerBinding import im.vector.app.features.location.UrlMapProvider import im.vector.app.features.location.zoomToBounds import im.vector.app.features.location.zoomToLocation +import kotlinx.coroutines.launch import timber.log.Timber import java.lang.ref.WeakReference import javax.inject.Inject @@ -51,13 +48,9 @@ import javax.inject.Inject /** * Screen showing a map with all the current users sharing their live location in a room. */ -@AndroidEntryPoint -class LocationLiveMapViewFragment : VectorBaseFragment() { - - @Inject - lateinit var urlMapProvider: UrlMapProvider - - private val args: LocationLiveMapViewArgs by args() +class LocationLiveMapViewFragment @Inject constructor( + private var urlMapProvider: UrlMapProvider, +) : VectorBaseFragment() { private val viewModel: LocationLiveMapViewModel by fragmentViewModel() @@ -71,16 +64,15 @@ class LocationLiveMapViewFragment : VectorBaseFragment - lifecycleScope.launchWhenCreated { + lifecycleScope.launch { mapboxMap.setStyle(urlMapProvider.getMapUrl()) { style -> mapStyle = style this@LocationLiveMapViewFragment.mapboxMap = WeakReference(mapboxMap) @@ -108,10 +100,8 @@ class LocationLiveMapViewFragment : VectorBaseFragment) { symbolManager?.let { sManager -> val latLngBoundsBuilder = LatLngBounds.Builder() - userLiveLocations.forEach { userLocation -> createOrUpdateSymbol(userLocation, sManager) - if (isMapFirstUpdate) { val latLng = LatLng(userLocation.locationData.latitude, userLocation.locationData.longitude) latLngBoundsBuilder.include(latLng) @@ -123,10 +113,10 @@ class LocationLiveMapViewFragment : VectorBaseFragment + val symbolId = state.mapSymbolIds[userLocation.userId] - if (symbolId == null) { + if (symbolId == null || symbolManager.annotations.get(symbolId) == null) { createSymbol(userLocation, symbolManager) } else { updateSymbol(symbolId, userLocation, symbolManager) @@ -137,7 +127,7 @@ class LocationLiveMapViewFragment : VectorBaseFragment, symbolManager: SymbolManager) { - val userIdsToRemove = viewModel.mapSymbolIds.keys.subtract(userLiveLocations.map { it.userId }.toSet()) - userIdsToRemove - .mapNotNull { userId -> - removeUserPinFromMapStyle(userId) - viewModel.mapSymbolIds[userId] - viewModel.mapSymbolIds.remove(userId) - } - .forEach { symbolId -> - Timber.d("trying to delete symbol with id: $symbolId") - symbolManager.annotations.get(symbolId)?.let { - symbolManager.delete(it) - } + private fun removeOutdatedSymbols(userLiveLocations: List, symbolManager: SymbolManager) = withState(viewModel) { state -> + val userIdsToRemove = state.mapSymbolIds.keys.subtract(userLiveLocations.map { it.userId }.toSet()) + userIdsToRemove.forEach { userId -> + removeUserPinFromMapStyle(userId) + viewModel.handle(LocationLiveMapAction.RemoveMapSymbol(userId)) + + state.mapSymbolIds[userId]?.let { symbolId -> + Timber.d("trying to delete symbol with id: $symbolId") + symbolManager.annotations.get(symbolId)?.let { + symbolManager.delete(it) } + } + } } private fun updateMapZoomWhenNeeded(userLiveLocations: List, latLngBoundsBuilder: LatLngBounds.Builder) { diff --git a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewModel.kt b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewModel.kt index 8c5f292f25..b14feea667 100644 --- a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewModel.kt @@ -38,11 +38,6 @@ class LocationLiveMapViewModel @AssistedInject constructor( companion object : MavericksViewModelFactory by hiltMavericksViewModelFactory() - /** - * Map to keep track of symbol ids associated to each user Id. - */ - val mapSymbolIds = mutableMapOf() - init { getListOfUserLiveLocationUseCase.execute(initialState.roomId) .onEach { setState { copy(userLocations = it) } } @@ -50,6 +45,23 @@ class LocationLiveMapViewModel @AssistedInject constructor( } override fun handle(action: LocationLiveMapAction) { - // do nothing, no action for now + when (action) { + is LocationLiveMapAction.AddMapSymbol -> handleAddMapSymbol(action) + is LocationLiveMapAction.RemoveMapSymbol -> handleRemoveMapSymbol(action) + } + } + + private fun handleAddMapSymbol(action: LocationLiveMapAction.AddMapSymbol) = withState { state -> + val newMapSymbolIds = state.mapSymbolIds.toMutableMap().apply { set(action.key, action.value) } + setState { + copy(mapSymbolIds = newMapSymbolIds) + } + } + + private fun handleRemoveMapSymbol(action: LocationLiveMapAction.RemoveMapSymbol) = withState { state -> + val newMapSymbolIds = state.mapSymbolIds.toMutableMap().apply { remove(action.key) } + setState { + copy(mapSymbolIds = newMapSymbolIds) + } } } diff --git a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewState.kt b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewState.kt index 9fa8635d82..6f21f71e80 100644 --- a/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewState.kt +++ b/vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewState.kt @@ -22,7 +22,11 @@ import im.vector.app.features.location.LocationData data class LocationLiveMapViewState( val roomId: String, - val userLocations: List = emptyList() + val userLocations: List = emptyList(), + /** + * Map to keep track of symbol ids associated to each user Id. + */ + val mapSymbolIds: Map = emptyMap() ) : MavericksState { constructor(locationLiveMapViewArgs: LocationLiveMapViewArgs) : this( roomId = locationLiveMapViewArgs.roomId