ref: migrate PagerPageHolder from RxJava to Coroutines#2928
ref: migrate PagerPageHolder from RxJava to Coroutines#2928
Conversation
Co-authored-by: nonproto <2092019+nonproto@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request migrates the image header reading logic in PagerPageHolder.kt from RxJava to Kotlin Coroutines, utilizing awaitCancellation() to maintain the input stream's lifecycle. The changes include replacing Subscription objects with Jobs and refactoring the asynchronous flow. Feedback focuses on removing a temporary utility script, updating outdated documentation that still references RxJava terminology, and optimizing coroutine usage by removing redundant launches and ensuring proper handling of CancellationException to maintain structured concurrency.
app/src/main/java/eu/kanade/tachiyomi/ui/reader/viewer/pager/PagerPageHolder.kt
Show resolved
Hide resolved
app/src/main/java/eu/kanade/tachiyomi/ui/reader/viewer/pager/PagerPageHolder.kt
Show resolved
Hide resolved
app/src/main/java/eu/kanade/tachiyomi/ui/reader/viewer/pager/PagerPageHolder.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: nonproto <2092019+nonproto@users.noreply.github.com>
Co-authored-by: nonproto <2092019+nonproto@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the image loading and header reading logic in PagerPageHolder from RxJava to Kotlin Coroutines, replacing Observable patterns with coroutine jobs and awaitCancellation(). The update includes a try/finally block to ensure streams are closed upon cancellation and a Python script to handle import cleanup. A high-priority issue was identified where image streams might be consumed before animation checks occur, alongside inefficient redundant I/O operations. The reviewer suggests reading the stream into a byte array once to ensure data availability for all processing steps.
| 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 |
There was a problem hiding this comment.
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.
| 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
- 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.
What: Removed RxJava imports (
Observable,Subscription,Schedulers) fromPagerPageHolder.ktand convertedreadImageHeaderSubscriptionto a CoroutineJob.Why: Modernization and reducing the footprint of the RxJava dependency.
Nuances: Translated
.subscribeOn(Schedulers.io())towithContext(Dispatchers.IO)and.observeOn(AndroidSchedulers.mainThread())towithContext(Dispatchers.Main). Handled stream lifecycle usingawaitCancellation()inside atry/finallyblock to mirror.flatMap { Observable.never<Unit>() }. Handled blocking IO operations appropriately.PR created automatically by Jules for task 15699169375434068070 started by @nonproto