Skip to content

Commit

Permalink
Fixed: Create only one reader and use it everywhere.
Browse files Browse the repository at this point in the history
* Now we are creating only one reader for both `zimFile`, `assetFileDescriptor` and using for all the features.
  • Loading branch information
MohitMaliFtechiz authored and MohitMaliFtechiz committed Dec 26, 2023
1 parent 5fe3f5e commit ab38adf
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ class KiwixReaderFragment : CoreReaderFragment() {
}

private fun closeZimBook() {
zimReaderContainer?.setZimFile(null)
zimReaderContainer?.setZimFileOrFileDescriptor(null, null, null)
}

override fun openHomeScreen() {
Expand Down Expand Up @@ -205,9 +205,7 @@ class KiwixReaderFragment : CoreReaderFragment() {

override fun onResume() {
super.onResume()
if (zimReaderContainer?.zimFile == null &&
zimReaderContainer?.zimFileReader?.assetFileDescriptor == null
) {
if (zimReaderContainer?.isValidZimFileReader == false) {
exitBook()
}
if (isFullScreenVideo) {
Expand Down Expand Up @@ -299,7 +297,11 @@ class KiwixReaderFragment : CoreReaderFragment() {
// pass this uri to zimFileReader, which is necessary for saving
// notes, bookmarks, history, and reopening the same ZIM file after the app closes.
getAssetFileDescriptorFromUri(activity, it)?.let { assetFileDescriptor ->
openZimFile(null, assetFileDescriptor = assetFileDescriptor, filePath = "$it")
openZimFile(
null,
assetFileDescriptor = assetFileDescriptor,
assetFileDescriptorPath = "$it"

Check warning on line 303 in app/src/main/java/org/kiwix/kiwixmobile/nav/destination/reader/KiwixReaderFragment.kt

View check run for this annotation

Codecov / codecov/patch

app/src/main/java/org/kiwix/kiwixmobile/nav/destination/reader/KiwixReaderFragment.kt#L300-L303

Added lines #L300 - L303 were not covered by tests
)
} ?: kotlin.run {
activity.toast(R.string.cannot_open_file)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ data class DeleteFiles(private val booksOnDiskListItems: List<BookOnDisk>) :
return fold(true) { acc, book ->
acc && deleteSpecificZimFile(book).also {
if (it && book.file.canonicalPath == zimReaderContainer.zimCanonicalPath) {
zimReaderContainer.setZimFile(null)
zimReaderContainer.setZimFileOrFileDescriptor(null)

Check warning on line 63 in app/src/main/java/org/kiwix/kiwixmobile/zimManager/fileselectView/effects/DeleteFiles.kt

View check run for this annotation

Codecov / codecov/patch

app/src/main/java/org/kiwix/kiwixmobile/zimManager/fileselectView/effects/DeleteFiles.kt#L63

Added line #L63 was not covered by tests
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1228,7 +1228,7 @@ abstract class CoreReaderFragment :
reopenBook()
}
tempWebViewForUndo?.let {
zimReaderContainer?.setZimFile(tempZimFileForUndo)
zimReaderContainer?.setZimFileOrFileDescriptor(tempZimFileForUndo)
webViewList.add(index, it)
tabsAdapter?.notifyDataSetChanged()
snackBarRoot?.let { root ->
Expand Down Expand Up @@ -1479,17 +1479,11 @@ abstract class CoreReaderFragment :
file: File?,
isCustomApp: Boolean = false,
assetFileDescriptor: AssetFileDescriptor? = null,
filePath: String? = null
assetFileDescriptorPath: String? = null
) {
if (hasPermission(Manifest.permission.READ_EXTERNAL_STORAGE) || isCustomApp) {
if (file?.isFileExist() == true) {
openAndSetInContainer(file = file)
updateTitle()
} else if (assetFileDescriptor != null) {
openAndSetInContainer(
assetFileDescriptor = assetFileDescriptor,
filePath = filePath
)
if (file?.isFileExist() == true || assetFileDescriptor != null) {
openAndSetInContainer(file = file, assetFileDescriptor, assetFileDescriptorPath)
updateTitle()
} else {
Log.w(TAG_KIWIX, "ZIM file doesn't exist at " + file?.absolutePath)
Expand Down Expand Up @@ -1522,24 +1516,23 @@ abstract class CoreReaderFragment :
private fun openAndSetInContainer(
file: File? = null,
assetFileDescriptor: AssetFileDescriptor? = null,
filePath: String? = null
assetFileDescriptorPath: String? = null

Check warning on line 1519 in core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreReaderFragment.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L1519 was not covered by tests
) {
try {
if (isNotPreviouslyOpenZim(file?.canonicalPath)) {
if (isNotPreviouslyOpenZim(file?.canonicalPath) &&
isNotPreviouslyOpenZim(assetFileDescriptorPath)
) {
webViewList.clear()
}
} catch (e: IOException) {
e.printStackTrace()
}
zimReaderContainer?.let { zimReaderContainer ->
if (assetFileDescriptor != null) {
zimReaderContainer.setZimFileDescriptor(
assetFileDescriptor,
filePath = filePath
)
} else {
zimReaderContainer.setZimFile(file)
}
zimReaderContainer.setZimFileOrFileDescriptor(
file,
assetFileDescriptor,
assetFileDescriptorPath
)

val zimFileReader = zimReaderContainer.zimFileReader
zimFileReader?.let { zimFileReader ->
Expand Down Expand Up @@ -1632,7 +1625,7 @@ abstract class CoreReaderFragment :

private fun restoreDeletedTabs() {
if (tempWebViewListForUndo.isNotEmpty()) {
zimReaderContainer?.setZimFile(tempZimFileForUndo)
zimReaderContainer?.setZimFileOrFileDescriptor(tempZimFileForUndo)
webViewList.addAll(tempWebViewListForUndo)
tabsAdapter?.notifyDataSetChanged()
snackBarRoot?.let { root ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ data class BookmarkItem(
) : this(
zimId = zimFileReader.id,
zimName = zimFileReader.name,
zimFilePath = zimFileReader.zimFile?.canonicalPath ?: zimFileReader.assetDescriptorFilePath,
zimFilePath = zimFileReader.zimCanonicalPath,

Check warning on line 54 in core/src/main/java/org/kiwix/kiwixmobile/core/page/bookmark/adapter/BookmarkItem.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/page/bookmark/adapter/BookmarkItem.kt#L54

Added line #L54 was not covered by tests
bookmarkUrl = url,
title = title,
favicon = zimFileReader.favicon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ sealed class HistoryListItem : PageRelated {
) : this(
zimId = zimFileReader.id,
zimName = zimFileReader.name,
zimFilePath = zimFileReader.zimFile?.canonicalPath
?: zimFileReader.assetDescriptorFilePath
?: "",
zimFilePath = zimFileReader.zimCanonicalPath ?: "",
favicon = zimFileReader.favicon,
historyUrl = url,
title = title,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ data class NoteListItem(
) : this(
zimId = zimFileReader.id,
title = title,
zimFilePath = zimFileReader.zimFile?.canonicalPath ?: zimFileReader.assetDescriptorFilePath,
zimFilePath = zimFileReader.zimCanonicalPath,

Check warning on line 38 in core/src/main/java/org/kiwix/kiwixmobile/core/page/notes/adapter/NoteListItem.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/page/notes/adapter/NoteListItem.kt#L38

Added line #L38 was not covered by tests
zimUrl = url,
favicon = zimFileReader.favicon,
noteFilePath = noteFilePath
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ data class ShowOpenNoteDialog(
// which is already set in zimReaderContainer, so there's no need to set it again.
item.zimFilePath?.let {
val file = File(it)
zimReaderContainer.setZimFile(file)
zimReaderContainer.setZimFileOrFileDescriptor(file)

Check warning on line 53 in core/src/main/java/org/kiwix/kiwixmobile/core/page/notes/viewmodel/effects/ShowOpenNoteDialog.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/page/notes/viewmodel/effects/ShowOpenNoteDialog.kt#L53

Added line #L53 was not covered by tests
}
effects.offer(OpenNote(item.noteFilePath, item.zimUrl, item.title))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import io.reactivex.schedulers.Schedulers
import org.kiwix.kiwixmobile.core.CoreApp
import org.kiwix.kiwixmobile.core.NightModeConfig
import org.kiwix.kiwixmobile.core.entity.LibraryNetworkEntity.Book
import org.kiwix.kiwixmobile.core.extensions.isFileExist
import org.kiwix.kiwixmobile.core.main.UNINITIALISER_ADDRESS
import org.kiwix.kiwixmobile.core.main.UNINITIALISE_HTML
import org.kiwix.kiwixmobile.core.reader.ZimFileReader.Companion.CONTENT_PREFIX
Expand Down Expand Up @@ -61,34 +62,57 @@ class ZimFileReader constructor(
private val searcher: SuggestionSearcher = SuggestionSearcher(jniKiwixReader)
) {
interface Factory {
fun create(file: File): ZimFileReader?
fun create(
assetFileDescriptor: AssetFileDescriptor,
file: File? = null,

Check warning on line 66 in core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt#L66

Added line #L66 was not covered by tests
assetFileDescriptor: AssetFileDescriptor? = null,
filePath: String? = null
): ZimFileReader?

class Impl @Inject constructor(private val nightModeConfig: NightModeConfig) :
Factory {
override fun create(file: File) =
override fun create(
file: File?,
assetFileDescriptor: AssetFileDescriptor?,
filePath: String?
) =
try {
when {
file != null -> createArchiveWithFile(file)
assetFileDescriptor != null -> {
createArchiveWithAssetFileDescriptor(assetFileDescriptor, filePath)

Check warning on line 82 in core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt#L82

Added line #L82 was not covered by tests
}

else -> null
}
} catch (ignore: JNIKiwixException) {
null
} catch (ignore: Exception) { // for handing the error, if any zim file is corrupted

Check warning on line 89 in core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt#L87-L89

Added lines #L87 - L89 were not covered by tests
null
}

private fun createArchiveWithFile(file: File): ZimFileReader? =
if (file.isFileExist()) {
ZimFileReader(
file,
zimFile = file,
nightModeConfig = nightModeConfig,
jniKiwixReader = Archive(file.canonicalPath)
).also {
Log.e(TAG, "create: ${file.path}")
}
} catch (ignore: JNIKiwixException) {
null
} catch (ignore: Exception) { // for handing the error, if any zim file is corrupted
} else {
Log.e(
TAG,
"Error in creating ZimFileReader," +
" because file does not exist on path: ${file.path}"

Check warning on line 106 in core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt#L103-L106

Added lines #L103 - L106 were not covered by tests
)
null
}

override fun create(
private fun createArchiveWithAssetFileDescriptor(
assetFileDescriptor: AssetFileDescriptor,
filePath: String?
): ZimFileReader? =
try {
if (assetFileDescriptor.parcelFileDescriptor.dup().fileDescriptor.valid()) {
ZimFileReader(
null,
assetFileDescriptor,
Expand All @@ -100,11 +124,14 @@ class ZimFileReader constructor(
assetFileDescriptor.length
)
).also {
Log.e(TAG, "create: with fileDescriptor")
Log.e(TAG, "created: with assetFileDescriptor $assetFileDescriptor}")

Check warning on line 127 in core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt#L127

Added line #L127 was not covered by tests
}
} catch (ignore: JNIKiwixException) {
null
} catch (ignore: Exception) { // for handing the error, if any zim file is corrupted
} else {
Log.e(
TAG,
"Error in creating ZimFileReader," +

Check warning on line 132 in core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt#L130-L132

Added lines #L130 - L132 were not covered by tests
" because provided assetFileDescriptor is not valid"
)
null
}
}
Expand Down Expand Up @@ -163,6 +190,15 @@ class ZimFileReader constructor(
null
}

/**
* Return the zimFile path if opened from file else return the filePath of assetFileDescriptor
*/
val zimCanonicalPath
get() = zimFile?.canonicalPath ?: assetDescriptorFilePath

val isValidZimFileReader
get() = zimFile != null || assetFileDescriptor != null

fun searchSuggestions(prefix: String): SuggestionSearch? =
try {
searcher.suggest(prefix)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package org.kiwix.kiwixmobile.core.reader

import android.content.res.AssetFileDescriptor
import android.webkit.WebResourceResponse
import org.kiwix.kiwixmobile.core.extensions.isFileExist
import org.kiwix.kiwixmobile.core.reader.ZimFileReader.Factory
import java.io.File
import java.net.HttpURLConnection
Expand All @@ -34,23 +33,18 @@ class ZimReaderContainer @Inject constructor(private val zimFileReaderFactory: F
field = value
}

fun setZimFile(file: File?) {
if (file?.canonicalPath == zimFileReader?.zimFile?.canonicalPath) {
return
}
zimFileReader =
if (file?.isFileExist() == true) zimFileReaderFactory.create(file)
else null
}

fun setZimFileDescriptor(
assetFileDescriptor: AssetFileDescriptor,
fun setZimFileOrFileDescriptor(
file: File? = null,

Check warning on line 37 in core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimReaderContainer.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L37 was not covered by tests
assetFileDescriptor: AssetFileDescriptor? = null,
filePath: String? = null
) {
if (isValidZimFileReader == true &&
file?.canonicalPath == zimFileReader?.zimFile?.canonicalPath
) {
return
}
zimFileReader =
if (assetFileDescriptor.parcelFileDescriptor.dup().fileDescriptor.valid())
zimFileReaderFactory.create(assetFileDescriptor, filePath)
else null
zimFileReaderFactory.create(file, assetFileDescriptor, filePath)
}

fun getPageUrlFromTitle(title: String) = zimFileReader?.getPageUrlFrom(title)
Expand Down Expand Up @@ -83,18 +77,23 @@ class ZimReaderContainer @Inject constructor(private val zimFileReaderFactory: F
}
}

fun copyReader(): ZimFileReader? = zimFile?.let(zimFileReaderFactory::create)
?: assetFileDescriptor?.let(zimFileReaderFactory::create)
fun copyReader(): ZimFileReader? =
zimFileReaderFactory.create(
file = zimFile,
assetFileDescriptor = assetFileDescriptor,
filePath = assetDescriptorPath
)

val zimFile get() = zimFileReader?.zimFile

val assetFileDescriptor get() = zimFileReader?.assetFileDescriptor

/**
* Return the zimFile path if opened from file else return the filePath of assetFileDescriptor
*/
val assetDescriptorPath get() = zimFileReader?.assetDescriptorFilePath

val isValidZimFileReader get() = zimFileReader?.isValidZimFileReader

val zimCanonicalPath
get() = zimFileReader?.zimFile?.canonicalPath ?: zimFileReader?.assetDescriptorFilePath
get() = zimFileReader?.zimCanonicalPath
val zimFileTitle get() = zimFileReader?.title
val mainPage get() = zimFileReader?.mainPage
val id get() = zimFileReader?.id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,7 @@ class CustomReaderFragment : CoreReaderFragment() {
onFilesFound = {
when (it) {
is ValidationState.HasFile -> {
if (it.assetFileDescriptor != null) {
openZimFile(null, true, it.assetFileDescriptor)
} else {
openZimFile(it.file, true)
}
openZimFile(it.file, true, it.assetFileDescriptor)
// Save book in the database to display it in `ZimHostFragment`.
zimReaderContainer?.zimFileReader?.let { zimFileReader ->
// Check if the file is not null. If the file is null,
Expand Down

0 comments on commit ab38adf

Please sign in to comment.