Merge pull request #4034 from kiwix/Fixes#4031

Fixed: Application crashing due to `Input dispatching timed out` when deleting the ZIM files.
kiwix · Oct 15, 2024 · bad8f4c · bad8f4c
2 parents 9c2b355 + 79a61e3
commit bad8f4c
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 103 deletions.
diff --git a/app/src/androidTest/java/org/kiwix/kiwixmobile/localLibrary/CopyMoveFileHandlerTest.kt b/app/src/androidTest/java/org/kiwix/kiwixmobile/localLibrary/CopyMoveFileHandlerTest.kt
@@ -24,12 +24,16 @@ import androidx.core.content.ContextCompat
 import androidx.core.content.edit
 import androidx.documentfile.provider.DocumentFile
 import androidx.lifecycle.Lifecycle
+import androidx.lifecycle.lifecycleScope
 import androidx.navigation.fragment.NavHostFragment
 import androidx.preference.PreferenceManager
 import androidx.test.core.app.ActivityScenario
 import androidx.test.internal.runner.junit4.statement.UiThreadStatement
 import androidx.test.platform.app.InstrumentationRegistry
 import androidx.test.uiautomator.UiDevice
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import org.junit.After
 import org.junit.Assert
 import org.junit.Before
@@ -251,20 +255,24 @@ class CopyMoveFileHandlerTest : BaseActivityTest() {
       destinationFile.name,
       "testCopyMove_1.zim"
     )
-    deleteBothPreviousFiles()
+    kiwixMainActivity.lifecycleScope.launch {
+      withContext(Dispatchers.IO) {
+        deleteBothPreviousFiles()
+      }
 
-    // test when there is no zim file available in the storage it should return the same fileName
-    selectedFile = File(parentFile, selectedFileName)
-    copyMoveFileHandler.setSelectedFileAndUri(null, DocumentFile.fromFile(selectedFile))
-    destinationFile = copyMoveFileHandler.getDestinationFile()
-    Assert.assertEquals(
-      destinationFile.name,
-      selectedFile.name
-    )
-    deleteBothPreviousFiles()
+      // test when there is no zim file available in the storage it should return the same fileName
+      selectedFile = File(parentFile, selectedFileName)
+      copyMoveFileHandler.setSelectedFileAndUri(null, DocumentFile.fromFile(selectedFile))
+      destinationFile = copyMoveFileHandler.getDestinationFile()
+      Assert.assertEquals(
+        destinationFile.name,
+        selectedFile.name
+      )
+      deleteBothPreviousFiles()
+    }
   }
 
-  private fun deleteBothPreviousFiles() {
+  private suspend fun deleteBothPreviousFiles() {
     selectedFile.deleteFile()
     destinationFile.deleteFile()
   }

diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/CopyMoveFileHandler.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/CopyMoveFileHandler.kt
@@ -301,7 +301,9 @@ class CopyMoveFileHandler @Inject constructor(
     }
     fileCopyMoveCallback?.onError(userFriendlyMessage).also {
       // Clean up the destination file if an error occurs
-      destinationFile.deleteFile()
+      lifecycleScope?.launch {
+        destinationFile.deleteFile()
+      }
     }
   }
 

diff --git a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/AppProgressListenerProvider.kt b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/AppProgressListenerProvider.kt
@@ -33,12 +33,12 @@ class AppProgressListenerProvider(
       if (contentLength == DEFAULT_INT_VALUE.toLong()) {
         ZERO
       } else {
-        (bytesRead * HUNDERED / contentLength).toInt() * 3
+        (bytesRead * 3 * HUNDERED / contentLength).coerceAtMost(HUNDERED.toLong())
       }
     zimManageViewModel.downloadProgress.postValue(
       zimManageViewModel.context.getString(
         R.string.downloading_library,
-        zimManageViewModel.context.getString(R.string.percentage, progress)
+        zimManageViewModel.context.getString(R.string.percentage, progress.toInt())
       )
     )
   }

diff --git a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/fileselectView/effects/DeleteFiles.kt b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/fileselectView/effects/DeleteFiles.kt
@@ -19,8 +19,12 @@
 package org.kiwix.kiwixmobile.zimManager.fileselectView.effects
 
 import androidx.appcompat.app.AppCompatActivity
-import org.kiwix.kiwixmobile.core.R
+import androidx.lifecycle.lifecycleScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import org.kiwix.kiwixmobile.cachedComponent
+import org.kiwix.kiwixmobile.core.R
 import org.kiwix.kiwixmobile.core.base.BaseActivity
 import org.kiwix.kiwixmobile.core.base.SideEffect
 import org.kiwix.kiwixmobile.core.dao.NewBookDao
@@ -46,17 +50,22 @@ data class DeleteFiles(private val booksOnDiskListItems: List<BookOnDisk>) :
     val name = booksOnDiskListItems.joinToString(separator = "\n") { it.book.title }
 
     dialogShower.show(DeleteZims(name), {
-      activity.toast(
-        if (booksOnDiskListItems.deleteAll()) {
-          R.string.delete_zims_toast
-        } else {
-          R.string.delete_zim_failed
+      activity.lifecycleScope.launch {
+        val deleteResult = withContext(Dispatchers.IO) {
+          booksOnDiskListItems.deleteAll()
         }
-      )
+        activity.toast(
+          if (deleteResult) {
+            R.string.delete_zims_toast
+          } else {
+            R.string.delete_zim_failed
+          }
+        )
+      }
     })
   }
 
-  private fun List<BookOnDisk>.deleteAll(): Boolean {
+  private suspend fun List<BookOnDisk>.deleteAll(): Boolean {
     return fold(true) { acc, book ->
       acc && deleteSpecificZimFile(book).also {
         if (it && book.zimReaderSource == zimReaderContainer.zimReaderSource) {
@@ -66,7 +75,7 @@ data class DeleteFiles(private val booksOnDiskListItems: List<BookOnDisk>) :
     }
   }
 
-  private fun deleteSpecificZimFile(book: BookOnDisk): Boolean {
+  private suspend fun deleteSpecificZimFile(book: BookOnDisk): Boolean {
     val file = book.zimReaderSource.file
     file?.let {
       @Suppress("UnreachableCode")

diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/dao/DownloadRoomDao.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/dao/DownloadRoomDao.kt
@@ -25,6 +25,9 @@ import androidx.room.Query
 import androidx.room.Update
 import io.reactivex.Flowable
 import io.reactivex.Single
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
 import org.kiwix.kiwixmobile.core.dao.entities.DownloadRoomEntity
 import org.kiwix.kiwixmobile.core.downloader.DownloadRequester
 import org.kiwix.kiwixmobile.core.downloader.downloadManager.Status
@@ -95,7 +98,9 @@ abstract class DownloadRoomDao {
   fun delete(downloadId: Long) {
     // remove the previous file from storage since we have cancelled the download.
     getEntityForDownloadId(downloadId)?.file?.let {
-      File(it).deleteFile()
+      CoroutineScope(Dispatchers.IO).launch {
+        File(it).deleteFile()
+      }
     }
     deleteDownloadByDownloadId(downloadId)
   }

diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/extensions/FileExtensions.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/extensions/FileExtensions.kt
@@ -47,8 +47,4 @@ fun File.canReadFile(): Boolean = runBlocking {
   }
 }
 
-fun File.deleteFile(): Boolean = runBlocking {
-  withContext(Dispatchers.IO) {
-    delete()
-  }
-}
+suspend fun File.deleteFile(): Boolean = withContext(Dispatchers.IO) { delete() }
diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/utils/files/FileUtils.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/utils/files/FileUtils.kt
@@ -18,29 +18,26 @@
 package org.kiwix.kiwixmobile.core.utils.files
 
 import android.annotation.SuppressLint
-import android.app.Activity
 import android.content.ContentUris
 import android.content.Context
-import android.content.Intent
 import android.content.res.AssetFileDescriptor
 import android.net.Uri
 import android.os.Build
 import android.os.Environment
 import android.provider.DocumentsContract
 import android.webkit.URLUtil
-import android.widget.Toast
 import androidx.core.content.ContextCompat
-import androidx.documentfile.provider.DocumentFile
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 import org.kiwix.kiwixmobile.core.CoreApp
 import org.kiwix.kiwixmobile.core.R
 import org.kiwix.kiwixmobile.core.downloader.ChunkUtils
 import org.kiwix.kiwixmobile.core.entity.LibraryNetworkEntity.Book
 import org.kiwix.kiwixmobile.core.extensions.deleteFile
 import org.kiwix.kiwixmobile.core.extensions.isFileExist
-import org.kiwix.kiwixmobile.core.extensions.toast
 import org.kiwix.kiwixmobile.core.reader.ZimReaderContainer
 import org.kiwix.kiwixmobile.core.utils.SharedPreferenceUtil
 import java.io.BufferedReader
@@ -51,6 +48,8 @@ import java.io.IOException
 
 object FileUtils {
 
+  private val fileOperationMutex = Mutex()
+
   @JvmStatic
   fun getFileCacheDir(context: Context): File? =
     if (Environment.MEDIA_MOUNTED == Environment.getExternalStorageState()) {
@@ -68,38 +67,38 @@ object FileUtils {
   }
 
   @JvmStatic
-  @Synchronized
-  fun deleteZimFile(path: String) {
-    var path = path
-    if (path.substring(path.length - ChunkUtils.PART.length) == ChunkUtils.PART) {
-      path = path.substring(0, path.length - ChunkUtils.PART.length)
-    }
-    Log.i("kiwix", "Deleting file: $path")
-    val file = File(path)
-    if (file.path.substring(file.path.length - 3) != "zim") {
-      var alphabetFirst = 'a'
-      fileloop@ while (alphabetFirst <= 'z') {
-        var alphabetSecond = 'a'
-        while (alphabetSecond <= 'z') {
-          val chunkPath = path.substring(0, path.length - 2) + alphabetFirst + alphabetSecond
-          val fileChunk = File(chunkPath)
-          if (fileChunk.isFileExist()) {
-            fileChunk.deleteFile()
-          } else if (!deleteZimFileParts(chunkPath)) {
-            break@fileloop
+  suspend fun deleteZimFile(path: String) {
+    fileOperationMutex.withLock {
+      var path = path
+      if (path.substring(path.length - ChunkUtils.PART.length) == ChunkUtils.PART) {
+        path = path.substring(0, path.length - ChunkUtils.PART.length)
+      }
+      val file = File(path)
+      if (file.path.substring(file.path.length - 3) != "zim") {
+        var alphabetFirst = 'a'
+        fileloop@ while (alphabetFirst <= 'z') {
+          var alphabetSecond = 'a'
+          while (alphabetSecond <= 'z') {
+            val chunkPath = path.substring(0, path.length - 2) + alphabetFirst + alphabetSecond
+            val fileChunk = File(chunkPath)
+            if (fileChunk.isFileExist()) {
+              fileChunk.deleteFile()
+            } else if (!deleteZimFileParts(chunkPath)) {
+              break@fileloop
+            }
+            alphabetSecond++
           }
-          alphabetSecond++
+          alphabetFirst++
         }
-        alphabetFirst++
+      } else {
+        file.deleteFile()
+        deleteZimFileParts(path)
       }
-    } else {
-      file.deleteFile()
-      deleteZimFileParts(path)
     }
   }
 
-  @Synchronized
-  private fun deleteZimFileParts(path: String): Boolean {
+  @Suppress("ReturnCount")
+  private suspend fun deleteZimFileParts(path: String): Boolean {
     val file = File(path + ChunkUtils.PART)
     if (file.isFileExist()) {
       file.deleteFile()
@@ -357,49 +356,6 @@ object FileUtils {
       .firstOrNull { it.path.contains(storageName) }
       ?.path?.substringBefore(context.getString(R.string.android_directory_seperator))
 
-  @SuppressLint("WrongConstant")
-  @JvmStatic
-  fun getPathFromUri(activity: Activity, data: Intent): String? {
-    val uri: Uri? = data.data
-    val takeFlags: Int = data.flags and (
-      Intent.FLAG_GRANT_READ_URI_PERMISSION
-        or Intent.FLAG_GRANT_WRITE_URI_PERMISSION
-      )
-    uri?.let {
-      activity.grantUriPermission(
-        activity.packageName, it,
-        Intent.FLAG_GRANT_READ_URI_PERMISSION or Intent.FLAG_GRANT_WRITE_URI_PERMISSION
-      )
-      activity.contentResolver.takePersistableUriPermission(it, takeFlags)
-
-      val dFile = DocumentFile.fromTreeUri(activity, it)
-      if (dFile != null) {
-        dFile.uri.path?.let { file ->
-          val originalPath = file.substring(
-            file.lastIndexOf(":") + 1
-          )
-          val path = "${activity.getExternalFilesDirs("")[1]}"
-          return@getPathFromUri path.substringBefore(
-            activity.getString(R.string.android_directory_seperator)
-          )
-            .plus(File.separator).plus(originalPath)
-        }
-      }
-      activity.toast(
-        activity.resources
-          .getString(R.string.system_unable_to_grant_permission_message),
-        Toast.LENGTH_SHORT
-      )
-    } ?: run {
-      activity.toast(
-        activity.resources
-          .getString(R.string.system_unable_to_grant_permission_message),
-        Toast.LENGTH_SHORT
-      )
-    }
-    return null
-  }
-
   /*
    * This method returns a file name guess from the url using URLUtils.guessFileName()
      method of android.webkit. which is using Uri.decode method to extract the filename