chinwag/chinwag-android
chinwag/chinwag-android
2
0
Fork 0
Code Issues Pull requests Releases Activity

fix race condition where multiple uploaded media can get same internal id (#2479)

Browse source 
* fix race condition where multiple uploaded media can get same internal id

* atomically update media stateflow

* atomically update media stateflow
This commit is contained in:
Konrad Pozniak 2022-05-03 19:12:35 +02:00 committed by GitHub
parent ce5ec15ff1
commit 444e7365c9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 72 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeActivity.kt
View file

@ -51,6 +51,7 @@ import androidx.core.view.ContentInfoCompat
import androidx.core.view.OnReceiveContentListener import androidx.core.view.OnReceiveContentListener
import androidx.core.view.isGone import androidx.core.view.isGone
import androidx.core.view.isVisible import androidx.core.view.isVisible
import androidx.lifecycle.asLiveData
import androidx.lifecycle.lifecycleScope import androidx.lifecycle.lifecycleScope
import androidx.preference.PreferenceManager import androidx.preference.PreferenceManager
import androidx.recyclerview.widget.LinearLayoutManager import androidx.recyclerview.widget.LinearLayoutManager
@ -344,14 +345,17 @@ class ComposeActivity :
viewModel.statusVisibility.observe { visibility -> viewModel.statusVisibility.observe { visibility ->
setStatusVisibility(visibility) setStatusVisibility(visibility)
} }
viewModel.media.observe { media -> lifecycleScope.launch {
mediaAdapter.submitList(media) viewModel.media.collect { media ->
if (media.size != mediaCount) { mediaAdapter.submitList(media)
mediaCount = media.size if (media.size != mediaCount) {
binding.composeMediaPreviewBar.visible(media.isNotEmpty()) mediaCount = media.size
updateSensitiveMediaToggle(viewModel.markMediaAsSensitive.value != false, viewModel.showContentWarning.value != false) binding.composeMediaPreviewBar.visible(media.isNotEmpty())
updateSensitiveMediaToggle(viewModel.markMediaAsSensitive.value != false, viewModel.showContentWarning.value != false)
}
} }
} }
viewModel.poll.observe { poll -> viewModel.poll.observe { poll ->
binding.pollPreview.visible(poll != null) binding.pollPreview.visible(poll != null)
poll?.let(binding.pollPreview::setPoll) poll?.let(binding.pollPreview::setPoll)
@ -364,7 +368,7 @@ class ComposeActivity :
} }
updateScheduleButton() updateScheduleButton()
} }
combineOptionalLiveData(viewModel.media, viewModel.poll) { media, poll -> combineOptionalLiveData(viewModel.media.asLiveData(), viewModel.poll) { media, poll ->
val active = poll == null && val active = poll == null &&
media!!.size != 4 && media!!.size != 4 &&
(media.isEmpty() || media.first().type == QueuedMedia.Type.IMAGE) (media.isEmpty() || media.first().type == QueuedMedia.Type.IMAGE)
@ -781,11 +785,11 @@ class ComposeActivity :
spoilerText = binding.composeContentWarningField.text.toString() spoilerText = binding.composeContentWarningField.text.toString()
} }
val characterCount = calculateTextLength() val characterCount = calculateTextLength()
if ((characterCount <= 0 || contentText.isBlank()) && viewModel.media.value!!.isEmpty()) { if ((characterCount <= 0 || contentText.isBlank()) && viewModel.media.value.isEmpty()) {
binding.composeEditField.error = getString(R.string.error_empty) binding.composeEditField.error = getString(R.string.error_empty)
enableButtons(true) enableButtons(true)
} else if (characterCount <= maximumTootCharacters) { } else if (characterCount <= maximumTootCharacters) {
if (viewModel.media.value!!.isNotEmpty()) { if (viewModel.media.value.isNotEmpty()) {
finishingUploadDialog = ProgressDialog.show( finishingUploadDialog = ProgressDialog.show(
this, getString(R.string.dialog_title_finishing_media_upload), this, getString(R.string.dialog_title_finishing_media_upload),
getString(R.string.dialog_message_uploading_media), true, true getString(R.string.dialog_message_uploading_media), true, true
@ -983,7 +987,7 @@ class ComposeActivity :
} }
data class QueuedMedia( data class QueuedMedia(
val localId: Long, val localId: Int,
val uri: Uri, val uri: Uri,
val type: Type, val type: Type,
val mediaSize: Long, val mediaSize: Long,

99
app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt
View file

@ -21,6 +21,7 @@ import androidx.core.net.toUri
import androidx.lifecycle.LiveData import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModel
import androidx.lifecycle.asLiveData
import androidx.lifecycle.viewModelScope import androidx.lifecycle.viewModelScope
import com.keylesspalace.tusky.components.compose.ComposeActivity.QueuedMedia import com.keylesspalace.tusky.components.compose.ComposeActivity.QueuedMedia
import com.keylesspalace.tusky.components.drafts.DraftHelper import com.keylesspalace.tusky.components.drafts.DraftHelper
@ -36,15 +37,17 @@ import com.keylesspalace.tusky.network.MastodonApi
import com.keylesspalace.tusky.service.ServiceClient import com.keylesspalace.tusky.service.ServiceClient
import com.keylesspalace.tusky.service.StatusToSend import com.keylesspalace.tusky.service.StatusToSend
import com.keylesspalace.tusky.util.combineLiveData import com.keylesspalace.tusky.util.combineLiveData
import com.keylesspalace.tusky.util.filter
import com.keylesspalace.tusky.util.map
import com.keylesspalace.tusky.util.randomAlphanumericString import com.keylesspalace.tusky.util.randomAlphanumericString
import com.keylesspalace.tusky.util.toLiveData import com.keylesspalace.tusky.util.toLiveData
import com.keylesspalace.tusky.util.withoutFirstWhich
import io.reactivex.rxjava3.core.Observable import io.reactivex.rxjava3.core.Observable
import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job import kotlinx.coroutines.Job
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.catch
import kotlinx.coroutines.flow.filter
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.flow.updateAndGet
import kotlinx.coroutines.launch import kotlinx.coroutines.launch
import kotlinx.coroutines.rx3.rxSingle import kotlinx.coroutines.rx3.rxSingle
import kotlinx.coroutines.withContext import kotlinx.coroutines.withContext
@ -84,10 +87,10 @@ class ComposeViewModel @Inject constructor(
val poll: MutableLiveData<NewPoll?> = mutableLiveData(null) val poll: MutableLiveData<NewPoll?> = mutableLiveData(null)
val scheduledAt: MutableLiveData<String?> = mutableLiveData(null) val scheduledAt: MutableLiveData<String?> = mutableLiveData(null)
val media = mutableLiveData<List<QueuedMedia>>(listOf()) val media: MutableStateFlow<List<QueuedMedia>> = MutableStateFlow(emptyList())
val uploadError = MutableLiveData<Throwable>() val uploadError = MutableLiveData<Throwable>()
private val mediaToJob = mutableMapOf<Long, Job>() private val mediaToJob = mutableMapOf<Int, Job>()
private val isEditingScheduledToot get() = !scheduledTootId.isNullOrEmpty() private val isEditingScheduledToot get() = !scheduledTootId.isNullOrEmpty()
@ -103,7 +106,7 @@ class ComposeViewModel @Inject constructor(
suspend fun pickMedia(mediaUri: Uri, description: String? = null): Result<QueuedMedia> = withContext(Dispatchers.IO) { suspend fun pickMedia(mediaUri: Uri, description: String? = null): Result<QueuedMedia> = withContext(Dispatchers.IO) {
try { try {
val (type, uri, size) = mediaUploader.prepareMedia(mediaUri) val (type, uri, size) = mediaUploader.prepareMedia(mediaUri)
val mediaItems = media.value!! val mediaItems = media.value
if (type != QueuedMedia.Type.IMAGE && if (type != QueuedMedia.Type.IMAGE &&
mediaItems.isNotEmpty() && mediaItems.isNotEmpty() &&
mediaItems[0].type == QueuedMedia.Type.IMAGE mediaItems[0].type == QueuedMedia.Type.IMAGE
@ -118,29 +121,31 @@ class ComposeViewModel @Inject constructor(
} }
} }
private fun addMediaToQueue( private suspend fun addMediaToQueue(
type: QueuedMedia.Type, type: QueuedMedia.Type,
uri: Uri, uri: Uri,
mediaSize: Long, mediaSize: Long,
description: String? = null description: String? = null
): QueuedMedia { ): QueuedMedia {
val mediaItem = QueuedMedia( val mediaItem = media.updateAndGet { mediaValue ->
localId = System.currentTimeMillis(), val mediaItem = QueuedMedia(
uri = uri, localId = (mediaValue.maxOfOrNull { it.localId } ?: 0) + 1,
type = type, uri = uri,
mediaSize = mediaSize, type = type,
description = description mediaSize = mediaSize,
) description = description
media.postValue(media.value!! + mediaItem) )
mediaValue + mediaItem
}.last()
mediaToJob[mediaItem.localId] = viewModelScope.launch { mediaToJob[mediaItem.localId] = viewModelScope.launch {
mediaUploader mediaUploader
.uploadMedia(mediaItem) .uploadMedia(mediaItem)
.catch { error -> .catch { error ->
media.postValue(media.value?.filter { it.localId != mediaItem.localId } ?: emptyList()) media.update { mediaValue -> mediaValue.filter { it.localId != mediaItem.localId } }
uploadError.postValue(error) uploadError.postValue(error)
} }
.collect { event -> .collect { event ->
val item = media.value?.find { it.localId == mediaItem.localId } val item = media.value.find { it.localId == mediaItem.localId }
?: return@collect ?: return@collect
val newMediaItem = when (event) { val newMediaItem = when (event) {
is UploadEvent.ProgressEvent -> is UploadEvent.ProgressEvent ->
@ -148,16 +153,14 @@ class ComposeViewModel @Inject constructor(
is UploadEvent.FinishedEvent -> is UploadEvent.FinishedEvent ->
item.copy(id = event.mediaId, uploadPercent = -1) item.copy(id = event.mediaId, uploadPercent = -1)
} }
synchronized(media) { media.update { mediaValue ->
val mediaValue = media.value!! mediaValue.map { mediaItem ->
val index = mediaValue.indexOfFirst { it.localId == newMediaItem.localId } if (mediaItem.localId == newMediaItem.localId) {
media.postValue( newMediaItem
if (index == -1) {
mediaValue + newMediaItem
} else { } else {
mediaValue.toMutableList().also { it[index] = newMediaItem } mediaItem
} }
) }
} }
} }
} }
@ -165,13 +168,23 @@ class ComposeViewModel @Inject constructor(
} }
private fun addUploadedMedia(id: String, type: QueuedMedia.Type, uri: Uri, description: String?) { private fun addUploadedMedia(id: String, type: QueuedMedia.Type, uri: Uri, description: String?) {
val mediaItem = QueuedMedia(System.currentTimeMillis(), uri, type, 0, -1, id, description) media.update { mediaValue ->
media.value = media.value!! + mediaItem val mediaItem = QueuedMedia(
localId = (mediaValue.maxOfOrNull { it.localId } ?: 0) + 1,
uri = uri,
type = type,
mediaSize = 0,
uploadPercent = -1,
id = id,
description = description
)
mediaValue + mediaItem
}
} }
fun removeMediaFromQueue(item: QueuedMedia) { fun removeMediaFromQueue(item: QueuedMedia) {
mediaToJob[item.localId]?.cancel() mediaToJob[item.localId]?.cancel()
media.value = media.value!!.withoutFirstWhich { it.localId == item.localId } media.update { mediaValue -> mediaValue.filter { it.localId == item.localId } }
} }
fun toggleMarkSensitive() { fun toggleMarkSensitive() {
@ -211,7 +224,7 @@ class ComposeViewModel @Inject constructor(
viewModelScope.launch { viewModelScope.launch {
val mediaUris: MutableList<String> = mutableListOf() val mediaUris: MutableList<String> = mutableListOf()
val mediaDescriptions: MutableList<String?> = mutableListOf() val mediaDescriptions: MutableList<String?> = mutableListOf()
media.value?.forEach { item -> media.value.forEach { item ->
mediaUris.add(item.uri.toString()) mediaUris.add(item.uri.toString())
mediaDescriptions.add(item.description) mediaDescriptions.add(item.description)
} }
@ -248,14 +261,14 @@ class ComposeViewModel @Inject constructor(
Observable.just(Unit) Observable.just(Unit)
}.toLiveData() }.toLiveData()
val sendObservable = media val sendFlow = media
.filter { items -> items.all { it.uploadPercent == -1 } } .filter { items -> items.all { it.uploadPercent == -1 } }
.map { .map {
val mediaIds: MutableList<String> = mutableListOf() val mediaIds: MutableList<String> = mutableListOf()
val mediaUris: MutableList<Uri> = mutableListOf() val mediaUris: MutableList<Uri> = mutableListOf()
val mediaDescriptions: MutableList<String> = mutableListOf() val mediaDescriptions: MutableList<String> = mutableListOf()
val mediaProcessed: MutableList<Boolean> = mutableListOf() val mediaProcessed: MutableList<Boolean> = mutableListOf()
for (item in media.value!!) { for (item in media.value) {
mediaIds.add(item.id!!) mediaIds.add(item.id!!)
mediaUris.add(item.uri) mediaUris.add(item.uri)
mediaDescriptions.add(item.description ?: "") mediaDescriptions.add(item.description ?: "")
@ -285,17 +298,21 @@ class ComposeViewModel @Inject constructor(
serviceClient.sendToot(tootToSend) serviceClient.sendToot(tootToSend)
} }
return combineLiveData(deletionObservable, sendObservable) { _, _ -> } return combineLiveData(deletionObservable, sendFlow.asLiveData()) { _, _ -> }
} }
suspend fun updateDescription(localId: Long, description: String): Boolean { suspend fun updateDescription(localId: Int, description: String): Boolean {
val newList = media.value!!.toMutableList() val newMediaList = media.updateAndGet { mediaValue ->
val index = newList.indexOfFirst { it.localId == localId } mediaValue.map { mediaItem ->
if (index != -1) { if (mediaItem.localId == localId) {
newList[index] = newList[index].copy(description = description) mediaItem.copy(description = description)
} else {
mediaItem
}
}
} }
media.value = newList
val updatedItem = newList.find { it.localId == localId } val updatedItem = newMediaList.find { it.localId == localId }
if (updatedItem?.id != null) { if (updatedItem?.id != null) {
return api.updateMedia(updatedItem.id, description) return api.updateMedia(updatedItem.id, description)
.fold({ .fold({
@ -387,8 +404,8 @@ class ComposeViewModel @Inject constructor(
val draftAttachments = composeOptions?.draftAttachments val draftAttachments = composeOptions?.draftAttachments
if (draftAttachments != null) { if (draftAttachments != null) {
// when coming from DraftActivity // when coming from DraftActivity
draftAttachments.forEach { attachment -> viewModelScope.launch {
viewModelScope.launch { draftAttachments.forEach { attachment ->
pickMedia(attachment.uri, attachment.description) pickMedia(attachment.uri, attachment.description)
} }
} }