Skip to content

ref: migrate PagerPageHolder from RxJava to Coroutines #2928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nonproto wants to merge 3 commits into from
Closed

ref: migrate PagerPageHolder from RxJava to Coroutines #2928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d59d7fa
ref: migrate PagerPageHolder from RxJava to Coroutines
google-labs-jules[bot] Apr 4, 2026
899d116
ref: migrate PagerPageHolder from RxJava to Coroutines
google-labs-jules[bot] Apr 4, 2026
0ac2100
ref: migrate PagerPageHolder from RxJava to Coroutines
google-labs-jules[bot] Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .jules/exorcist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
## 2024-05-24 - [Replicate Observable.never] **Learning:** [RxJava's flatMap { Observable.never<Unit>() } keeps a stream alive indefinitely until unsubscribed] **Action:** [Use awaitCancellation() from kotlinx.coroutines inside a try/finally block to keep the Coroutine alive and handle cleanup upon cancellation]
154 changes: 81 additions & 73 deletions app/src/main/java/eu/kanade/tachiyomi/ui/reader/viewer/pager/PagerPageHolder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,11 @@ import eu.kanade.tachiyomi.widget.ViewPagerAdapter
import kotlin.math.min
import kotlin.math.roundToInt
import kotlinx.coroutines.Dispatchers.Default
import kotlinx.coroutines.Dispatchers.IO
import kotlinx.coroutines.Dispatchers.Main
import kotlinx.coroutines.Job
import kotlinx.coroutines.MainScope
import kotlinx.coroutines.awaitCancellation
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.collectLatest
import kotlinx.coroutines.launch
Expand All @@ -51,10 +54,6 @@ import okio.source
import org.nekomanga.R
import org.nekomanga.domain.reader.ReaderPreferences
import org.nekomanga.logging.TimberKt
import rx.Observable
import rx.Subscription
import rx.android.schedulers.AndroidSchedulers
import rx.schedulers.Schedulers
import uy.kohesive.injekt.injectLazy

/** View of the ViewPager that contains a page of a chapter. */
Expand Down Expand Up @@ -97,10 +96,10 @@ class PagerPageHolder(
private var extraProgressJob: Job? = null

/**
* Subscription used to read the header of the image. This is needed in order to instantiate the
* Job used to read the header of the image. This is needed in order to instantiate the
* appropiate image view depending if the image is animated (GIF).
*/
private var readImageHeaderSubscription: Subscription? = null
private var readImageHeaderJob: Job? = null
Comment thread
nonproto marked this conversation as resolved.

private var status = Page.State.READY
private var extraStatus = Page.State.READY
Expand Down Expand Up @@ -173,7 +172,7 @@ class PagerPageHolder(
cancelLoadJob(1)
cancelProgressJob(2)
cancelLoadJob(2)
unsubscribeReadImageHeader()
cancelReadImageHeaderJob()
(pageView as? SubsamplingScaleImageView)?.setOnImageEventListener(null)
}

Expand Down Expand Up @@ -412,10 +411,10 @@ class PagerPageHolder(
}
}

/** Unsubscribes from the read image header subscription. */
private fun unsubscribeReadImageHeader() {
readImageHeaderSubscription?.unsubscribe()
readImageHeaderSubscription = null
/** Cancels the read image header job. */
private fun cancelReadImageHeaderJob() {
Comment thread
nonproto marked this conversation as resolved.
readImageHeaderJob?.cancel()
readImageHeaderJob = null
}

/** Called when the page is queued. */
Expand Down Expand Up @@ -450,60 +449,76 @@ class PagerPageHolder(
retryButton?.isVisible = false
decodeErrorLayout?.isVisible = false

unsubscribeReadImageHeader()
cancelReadImageHeaderJob()
val streamFn = page.stream ?: return
val streamFn2 = extraPage?.stream

var openStream: BufferedSource? = null

readImageHeaderSubscription =
Observable.fromCallable {
val stream = streamFn().source().buffer()

val stream2 = streamFn2?.invoke()?.source()?.buffer()
openStream =
when (
viewer.config.doublePageRotate &&
stream2 == null &&
ImageUtil.isWideImage(stream)
) {
true -> {
val rotation =
if (viewer.config.doublePageRotateReverse) -90f else 90f
ImageUtil.rotateImage(stream, rotation)
}
false -> this@PagerPageHolder.mergeOrSplitPages(stream, stream2)
readImageHeaderJob =
scope.launch(IO) {
try {
val (isAnimated, bytesArray) =
try {
val stream = streamFn().source().buffer()

val stream2 = streamFn2?.invoke()?.source()?.buffer()
openStream =
when (
viewer.config.doublePageRotate &&
stream2 == null &&
ImageUtil.isWideImage(stream)
) {
true -> {
val rotation =
if (viewer.config.doublePageRotateReverse) -90f else 90f
ImageUtil.rotateImage(stream, rotation)
}
false -> this@PagerPageHolder.mergeOrSplitPages(stream, stream2)
}

val animated =
ImageUtil.isAnimatedAndSupported(stream) ||
if (stream2 != null) ImageUtil.isAnimatedAndSupported(stream2)
else false
Comment on lines +463 to +483
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There's a potential bug where streams are checked for animation after they might have been consumed by processing functions. Additionally, to avoid inefficient multiple disk I/O operations when the image data is needed multiple times (for animation checks, wide image checks, and rotation/merging), the file should be read into a byte array once and wrapped in a ByteArrayInputStream for subsequent operations.

Suggested change
val stream = streamFn().source().buffer()
val stream2 = streamFn2?.invoke()?.source()?.buffer()
openStream =
when (
viewer.config.doublePageRotate &&
stream2 == null &&
ImageUtil.isWideImage(stream)
) {
true -> {
val rotation =
if (viewer.config.doublePageRotateReverse) -90f else 90f
ImageUtil.rotateImage(stream, rotation)
}
false -> this@PagerPageHolder.mergeOrSplitPages(stream, stream2)
}
val animated =
ImageUtil.isAnimatedAndSupported(stream) ||
if (stream2 != null) ImageUtil.isAnimatedAndSupported(stream2)
else false
val bytes = streamFn().use { it.source().buffer().readByteArray() }
val bytes2 = streamFn2?.invoke()?.use { it.source().buffer().readByteArray() }
val animated = ImageUtil.isAnimatedAndSupported(bytes.inputStream()) ||
(bytes2 != null && ImageUtil.isAnimatedAndSupported(bytes2.inputStream()))
openStream =
when (
viewer.config.doublePageRotate &&
bytes2 == null &&
ImageUtil.isWideImage(bytes.inputStream())
) {
true -> {
val rotation =
if (viewer.config.doublePageRotateReverse) -90f else 90f
ImageUtil.rotateImage(bytes.inputStream(), rotation)
}
false -> this@PagerPageHolder.mergeOrSplitPages(bytes.inputStream(), bytes2?.inputStream())
}
References
  1. To avoid inefficient multiple disk I/O operations when an InputStream is needed multiple times from the same file, read the file into a byte array once and use ByteArrayInputStream for subsequent stream operations.


val bytes =
if (
!animated &&
viewer.config.readerTheme >= 2 &&
!(page.bg != null &&
page.bgType ==
getBGType(viewer.config.readerTheme, context) +
item.hashCode())
) {
openStream?.readByteArray()
} else null

animated to bytes
} catch (e: Exception) {
TimberKt.e(e) { "Error checking image header" }
return@launch
}

ImageUtil.isAnimatedAndSupported(stream) ||
if (stream2 != null) ImageUtil.isAnimatedAndSupported(stream2) else false
}
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.doOnNext { isAnimated ->
if (!isAnimated) {
if (viewer.config.readerTheme >= 2) {
if (
page.bg != null &&
page.bgType ==
getBGType(viewer.config.readerTheme, context) +
item.hashCode()
) {
setImage(openStream!!, false, imageConfig)
pageView?.background = page.bg
}
// if the user switches to automatic when pages are already cached, the
// bg needs to be loaded
else {
val bytesArray = openStream!!.readByteArray()
val bytesSource = Buffer().write(bytesArray)
setImage(bytesSource, false, imageConfig)
bytesSource.close()

scope.launchUI {
withContext(Main) {
if (!isAnimated) {
if (viewer.config.readerTheme >= 2) {
if (bytesArray == null && page.bg != null) {
openStream?.let { setImage(it, false, imageConfig) }
pageView?.background = page.bg
}
// if the user switches to automatic when pages are already cached,
// the
// bg needs to be loaded
else if (bytesArray != null) {
val bytesSource = Buffer().write(bytesArray)
setImage(bytesSource, false, imageConfig)
bytesSource.close()

try {
pageView?.background = setBG(bytesArray)
} catch (e: Exception) {
if (e is CancellationException) throw e
TimberKt.e(e) { "Error setting BG" }
pageView?.background = ColorDrawable(Color.WHITE)
} finally {
Expand All @@ -513,34 +528,27 @@ class PagerPageHolder(
item.hashCode()
}
}
} else {
openStream?.let { setImage(it, false, imageConfig) }
}
} else {
setImage(openStream!!, false, imageConfig)
}
} else {
setImage(openStream!!, true, imageConfig)
if (viewer.config.readerTheme >= 2 && page.bg != null) {
pageView?.background = page.bg
openStream?.let { setImage(it, true, imageConfig) }
if (viewer.config.readerTheme >= 2 && page.bg != null) {
pageView?.background = page.bg
}
}
}
}
// Keep the Rx stream alive to close the input stream only when unsubscribed
.flatMap { Observable.never<Unit>() }
.doOnUnsubscribe {
try {
openStream?.close()
} catch (e: Exception) {
TimberKt.e(e) { "Error closing stream" }
}
}
.doOnError {

// Keep the Coroutine alive to close the input stream only when cancelled
awaitCancellation()
} finally {
try {
openStream?.close()
} catch (e: Exception) {
TimberKt.e(e) { "Error closing stream" }
}
}
.subscribe({}, {})
}
}

private val imageConfig: Config
Expand Down
8 changes: 8 additions & 0 deletions patch_pr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
with open("app/src/main/java/eu/kanade/tachiyomi/ui/reader/viewer/pager/PagerPageHolder.kt", "r") as f:
content = f.read()

# Fix the import inside the catch block that might have been left
content = content.replace(" import kotlinx.coroutines.CancellationException\n", "")

with open("app/src/main/java/eu/kanade/tachiyomi/ui/reader/viewer/pager/PagerPageHolder.kt", "w") as f:
f.write(content)
Loading