Skip to content

Commit

Permalink
Fix already used name not being updated quickly enough
Browse files Browse the repository at this point in the history
When a file is being imported, if a new file with the same name was picked, it would reuse a duplicated name and crash the UI
  • Loading branch information
LunarX committed Oct 18, 2024
1 parent 12cea99 commit a57de00
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import com.infomaniak.swisstransfer.ui.components.FileUi
import com.infomaniak.swisstransfer.ui.screen.newtransfer.TransferFilesManager.PickedFile
import dagger.hilt.android.lifecycle.HiltViewModel
import dagger.hilt.android.qualifiers.ApplicationContext
import io.sentry.Sentry
import io.sentry.SentryLevel
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.FlowPreview
import kotlinx.coroutines.channels.Channel
Expand All @@ -45,6 +47,11 @@ class NewTransferViewModel @Inject constructor(
private val savedStateHandle: SavedStateHandle,
) : ViewModel() {

// Importing a file locally can take up time. We can't base the list of already used names on _files because a new import with
// the same name could occur while the file is not finished importing yet.
// This list needs to mark a name a "taken" as soon as the file is queued to be imported and until the file is removed from
// the list of already imported files we listen to in the LazyRow.
private val alreadyUsedFileNames = AlreadyUsedFileNamesSet()
private val _files = MutableStateFlow<List<FileUi>>(emptyList())
private val files: StateFlow<List<FileUi>> = _files
@OptIn(FlowPreview::class)
Expand Down Expand Up @@ -91,25 +98,33 @@ class NewTransferViewModel @Inject constructor(

fun addFiles(uris: List<Uri>) {
viewModelScope.launch(Dispatchers.IO) {
val alreadyUsedFileNames = buildSet { files.value.forEach { add(it.fileName) } }
val newFiles = transferFilesManager.getFiles(uris, alreadyUsedFileNames)
val newFiles = transferFilesManager.getFiles(uris, isAlreadyUsed = { alreadyUsedFileNames.contains(it) })

alreadyUsedFileNames.addAll(newFiles.map { it.fileName })

newFiles.forEach { filesToImport.send(it) }
}
}

fun removeFileByUid(uid: String) {
viewModelScope.launch(Dispatchers.IO) {
var fileName: String? = null

filesMutationMutex.withLock {
val files = _files.value.toMutableList()

val index = files.indexOfFirst { it.uid == uid }.takeIf { it != -1 } ?: return@withLock
val fileToRemove = files.removeAt(index)
fileName = fileToRemove.fileName

runCatching { File(fileToRemove.uri).delete() }

_files.value = files
}

fileName?.let {
alreadyUsedFileNames.remove(it)
}
}
}

Expand All @@ -126,9 +141,9 @@ class NewTransferViewModel @Inject constructor(
}
}

filesMutationMutex.withLock {
_files.value = restoredFileData
}
alreadyUsedFileNames.addAll(restoredFileData.map { it.fileName })

filesMutationMutex.withLock { _files.value += restoredFileData }
}

private fun removeLocalCopyFolder() {
Expand Down Expand Up @@ -217,6 +232,15 @@ class NewTransferViewModel @Inject constructor(
}
}

class AlreadyUsedFileNamesSet {
private val alreadyUsedFileNames = mutableSetOf<String>()
private val mutex = Mutex()

suspend fun contains(fileName: String): Boolean = mutex.withLock { alreadyUsedFileNames.contains(fileName) }
suspend fun addAll(filesNames: List<String>): Boolean = mutex.withLock { alreadyUsedFileNames.addAll(filesNames) }
suspend fun remove(filesName: String): Boolean = mutex.withLock { alreadyUsedFileNames.remove(filesName) }
}

companion object {
private const val TAG = "File importation"
const val LOCAL_COPY_FOLDER = "local_copy_folder"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,22 @@ import androidx.core.net.toUri
import com.infomaniak.swisstransfer.ui.components.FileUi
import com.infomaniak.swisstransfer.ui.utils.FileNameUtils
import dagger.hilt.android.qualifiers.ApplicationContext
import io.sentry.Sentry
import java.io.File
import javax.inject.Inject
import javax.inject.Singleton

@Singleton
class TransferFilesManager @Inject constructor(@ApplicationContext private val appContext: Context) {
fun getFiles(uris: List<Uri>, alreadyUsedFileNames: Set<String>): MutableSet<PickedFile> {
val currentUsedFileNames = alreadyUsedFileNames.toMutableSet()
val files = mutableSetOf<PickedFile>()
suspend fun getFiles(uris: List<Uri>, isAlreadyUsed: suspend (String) -> Boolean): Set<PickedFile> {
val currentUsedFileNames = mutableSetOf<String>()

uris.forEach { uri ->
getFile(uri, currentUsedFileNames)?.let { file ->
currentUsedFileNames += file.fileName
files += file
val files = buildSet {
uris.forEach { uri ->
getFile(uri, isAlreadyUsed = { isAlreadyUsed(it) || currentUsedFileNames.contains(it) })?.let { file ->
currentUsedFileNames += file.fileName
add(file)
}
}
}

Expand All @@ -62,12 +64,12 @@ class TransferFilesManager @Inject constructor(@ApplicationContext private val a
}
}

private fun getFile(uri: Uri, alreadyUsedFileNames: Set<String>): PickedFile? {
private suspend fun getFile(uri: Uri, isAlreadyUsed: suspend (String) -> Boolean): PickedFile? {
val contentResolver: ContentResolver = appContext.contentResolver
val cursor: Cursor? = contentResolver.query(uri, null, null, null, null)

return cursor?.getFileNameAndSize()?.let { (name, size) ->
val uniqueName = FileNameUtils.postfixExistingFileNames(name, alreadyUsedFileNames)
val uniqueName = FileNameUtils.postfixExistingFileNames(name, isAlreadyUsed)
PickedFile(uniqueName, size, uri)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ package com.infomaniak.swisstransfer.ui.utils

object FileNameUtils {

fun postfixExistingFileNames(fileName: String, existingFileNames: Set<String>): String {
return if (fileName in existingFileNames) {
suspend fun postfixExistingFileNames(fileName: String, isAlreadyUsed: suspend (String) -> Boolean): String {
return if (isAlreadyUsed(fileName)) {
val postfixedFileName = PostfixedFileName.fromFileName(fileName)

while (postfixedFileName.fullName() in existingFileNames) {
while (isAlreadyUsed(postfixedFileName.fullName())) {
postfixedFileName.incrementPostfix()
}

Expand Down

0 comments on commit a57de00

Please sign in to comment.