From b1d5cb548fe086857dc470c826027141aa0f4a6b Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Thu, 25 Jul 2024 18:43:34 +0200 Subject: [PATCH] fix: Don't crash due to Filters/ServerRepository race condition (#837) The `canFilter()` implementation could crash if `server` (marked `lateinit`) hadn't been initialised at the point of use. Fix this by removing it and adjusting the two callers to use the `filters` flow and take appropriate action on error. --- .../main/java/app/pachli/TimelineActivity.kt | 25 ++++++------- .../preference/AccountPreferencesFragment.kt | 36 +++++++++++++++++-- .../core/data/repository/FiltersRepository.kt | 6 +--- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/app/pachli/TimelineActivity.kt b/app/src/main/java/app/pachli/TimelineActivity.kt index ad778354b..0397bee1e 100644 --- a/app/src/main/java/app/pachli/TimelineActivity.kt +++ b/app/src/main/java/app/pachli/TimelineActivity.kt @@ -33,6 +33,7 @@ import app.pachli.core.common.util.unsafeLazy import app.pachli.core.data.model.Filter import app.pachli.core.data.model.NewFilterKeyword import app.pachli.core.data.repository.FilterEdit +import app.pachli.core.data.repository.FiltersError import app.pachli.core.data.repository.FiltersRepository import app.pachli.core.data.repository.NewFilter import app.pachli.core.model.Timeline @@ -132,7 +133,7 @@ class TimelineActivity : BottomSheetActivity(), AppBarLayoutHost, ActionButtonAc unmuteTagItem = menu.findItem(R.id.action_unmute_hashtag) followTagItem?.isVisible = tagEntity.following == false unfollowTagItem?.isVisible = tagEntity.following == true - updateMuteTagMenuItems() + updateMuteTagMenuItems(tag) }, { Timber.w(it, "Failed to query tag #%s", tag) @@ -230,18 +231,10 @@ class TimelineActivity : BottomSheetActivity(), AppBarLayoutHost, ActionButtonAc } /** - * Determine if the current hashtag is muted, and update the UI state accordingly. + * Determine if the given hashtag is muted, and update the UI state accordingly. */ - private fun updateMuteTagMenuItems() { - val tagWithHash = hashtag?.let { "#$it" } ?: return - - // If the server can't filter then it's impossible to mute hashtags, so disable - // the functionality. - if (!filtersRepository.canFilter()) { - muteTagItem?.isVisible = false - unmuteTagItem?.isVisible = false - return - } + private fun updateMuteTagMenuItems(tag: String) { + val tagWithHash = "#$tag" muteTagItem?.isVisible = true muteTagItem?.isEnabled = false @@ -256,6 +249,14 @@ class TimelineActivity : BottomSheetActivity(), AppBarLayoutHost, ActionButtonAc } updateTagMuteState(mutedFilter != null) } + result.onFailure { error -> + // If the server can't filter then it's impossible to mute hashtags, + // so disable the functionality. + if (error is FiltersError.ServerDoesNotFilter) { + muteTagItem?.isVisible = false + unmuteTagItem?.isVisible = false + } + } } } } diff --git a/app/src/main/java/app/pachli/components/preference/AccountPreferencesFragment.kt b/app/src/main/java/app/pachli/components/preference/AccountPreferencesFragment.kt index 8f433d6c9..bfa800add 100644 --- a/app/src/main/java/app/pachli/components/preference/AccountPreferencesFragment.kt +++ b/app/src/main/java/app/pachli/components/preference/AccountPreferencesFragment.kt @@ -19,7 +19,12 @@ package app.pachli.components.preference import android.content.Intent import android.os.Build import android.os.Bundle +import android.view.View import androidx.annotation.DrawableRes +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.lifecycleScope +import androidx.lifecycle.repeatOnLifecycle +import androidx.preference.Preference import androidx.preference.PreferenceFragmentCompat import app.pachli.BuildConfig import app.pachli.R @@ -55,11 +60,13 @@ import app.pachli.util.getInitialLanguages import app.pachli.util.getLocaleList import app.pachli.util.getPachliDisplayName import app.pachli.util.iconRes +import com.github.michaelbull.result.Ok import com.google.android.material.snackbar.Snackbar import com.mikepenz.iconics.IconicsDrawable import com.mikepenz.iconics.typeface.library.googlematerial.GoogleMaterial import dagger.hilt.android.AndroidEntryPoint import javax.inject.Inject +import kotlinx.coroutines.launch import retrofit2.Call import retrofit2.Callback import retrofit2.Response @@ -84,6 +91,28 @@ class AccountPreferencesFragment : PreferenceFragmentCompat() { private val iconSize by unsafeLazy { resources.getDimensionPixelSize(DR.dimen.preference_icon_size) } + /** + * The filter preference. + * + * Is enabled/disabled at runtime. + */ + private lateinit var filterPreference: Preference + + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { + viewLifecycleOwner.lifecycleScope.launch { + viewLifecycleOwner.repeatOnLifecycle(Lifecycle.State.RESUMED) { + // Enable/disable the filter preference based on info from + // FiltersRespository. filterPreferences is safe to access here, + // it was populated in onCreatePreferences, called by onCreate + // before onViewCreated is called. + filtersRepository.filters.collect { filters -> + filterPreference.isEnabled = filters is Ok + } + } + } + return super.onViewCreated(view, savedInstanceState) + } + override fun onCreatePreferences(savedInstanceState: Bundle?, rootKey: String?) { val context = requireContext() makePreferenceScreen { @@ -158,7 +187,7 @@ class AccountPreferencesFragment : PreferenceFragmentCompat() { } } - preference { + filterPreference = preference { setTitle(R.string.pref_title_timeline_filters) setIcon(R.drawable.ic_filter_24dp) setOnPreferenceClickListener { @@ -166,8 +195,9 @@ class AccountPreferencesFragment : PreferenceFragmentCompat() { activity?.startActivityWithTransition(intent, TransitionKind.SLIDE_FROM_END) true } - isEnabled = filtersRepository.canFilter() - if (!isEnabled) summary = context.getString(R.string.pref_summary_timeline_filters) + setSummaryProvider { + if (it.isEnabled) "" else context.getString(R.string.pref_summary_timeline_filters) + } } preferenceCategory(R.string.pref_publishing) { diff --git a/core/data/src/main/kotlin/app/pachli/core/data/repository/FiltersRepository.kt b/core/data/src/main/kotlin/app/pachli/core/data/repository/FiltersRepository.kt index 220de0b79..cdedeb09e 100644 --- a/core/data/src/main/kotlin/app/pachli/core/data/repository/FiltersRepository.kt +++ b/core/data/src/main/kotlin/app/pachli/core/data/repository/FiltersRepository.kt @@ -40,7 +40,6 @@ import com.github.michaelbull.result.Ok import com.github.michaelbull.result.Result import com.github.michaelbull.result.andThen import com.github.michaelbull.result.coroutines.binding.binding -import com.github.michaelbull.result.get import com.github.michaelbull.result.map import com.github.michaelbull.result.mapError import com.github.michaelbull.result.mapResult @@ -171,7 +170,7 @@ data class Filters( class FiltersRepository @Inject constructor( @ApplicationScope private val externalScope: CoroutineScope, private val mastodonApi: MastodonApi, - private val serverRepository: ServerRepository, + serverRepository: ServerRepository, ) { /** Flow where emissions trigger fresh loads from the server. */ private val reload = MutableSharedFlow(replay = 1).apply { tryEmit(Unit) } @@ -197,9 +196,6 @@ class FiltersRepository @Inject constructor( suspend fun reload() = reload.emit(Unit) - /** @return True if the user's server can filter, false otherwise. */ - fun canFilter() = server.get()?.let { it.canFilterV1() || it.canFilterV2() } ?: false - /** Get a specific filter from the server, by [filterId]. */ suspend fun getFilter(filterId: String): Result = binding { val server = server.bind()