From 346dabffc519cf79f11e67ae5f4bfbc97e85589c Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Thu, 1 Jun 2023 19:19:18 +0200 Subject: [PATCH] Write notification account information to the correct account (#3697) Don't use `accountManager.activeAccount` throughout the code. Instead, get the active account once, and use that over the life of the viewmodel. As shown in https://github.com/tuskyapp/Tusky/issues/3689#issuecomment-1567219338 the active account can change before the view model is destroyed, and if that happens account information for the account will be written to the wrong account. --- .../com/keylesspalace/tusky/BaseActivity.java | 7 ++++++ .../notifications/NotificationHelper.java | 4 +--- .../notifications/NotificationsFragment.kt | 10 ++++---- .../notifications/NotificationsViewModel.kt | 24 +++++++++---------- .../keylesspalace/tusky/db/AccountManager.kt | 2 +- 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/BaseActivity.java b/app/src/main/java/com/keylesspalace/tusky/BaseActivity.java index f821dc6a..c6442de4 100644 --- a/app/src/main/java/com/keylesspalace/tusky/BaseActivity.java +++ b/app/src/main/java/com/keylesspalace/tusky/BaseActivity.java @@ -212,6 +212,13 @@ public abstract class BaseActivity extends AppCompatActivity implements Injectab } } + // TODO: This changes the accountManager's activeAccount property, but does not do any + // of the work that AccountManager.setActiveAccount() does. In particular: + // + // - The current active account is not saved + // - The account passed as parameter here goes not have its `isActive` property set + // + // Is that deliberate? Or is this a bug? public void openAsAccount(@NonNull String url, @NonNull AccountEntity account) { accountManager.setActiveAccount(account); Intent intent = new Intent(this, MainActivity.class); diff --git a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationHelper.java b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationHelper.java index 638ab8e1..1127c261 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationHelper.java +++ b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationHelper.java @@ -606,9 +606,7 @@ public class NotificationHelper { Log.d(TAG, "disabled notification checks"); } - public static void clearNotificationsForActiveAccount(@NonNull Context context, @NonNull AccountManager accountManager) { - AccountEntity account = accountManager.getActiveAccount(); - if (account == null) return; + public static void clearNotificationsForAccount(@NonNull Context context, @NonNull AccountEntity account) { int accountId = (int) account.getId(); NotificationManager notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); diff --git a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsFragment.kt b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsFragment.kt index ab5c1b53..352e87ca 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsFragment.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsFragment.kt @@ -106,7 +106,7 @@ class NotificationsFragment : adapter = NotificationsPagingAdapter( notificationDiffCallback, - accountId = accountManager.activeAccount!!.accountId, + accountId = viewModel.account.accountId, statusActionListener = this, notificationActionListener = this, accountActionListener = this, @@ -460,7 +460,7 @@ class NotificationsFragment : override fun onRefresh() { binding.progressBar.isVisible = false adapter.refresh() - NotificationHelper.clearNotificationsForActiveAccount(requireContext(), accountManager) + NotificationHelper.clearNotificationsForAccount(requireContext(), viewModel.account) } override fun onPause() { @@ -477,7 +477,7 @@ class NotificationsFragment : override fun onResume() { super.onResume() - NotificationHelper.clearNotificationsForActiveAccount(requireContext(), accountManager) + NotificationHelper.clearNotificationsForAccount(requireContext(), viewModel.account) } override fun onReply(position: Int) { @@ -606,7 +606,7 @@ class NotificationsFragment : override fun onViewReport(reportId: String) { requireContext().openLink( - "https://${accountManager.activeAccount!!.domain}/admin/reports/$reportId" + "https://${viewModel.account.domain}/admin/reports/$reportId" ) } @@ -622,7 +622,7 @@ class NotificationsFragment : } companion object { - private const val TAG = "NotificationF" + private const val TAG = "NotificationsFragment" fun newInstance() = NotificationsFragment() private val notificationDiffCallback: DiffUtil.ItemCallback = diff --git a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt index 3c07ed01..0cf5d46b 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt @@ -282,6 +282,8 @@ class NotificationsViewModel @Inject constructor( private val timelineCases: TimelineCases, private val eventHub: EventHub ) : ViewModel() { + /** The account to display notifications for */ + val account = accountManager.activeAccount!! val uiState: StateFlow @@ -316,16 +318,14 @@ class NotificationsViewModel @Inject constructor( // Save each change back to the active account .onEach { action -> Log.d(TAG, "notificationFilter: $action") - accountManager.activeAccount?.let { account -> - account.notificationsFilter = serialize(action.filter) - accountManager.saveAccount(account) - } + account.notificationsFilter = serialize(action.filter) + accountManager.saveAccount(account) } // Load the initial filter from the active account .onStart { emit( InfallibleUiAction.ApplyFilter( - filter = deserialize(accountManager.activeAccount?.notificationsFilter) + filter = deserialize(account.notificationsFilter) ) ) } @@ -336,11 +336,9 @@ class NotificationsViewModel @Inject constructor( .filterIsInstance() .distinctUntilChanged() .collectLatest { action -> - Log.d(TAG, "Saving visible ID: ${action.visibleId}") - accountManager.activeAccount?.let { account -> - account.lastNotificationId = action.visibleId - accountManager.saveAccount(account) - } + Log.d(TAG, "Saving visible ID: ${action.visibleId}, active account = ${account.id}") + account.lastNotificationId = action.visibleId + accountManager.saveAccount(account) } } @@ -351,7 +349,7 @@ class NotificationsViewModel @Inject constructor( statusDisplayOptions = MutableStateFlow( StatusDisplayOptions.from( preferences, - accountManager.activeAccount!! + account ) ) @@ -363,7 +361,7 @@ class NotificationsViewModel @Inject constructor( statusDisplayOptions.value.make( preferences, it.preferenceKey, - accountManager.activeAccount!! + account ) } .collect { @@ -494,7 +492,7 @@ class NotificationsViewModel @Inject constructor( // The database stores "0" as the last notification ID if notifications have not been // fetched. Convert to null to ensure a full fetch in this case private fun getInitialKey(): String? { - val initialKey = when (val id = accountManager.activeAccount?.lastNotificationId) { + val initialKey = when (val id = account.lastNotificationId) { "0" -> null else -> id } diff --git a/app/src/main/java/com/keylesspalace/tusky/db/AccountManager.kt b/app/src/main/java/com/keylesspalace/tusky/db/AccountManager.kt index 46805861..0d1ea37b 100644 --- a/app/src/main/java/com/keylesspalace/tusky/db/AccountManager.kt +++ b/app/src/main/java/com/keylesspalace/tusky/db/AccountManager.kt @@ -52,7 +52,7 @@ class AccountManager @Inject constructor(db: AppDatabase) { /** * Adds a new account and makes it the active account. * @param accessToken the access token for the new account - * @param domain the domain of the accounts Mastodon instance + * @param domain the domain of the account's Mastodon instance * @param clientId the oauth client id used to sign in the account * @param clientSecret the oauth client secret used to sign in the account * @param oauthScopes the oauth scopes granted to the account