Mastodon
/
Tusky-App-Android
mirror of https://github.com/tuskyapp/Tusky
1
0
Fork 0
Code Issues Releases Wiki Activity

Prevent parallel loading and fix duplicate ViewModel state collection in FiltersActivity (#4472) 

This pull request fixes the following issues:

- `FiltersActivity` launches a new coroutine to collect the ViewModel
state every time the Activity is resumed, without cancelling the
previous coroutine.
- `FiltersActivity` reloads the filters in `onResume()`, even if loading
is already in progress (without cancelling the current loading). This
can lead to inconsistent state.

List of improvements:
- Implement `launchAndRepeatOnLifecycle()` to combine
`coroutineScope.launch()` with `repeatOnLifecycle()` for the same
`Lifecycle`. Use it in `FiltersActivity` to update the view only when
the Activity is visible.
- Optimize the filters loading: load them when `FiltersViewModel` is
created and when returning from `EditFilterActivity` (when receiving the
Activity result). Cancel the load already in progress, if any.
- use `MutableStateFlow.update()` to update the state in a thread-safe
way.
- Turn `FiltersViewModel.deleteFilter()` into a suspending function in
order to perform the update in the coroutinescope of the Activity
lifecycle, so the View passed as argument doesn't leak.
- Wait for an ongoing load operation to complete before performing a
delete filter operation, so the state stays consistent.
- Add `Intent.withSlideInAnimation()` as a simpler and more flexible
alternative to `Activity.startActivityWithSlideInAnimation(Intent)`.
This commit is contained in:
Christophe Beyls 2024-05-31 13:42:21 +02:00 committed by GitHub
parent 0483440381
commit d200d1e15e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 101 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
app/src/main/java/com/keylesspalace/tusky/components/filters/FiltersActivity.kt
View File

@ -3,6 +3,7 @@ package com.keylesspalace.tusky.components.filters
import android.content.DialogInterface.BUTTON_POSITIVE
import android.content.Intent
import android.os.Bundle
import androidx.activity.result.contract.ActivityResultContracts
import androidx.activity.viewModels
import androidx.lifecycle.lifecycleScope
import com.keylesspalace.tusky.BaseActivity
@ -10,10 +11,11 @@ import com.keylesspalace.tusky.R
import com.keylesspalace.tusky.databinding.ActivityFiltersBinding
import com.keylesspalace.tusky.entity.Filter
import com.keylesspalace.tusky.util.hide
import com.keylesspalace.tusky.util.launchAndRepeatOnLifecycle
import com.keylesspalace.tusky.util.show
import com.keylesspalace.tusky.util.startActivityWithSlideInAnimation
import com.keylesspalace.tusky.util.viewBinding
import com.keylesspalace.tusky.util.visible
import com.keylesspalace.tusky.util.withSlideInAnimation
import dagger.hilt.android.AndroidEntryPoint
import kotlinx.coroutines.launch
@ -23,6 +25,12 @@ class FiltersActivity : BaseActivity(), FiltersListener {
private val binding by viewBinding(ActivityFiltersBinding::inflate)
private val viewModel: FiltersViewModel by viewModels()
private val editFilterLauncher =
registerForActivityResult(ActivityResultContracts.StartActivityForResult()) {
// refresh the filters upon returning from EditFilterActivity
reloadFilters()
}
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
@ -38,20 +46,16 @@ class FiltersActivity : BaseActivity(), FiltersListener {
launchEditFilterActivity()
}
binding.swipeRefreshLayout.setOnRefreshListener { loadFilters() }
binding.swipeRefreshLayout.setOnRefreshListener { reloadFilters() }
binding.swipeRefreshLayout.setColorSchemeResources(R.color.tusky_blue)
setTitle(R.string.pref_title_timeline_filters)
}
override fun onResume() {
super.onResume()
loadFilters()
observeViewModel()
}
private fun observeViewModel() {
lifecycleScope.launch {
launchAndRepeatOnLifecycle {
viewModel.state.collect { state ->
binding.progressBar.visible(
state.loadingState == FiltersViewModel.LoadingState.LOADING
@ -68,7 +72,7 @@ class FiltersActivity : BaseActivity(), FiltersListener {
R.drawable.errorphant_offline,
R.string.error_network
) {
loadFilters()
reloadFilters()
}
binding.messageView.show()
}
@ -77,7 +81,7 @@ class FiltersActivity : BaseActivity(), FiltersListener {
R.drawable.errorphant_error,
R.string.error_generic
) {
loadFilters()
reloadFilters()
}
binding.messageView.show()
}
@ -99,8 +103,8 @@ class FiltersActivity : BaseActivity(), FiltersListener {
}
}
private fun loadFilters() {
viewModel.load()
private fun reloadFilters() {
viewModel.reload()
}
private fun launchEditFilterActivity(filter: Filter? = null) {
@ -108,8 +112,8 @@ class FiltersActivity : BaseActivity(), FiltersListener {
if (filter != null) {
putExtra(EditFilterActivity.FILTER_TO_EDIT, filter)
}
}
startActivityWithSlideInAnimation(intent)
}.withSlideInAnimation()
editFilterLauncher.launch(intent)
}
override fun deleteFilter(filter: Filter) {

48
app/src/main/java/com/keylesspalace/tusky/components/filters/FiltersViewModel.kt
View File

@ -15,6 +15,9 @@ import javax.inject.Inject
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.flow.collectLatest
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.launch
@HiltViewModel
@ -36,10 +39,18 @@ class FiltersViewModel @Inject constructor(
private val _state = MutableStateFlow(State(emptyList(), LoadingState.INITIAL))
val state: StateFlow<State> = _state.asStateFlow()
fun load() {
this@FiltersViewModel._state.value = _state.value.copy(loadingState = LoadingState.LOADING)
private val loadTrigger = MutableStateFlow(0)
init {
viewModelScope.launch {
observeLoad()
}
}
private suspend fun observeLoad() {
loadTrigger.collectLatest {
this@FiltersViewModel._state.update { it.copy(loadingState = LoadingState.LOADING) }
api.getFilters().fold(
{ filters ->
this@FiltersViewModel._state.value = State(filters, LoadingState.LOADED)
@ -52,28 +63,34 @@ class FiltersViewModel @Inject constructor(
},
{ _ ->
// TODO log errors (also below)
this@FiltersViewModel._state.value = _state.value.copy(loadingState = LoadingState.ERROR_OTHER)
this@FiltersViewModel._state.update { it.copy(loadingState = LoadingState.ERROR_OTHER) }
}
)
this@FiltersViewModel._state.value = _state.value.copy(loadingState = LoadingState.ERROR_OTHER)
this@FiltersViewModel._state.update { it.copy(loadingState = LoadingState.ERROR_OTHER) }
} else {
this@FiltersViewModel._state.value = _state.value.copy(loadingState = LoadingState.ERROR_NETWORK)
this@FiltersViewModel._state.update { it.copy(loadingState = LoadingState.ERROR_NETWORK) }
}
}
)
}
}
fun deleteFilter(filter: Filter, parent: View) {
viewModelScope.launch {
fun reload() {
loadTrigger.update { it + 1 }
}
suspend fun deleteFilter(filter: Filter, parent: View) {
// First wait for a pending loading operation to complete
_state.first { it.loadingState > LoadingState.LOADING }
api.deleteFilter(filter.id).fold(
{
this@FiltersViewModel._state.value = State(
this@FiltersViewModel._state.value.filters.filter {
it.id != filter.id
},
this@FiltersViewModel._state.update { currentState ->
State(
currentState.filters.filter { it.id != filter.id },
LoadingState.LOADED
)
}
for (context in filter.context) {
eventHub.dispatch(PreferenceChangedEvent(context))
}
@ -82,12 +99,12 @@ class FiltersViewModel @Inject constructor(
if (throwable.isHttpNotFound()) {
api.deleteFilterV1(filter.id).fold(
{
this@FiltersViewModel._state.value = State(
this@FiltersViewModel._state.value.filters.filter {
it.id != filter.id
},
this@FiltersViewModel._state.update { currentState ->
State(
currentState.filters.filter { it.id != filter.id },
LoadingState.LOADED
)
}
},
{
Snackbar.make(
@ -107,5 +124,4 @@ class FiltersViewModel @Inject constructor(
}
)
}
}
}

7
app/src/main/java/com/keylesspalace/tusky/util/ActivityExtensions.kt
View File

@ -12,10 +12,13 @@ import androidx.lifecycle.LifecycleEventObserver
import com.keylesspalace.tusky.BaseActivity
fun Activity.startActivityWithSlideInAnimation(intent: Intent) {
startActivity(intent.withSlideInAnimation())
}
fun Intent.withSlideInAnimation(): Intent {
// the new transition api needs to be called by the activity that is the result of the transition,
// so we pass a flag that BaseActivity will respect.
intent.putExtra(BaseActivity.OPEN_WITH_SLIDE_IN, true)
startActivity(intent)
return putExtra(BaseActivity.OPEN_WITH_SLIDE_IN, true)
}
/**

21
app/src/main/java/com/keylesspalace/tusky/util/LifecycleExtensions.kt Normal file
View File

@ -0,0 +1,21 @@
package com.keylesspalace.tusky.util
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.coroutineScope
import androidx.lifecycle.repeatOnLifecycle
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.launch
fun Lifecycle.launchAndRepeatOnLifecycle(
state: Lifecycle.State = Lifecycle.State.STARTED,
block: suspend CoroutineScope.() -> Unit
): Job = coroutineScope.launch {
repeatOnLifecycle(state, block)
}
fun LifecycleOwner.launchAndRepeatOnLifecycle(
state: Lifecycle.State = Lifecycle.State.STARTED,
block: suspend CoroutineScope.() -> Unit
): Job = lifecycle.launchAndRepeatOnLifecycle(state, block)