Fix show only personal setting not updating the view immediately (#1238)

* Split pair of show only personal settings * Create and consume showOnlyPersonal settings separately * Fix showOnlyPersonal flow state change not triggering re-emission * Combine if statements * Minor refactoring (lift out "if") * Create separate reload methods * Reload on model creation * Use viewmodel scope * Move init after relevant method declarations * Add kdoc * Remove deprecated getShowOnlyPersonalPair --------- Co-authored-by: Ricki Hirner <[email protected]>
bitfireAT · Jan 20, 2025 · 1cc9b4b · 1cc9b4b
1 parent 226583f
commit 1cc9b4b
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 63 deletions.
diff --git a/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettings.kt b/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettings.kt
@@ -286,15 +286,25 @@ class AccountSettings @AssistedInject constructor(
 
     // UI settings
 
-    data class ShowOnlyPersonal(
-        val onlyPersonal: Boolean,
-        val locked: Boolean
-    )
+    /**
+     * Whether to show only personal collections in the UI
+     *
+     * @return *true* if only personal collections shall be shown; *false* otherwise
+     */
+    fun getShowOnlyPersonal(): Boolean = when (settingsManager.getIntOrNull(KEY_SHOW_ONLY_PERSONAL)) {
+        0 -> false
+        1 -> true
+        else /* including -1 */ -> accountManager.getUserData(account, KEY_SHOW_ONLY_PERSONAL) != null
+    }
 
-    fun getShowOnlyPersonal(): ShowOnlyPersonal {
-        @Suppress("DEPRECATION")
-        val pair = getShowOnlyPersonalPair()
-        return ShowOnlyPersonal(onlyPersonal = pair.first, locked = !pair.second)
+    /**
+     * Whether the user shall be able to change the setting (= setting not locked)
+     *
+     * @return *true* if the setting is locked; *false* otherwise
+     */
+    fun getShowOnlyPersonalLocked(): Boolean = when (settingsManager.getIntOrNull(KEY_SHOW_ONLY_PERSONAL)) {
+        0, 1 -> true
+        else /* including -1 */ -> false
     }
 
     /**
@@ -307,11 +317,11 @@ class AccountSettings @AssistedInject constructor(
      */
     @Deprecated("Use getShowOnlyPersonal() instead", replaceWith = ReplaceWith("getShowOnlyPersonal()"))
     fun getShowOnlyPersonalPair(): Pair<Boolean, Boolean> =
-            when (settingsManager.getIntOrNull(KEY_SHOW_ONLY_PERSONAL)) {
-                0 -> Pair(false, false)
-                1 -> Pair(true, false)
-                else /* including -1 */ -> Pair(accountManager.getUserData(account, KEY_SHOW_ONLY_PERSONAL) != null, true)
-            }
+        when (settingsManager.getIntOrNull(KEY_SHOW_ONLY_PERSONAL)) {
+            0 -> Pair(false, false)
+            1 -> Pair(true, false)
+            else /* including -1 */ -> Pair(accountManager.getUserData(account, KEY_SHOW_ONLY_PERSONAL) != null, true)
+        }
 
     fun setShowOnlyPersonal(showOnlyPersonal: Boolean) {
         accountManager.setAndVerifyUserData(account, KEY_SHOW_ONLY_PERSONAL, if (showOnlyPersonal) "1" else null)

diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/account/AccountScreen.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/account/AccountScreen.kt
@@ -69,7 +69,6 @@ import androidx.paging.compose.LazyPagingItems
 import androidx.paging.compose.collectAsLazyPagingItems
 import at.bitfire.davdroid.R
 import at.bitfire.davdroid.db.Collection
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.davdroid.ui.AppTheme
 import at.bitfire.davdroid.ui.PermissionsActivity
 import at.bitfire.davdroid.ui.account.AccountProgress
@@ -114,9 +113,8 @@ fun AccountScreen(
         error = model.error,
         onResetError = model::resetError,
         invalidAccount = model.invalidAccount.collectAsStateWithLifecycle(false).value,
-        showOnlyPersonal = model.showOnlyPersonal.collectAsStateWithLifecycle(
-            initialValue = AccountSettings.ShowOnlyPersonal(onlyPersonal = false, locked = false)
-        ).value,
+        showOnlyPersonal = model.showOnlyPersonal.collectAsStateWithLifecycle().value,
+        showOnlyPersonalLocked = model.showOnlyPersonalLocked.collectAsStateWithLifecycle().value,
         onSetShowOnlyPersonal = model::setShowOnlyPersonal,
         hasCardDav = cardDavService != null,
         canCreateAddressBook = model.canCreateAddressBook.collectAsStateWithLifecycle(false).value,
@@ -169,7 +167,8 @@ fun AccountScreen(
     error: String? = null,
     onResetError: () -> Unit = {},
     invalidAccount: Boolean = false,
-    showOnlyPersonal: AccountSettings.ShowOnlyPersonal,
+    showOnlyPersonal: Boolean = false,
+    showOnlyPersonalLocked: Boolean = false,
     onSetShowOnlyPersonal: (showOnlyPersonal: Boolean) -> Unit = {},
     hasCardDav: Boolean,
     canCreateAddressBook: Boolean,
@@ -257,6 +256,7 @@ fun AccountScreen(
                             canCreateCalendar = canCreateCalendar,
                             onCreateCalendar = onCreateCalendar,
                             showOnlyPersonal = showOnlyPersonal,
+                            showOnlyPersonalLocked = showOnlyPersonalLocked,
                             onSetShowOnlyPersonal = onSetShowOnlyPersonal,
                             currentPage = pagerState.currentPage,
                             idxCardDav = idxCardDav,
@@ -440,7 +440,8 @@ fun AccountScreen_Actions(
     onCreateAddressBook: () -> Unit,
     canCreateCalendar: Boolean,
     onCreateCalendar: () -> Unit,
-    showOnlyPersonal: AccountSettings.ShowOnlyPersonal,
+    showOnlyPersonal: Boolean,
+    showOnlyPersonalLocked: Boolean,
     onSetShowOnlyPersonal: (showOnlyPersonal: Boolean) -> Unit,
     currentPage: Int,
     idxCardDav: Int?,
@@ -513,8 +514,8 @@ fun AccountScreen_Actions(
                     LocalMinimumInteractiveComponentSize provides Dp.Unspecified
                 ) {
                     Checkbox(
-                        checked = showOnlyPersonal.onlyPersonal,
-                        enabled = !showOnlyPersonal.locked,
+                        checked = showOnlyPersonal,
+                        enabled = !showOnlyPersonalLocked,
                         onCheckedChange = {
                             onSetShowOnlyPersonal(it)
                             overflowOpen = false
@@ -527,10 +528,10 @@ fun AccountScreen_Actions(
                 Text(stringResource(R.string.account_only_personal))
             },
             onClick = {
-                onSetShowOnlyPersonal(!showOnlyPersonal.onlyPersonal)
+                onSetShowOnlyPersonal(!showOnlyPersonal)
                 overflowOpen = false
             },
-            enabled = !showOnlyPersonal.locked
+            enabled = !showOnlyPersonalLocked
         )
 
         // rename account
@@ -651,7 +652,8 @@ fun AccountScreen_ServiceTab(
 fun AccountScreen_Preview() {
     AccountScreen(
         accountName = "[email protected]",
-        showOnlyPersonal = AccountSettings.ShowOnlyPersonal(onlyPersonal = false, locked = true),
+        showOnlyPersonal = false,
+        showOnlyPersonalLocked = true,
         hasCardDav = true,
         canCreateAddressBook = false,
         cardDavProgress = AccountProgress.Active,

diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/account/AccountScreenModel.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/account/AccountScreenModel.kt
@@ -9,11 +9,7 @@ import android.content.Context
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.setValue
-import androidx.lifecycle.LiveData
-import androidx.lifecycle.MutableLiveData
 import androidx.lifecycle.ViewModel
-import androidx.lifecycle.asFlow
-import androidx.lifecycle.switchMap
 import androidx.lifecycle.viewModelScope
 import at.bitfire.davdroid.R
 import at.bitfire.davdroid.db.Collection
@@ -30,14 +26,15 @@ import dagger.assisted.AssistedFactory
 import dagger.assisted.AssistedInject
 import dagger.hilt.android.lifecycle.HiltViewModel
 import dagger.hilt.android.qualifiers.ApplicationContext
-import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.SharingStarted
+import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.flow.stateIn
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import java.util.logging.Level
 import java.util.logging.Logger
 
@@ -62,26 +59,45 @@ class AccountScreenModel @AssistedInject constructor(
         fun create(account: Account): AccountScreenModel
     }
 
+    /**
+     * Only acquire account settings on a worker thread!
+     */
+    private val accountSettings by lazy { accountSettingsFactory.create(account) }
+
     /** whether the account is invalid and the screen shall be closed */
     val invalidAccount = accountRepository.getAllFlow().map { accounts ->
         !accounts.contains(account)
     }
 
-    private val refreshSettingsSignal = MutableLiveData(Unit)
-    val showOnlyPersonal = refreshSettingsSignal.switchMap<Unit, AccountSettings.ShowOnlyPersonal> {
-        object : LiveData<AccountSettings.ShowOnlyPersonal>() {
-            init {
-                viewModelScope.launch(Dispatchers.IO) {
-                    val settings = accountSettingsFactory.create(account)
-                    postValue(settings.getShowOnlyPersonal())
-                }
-            }
+    /**
+     * Whether to show only personal collections.
+     */
+    private val _showOnlyPersonal = MutableStateFlow(false)
+    val showOnlyPersonal = _showOnlyPersonal.asStateFlow()
+    private suspend fun reloadShowOnlyPersonal() = withContext(Dispatchers.Default) {
+        _showOnlyPersonal.value = accountSettings.getShowOnlyPersonal()
+    }
+    fun setShowOnlyPersonal(showOnlyPersonal: Boolean) {
+        viewModelScope.launch {
+            accountSettings.setShowOnlyPersonal(showOnlyPersonal)
+            reloadShowOnlyPersonal()
+        }
+    }
+
+    /**
+     * Whether the user setting to show only personal collections is locked.
+     */
+    private var _showOnlyPersonalLocked = MutableStateFlow(false)
+    val showOnlyPersonalLocked = _showOnlyPersonalLocked.asStateFlow()
+    private suspend fun reloadShowOnlyPersonalLocked() = withContext(Dispatchers.Default) {
+        _showOnlyPersonalLocked.value = accountSettings.getShowOnlyPersonalLocked()
+    }
+
+    init {
+        viewModelScope.launch {
+            reloadShowOnlyPersonal()
+            reloadShowOnlyPersonalLocked()
         }
-    }.asFlow()
-    fun setShowOnlyPersonal(showOnlyPersonal: Boolean) = viewModelScope.launch(Dispatchers.IO) {
-        val settings = accountSettingsFactory.create(account)
-        settings.setShowOnlyPersonal(showOnlyPersonal)
-        refreshSettingsSignal.postValue(Unit)
     }
 
     val cardDavSvc = serviceRepository
@@ -130,11 +146,9 @@ class AccountScreenModel @AssistedInject constructor(
 
     // actions
 
-    private val notInterruptibleScope = CoroutineScope(SupervisorJob())
-
     /** Deletes the account from the system (won't touch collections on the server). */
     fun deleteAccount() {
-        notInterruptibleScope.launch {
+        viewModelScope.launch {
             accountRepository.delete(account.name)
         }
     }
@@ -154,7 +168,7 @@ class AccountScreenModel @AssistedInject constructor(
      * @param newName new account name
      */
     fun renameAccount(newName: String) {
-        notInterruptibleScope.launch {
+        viewModelScope.launch {
             try {
                 accountRepository.rename(account.name, newName)
 

diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/account/GetServiceCollectionPagerUseCase.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/account/GetServiceCollectionPagerUseCase.kt
@@ -11,13 +11,12 @@ import androidx.paging.map
 import at.bitfire.davdroid.db.Collection
 import at.bitfire.davdroid.db.Service
 import at.bitfire.davdroid.repository.DavCollectionRepository
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.davdroid.settings.Settings
 import at.bitfire.davdroid.settings.SettingsManager
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.combine
-import kotlinx.coroutines.flow.flattenConcat
+import kotlinx.coroutines.flow.flatMapLatest
 import kotlinx.coroutines.flow.flowOf
 import kotlinx.coroutines.flow.map
 import javax.inject.Inject
@@ -43,35 +42,30 @@ class GetServiceCollectionPagerUseCase @Inject constructor(
     operator fun invoke(
         serviceFlow: Flow<Service?>,
         collectionType: String,
-        showOnlyPersonalFlow: Flow<AccountSettings.ShowOnlyPersonal?>
+        showOnlyPersonalFlow: Flow<Boolean>
     ): Flow<PagingData<Collection>> =
         combine(serviceFlow, showOnlyPersonalFlow, forceReadOnlyAddressBooksFlow) { service, onlyPersonal, forceReadOnlyAddressBooks ->
-            if (service == null)
-                flowOf(PagingData.empty<Collection>())
-            else {
+            service?.let { service ->
                 val dataFlow = Pager(
                     config = PagingConfig(PAGER_SIZE),
                     pagingSourceFactory = {
-                        if (onlyPersonal?.onlyPersonal == true)
+                        if (onlyPersonal == true)
                             collectionRepository.pagePersonalByServiceAndType(service.id, collectionType)
                         else
                             collectionRepository.pageByServiceAndType(service.id, collectionType)
                     }
                 ).flow
 
-                // generate resulting flow; set "forceReadOnly" for every address book if requested
-                if (forceReadOnlyAddressBooks)
+                // set "forceReadOnly" for every address book if requested
+                if (forceReadOnlyAddressBooks && collectionType == Collection.TYPE_ADDRESSBOOK)
                     dataFlow.map { pagingData ->
                         pagingData.map { collection ->
-                            if (collectionType == Collection.TYPE_ADDRESSBOOK)
-                                collection.copy(forceReadOnly = true)
-                            else
-                                collection
+                            collection.copy(forceReadOnly = true)
                         }
                     }
                 else
                     dataFlow
-            }
-        }.flattenConcat()
+            } ?: flowOf(PagingData.empty())
+        }.flatMapLatest { it }
 
 }