From e2179a66698fe7be4f158b8a63ac1f96bc4e5b28 Mon Sep 17 00:00:00 2001 From: arkon Date: Tue, 22 Nov 2022 23:12:23 -0500 Subject: [PATCH] Avoid concurrency issues when reordering categories Maybe fixes #8372 --- .../category/interactor/ReorderCategory.kt | 68 ++++++++++++------- .../components/ChangeCategoryDialog.kt | 3 +- .../components/DeleteLibraryMangaDialog.kt | 3 +- .../ui/category/CategoryScreenModel.kt | 16 ++--- 4 files changed, 53 insertions(+), 37 deletions(-) diff --git a/app/src/main/java/eu/kanade/domain/category/interactor/ReorderCategory.kt b/app/src/main/java/eu/kanade/domain/category/interactor/ReorderCategory.kt index dfd591a467..11b4318b96 100644 --- a/app/src/main/java/eu/kanade/domain/category/interactor/ReorderCategory.kt +++ b/app/src/main/java/eu/kanade/domain/category/interactor/ReorderCategory.kt @@ -5,46 +5,66 @@ import eu.kanade.domain.category.model.CategoryUpdate import eu.kanade.domain.category.repository.CategoryRepository import eu.kanade.tachiyomi.util.lang.withNonCancellableContext import eu.kanade.tachiyomi.util.system.logcat +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import logcat.LogPriority +import java.util.Collections class ReorderCategory( private val categoryRepository: CategoryRepository, ) { - suspend fun await(categoryId: Long, newPosition: Int) = withNonCancellableContext { - val categories = categoryRepository.getAll().filterNot(Category::isSystemCategory) + private val mutex = Mutex() - val currentIndex = categories.indexOfFirst { it.id == categoryId } - if (currentIndex == newPosition) { - return@withNonCancellableContext Result.Unchanged - } + suspend fun moveUp(category: Category): Result = + await(category, MoveTo.UP) - val reorderedCategories = categories.toMutableList() - val reorderedCategory = reorderedCategories.removeAt(currentIndex) - reorderedCategories.add(newPosition, reorderedCategory) + suspend fun moveDown(category: Category): Result = + await(category, MoveTo.DOWN) - val updates = reorderedCategories.mapIndexed { index, category -> - CategoryUpdate( - id = category.id, - order = index.toLong(), - ) - } + private suspend fun await(category: Category, moveTo: MoveTo) = withNonCancellableContext { + mutex.withLock { + val categories = categoryRepository.getAll() + .filterNot(Category::isSystemCategory) + .toMutableList() - try { - categoryRepository.updatePartial(updates) - Result.Success - } catch (e: Exception) { - logcat(LogPriority.ERROR, e) - Result.InternalError(e) + val newPosition = when (moveTo) { + MoveTo.UP -> category.order - 1 + MoveTo.DOWN -> category.order + 1 + }.toInt() + + val currentIndex = categories.indexOfFirst { it.id == category.id } + if (currentIndex == newPosition) { + return@withNonCancellableContext Result.Unchanged + } + + Collections.swap(categories, currentIndex, newPosition) + + val updates = categories.mapIndexed { index, category -> + CategoryUpdate( + id = category.id, + order = index.toLong(), + ) + } + + try { + categoryRepository.updatePartial(updates) + Result.Success + } catch (e: Exception) { + logcat(LogPriority.ERROR, e) + Result.InternalError(e) + } } } - suspend fun await(category: Category, newPosition: Long): Result = - await(category.id, newPosition.toInt()) - sealed class Result { object Success : Result() object Unchanged : Result() data class InternalError(val error: Throwable) : Result() } + + private enum class MoveTo { + UP, + DOWN, + } } diff --git a/app/src/main/java/eu/kanade/presentation/components/ChangeCategoryDialog.kt b/app/src/main/java/eu/kanade/presentation/components/ChangeCategoryDialog.kt index b3d611a748..24ce78a988 100644 --- a/app/src/main/java/eu/kanade/presentation/components/ChangeCategoryDialog.kt +++ b/app/src/main/java/eu/kanade/presentation/components/ChangeCategoryDialog.kt @@ -96,8 +96,7 @@ fun ChangeCategoryDialog( val index = selection.indexOf(it) if (index != -1) { val mutableList = selection.toMutableList() - mutableList.removeAt(index) - mutableList.add(index, it.next()) + mutableList[index] = it.next() selection = mutableList.toList() } } diff --git a/app/src/main/java/eu/kanade/presentation/components/DeleteLibraryMangaDialog.kt b/app/src/main/java/eu/kanade/presentation/components/DeleteLibraryMangaDialog.kt index 8154d55089..1411a57556 100644 --- a/app/src/main/java/eu/kanade/presentation/components/DeleteLibraryMangaDialog.kt +++ b/app/src/main/java/eu/kanade/presentation/components/DeleteLibraryMangaDialog.kt @@ -64,8 +64,7 @@ fun DeleteLibraryMangaDialog( val onCheck = { val index = list.indexOf(state) val mutableList = list.toMutableList() - mutableList.removeAt(index) - mutableList.add(index, state.next() as CheckboxState.State) + mutableList[index] = state.next() as CheckboxState.State list = mutableList.toList() } diff --git a/app/src/main/java/eu/kanade/tachiyomi/ui/category/CategoryScreenModel.kt b/app/src/main/java/eu/kanade/tachiyomi/ui/category/CategoryScreenModel.kt index 9b6cdb6792..19ee7df94f 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/ui/category/CategoryScreenModel.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/ui/category/CategoryScreenModel.kt @@ -48,7 +48,7 @@ class CategoryScreenModel( when (createCategoryWithName.await(name)) { is CreateCategoryWithName.Result.InternalError -> _events.send(CategoryEvent.InternalError) CreateCategoryWithName.Result.NameAlreadyExistsError -> _events.send(CategoryEvent.CategoryWithNameAlreadyExists) - CreateCategoryWithName.Result.Success -> {} + else -> {} } } } @@ -57,27 +57,25 @@ class CategoryScreenModel( coroutineScope.launch { when (deleteCategory.await(categoryId = categoryId)) { is DeleteCategory.Result.InternalError -> _events.send(CategoryEvent.InternalError) - DeleteCategory.Result.Success -> {} + else -> {} } } } fun moveUp(category: Category) { coroutineScope.launch { - when (reorderCategory.await(category, category.order - 1)) { + when (reorderCategory.moveUp(category)) { is ReorderCategory.Result.InternalError -> _events.send(CategoryEvent.InternalError) - ReorderCategory.Result.Success -> {} - ReorderCategory.Result.Unchanged -> {} + else -> {} } } } fun moveDown(category: Category) { coroutineScope.launch { - when (reorderCategory.await(category, category.order + 1)) { + when (reorderCategory.moveDown(category)) { is ReorderCategory.Result.InternalError -> _events.send(CategoryEvent.InternalError) - ReorderCategory.Result.Success -> {} - ReorderCategory.Result.Unchanged -> {} + else -> {} } } } @@ -87,7 +85,7 @@ class CategoryScreenModel( when (renameCategory.await(category, name)) { is RenameCategory.Result.InternalError -> _events.send(CategoryEvent.InternalError) RenameCategory.Result.NameAlreadyExistsError -> _events.send(CategoryEvent.CategoryWithNameAlreadyExists) - RenameCategory.Result.Success -> {} + else -> {} } } }