From c2cf18827a0ab5fa8947f6ee64bb60d6ce4ab33f Mon Sep 17 00:00:00 2001
From: Gibran Chevalley <gibran.chevalley@infomaniak.com>
Date: Mon, 21 Oct 2024 13:55:52 +0200
Subject: [PATCH] Simplify and remove bug potentiel from alreadyUsedFiles
 record of names

---
 .../newtransfer/ImportationFilesManager.kt    | 38 ++++++++++---------
 .../newtransfer/NewTransferViewModel.kt       |  2 +-
 .../swisstransfer/ui/utils/FileNameUtils.kt   |  2 +-
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/ImportationFilesManager.kt b/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/ImportationFilesManager.kt
index 0a2dc505f..ec3f8024d 100644
--- a/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/ImportationFilesManager.kt
+++ b/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/ImportationFilesManager.kt
@@ -61,11 +61,7 @@ class ImportationFilesManager @Inject constructor(@ApplicationContext private va
     private fun getGetLocalCopyFolderOrCopy() = localCopyFolder.apply { if (!exists()) mkdirs() }
 
     suspend fun addFiles(uris: List<Uri>) {
-        val newFiles = getFiles(uris, isAlreadyUsed = { alreadyUsedFileNames.contains(it) })
-
-        alreadyUsedFileNames.addAll(newFiles.map { it.fileName })
-
-        newFiles.forEach { filesToImport.send(it) }
+        getFiles(uris).forEach { filesToImport.send(it) }
     }
 
     suspend fun removeFileByUid(uid: String) {
@@ -95,7 +91,7 @@ class ImportationFilesManager @Inject constructor(@ApplicationContext private va
         _importedFiles.addAll(restoredFileData)
     }
 
-    suspend fun copyPickedFilesToLocalStorage() {
+    suspend fun continuouslyCopyPickedFilesToLocalStorage() {
         filesToImport.consume { fileToImport ->
             Log.i(TAG, "Importing ${fileToImport.uri}")
             val copiedFile = copyFileLocally(fileToImport.uri, fileToImport.fileName)
@@ -135,27 +131,32 @@ class ImportationFilesManager @Inject constructor(@ApplicationContext private va
         }
     }
 
-    private suspend fun getFiles(uris: List<Uri>, isAlreadyUsed: suspend (String) -> Boolean): Set<PickedFile> {
-        val currentUsedFileNames = mutableSetOf<String>()
-
+    private suspend fun getFiles(uris: List<Uri>): Set<PickedFile> {
         val files = buildSet {
             uris.forEach { uri ->
-                getFile(uri, isAlreadyUsed = { isAlreadyUsed(it) || currentUsedFileNames.contains(it) })?.let { file ->
-                    currentUsedFileNames += file.fileName
-                    add(file)
-                }
+                getFile(uri)?.let(::add)
             }
         }
 
         return files
     }
 
-    private suspend fun getFile(uri: Uri, isAlreadyUsed: suspend (String) -> Boolean): PickedFile? {
+    private suspend fun getFile(uri: Uri): 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, isAlreadyUsed)
+            val uniqueName: String
+
+            alreadyUsedFileNames.mutex.withLock {
+                uniqueName = FileNameUtils.postfixExistingFileNames(
+                    fileName = name,
+                    isAlreadyUsed = { alreadyUsedFileNames.contains(it) }
+                )
+
+                alreadyUsedFileNames.add(uniqueName)
+            }
+
             PickedFile(uniqueName, size, uri)
         }
     }
@@ -238,9 +239,12 @@ class ImportationFilesManager @Inject constructor(@ApplicationContext private va
 
     class AlreadyUsedFileNamesSet {
         private val alreadyUsedFileNames = mutableSetOf<String>()
-        private val mutex = Mutex()
+        val mutex = Mutex()
+
+        // No need for the mutex because this code is already called inside of the lock
+        fun contains(fileName: String): Boolean = alreadyUsedFileNames.contains(fileName)
+        fun add(fileName: String): Boolean = alreadyUsedFileNames.add(fileName)
 
-        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) }
     }
diff --git a/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/NewTransferViewModel.kt b/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/NewTransferViewModel.kt
index b6afcf464..6f479a1d8 100644
--- a/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/NewTransferViewModel.kt
+++ b/app/src/main/java/com/infomaniak/swisstransfer/ui/screen/newtransfer/NewTransferViewModel.kt
@@ -66,7 +66,7 @@ class NewTransferViewModel @Inject constructor(
                 importationFilesManager.restoreAlreadyImportedFiles()
             }
 
-            importationFilesManager.copyPickedFilesToLocalStorage()
+            importationFilesManager.continuouslyCopyPickedFilesToLocalStorage()
         }
     }
 
diff --git a/app/src/main/java/com/infomaniak/swisstransfer/ui/utils/FileNameUtils.kt b/app/src/main/java/com/infomaniak/swisstransfer/ui/utils/FileNameUtils.kt
index 705bde4b0..1276e2d07 100644
--- a/app/src/main/java/com/infomaniak/swisstransfer/ui/utils/FileNameUtils.kt
+++ b/app/src/main/java/com/infomaniak/swisstransfer/ui/utils/FileNameUtils.kt
@@ -19,7 +19,7 @@ package com.infomaniak.swisstransfer.ui.utils
 
 object FileNameUtils {
 
-    suspend fun postfixExistingFileNames(fileName: String, isAlreadyUsed: suspend (String) -> Boolean): String {
+    fun postfixExistingFileNames(fileName: String, isAlreadyUsed: (String) -> Boolean): String {
         return if (isAlreadyUsed(fileName)) {
             val postfixedFileName = PostfixedFileName.fromFileName(fileName)