From 632282d0e2d0be969902f964ee76ac9fef5247ed Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Wed, 20 Nov 2024 19:28:29 +0100 Subject: [PATCH] fix: Prevent crash when showing account chooser (#1117) Chooser dialog could start before any accounts have loaded. Fix by collecting the account flow and waiting for the first emission (convert the flow to shared instead of state so there's no initial empty list). Guard against the potential for a similar issue when fetching notifications. Order the list of accounts with active account first so that code that skips it by ignoring the first item works correctly. --- .../notifications/NotificationFetcher.kt | 8 ++- .../app/pachli/core/activity/BaseActivity.kt | 70 +++++++++++-------- .../core/data/repository/AccountManager.kt | 7 +- .../pachli/core/database/dao/AccountDao.kt | 2 +- 4 files changed, 50 insertions(+), 37 deletions(-) diff --git a/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt b/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt index f0c2b789f..91439014e 100644 --- a/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt +++ b/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt @@ -40,6 +40,8 @@ import kotlin.collections.set import kotlin.math.min import kotlin.time.Duration.Companion.milliseconds import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.take import timber.log.Timber /** @@ -57,11 +59,11 @@ class NotificationFetcher @Inject constructor( @ApplicationContext private val context: Context, ) { suspend fun fetchAndShow(pachliAccountId: Long) { - Timber.d("NotificationFetcher.fetchAndShow() started") + Timber.d("NotificationFetcher.fetchAndShow(%d) started", pachliAccountId) val accounts = buildList { if (pachliAccountId == NotificationWorker.ALL_ACCOUNTS) { - addAll(accountManager.accountsOrderedByActive) + addAll(accountManager.accountsOrderedByActiveFlow.take(1).first()) } else { accountManager.getAccountById(pachliAccountId)?.let { add(it) } } @@ -69,7 +71,7 @@ class NotificationFetcher @Inject constructor( for (account in accounts) { Timber.d( - "Checking %s$, notificationsEnabled = %s", + "Checking %s, notificationsEnabled = %s", account.fullName, account.notificationsEnabled, ) diff --git a/core/activity/src/main/kotlin/app/pachli/core/activity/BaseActivity.kt b/core/activity/src/main/kotlin/app/pachli/core/activity/BaseActivity.kt index 0133ce773..3ba7f5875 100644 --- a/core/activity/src/main/kotlin/app/pachli/core/activity/BaseActivity.kt +++ b/core/activity/src/main/kotlin/app/pachli/core/activity/BaseActivity.kt @@ -59,6 +59,8 @@ import dagger.hilt.android.EntryPointAccessors.fromApplication import dagger.hilt.components.SingletonComponent import javax.inject.Inject import kotlin.properties.Delegates +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.take import kotlinx.coroutines.launch import timber.log.Timber @@ -229,47 +231,55 @@ abstract class BaseActivity : AppCompatActivity(), MenuProvider { bar.show() } + /** + * Displays a dialog allowing the user to choose from the available accounts. + * + * @param dialogTitle + * @parma showActiveAccount True if the active account should be included in + * the list of accounts. + * @parma listener + */ fun showAccountChooserDialog( dialogTitle: CharSequence?, showActiveAccount: Boolean, listener: AccountSelectionListener, ) { - val accounts = accountManager.accountsOrderedByActive.toMutableList() - val activeAccount = accounts.first() - when (accounts.size) { - 1 -> { - listener.onAccountSelected(activeAccount) - return - } + lifecycleScope.launch { + val accounts = accountManager.accountsOrderedByActiveFlow.take(1).first().toMutableList() + val activeAccount = accounts.first() + when (accounts.size) { + 1 -> { + listener.onAccountSelected(activeAccount) + return@launch + } - 2 -> { - if (!showActiveAccount) { - for (account in accounts) { - if (activeAccount !== account) { - listener.onAccountSelected(account) - return + 2 -> { + if (!showActiveAccount) { + for (account in accounts) { + if (activeAccount !== account) { + listener.onAccountSelected(account) + return@launch + } } } } } - } - if (!showActiveAccount) { - accounts.remove(activeAccount) - } - val adapter = AccountSelectionAdapter( - this, - sharedPreferencesRepository.animateAvatars, - sharedPreferencesRepository.animateEmojis, - ) - adapter.addAll(accounts) - AlertDialog.Builder(this) - .setTitle(dialogTitle) - .setAdapter(adapter) { _: DialogInterface?, index: Int -> - listener.onAccountSelected( - accounts[index], - ) + if (!showActiveAccount) { + accounts.remove(activeAccount) } - .show() + val adapter = AccountSelectionAdapter( + this@BaseActivity, + sharedPreferencesRepository.animateAvatars, + sharedPreferencesRepository.animateEmojis, + ) + adapter.addAll(accounts) + AlertDialog.Builder(this@BaseActivity) + .setTitle(dialogTitle) + .setAdapter(adapter) { _: DialogInterface?, index: Int -> + listener.onAccountSelected(accounts[index]) + } + .show() + } } val openAsText: String? diff --git a/core/data/src/main/kotlin/app/pachli/core/data/repository/AccountManager.kt b/core/data/src/main/kotlin/app/pachli/core/data/repository/AccountManager.kt index aa8b3975d..99ea52a7b 100644 --- a/core/data/src/main/kotlin/app/pachli/core/data/repository/AccountManager.kt +++ b/core/data/src/main/kotlin/app/pachli/core/data/repository/AccountManager.kt @@ -68,6 +68,7 @@ import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.shareIn import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import timber.log.Timber @@ -190,11 +191,11 @@ class AccountManager @Inject constructor( val accounts: List get() = accountsFlow.value - private val accountsOrderedByActiveFlow = accountDao.getAccountsOrderedByActive() - .stateIn(externalScope, SharingStarted.Eagerly, emptyList()) + val accountsOrderedByActiveFlow = accountDao.getAccountsOrderedByActive() + .shareIn(externalScope, SharingStarted.Eagerly, replay = 1) val accountsOrderedByActive: List - get() = accountsOrderedByActiveFlow.value + get() = accountsOrderedByActiveFlow.replayCache.first() @Deprecated("Caller should use getPachliAccountFlow with a specific account ID") val activePachliAccountFlow = accountDao.getActivePachliAccountFlow() diff --git a/core/database/src/main/kotlin/app/pachli/core/database/dao/AccountDao.kt b/core/database/src/main/kotlin/app/pachli/core/database/dao/AccountDao.kt index 6e49cddec..100375106 100644 --- a/core/database/src/main/kotlin/app/pachli/core/database/dao/AccountDao.kt +++ b/core/database/src/main/kotlin/app/pachli/core/database/dao/AccountDao.kt @@ -86,7 +86,7 @@ interface AccountDao { """ SELECT * FROM AccountEntity - ORDER BY isActive, id ASC + ORDER BY isActive DESC, id ASC """, ) fun getAccountsOrderedByActive(): Flow>