1.5.0-beta.1: migrate Android to Kotlin and iOS to Swift - #345
Open
endigo wants to merge 35 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (48)
WalkthroughThe release migrates Android and iOS PDF platform views to Kotlin and Swift. It adds password unlocking, color modes, page alignment, tap callbacks, screenshots, rendering controls, integration tests, documentation, and version ChangesPDF view API and settings
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FlutterApp
participant PDFViewController
participant NativePDFView
participant PDFKitOrAndroidPdfViewer
FlutterApp->>NativePDFView: create view with resolved settings
NativePDFView->>PDFKitOrAndroidPdfViewer: load PDF
PDFKitOrAndroidPdfViewer-->>NativePDFView: password or render callback
NativePDFView-->>PDFViewController: send method-channel event
PDFViewController-->>FlutterApp: invoke password or tap callback
FlutterApp->>PDFViewController: unlock with password
PDFViewController->>NativePDFView: invoke unlock
NativePDFView->>PDFKitOrAndroidPdfViewer: reload document
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
endigo
force-pushed
the
migrate/kotlin-swift
branch
from
August 2, 2026 08:20
3126c9a to
e1edb3f
Compare
Owner
Author
|
Rebased onto the current
Verification after rebase: Android 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt (2)
474-477: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
linkHandlerasPDFLinkHandlerto remove the cast.The field at line 41 is typed
LinkHandler, but line 65 always assigns aPDFLinkHandler. Change the field type toPDFLinkHandlerand delete the downcast.♻️ Proposed change
- "preventLinkNavigation" -> { - val plh = this.linkHandler as PDFLinkHandler - plh.setPreventLinkNavigation(getBoolean(settings, key)) - } + "preventLinkNavigation" -> + linkHandler.setPreventLinkNavigation(getBoolean(settings, key))Apply this to the field declaration at line 41:
- private val linkHandler: LinkHandler + private val linkHandler: PDFLinkHandler🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt` around lines 474 - 477, Change the `linkHandler` field declaration in `FlutterPDFView` from `LinkHandler` to `PDFLinkHandler`, then remove the explicit cast in the `"preventLinkNavigation"` settings branch and call `setPreventLinkNavigation` directly on `linkHandler`.
79-83: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse a checked cast for
backgroundColor, likethumbnailRatio.
backgroundColor as NumberthrowsClassCastExceptionif Dart sends a non-numeric value. Lines 69-77 already use anis Numbercheck forthumbnailRatio. Apply the same pattern here for consistency.♻️ Proposed change
- val backgroundColor = params["backgroundColor"] - if (backgroundColor != null) { - val color = (backgroundColor as Number).toInt() - view.setBackgroundColor(color) - } + val backgroundColor = params["backgroundColor"] + if (backgroundColor is Number) { + view.setBackgroundColor(backgroundColor.toInt()) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt` around lines 79 - 83, Update the backgroundColor handling in FlutterPDFView so it validates the value with an is Number check before converting it and calling view.setBackgroundColor, matching the existing thumbnailRatio pattern; avoid the unchecked cast that can throw ClassCastException for non-numeric values.android/build.gradle (1)
4-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRespect the host app’s Kotlin Gradle Plugin configuration.
This
classpathdeclaration may conflict with future Flutter/Flutter app Gradle scripts that declare KGP through declarative plugin DSL or a different version. Migrate to the recommended declarative Gradle Plugin DSL, or follow Flutter’s plugin author guidance for Kotlin Gradle Plugin handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/build.gradle` around lines 4 - 17, Update the root buildscript configuration around kotlin_version and the org.jetbrains.kotlin:kotlin-gradle-plugin classpath to avoid pinning or directly declaring the Kotlin Gradle Plugin, allowing the host Flutter app’s declarative plugin configuration and version to control it. Follow Flutter’s plugin author guidance while preserving the existing repository setup.ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift (4)
616-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
FlutterPlatformViewconformance.
PDFViewControlleris the registered platform view, and itsview()returns theFlutterPDFViewcontainer. This secondview()returns the innerPDFViewinstead. Two differentFlutterPlatformViewimplementations return two different views, so a later change that passesFlutterPDFViewto Flutter would detach the container that performs the layout inlayoutSubviews.PDFViewControlleralso declaresPDFViewDelegateat Line 111 but never becomes the delegate; onlyFlutterPDFViewdoes.♻️ Proposed cleanup
-final class FlutterPDFView: UIView, FlutterPlatformView, PDFViewDelegate, +final class FlutterPDFView: UIView, PDFViewDelegate, UIGestureRecognizerDelegate, UIScrollViewDelegate- func view() -> UIView { - pdfView - } --final class PDFViewController: NSObject, FlutterPlatformView, PDFViewDelegate { +final class PDFViewController: NSObject, FlutterPlatformView {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around lines 616 - 618, Remove the unused FlutterPlatformView conformance and its associated view() method from PDFViewController. Keep PDFViewController as the registered platform view returning the FlutterPDFView container, and preserve FlutterPDFView’s layout and PDFViewDelegate responsibilities.
622-624: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn the live page count.
pageCountis assigned only inlayoutSubviews(Line 564). If Dart callspageCountfromonViewCreated, the first layout pass may not have run yet, so the handler returnsnil. The document is already available, so read the count from it.♻️ Proposed change
func getPageCount(_: FlutterMethodCall, result: FlutterResult) { - result(pageCount) + result(pageCount ?? (pdfView.document.map { NSNumber(value: $0.pageCount) })) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around lines 622 - 624, Update getPageCount to return the current page count directly from the loaded document rather than the cached pageCount property, while preserving the existing FlutterResult response contract.
821-832: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRestrict the schemes that a PDF link can launch.
UIApplication.openaccepts any scheme that a registered app claims, not onlyhttpandhttps. A PDF from an untrusted source can therefore embedtel:,sms:,facetime:, or a third-party app scheme, and a tap starts that action. Limit the launch to web schemes, and let the DartonLinkHandlercallback handle everything else.🛡️ Proposed guard
func pdfViewWillClick(onLink _: PDFView, with url: URL) { - if !preventLinkNavigation { + let scheme = url.scheme?.lowercased() + if !preventLinkNavigation, scheme == "http" || scheme == "https" { UIApplication.shared.open(url, options: [:]) { success in🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around lines 821 - 832, Update pdfViewWillClick so UIApplication.shared.open is invoked only when url.scheme is http or https (case-insensitively); leave other schemes exclusively to the existing onLinkHandler callback, preserving the preventLinkNavigation behavior for allowed web links.
518-531: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider throttling
onDrawnotifications.The observation posts one
onDrawmethod-channel message for everycontentOffsetchange. During a scroll this can reach display-refresh frequency, so the Dart isolate receives up to 120 messages per second. The Android implementation throttles the equivalent callback withDRAW_THROTTLE_MS(seeandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt,onDraw). A timestamp guard restores parity and lowers channel traffic.♻️ Proposed throttle
+ /// Matches the Android `DRAW_THROTTLE_MS` guard. + private static let drawThrottle: TimeInterval = 0.016 + private var lastDrawTime: TimeInterval = 0 + private func startObserving() { guard let scrollView, contentOffsetObservation == nil else { return } contentOffsetObservation = scrollView.observe( \.contentOffset, options: [.new, .old] ) { [weak self] _, change in let newOffset = change.newValue ?? .zero let oldOffset = change.oldValue ?? .zero guard newOffset != oldOffset else { return } DispatchQueue.main.async { [weak self] in - self?.handleOnDraw() + guard let self else { return } + let now = CACurrentMediaTime() + guard now - lastDrawTime >= Self.drawThrottle else { return } + lastDrawTime = now + handleOnDraw() } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around lines 518 - 531, Throttle onDraw notifications in startObserving by adding a timestamp guard equivalent to Android’s DRAW_THROTTLE_MS behavior. Track the last dispatched draw time and invoke handleOnDraw only when the throttle interval has elapsed, while preserving the existing content-offset change check and main-thread dispatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt`:
- Around line 412-423: Align setPage behavior across Android and iOS by choosing
a single out-of-range page contract and applying it consistently, preferably
validating the page index before invoking jumpTo and returning the same
INVALID_PAGE error response as iOS. In FlutterPDFView.setPage, parse the page
argument without assuming Int so decoded Long values do not cause
ClassCastException, then validate and preserve the agreed result behavior; if
clamping is intentionally retained, update the Dart API documentation and ensure
both platforms follow it.
- Around line 540-558: Replace unsafe platform-channel casts with safe casts
across the affected sites: in FlutterPDFView.kt lines 540-558, update
getBoolean, getString, and getInt to use as? Boolean, as? String, and
Number-to-int conversion; in FlutterPDFView.kt lines 79-83, guard
backgroundColor with an is Number check; in FlutterPDFView.kt lines 124-141, use
as? String and as? ByteArray so mismatches reach the existing null-config path;
and in PDFViewFactory.kt lines 14-17, safely cast args to the expected map and
fall back to emptyMap().
- Around line 540-558: Update getInt, getString, and getBoolean in
FlutterPDFView to use safe casts: convert numeric values through as? Number
followed by toInt(), cast strings with as? String, and cast booleans with as?
Boolean, preserving each helper’s absent-value defaults. Update
FlutterPDFViewParamsTest to assert the resulting defaults/null behavior instead
of expecting ClassCastException for wrong-type inputs.
In `@CHANGELOG.md`:
- Around line 5-8: Update the iOS changelog entry to distinguish the Swift
toolchain/manifest version from the CocoaPods source language mode: do not claim
migration to Swift 5.9 while ios/flutter_pdfview.podspec declares Swift 5.0.
Either revise the release note to match the existing podspec configuration or
update that configuration consistently before retaining the 5.9 wording.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift`:
- Around line 764-774: Update setZoomLimits to persist the requested minZoom and
maxZoom values in the stored minScaleFactor and maxScaleFactor properties used
by applyLayoutUpdates, rather than directly assigning computed PDFKit limits.
Remove the unsafe fitScale-based assignments from this handler so zero or NaN
fit scales cannot reach PDFKit, and let applyLayoutUpdates perform its existing
guarded calculation and application.
---
Nitpick comments:
In `@android/build.gradle`:
- Around line 4-17: Update the root buildscript configuration around
kotlin_version and the org.jetbrains.kotlin:kotlin-gradle-plugin classpath to
avoid pinning or directly declaring the Kotlin Gradle Plugin, allowing the host
Flutter app’s declarative plugin configuration and version to control it. Follow
Flutter’s plugin author guidance while preserving the existing repository setup.
In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt`:
- Around line 474-477: Change the `linkHandler` field declaration in
`FlutterPDFView` from `LinkHandler` to `PDFLinkHandler`, then remove the
explicit cast in the `"preventLinkNavigation"` settings branch and call
`setPreventLinkNavigation` directly on `linkHandler`.
- Around line 79-83: Update the backgroundColor handling in FlutterPDFView so it
validates the value with an is Number check before converting it and calling
view.setBackgroundColor, matching the existing thumbnailRatio pattern; avoid the
unchecked cast that can throw ClassCastException for non-numeric values.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift`:
- Around line 616-618: Remove the unused FlutterPlatformView conformance and its
associated view() method from PDFViewController. Keep PDFViewController as the
registered platform view returning the FlutterPDFView container, and preserve
FlutterPDFView’s layout and PDFViewDelegate responsibilities.
- Around line 622-624: Update getPageCount to return the current page count
directly from the loaded document rather than the cached pageCount property,
while preserving the existing FlutterResult response contract.
- Around line 821-832: Update pdfViewWillClick so UIApplication.shared.open is
invoked only when url.scheme is http or https (case-insensitively); leave other
schemes exclusively to the existing onLinkHandler callback, preserving the
preventLinkNavigation behavior for allowed web links.
- Around line 518-531: Throttle onDraw notifications in startObserving by adding
a timestamp guard equivalent to Android’s DRAW_THROTTLE_MS behavior. Track the
last dispatched draw time and invoke handleOnDraw only when the throttle
interval has elapsed, while preserving the existing content-offset change check
and main-thread dispatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bf4957d-1b14-4805-9b11-c0b6dd834c74
⛔ Files ignored due to path filters (1)
example/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
CHANGELOG.mdandroid/build.gradleandroid/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.javaandroid/src/main/java/io/endigo/plugins/pdfviewflutter/PDFLinkHandler.javaandroid/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFactory.javaandroid/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.javaandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.ktandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFLinkHandler.ktandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFactory.ktandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.ktexample/android/gradle.propertiesios/flutter_pdfview.podspecios/flutter_pdfview/Package.swiftios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.mios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swiftios/flutter_pdfview/Sources/flutter_pdfview/PDFViewFlutterPlugin.mios/flutter_pdfview/Sources/flutter_pdfview/PDFViewFlutterPlugin.swiftios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/FlutterPDFView.hios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/PDFViewFlutterPlugin.hios/flutter_pdfview/Sources/flutter_pdfview_objc/FPVExceptionCatcher.mios/flutter_pdfview/Sources/flutter_pdfview_objc/include/FPVExceptionCatcher.hpubspec.yaml
💤 Files with no reviewable changes (8)
- ios/flutter_pdfview/Sources/flutter_pdfview/PDFViewFlutterPlugin.m
- android/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.java
- ios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/PDFViewFlutterPlugin.h
- android/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFactory.java
- android/src/main/java/io/endigo/plugins/pdfviewflutter/PDFLinkHandler.java
- ios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/FlutterPDFView.h
- android/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.java
- ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m
Android (Java -> Kotlin, KGP 2.0.0, JVM target 17): - All four plugin classes converted; src/main is now pure Kotlin - Behavior-preserving port: param-getter semantics, link handling, and thumbnailRatio clamp verified by the existing 64 native unit tests running unchanged against the Kotlin sources iOS (Objective-C -> Swift 5.9): - Plugin, factory, controller, and platform view converted - Registered class name FLTPDFViewFlutterPlugin preserved via @objc; UIKit/PDFKit delegate selectors keep their ObjC names - NSException guards preserved through an Objective-C shim target (FPVExceptionCatcher) since Swift cannot catch NSException; SPM uses a dedicated target, CocoaPods compiles the mixed sources in one pod - KVO on scrollView contentOffset moved to block-based observation with explicit invalidation in deinit - Verified: flutter build ios (SPM path), pod lib lint dynamic and static, registrant symbol check via nm Dart: - Harden platform-view remount race: generation-tag creation callbacks so a late callback from a disposed view cannot complete the new controller Verified on final tree: flutter analyze clean, 76 plugin + 5 example Dart tests, 64 Android unit tests, example APK and iOS builds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
The Objective-C fix (pdfURLFromFilePath) landed on the release branch after the Swift port was cut: URL(string:) returns nil for unescaped characters and falling back to fileURLWithPath on the full file:// string treats the scheme as part of the path. Replicate the helper in Swift, including file://localhost and file://hostname handling and percent-decoding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
The migration branch was cut before 1819d87 landed on the release branch; its changes to the deleted Java/ObjC sources are re-applied to the ported implementations: - PDFLinkHandler.kt: only auto-launch http(s) links, add CATEGORY_BROWSABLE, catch RuntimeException so hostile file:// or intent:// links cannot crash the host app (FileUriExposedException is not an ActivityNotFoundException) - FlutterPDFView.kt: always recycle Pdfium via the main-thread handler on dispose (View.post is dropped on detached views and leaked Pdfium, #261); split "PDFView disposed" from "No pages loaded" in getCurrentPageSize - FlutterPDFView.swift: replace the single 0.1s scroll-config delay with a 5-attempt retry (PDFKit may expose its scroll view late) and report render completion on the exception path too Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
Re-applies b503410 (landed on main after the migration branch was cut) to the Swift implementation: fitPolicy (WIDTH/HEIGHT/BOTH) parity with Android, autoScales managed manually so spacing and zoom stay independent, re-fit after placeholder bounds and rotation preserving relative user zoom, rotation-aware fit-scale computation, and fit-state resets in reload/setZoomLimits/onDoubleTap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
endigo
force-pushed
the
migrate/kotlin-swift
branch
from
August 2, 2026 08:57
e1edb3f to
54d194e
Compare
Address CodeRabbit review on the migration PR: - setZoomLimits: validate arguments like Android (INVALID_ARGS for zero/inverted limits), and skip the immediate PDFKit application when the fit scale is still 0/NaN pre-layout — the persisted multipliers are applied by the next layout pass instead (prevents NaN reaching PDFKit, same class as #268) - podspec swift_version 5.0 -> 5.9 to match Package.swift and the changelog wording Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
Replace AndroidPdfViewer setNightMode with a ColorMatrixColorFilter on a hardware layer so dark mode preserves hue. PdfColorMatrix holds the involution matrix (null = light); applySettings records colorMode and backgroundColor then applies once; gutters use M(bg); screenshots use saveLayer so captures match the on-screen theme.
Adds proper dark/light theming to the iOS view and makes runtime setting updates take effect at all. FPVThemedPage is a PDFPage subclass installed unconditionally through PDFDocumentDelegate.classForPage(), so flipping the mode never has to re-instantiate pages. It reads the mode from the document delegate at draw time: light mode is a plain super.draw, dark mode renders super into an offscreen bitmap sized to the clip bounding box (PDFView tiles zoomed pages), prefilled white because PDFPage.draw does not clear its background, and runs the shared luminance-inverting matrix over it via CIColorMatrix. That matrix inverts lightness while preserving hue, so photos stay recognisable instead of becoming negatives, and it matches the Android constant. onUpdateSettings was a no-op stub that silently dropped all seven keys Dart sends. It now applies colorMode (plus the deprecated nightMode bool), backgroundColor, preventLinkNavigation, enableSwipe and min/maxZoom, and accepts-and-ignores pageFling/pageSnap and anything unknown rather than throwing. Note this changes behaviour for apps that relied on updates being dropped. A colorMode change re-renders through a position-preserving variant of reload(): PDFKit caches rendered pages with no cache-flush API, so the document is handed back to the view, then page/scale/scroll offset are restored and the zoom limits re-derived (reassigning the document resets PDFKit's min/max). The #150 fit state is deliberately left untouched so the next layout pass does not re-fit, and page-changed callbacks are suppressed across the swap. catchingNSException and the NSError helpers become internal so the new file can guard its PDFKit calls the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X4UXNB2fV5BC8UUqzvBq6Q
Expose colorMode (light/dark/system) on PDFView, resolve system from Theme brightness, deprecate nightMode, push colorMode and backgroundColor in updatesMap, and update example/docs/tests accordingly.
# Conflicts: # CHANGELOG.md # example/pubspec.lock # pubspec.yaml
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
Owner
Author
1.5.0-beta.1 publishedPushed two commits and published the beta:
Gates re-run on the merged state
Per the release policy, target full |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.kt (1)
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the empty lifecycle override with an explicit no-op.
Detekt reports
EmptyFunctionBlockforonDetachedFromEngine. If no engine-scoped resource requires cleanup, use an expression body to preserve the no-op and remove the warning. IfPDFViewFactoryowns such resources, release them here instead.Proposed change
- override fun onDetachedFromEngine(binding: FlutterPlugin.FlutterPluginBinding) { - } + override fun onDetachedFromEngine(binding: FlutterPlugin.FlutterPluginBinding) = Unit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.kt` around lines 18 - 19, Update the PDFViewFlutterPlugin.onDetachedFromEngine override to use an explicit expression-body no-op when no engine-scoped cleanup is needed, eliminating the EmptyFunctionBlock warning; if PDFViewFactory owns engine-scoped resources, release them in this lifecycle method instead.Source: Linters/SAST tools
ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift (1)
540-553: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThrottle
onDrawto match the Android implementation.The observation posts
handleOnDrawfor everycontentOffsetchange. Each call sends a method-channel message. During a scroll, this produces one message per frame or more. The Android implementation throttles the same callback withDRAW_THROTTLE_MS(seeandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt). Add an equivalent time-based throttle so both platforms send comparable traffic.♻️ Proposed throttle
+ /// Matches Android's DRAW_THROTTLE_MS so both platforms emit onDraw at a + /// comparable rate. + private static let drawThrottle: TimeInterval = 1.0 / 60.0 + private var lastDrawTime: TimeInterval = 0 + private func startObserving() { guard let scrollView, contentOffsetObservation == nil else { return } contentOffsetObservation = scrollView.observe( \.contentOffset, options: [.new, .old] ) { [weak self] _, change in let newOffset = change.newValue ?? .zero let oldOffset = change.oldValue ?? .zero guard newOffset != oldOffset else { return } DispatchQueue.main.async { [weak self] in - self?.handleOnDraw() + guard let self else { return } + let now = CACurrentMediaTime() + guard now - lastDrawTime >= Self.drawThrottle else { return } + lastDrawTime = now + handleOnDraw() } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around lines 540 - 553, Update startObserving and the handleOnDraw dispatch path to apply an Android-equivalent time-based throttle using DRAW_THROTTLE_MS (or the corresponding iOS duration), ensuring contentOffset changes within the throttle window do not send additional onDraw method-channel messages while preserving the existing callback behavior after the interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift`:
- Around line 974-985: Update pdfViewWillClick(onLink:with:) so
UIApplication.shared.open is called only when the URL scheme is HTTP or HTTPS,
using a case-insensitive check. Preserve invoking
controller?.invokeChannelMethod("onLinkHandler", arguments: url.absoluteString)
for every link, regardless of scheme.
- Around line 839-863: The reload flow in reload() must reset didLoadComplete
and hasSentInitialPage before reconfiguring the PDF view, then invoke
handleRenderCompleted(document.pageCount) after the replacement document is
configured so the new document emits its load-completion callback. Preserve the
existing reload result behavior.
In `@README.md`:
- Line 26: Update the dependency code fence in README.md to use the yaml
language identifier, changing the opening fence from ``` to ```yaml while
preserving the block contents.
- Line 21: Update the “Trying the 1.5.0 beta” heading from level 4 to level 3 so
it follows the preceding level-2 heading and preserves the README heading
hierarchy.
---
Nitpick comments:
In
`@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.kt`:
- Around line 18-19: Update the PDFViewFlutterPlugin.onDetachedFromEngine
override to use an explicit expression-body no-op when no engine-scoped cleanup
is needed, eliminating the EmptyFunctionBlock warning; if PDFViewFactory owns
engine-scoped resources, release them in this lifecycle method instead.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift`:
- Around line 540-553: Update startObserving and the handleOnDraw dispatch path
to apply an Android-equivalent time-based throttle using DRAW_THROTTLE_MS (or
the corresponding iOS duration), ensuring contentOffset changes within the
throttle window do not send additional onDraw method-channel messages while
preserving the existing callback behavior after the interval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34316dc3-7220-4f9a-920a-639c38191e6e
⛔ Files ignored due to path filters (1)
example/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
CHANGELOG.mdREADME.mdandroid/build.gradleandroid/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.javaandroid/src/main/java/io/endigo/plugins/pdfviewflutter/PDFLinkHandler.javaandroid/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFactory.javaandroid/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.javaandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.ktandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFLinkHandler.ktandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFactory.ktandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.ktexample/android/gradle.propertiesios/flutter_pdfview.podspecios/flutter_pdfview/Package.swiftios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.mios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swiftios/flutter_pdfview/Sources/flutter_pdfview/PDFViewFlutterPlugin.mios/flutter_pdfview/Sources/flutter_pdfview/PDFViewFlutterPlugin.swiftios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/FlutterPDFView.hios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/PDFViewFlutterPlugin.hios/flutter_pdfview/Sources/flutter_pdfview_objc/FPVExceptionCatcher.mios/flutter_pdfview/Sources/flutter_pdfview_objc/include/FPVExceptionCatcher.hpubspec.yaml
💤 Files with no reviewable changes (8)
- ios/flutter_pdfview/Sources/flutter_pdfview/PDFViewFlutterPlugin.m
- ios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/PDFViewFlutterPlugin.h
- android/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFactory.java
- ios/flutter_pdfview/Sources/flutter_pdfview/include/flutter_pdfview/FlutterPDFView.h
- android/src/main/java/io/endigo/plugins/pdfviewflutter/PDFViewFlutterPlugin.java
- ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m
- android/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.java
- android/src/main/java/io/endigo/plugins/pdfviewflutter/PDFLinkHandler.java
🚧 Files skipped from review as they are similar to previous changes (9)
- example/android/gradle.properties
- ios/flutter_pdfview.podspec
- ios/flutter_pdfview/Sources/flutter_pdfview/PDFViewFlutterPlugin.swift
- pubspec.yaml
- ios/flutter_pdfview/Sources/flutter_pdfview_objc/include/FPVExceptionCatcher.h
- CHANGELOG.md
- android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PDFViewFactory.kt
- ios/flutter_pdfview/Package.swift
- android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt
Comment on lines
+839
to
+863
| func reload(_: FlutterMethodCall, result: FlutterResult) { | ||
| pdfView.document = document | ||
| hasAppliedInitialFit = false | ||
| lastFitScale = 0 | ||
| lastLayoutSize = .zero | ||
| if let document, document.pageCount > 0, let firstPage = document.page(at: 0) { | ||
| pdfView.go(to: firstPage) | ||
|
|
||
| let pageBounds = firstPage.bounds(for: .mediaBox) | ||
| pdfView.go( | ||
| to: CGRect(x: 0, y: pageBounds.size.height, width: 1, height: 1), | ||
| on: firstPage | ||
| ) | ||
|
|
||
| let fitScale = fitScaleForCurrentPolicy() | ||
| if fitScale.isFinite, fitScale > 0 { | ||
| pdfView.scaleFactor = fitScale | ||
| lastFitScale = fitScale | ||
| lastLayoutSize = bounds.size | ||
| hasAppliedInitialFit = true | ||
| } | ||
| } | ||
|
|
||
| result(NSNumber(value: true)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare reload behaviour and render callbacks across platforms and Dart.
fd -e kt -e dart -e swift | xargs rg -n -C6 '\breload\b|onLoadComplete|onRender'Repository: endigo/flutter_pdfview
Length of output: 36787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "iOS FlutterPDFView.swift relevant sections"
sed -n '120,200p' ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift
sed -n '800,970p' ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift
echo
echo "Android FlutterPDFView.kt relevant sections"
sed -n '130,205p' android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt
sed -n '382,400p' android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt
echo
echo "Dart reload and callback handling"
sed -n '186,192p' lib/src/pdf_view_controller.dart
sed -n '50,70p' lib/src/pdf_view_controller.dart
echo
echo "Find document loading / initial render triggers in iOS"
rg -n -C 5 'loadDocument|handleRenderCompleted|didLoadComplete|hasSentInitialPage|invokeChannelMethod\("onRender"|onLoadComplete' ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swiftRepository: endigo/flutter_pdfview
Length of output: 18296
Reload should reset completion state before reconfiguring the iOS PDF view.
reload() replaces the document but leaves didLoadComplete true and hasSentInitialPage true. handleRenderCompleted() emits onLoadComplete only while didLoadComplete is false, so reload callbacks can miss the new document state. Reset these flags in reload() and call handleRenderCompleted(document.pageCount) when the reload completes, matching Android’s callback contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around
lines 839 - 863, The reload flow in reload() must reset didLoadComplete and
hasSentInitialPage before reconfiguring the PDF view, then invoke
handleRenderCompleted(document.pageCount) after the replacement document is
configured so the new document emits its load-completion callback. Preserve the
existing reload result behavior.
| flutter_pdfview: ^1.4.5 | ||
| ``` | ||
|
|
||
| #### Trying the 1.5.0 beta |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a level-3 heading for the beta section.
The preceding heading is level 2, but this heading is level 4. Change #### Trying the 1.5.0 beta to ### Trying the 1.5.0 beta to preserve the heading hierarchy and resolve MD001.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 21-21: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 21, Update the “Trying the 1.5.0 beta” heading from level
4 to level 3 so it follows the preceding level-2 heading and preserves the
README heading hierarchy.
Source: Linters/SAST tools
| `1.5.0-beta.1` ports the native implementations to Kotlin (Android) and Swift (iOS) with no | ||
| public Dart API changes. Pre-releases are not picked up by a `^` constraint, so pin it explicitly: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the dependency code fence.
Use ```yaml instead of ``` so the fenced block identifies its syntax and resolves MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 26, Update the dependency code fence in README.md to use
the yaml language identifier, changing the opening fence from ``` to ```yaml
while preserving the block contents.
Source: Linters/SAST tools
…213) ColorFiltered and ShaderMask do not apply to UiKitView contents; this is a Flutter composition limitation, not a plugin bug. Document official evidence and practical workarounds (nightMode, scrim/blur layout, screenshot + filter).
gestureRecognizers + TapGestureRecognizer is unreliable on platform views. Report single taps from native AndroidPdfViewer / PDFKit via onTap instead.
Pdfium paints widget appearance streams and does not regenerate broken/missing appearances like Adobe. Document producer-side workarounds after investigating the sample PDF's first PDTextField.
Defer document open until the platform view has a usable non-zero size, keep the native view hidden until first successful fit/render, and re-fit when the first layout settles. iOS open paths now report missing, unreadable, empty, or corrupt documents via onError and force a layout pass after attach so PDFs are not stuck blank until background/foreground. Addresses #40, #127, #190. No package version bump (coordinator release).
Hybrid composition and hardware layers made View.draw / drawing-cache screenshots blank white. Android now prefers PixelCopy of the platform view with a software-layer draw fallback; iOS implements getScreenshot by rasterizing the PDFKit layer and falling back to PDFPage.draw.
Android: density-aware page-part cache, apply Dart thumbnailRatio 0.8 when omitted (library default 0.3), keep useBestQuality / enableAntialiasing / enableRenderDuringScale true when params missing. iOS: pin PDFView contentScaleFactor to screen scale. Document quality knobs and hard Pdfium spatial-resolution limits in README.
An encrypted PDF used to leave a blank view with no way to supply a password: Android surfaced Pdfium's failure only as an opaque onError string, and iOS skipped the setup a document needs to render, so even a correct password could not recover the view. - New `onPasswordRequired` callback reporting whether the document needs a password (`PDFPasswordFailure.missing`) or rejected the one it was given (`PDFPasswordFailure.incorrect`) - New `PDFViewController.unlock(password)`, and `password` is now part of the settings diff, so both the imperative and the declarative route reopen the document inside the existing platform view — a wrong password can be retried without recreating the viewer - Android: recognise `PdfPasswordException` (including wrapped and repackaged variants) and re-run the configurator with the new password, completing `unlock` from the load callbacks - iOS: defer the page/scroll setup while the document is locked and run it once a password opens it; `getPageCount()` no longer returns null between the render callback and the first layout pass Tests: Dart unit coverage for the channel contract, Android unit coverage for the exception detection, and an integration_test suite driving the real native viewers against a checked-in encrypted fixture (generated by scripts/make_protected_pdf.py). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
feat: unlock password-protected documents (#274)
Unlock password-protected documents (#274) after merge of #359. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
endigo
force-pushed
the
migrate/kotlin-swift
branch
from
August 3, 2026 14:02
f83bdfc to
c1e9c27
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift (2)
1715-1726: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict automatic link launching to HTTP(S).
pdfViewWillClick(onLink:with:)still opens every URL the document contains. A PDF is untrusted input, so a crafted document can triggertel:,sms:,mailto:, or a custom app scheme with no user confirmation.PDFLinkHandler.kton Android limits automatic launching to HTTP(S), so the platforms diverge.A previous review flagged this and the thread is marked as addressed, but the reviewed code contains no scheme check.
🔒️ Proposed fix
func pdfViewWillClick(onLink _: PDFView, with url: URL) { - if !preventLinkNavigation { + let scheme = url.scheme?.lowercased() + let isWebLink = scheme == "http" || scheme == "https" + if !preventLinkNavigation, isWebLink { UIApplication.shared.open(url, options: [:]) { success in🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around lines 1715 - 1726, Update pdfViewWillClick(onLink:with:) so automatic UIApplication.shared.open execution occurs only when url.scheme is HTTP or HTTPS, using a case-insensitive comparison; continue invoking onLinkHandler for every clicked URL.
1154-1189: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
reloaddoes not reset the completion flags and never reports render completion.
reloadreplaces the document and resets the fit state, but it leavesdidLoadCompleteandhasSentInitialPageset totrue, and it does not callhandleRenderCompleted. Dart therefore receives noonRenderand noonLoadCompleteafterPDFViewController.reload(). Android'sreloadre-runs the configurator, so both callbacks fire there.Reset the flags and report completion so both platforms deliver the same callbacks.
🐛 Proposed fix
pdfView.document = document hasAppliedInitialFit = false lastFitScale = 0 lastLayoutSize = .zero isContentRevealed = false pdfView.isHidden = true + didLoadComplete = false + hasSentInitialPage = false + defaultPageSet = false if let document, document.pageCount > 0, let firstPage = document.page(at: 0) {Then report completion before returning the result:
if let document { handleRenderCompleted(NSNumber(value: document.pageCount)) } result(NSNumber(value: true))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around lines 1154 - 1189, Update reload to reset didLoadComplete and hasSentInitialPage when replacing the document, then invoke handleRenderCompleted with the reloaded document’s page count before returning the successful result. Preserve the existing deferred-load path and fit/reset behavior.
🧹 Nitpick comments (4)
ios/flutter_pdfview/Sources/flutter_pdfview/FPVThemedPage.swift (1)
39-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse exact fractions so the matrix matches Android and stays an involution.
The Android constant uses
1f/3fand-2f/3f. Here the coefficients are rounded to0.333and-0.667, so each row sums to-1.001instead of-1. White maps to-0.001and clamps to 0, andM(M(v))drifts by about 0.1% per pass. The doc comment above claims the matrix is shared with Android and is an involution.♻️ Proposed fix
- private static let diagonal: CGFloat = 0.333 - private static let offDiagonal: CGFloat = -0.667 + private static let diagonal: CGFloat = 1.0 / 3.0 + private static let offDiagonal: CGFloat = -2.0 / 3.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FPVThemedPage.swift` around lines 39 - 47, Update the diagonal and offDiagonal constants used by luminanceInvertR, luminanceInvertG, and luminanceInvertB to use exact one-third and negative two-thirds fractions, matching the Android constants and preserving the matrix’s involution property; leave the alpha and bias vectors unchanged.android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt (2)
858-921: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
centeredSecondaryOffsetanddocumentFitsAlongPrimaryboth scan every page.Each call iterates
pageCountand callsview.getPageSize(i).applyPagePlacementruns after load, after everysetPage, and after eachpageAlignmentupdate, so a large document performs two full scans per navigation on the main thread.Cache the maximum secondary dimension and the total primary length per document and per zoom level, and invalidate the cache on reload or zoom change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt` around lines 858 - 921, Cache the computed maximum secondary dimension and total primary document length used by centeredSecondaryOffset and documentFitsAlongPrimary, keyed by the current document and zoom level, so applyPagePlacement avoids rescanning every page on navigation. Reuse cached values in both helpers and invalidate the cache whenever the document reloads or the zoom changes.
884-898: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReflection on
PDFView.currentYOffsetcan silently disable top alignment in release builds.
forcePrimaryOffsetIfShortresolves the field by name throughgetDeclaredField. If R8 renames or removes that field in the app's release build, the lookup throws, thecatchlogs a warning, andPageAlignment.topsilently falls back to centered layout. The failure is invisible to the Dart API, so users see a behavior difference between debug and release.Add a consumer ProGuard rule that keeps these fields, or replace the reflection with a public API call such as
moveTocombined with the offsets computed fromdocumentFitsAlongPrimary.Also confirm the field names exist in AndroidPdfViewer 3.2.8 and are not obfuscated by the plugin's own consumer rules.
#!/bin/bash # Check for consumer ProGuard rules that protect the reflected AndroidPdfViewer fields. fd -t f -e pro -e txt . android | while IFS= read -r f; do echo "=== $f ==="; cat "$f" done rg -n 'consumerProguardFiles|minifyEnabled|proguard' --glob '*.gradle' --glob '*.gradle.kts' rg -n 'currentXOffset|currentYOffset|getDeclaredField' -g '*.kt' -g '*.java'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt` around lines 884 - 898, Protect the reflected AndroidPdfViewer fields used by forcePrimaryOffsetIfShort with a consumer ProGuard rule, covering currentXOffset and currentYOffset and preserving their names for AndroidPdfViewer 3.2.8. Verify the fields exist in that dependency and that the rule is included by the plugin so release builds retain the top-alignment behavior.test/page_alignment_test.dart (1)
19-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated platform-view mock harness in
test/page_alignment_test.dartandtest/password_test.dart. Both new test files copy the sameSystemChannels.platform_viewscreatehandler, creation-params decoding, per-view channel recording, and teardown loop that already exists intest/creation_params_test.dart. One shared helper removes the need to keep three copies in sync.
test/page_alignment_test.dart#L19-L67: replace the localsetUp/tearDownwith the shared harness.test/password_test.dart#L157-L205: replace the localsetUp/tearDownin thepassword changes over the method channelgroup with the same shared harness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/page_alignment_test.dart` around lines 19 - 67, Extract the duplicated platform-view mock setup, parameter decoding, per-view channel recording, and teardown into one shared test helper, following the existing harness in test/creation_params_test.dart. Replace the local setUp/tearDown blocks in test/page_alignment_test.dart lines 19-67 and test/password_test.dart lines 157-205 with that helper; both sites require the same direct change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@example/integration_test/password_test.dart`:
- Around line 185-188: Wrap the awaited reported.controller!.getPageCount() call
in tester.runAsync, matching the existing unlock and setPage patterns. Apply the
same wrapping to the corresponding controller calls around the other affected
assertions in this test, including the cases near the later referenced
locations, while preserving their existing expectations.
In `@example/lib/main.dart`:
- Around line 282-338: Update _promptForPassword to create the
TextEditingController before calling showDialog instead of inside its builder,
and dispose it in a finally block that wraps the dialog interaction. Preserve
the existing cancel, submit, and dismissal behavior while ensuring disposal
occurs on every exit path.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift`:
- Around line 1233-1235: Update the password handling in FlutterPDFView’s
settings application to recognize an NSNull value from
_PDFViewSettings.updatesMap as a cleared password and invoke applyPassword
accordingly, matching Android’s document-reopen behavior. Preserve the existing
handling for non-null String passwords.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FPVThemedPage.swift`:
- Around line 85-89: Synchronize access to isDarkMode between setColorMode and
FPVThemedPage.draw(with:to:), using the existing FlutterPDFView state-management
pattern or an os_unfair_lock/atomic wrapper. Ensure draw reads a consistent
snapshot while main-thread updates during loadDocument and onUpdateSettings
remain safe.
---
Duplicate comments:
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift`:
- Around line 1715-1726: Update pdfViewWillClick(onLink:with:) so automatic
UIApplication.shared.open execution occurs only when url.scheme is HTTP or
HTTPS, using a case-insensitive comparison; continue invoking onLinkHandler for
every clicked URL.
- Around line 1154-1189: Update reload to reset didLoadComplete and
hasSentInitialPage when replacing the document, then invoke
handleRenderCompleted with the reloaded document’s page count before returning
the successful result. Preserve the existing deferred-load path and fit/reset
behavior.
---
Nitpick comments:
In `@android/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.kt`:
- Around line 858-921: Cache the computed maximum secondary dimension and total
primary document length used by centeredSecondaryOffset and
documentFitsAlongPrimary, keyed by the current document and zoom level, so
applyPagePlacement avoids rescanning every page on navigation. Reuse cached
values in both helpers and invalidate the cache whenever the document reloads or
the zoom changes.
- Around line 884-898: Protect the reflected AndroidPdfViewer fields used by
forcePrimaryOffsetIfShort with a consumer ProGuard rule, covering currentXOffset
and currentYOffset and preserving their names for AndroidPdfViewer 3.2.8. Verify
the fields exist in that dependency and that the rule is included by the plugin
so release builds retain the top-alignment behavior.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FPVThemedPage.swift`:
- Around line 39-47: Update the diagonal and offDiagonal constants used by
luminanceInvertR, luminanceInvertG, and luminanceInvertB to use exact one-third
and negative two-thirds fractions, matching the Android constants and preserving
the matrix’s involution property; leave the alpha and bias vectors unchanged.
In `@test/page_alignment_test.dart`:
- Around line 19-67: Extract the duplicated platform-view mock setup, parameter
decoding, per-view channel recording, and teardown into one shared test helper,
following the existing harness in test/creation_params_test.dart. Replace the
local setUp/tearDown blocks in test/page_alignment_test.dart lines 19-67 and
test/password_test.dart lines 157-205 with that helper; both sites require the
same direct change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be1ca432-00e8-4309-bf82-0e33492fe083
⛔ Files ignored due to path filters (2)
example/assets/demo-protected.pdfis excluded by!**/*.pdfexample/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
CHANGELOG.mdREADME.mdandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/FlutterPDFView.ktandroid/src/main/kotlin/io/endigo/plugins/pdfviewflutter/PdfColorMatrix.ktandroid/src/test/java/com/example/pdfiumfork/PdfPasswordException.javaandroid/src/test/java/io/endigo/plugins/pdfviewflutter/FlutterPDFViewColorModeTest.javaandroid/src/test/java/io/endigo/plugins/pdfviewflutter/FlutterPDFViewParamsTest.javaandroid/src/test/java/io/endigo/plugins/pdfviewflutter/FlutterPDFViewPasswordTest.javaandroid/src/test/java/io/endigo/plugins/pdfviewflutter/FlutterPDFViewQualityDefaultsTest.javaandroid/src/test/java/io/endigo/plugins/pdfviewflutter/FlutterPDFViewScreenshotTest.javaandroid/src/test/java/io/endigo/plugins/pdfviewflutter/FlutterPDFViewThumbnailRatioTest.javaandroid/src/test/java/io/endigo/plugins/pdfviewflutter/PdfColorMatrixTest.javaexample/integration_test/password_test.dartexample/lib/main.dartexample/pubspec.yamlexample/test/widget_test.dartios/flutter_pdfview/Sources/flutter_pdfview/FPVThemedPage.swiftios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swiftlib/flutter_pdfview.dartlib/src/pdf_view.dartlib/src/pdf_view_controller.dartlib/src/pdf_view_settings.dartlib/src/types.dartpubspec.yamlscripts/make_protected_pdf.pytest/creation_params_test.darttest/flutter_pdfview_test.darttest/page_alignment_test.darttest/password_test.darttest/pdf_view_controller_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- pubspec.yaml
Comment on lines
+185
to
+188
| expect(await waitFor(tester, () => reported.loadedPages != null), isTrue); | ||
| expect(reported.passwordFailures, isEmpty); | ||
| expect(await reported.controller!.getPageCount(), protectedPageCount); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wrap the controller calls in tester.runAsync for consistency.
Line 187 awaits getPageCount() inside the fake-async test zone. The other platform-channel calls in this file (unlock, setPage) run inside tester.runAsync. A native reply that needs real asynchronous time can stall in the test zone and time out. The same pattern appears at Line 244 and Line 323.
♻️ Proposed change
- expect(await reported.controller!.getPageCount(), protectedPageCount);
+ late int? pageCount;
+ await tester.runAsync(() async {
+ pageCount = await reported.controller!.getPageCount();
+ });
+ expect(pageCount, protectedPageCount);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(await waitFor(tester, () => reported.loadedPages != null), isTrue); | |
| expect(reported.passwordFailures, isEmpty); | |
| expect(await reported.controller!.getPageCount(), protectedPageCount); | |
| expect(await waitFor(tester, () => reported.loadedPages != null), isTrue); | |
| expect(reported.passwordFailures, isEmpty); | |
| late int? pageCount; | |
| await tester.runAsync(() async { | |
| pageCount = await reported.controller!.getPageCount(); | |
| }); | |
| expect(pageCount, protectedPageCount); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@example/integration_test/password_test.dart` around lines 185 - 188, Wrap the
awaited reported.controller!.getPageCount() call in tester.runAsync, matching
the existing unlock and setPage patterns. Apply the same wrapping to the
corresponding controller calls around the other affected assertions in this
test, including the cases near the later referenced locations, while preserving
their existing expectations.
Comment on lines
+282
to
+338
| /// Guards against stacking dialogs: a rejected password reports again. | ||
| bool _isPrompting = false; | ||
|
|
||
| /// Asks for a password and hands it to the controller, which reopens the | ||
| /// document in place. | ||
| Future<void> _promptForPassword(PDFPasswordFailure failure) async { | ||
| if (_isPrompting) { | ||
| return; | ||
| } | ||
| _isPrompting = true; | ||
| // The prompt replaces the error banner for this failure. | ||
| setState(() { | ||
| _errorMessage = ''; | ||
| }); | ||
| try { | ||
| final PDFViewController controller = await _controller.future; | ||
| if (!mounted) { | ||
| return; | ||
| } | ||
| final String? password = await showDialog<String>( | ||
| context: context, | ||
| barrierDismissible: false, | ||
| builder: (BuildContext context) { | ||
| final TextEditingController field = TextEditingController(); | ||
| return AlertDialog( | ||
| title: const Text('Password required'), | ||
| content: TextField( | ||
| controller: field, | ||
| autofocus: true, | ||
| obscureText: true, | ||
| decoration: InputDecoration( | ||
| labelText: 'Password', | ||
| errorText: failure == PDFPasswordFailure.incorrect | ||
| ? 'That password did not open the document' | ||
| : null, | ||
| ), | ||
| onSubmitted: (String value) => Navigator.pop(context, value), | ||
| ), | ||
| actions: <Widget>[ | ||
| TextButton(onPressed: () => Navigator.pop(context), child: const Text('Cancel')), | ||
| TextButton( | ||
| onPressed: () => Navigator.pop(context, field.text), | ||
| child: const Text('Open'), | ||
| ), | ||
| ], | ||
| ); | ||
| }, | ||
| ); | ||
| if (password == null || !mounted) { | ||
| return; | ||
| } | ||
| final bool unlocked = await controller.unlock(password); | ||
| debugPrint(unlocked ? 'document unlocked' : 'wrong password'); | ||
| } finally { | ||
| _isPrompting = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Dispose the TextEditingController created for the password dialog.
The field controller at line 305 is created inside the showDialog builder. Nothing disposes it, whichever path the dialog takes: cancel, submit, or dismiss. This leaks a controller on every password prompt. Move field creation before the showDialog call and dispose it in a finally block so it is guaranteed to run.
🧹 Proposed fix to dispose the controller
try {
final PDFViewController controller = await _controller.future;
if (!mounted) {
return;
}
+ final TextEditingController field = TextEditingController();
final String? password = await showDialog<String>(
context: context,
barrierDismissible: false,
builder: (BuildContext context) {
- final TextEditingController field = TextEditingController();
return AlertDialog(
title: const Text('Password required'),
content: TextField(
controller: field,
autofocus: true,
obscureText: true,
decoration: InputDecoration(
labelText: 'Password',
errorText: failure == PDFPasswordFailure.incorrect
? 'That password did not open the document'
: null,
),
onSubmitted: (String value) => Navigator.pop(context, value),
),
actions: <Widget>[
TextButton(onPressed: () => Navigator.pop(context), child: const Text('Cancel')),
TextButton(
onPressed: () => Navigator.pop(context, field.text),
child: const Text('Open'),
),
],
);
},
);
+ field.dispose();
if (password == null || !mounted) {
return;
}
final bool unlocked = await controller.unlock(password);
debugPrint(unlocked ? 'document unlocked' : 'wrong password');
} finally {
_isPrompting = false;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@example/lib/main.dart` around lines 282 - 338, Update _promptForPassword to
create the TextEditingController before calling showDialog instead of inside its
builder, and dispose it in a finally block that wraps the dialog interaction.
Preserve the existing cancel, submit, and dismissal behavior while ensuring
disposal occurs on every exit path.
Comment on lines
+1233
to
+1235
| if let password = settings["password"] as? String { | ||
| applyPassword(password) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A cleared password is ignored on iOS but reopens the document on Android.
settings["password"] as? String fails for NSNull, so clearing PDFView.password at runtime is a no-op here. Dart emits updates['password'] = null in _PDFViewSettings.updatesMap, and Android's applySettings calls applyPassword(null, null), which recycles and reopens the document without a password.
Either handle the NSNull case the same way, or state the platform difference in the PDFView.password doc comment, which currently says only that changing it reopens the document.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.swift` around
lines 1233 - 1235, Update the password handling in FlutterPDFView’s settings
application to recognize an NSNull value from _PDFViewSettings.updatesMap as a
cleared password and invoke applyPassword accordingly, matching Android’s
document-reopen behavior. Preserve the existing handling for non-null String
passwords.
Comment on lines
+85
to
+89
| override func draw(with box: PDFDisplayBox, to context: CGContext) { | ||
| guard let owner = document?.delegate as? FlutterPDFView, owner.isDarkMode else { | ||
| super.draw(with: box, to: context) | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
isDarkMode is read off the main thread without synchronization.
draw(with:to:) reads owner.isDarkMode. The comment on isDarkMode in FlutterPDFView.swift states that PDFKit renders pages off the main thread, while setColorMode writes the property from the main thread during loadDocument and onUpdateSettings. That is an unsynchronized cross-thread access to a mutable Bool.
The visible effect is limited, because rerenderPreservingPosition() re-renders after a mode flip. Still, make the access explicit: guard the property with an os_unfair_lock or an atomic wrapper, or snapshot the mode into a let that only the main thread replaces alongside the document swap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FPVThemedPage.swift` around lines
85 - 89, Synchronize access to isDarkMode between setColorMode and
FPVThemedPage.draw(with:to:), using the existing FlutterPDFView state-management
pattern or an os_unfair_lock/atomic wrapper. Ensure draw reads a consistent
snapshot while main-thread updates during loadDocument and onUpdateSettings
remain safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full native-language migration. No public Dart API changes; version bumped to 1.5.0-beta.1.
Status: #344 is merged, and
mainat 1.4.5 stable has been merged into this branch. Published to pub.dev as1.5.0-beta.1; beta feedback is tracked in #351.Android: Java → Kotlin
src/mainis now pure Kotlin); KGP 2.0.0, JVM target 17 — 2.0.0 deliberately matches what Flutter's Gradle tooling puts on the classpathClassCastExceptionon wrong-typed params, silentgetFloatfallback)getFitPolicywith an explicit-null value falls toBOTHinstead of NPE (unreachable from Dart)iOS: Objective-C → Swift 5.9
FLTPDFViewFlutterPluginpreserved via@objc(verified in the generated registrant and vianmon the built binary — all UIKit/PDFKit delegate selectors keep their ObjC names)NSException, so the four@try/@catchguards around PDFKit route through a small Objective-C shim (FPVExceptionCatcher): a separate SPM target, mixed into the single pod for CocoaPodsdeinit; full teardown semantics preserved (Memory leak #261)file:URI fix from the release branch is ported to Swift (pdfURL(fromFilePath:)), includingfile://localhost/hostname handling and percent-decodingVerification (after rebase onto version-1.4.5-beta.4)
testDebugUnitTest64/64,flutter build apk --debug✓flutter build ios --debug --no-codesign✓ (SPM path);pod lib lintpassed in both dynamic and static linkageflutter analyzeclean, 76/76 testsKnown follow-ups (non-blocking)
🤖 Generated with Claude Code
https://claude.ai/code/session_01XKmmG2v9eJSWFDLy6mRe8F
Summary by CodeRabbit