Merge pull request #3937 from CalebKL/task/anr-fix-zimfile-reader

Task/anr fix zimfile reader

app/src/main/java/org/kiwix/kiwixmobile/nav/destination/reader/KiwixReaderFragment.kt

@@ -159,12 +159,25 @@ class KiwixReaderFragment : CoreReaderFragment() {
   override fun openHomeScreen() {
     Handler(Looper.getMainLooper()).postDelayed({
       if (webViewList.size == 0) {
-        hideTabSwitcher()
+        hideTabSwitcher(false)
       }
     }, HIDE_TAB_SWITCHER_DELAY)
   }
 
-  override fun hideTabSwitcher() {
+  /**
+   * Hides the tab switcher and optionally closes the ZIM book based on the `shouldCloseZimBook` parameter.
+   *
+   * @param shouldCloseZimBook If `true`, the ZIM book will be closed, and the `ZimFileReader` will be set to `null`.
+   * If `false`, it skips setting the `ZimFileReader` to `null`. This is particularly useful when restoring tabs,
+   * as setting the `ZimFileReader` to `null` would require re-creating it, which is a resource-intensive operation,
+   * especially for large ZIM files.
+   *
+   * Refer to the following methods for more details:
+   * @See exitBook
+   * @see closeTab
+   * @see closeAllTabs
+   */
+  override fun hideTabSwitcher(shouldCloseZimBook: Boolean) {
     actionBar?.let { actionBar ->
       actionBar.setDisplayShowTitleEnabled(true)
       toolbar?.let { activity?.setupDrawerToggle(it, true) }
@@ -181,7 +194,7 @@ class KiwixReaderFragment : CoreReaderFragment() {
       }
       mainMenu?.showWebViewOptions(true)
       if (webViewList.isEmpty()) {
-        exitBook()
+        exitBook(shouldCloseZimBook)
       } else {
         // Reset the top margin of web views to 0 to remove any previously set margin
         // This ensures that the web views are displayed without any additional

core/src/main/java/org/kiwix/kiwixmobile/core/StorageObserver.kt

@@ -19,9 +19,12 @@
 package org.kiwix.kiwixmobile.core
 
 import io.reactivex.Flowable
+import io.reactivex.Single
 import io.reactivex.functions.BiFunction
 import io.reactivex.schedulers.Schedulers
-import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
 import org.kiwix.kiwixmobile.core.dao.DownloadRoomDao
 import org.kiwix.kiwixmobile.core.dao.LibkiwixBookmarks
 import org.kiwix.kiwixmobile.core.downloader.model.DownloadModel
@@ -45,7 +48,17 @@ class StorageObserver @Inject constructor(
   ): Flowable<List<BookOnDisk>> {
     return scanFiles(scanningProgressListener)
       .withLatestFrom(downloadRoomDao.downloads(), BiFunction(::toFilesThatAreNotDownloading))
-      .map { it.mapNotNull(::convertToBookOnDisk) }
+      .flatMapSingle { files ->
+        Single.create { emitter ->
+          CoroutineScope(Dispatchers.IO).launch {
+            try {
+              emitter.onSuccess(files.mapNotNull { convertToBookOnDisk(it) })
+            } catch (ignore: Exception) {
+              emitter.onError(ignore)
+            }
+          }
+        }
+      }
   }
 
   private fun scanFiles(scanningProgressListener: ScanningProgressListener) =
@@ -57,7 +70,7 @@ class StorageObserver @Inject constructor(
   private fun fileHasNoMatchingDownload(downloads: List<DownloadModel>, file: File) =
     downloads.firstOrNull { file.absolutePath.endsWith(it.fileNameFromUrl) } == null
 
-  private fun convertToBookOnDisk(file: File) = runBlocking {
+  private suspend fun convertToBookOnDisk(file: File) =
     zimReaderFactory.create(ZimReaderSource(file))
       ?.let { zimFileReader ->
         BookOnDisk(zimFileReader).also {
@@ -67,4 +80,3 @@ class StorageObserver @Inject constructor(
         }
       }
 }
-}

core/src/main/java/org/kiwix/kiwixmobile/core/dao/LibkiwixBookmarks.kt

@@ -129,10 +129,10 @@ class LibkiwixBookmarks @Inject constructor(
     } ?: emptyList()
   }
 
-  fun bookmarkUrlsForCurrentBook(zimFileReader: ZimFileReader): Flowable<List<String>> =
+  fun bookmarkUrlsForCurrentBook(zimId: String): Flowable<List<String>> =
     flowableBookmarkList()
       .map { bookmarksList ->
-        bookmarksList.filter { it.zimId == zimFileReader.id }
+        bookmarksList.filter { it.zimId == zimId }
           .map(LibkiwixBookmarkItem::bookmarkUrl)
       }
       .subscribeOn(Schedulers.io())

core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreReaderFragment.kt

@@ -860,7 +860,13 @@ abstract class CoreReaderFragment :
     view?.startAnimation(AnimationUtils.loadAnimation(view.context, anim))
   }
 
-  protected open fun hideTabSwitcher() {
+  /**
+   * @param shouldCloseZimBook A flag to indicate whether the ZIM book should be closed.
+   *        - Default is `true`, which ensures normal behavior for most scenarios.
+   *        - If `false`, the ZIM book is not closed. This is useful in cases where the user restores tabs,
+   *          as closing the ZIM book would require reloading the ZIM file, which can be a resource-intensive operation.
+   */
+  protected open fun hideTabSwitcher(shouldCloseZimBook: Boolean = true) {
     actionBar?.apply {
       setDisplayShowTitleEnabled(true)
     }
@@ -1388,7 +1394,17 @@ abstract class CoreReaderFragment :
         .setAction(R.string.undo) { undoButton ->
           undoButton.isEnabled = false
           restoreDeletedTab(index)
-        }.show()
+        }.addCallback(object : Snackbar.Callback() {
+          override fun onDismissed(transientBottomBar: Snackbar?, event: Int) {
+            super.onDismissed(transientBottomBar, event)
+            // If the undo button is not clicked and no tabs are left, exit the book and
+            // clean up resources.
+            if (event != DISMISS_EVENT_ACTION && webViewList.isEmpty()) {
+              closeZimBook()
+            }
+          }
+        })
+        .show()
     }
     openHomeScreen()
   }
@@ -1399,19 +1415,23 @@ abstract class CoreReaderFragment :
     mainMenu?.showBookSpecificMenuItems()
   }
 
-  protected fun exitBook() {
+  protected fun exitBook(shouldCloseZimBook: Boolean = true) {
     showNoBookOpenViews()
     bottomToolbar?.visibility = View.GONE
     actionBar?.title = getString(R.string.reader)
     contentFrame?.visibility = View.GONE
     hideProgressBar()
     mainMenu?.hideBookSpecificMenuItems()
+    if (shouldCloseZimBook) {
       closeZimBook()
     }
+  }
 
   fun closeZimBook() {
+    lifecycleScope.launch {
       zimReaderContainer?.setZimReaderSource(null)
     }
+  }
 
   protected fun showProgressBarWithProgress(progress: Int) {
     progressBar?.apply {
@@ -1443,7 +1463,6 @@ abstract class CoreReaderFragment :
           LinearLayout.LayoutParams.MATCH_PARENT
         )
       }
-      zimReaderContainer?.setZimReaderSource(tempZimSourceForUndo)
       webViewList.add(index, it)
       tabsAdapter?.notifyDataSetChanged()
       snackBarRoot?.let { root ->
@@ -1800,7 +1819,7 @@ abstract class CoreReaderFragment :
   protected fun setUpBookmarks(zimFileReader: ZimFileReader) {
     safeDispose()
     bookmarkingDisposable = Flowable.combineLatest(
-      libkiwixBookmarks?.bookmarkUrlsForCurrentBook(zimFileReader),
+      libkiwixBookmarks?.bookmarkUrlsForCurrentBook(zimFileReader.id),
       webUrlsProcessor,
       List<String?>::contains
     )
@@ -1876,7 +1895,16 @@ abstract class CoreReaderFragment :
           setIsCloseAllTabButtonClickable(true)
           restoreDeletedTabs()
         }
-      }.show()
+      }.addCallback(object : Snackbar.Callback() {
+        override fun onDismissed(transientBottomBar: Snackbar?, event: Int) {
+          super.onDismissed(transientBottomBar, event)
+          // If the undo button is not clicked and no tabs are left, exit the book and
+          // clean up resources.
+          if (event != DISMISS_EVENT_ACTION && webViewList.isEmpty()) {
+            closeZimBook()
+          }
+        }
+      }).show()
     }
   }
 
@@ -1886,7 +1914,6 @@ abstract class CoreReaderFragment :
 
   private fun restoreDeletedTabs() {
     if (tempWebViewListForUndo.isNotEmpty()) {
-      zimReaderContainer?.setZimReaderSource(tempZimSourceForUndo)
       webViewList.addAll(tempWebViewListForUndo)
       tabsAdapter?.notifyDataSetChanged()
       snackBarRoot?.let { root ->

core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimReaderContainer.kt

@@ -18,7 +18,9 @@
 package org.kiwix.kiwixmobile.core.reader
 
 import android.webkit.WebResourceResponse
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.withContext
 import org.kiwix.kiwixmobile.core.reader.ZimFileReader.Factory
 import java.net.HttpURLConnection
 import javax.inject.Inject
@@ -32,11 +34,11 @@ class ZimReaderContainer @Inject constructor(private val zimFileReaderFactory: F
       field = value
     }
 
-  fun setZimReaderSource(zimReaderSource: ZimReaderSource?) {
+  suspend fun setZimReaderSource(zimReaderSource: ZimReaderSource?) {
     if (zimReaderSource == zimFileReader?.zimReaderSource) {
       return
     }
-    zimFileReader = runBlocking {
+    zimFileReader = withContext(Dispatchers.IO) {
       if (zimReaderSource?.exists() == true && zimReaderSource.canOpenInLibkiwix())
         zimFileReaderFactory.create(zimReaderSource)
       else null

core/src/test/java/org/kiwix/kiwixmobile/core/StorageObserverTest.kt

@@ -42,6 +42,7 @@ import org.kiwix.sharedFunctions.bookOnDisk
 import org.kiwix.sharedFunctions.resetSchedulers
 import org.kiwix.sharedFunctions.setScheduler
 import java.io.File
+import java.util.concurrent.TimeUnit
 
 class StorageObserverTest {
 
@@ -106,6 +107,7 @@ class StorageObserverTest {
     .also {
       downloads.offer(listOf(downloadModel))
       files.offer(listOf(file))
+      it.awaitDone(2, TimeUnit.SECONDS)
     }
 
   private fun withFiltering() {