fix: ios callbacks, zoom config, privacy manifest#336
Conversation
Closes #334 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Closes #271 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Dispatch onError asynchronously so Dart handler is ready (#211) - Fire onPageChanged after initial render completes (#66) - Preserve user-configured maxScaleFactor in layoutSubviews (#247) - Add minScaleFactor support from Dart parameters Closes #211 Closes #66 Closes #247 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Closes #296 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds min/max zoom settings to the PDFView API and implements them on Android and iOS; updates Android Java compatibility to Java 17; raises iOS minimum deployment to 13.0 and adds an iOS privacy resource; removes a night mode color from the example; bumps package version to 1.4.5 and updates CHANGELOG. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
android/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.java (1)
119-123: Consider validating zoom constraints.The implementation works correctly for typical use cases, but there's no validation that
maxZoom >= minZoom. If a caller accidentally passes inverted values (e.g.,maxZoom=1.0, minZoom=2.0), the behavior depends on the underlying library.🛡️ Proposed defensive validation
Float maxZoom = getFloat(params, "maxZoom"); Float minZoom = getFloat(params, "minZoom"); - pdfView.setMaxZoom(maxZoom != null ? maxZoom : DEFAULT_MAX_ZOOM); - pdfView.setMinZoom(minZoom != null ? minZoom : DEFAULT_MIN_ZOOM); + float effectiveMax = maxZoom != null ? maxZoom : DEFAULT_MAX_ZOOM; + float effectiveMin = minZoom != null ? minZoom : DEFAULT_MIN_ZOOM; + if (effectiveMin > effectiveMax) { + effectiveMin = effectiveMax; + } + pdfView.setMaxZoom(effectiveMax); + pdfView.setMinZoom(effectiveMin);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.java` around lines 119 - 123, Validate and enforce that maxZoom >= minZoom before calling pdfView.setMaxZoom and setMinZoom: after retrieving Float maxZoom and minZoom via getFloat (and applying DEFAULT_MAX_ZOOM/DEFAULT_MIN_ZOOM fallbacks), check the two values and if maxZoom < minZoom either swap them or clamp them (e.g., set maxZoom = Math.max(maxZoom, minZoom) and minZoom = Math.min(maxZoom, minZoom)) so the pdfView.setMaxZoom and pdfView.setMinZoom calls always receive consistent constraints; make this change in the block that currently calls pdfView.setMaxZoom(...) and pdfView.setMinZoom(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m`:
- Around line 317-323: The synthetic initial onPageChanged currently sent from
handleRenderCompleted: can fire before layoutSubviews applies _defaultPage and
can double-fire alongside PDFViewPageChangedNotification; update
handleRenderCompleted: (and/or the code that sends the synthetic event) to defer
emitting the onPageChanged (via [_controller
invokeChannelMethod:@"onPageChanged" ...]) until the PDFView’s current page
matches the intended initial page (_defaultPage) or until layoutSubviews has
applied the page, and guard with a boolean flag (e.g., _hasSentInitialPage) so
that once the correct initial page is observed you send exactly one synthetic
event and ignore subsequent PDFViewPageChangedNotification duplicates.
- Around line 267-271: The code uses fmin when computing _pdfView.minScaleFactor
which forces any caller-provided _minScaleFactor larger than fitScale to be
lowered; change the logic so minScaleFactor is the greater of fitScale and
_minScaleFactor (use fmax semantics) when _minScaleFactor > 0 so a higher
minZoom is enforced; update the assignment that references fitScale,
_minScaleFactor and _pdfView.minScaleFactor (and keep the rest of the block
handling _pdfView.maxScaleFactor and _autoSpacing/_pdfView.scaleFactor
unchanged).
In `@lib/flutter_pdfview.dart`:
- Around line 314-315: The rebuild/update path is missing maxZoom and minZoom so
changes are ignored; update the FlutterPdfView widget's updatesMap() method to
include the maxZoom and minZoom fields (same keys used in creation) and handle
nulls consistently (omit or set to null) so the platform view receives updated
zoom bounds; also mirror this inclusion where creation/update maps are built
(the same code paths referenced around the constructor fields maxZoom/minZoom
and the other map-building helpers mentioned) so both initial creation and
subsequent updates propagate the new values.
In `@pubspec.yaml`:
- Line 3: The package version must be bumped to a breaking release because
raising the iOS minimum to 13.0 in ios/flutter_pdfview/Package.swift and
ios/flutter_pdfview.podspec is not backward-compatible; update the version field
in pubspec.yaml from 1.4.5 to a new major release (e.g., 2.0.0), update any
related metadata (CHANGELOG entry and release notes) to indicate the breaking
change, and regenerate/update lockfiles or CI artifacts as needed so consumers
cannot accidentally receive this change via a patch semver range.
---
Nitpick comments:
In `@android/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.java`:
- Around line 119-123: Validate and enforce that maxZoom >= minZoom before
calling pdfView.setMaxZoom and setMinZoom: after retrieving Float maxZoom and
minZoom via getFloat (and applying DEFAULT_MAX_ZOOM/DEFAULT_MIN_ZOOM fallbacks),
check the two values and if maxZoom < minZoom either swap them or clamp them
(e.g., set maxZoom = Math.max(maxZoom, minZoom) and minZoom = Math.min(maxZoom,
minZoom)) so the pdfView.setMaxZoom and pdfView.setMinZoom calls always receive
consistent constraints; make this change in the block that currently calls
pdfView.setMaxZoom(...) and pdfView.setMinZoom(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d378997-1fb7-41db-bde8-43367116ca9a
📒 Files selected for processing (10)
CHANGELOG.mdandroid/build.gradleandroid/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.javaexample/lib/main.dartios/flutter_pdfview.podspecios/flutter_pdfview/Package.swiftios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.mios/flutter_pdfview/Sources/flutter_pdfview/PrivacyInfo.xcprivacylib/flutter_pdfview.dartpubspec.yaml
💤 Files with no reviewable changes (1)
- example/lib/main.dart
| name: flutter_pdfview | ||
| description: A Flutter plugin that provides a PDFView widget on Android and iOS. | ||
| version: 1.4.4 | ||
| version: 1.4.5 |
There was a problem hiding this comment.
Version this as a breaking release.
This PR also raises the iOS minimum to 13.0 in ios/flutter_pdfview/Package.swift and ios/flutter_pdfview.podspec. That is a compatibility break for consumers still on iOS 12, so shipping it as 1.4.5 means ^1.4.x users can pick up a breaking change on a patch update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pubspec.yaml` at line 3, The package version must be bumped to a breaking
release because raising the iOS minimum to 13.0 in
ios/flutter_pdfview/Package.swift and ios/flutter_pdfview.podspec is not
backward-compatible; update the version field in pubspec.yaml from 1.4.5 to a
new major release (e.g., 2.0.0), update any related metadata (CHANGELOG entry
and release notes) to indicate the breaking change, and regenerate/update
lockfiles or CI artifacts as needed so consumers cannot accidentally receive
this change via a patch semver range.
- iOS: use user's minScaleFactor directly instead of fmin with fitScale - iOS: defer initial onPageChanged until layoutSubviews applies defaultPage - iOS: guard against double-fire with _hasSentInitialPage flag - Android: validate maxZoom >= minZoom with defensive clamping - Dart: propagate maxZoom/minZoom in updatesMap for widget rebuilds Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m (1)
282-289:⚠️ Potential issue | 🟠 MajorThe initial
onPageChangedcan still be emitted twice.
_hasSentInitialPageonly guards the synthetic callback inlayoutSubviews.handlePageChanged:still forwardsPDFViewPageChangedNotificationunconditionally, so first load can deliver the same page once from the notification and again from this block after_defaultPageis applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m` around lines 282 - 289, The initial onPageChanged can still fire twice because layoutSubviews' synthetic callback is guarded by _hasSentInitialPage but handlePageChanged: still forwards PDFViewPageChangedNotification unconditionally; modify handlePageChanged: to early-return when the initial synthetic event has already been sent or when _defaultPageSet is true and the notification's page matches _pdfView.currentPage to avoid duplicates, using the existing _hasSentInitialPage/_defaultPageSet flags and the same invokeChannelMethod call on _controller to only emit when appropriate; also ensure _hasSentInitialPage is set before emitting either path so both codepaths share the same guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.java`:
- Around line 120-128: The rebuild path is missing handling for maxZoom/minZoom
so applySettings() throws on runtime updates; update the applySettings(String
key, Object value) logic in FlutterPDFView to add branches for "maxZoom" and
"minZoom" (same parsing as used at creation via getFloat(params, "...")) and
call pdfView.setMaxZoom(effectiveMax) and pdfView.setMinZoom(effectiveMin)
accordingly, ensuring you coerce nulls to DEFAULT_MAX_ZOOM/DEFAULT_MIN_ZOOM and
enforce effectiveMin <= effectiveMax before calling setMinZoom/setMaxZoom; this
aligns applySettings() with PDFViewController._updateSettings() and prevents the
IllegalArgumentException for unknown settings.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m`:
- Around line 174-180: The zoom min/max are only applied at init
(_maxScaleFactor, _minScaleFactor and _pdfView.maxScaleFactor) so updates via
updateSettings are ignored; modify the view's update handler (onUpdateSettings:
or whichever method processes updated args) to read args[@"maxZoom"] and
args[@"minZoom"], validate them (fall back to defaults if <=0), assign to
_maxScaleFactor and _minScaleFactor, and set the platform view's
pdfView.maxScaleFactor and pdfView.minScaleFactor accordingly (ensure UI updates
happen on the main thread).
In `@lib/flutter_pdfview.dart`:
- Around line 46-51: Make minZoom nullable and default to null so iOS can
preserve fit-to-page: change the constructor parameter type to allow null
(minZoom = null), update the assert that currently enforces minZoom > 0 to only
assert when minZoom is non-null (e.g., assert(minZoom == null || minZoom > 0,
...)), and update the assert comparing maxZoom >= minZoom to only run when
minZoom is non-null (e.g., assert(minZoom == null || maxZoom >= minZoom, ...)).
Also update the parameter/doc comment to describe minZoom as an optional lower
zoom bound (not "fit to page"). Ensure references to _minScaleFactor/_minZoom in
platform code remain compatible with a null/non-positive value logic.
---
Duplicate comments:
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m`:
- Around line 282-289: The initial onPageChanged can still fire twice because
layoutSubviews' synthetic callback is guarded by _hasSentInitialPage but
handlePageChanged: still forwards PDFViewPageChangedNotification
unconditionally; modify handlePageChanged: to early-return when the initial
synthetic event has already been sent or when _defaultPageSet is true and the
notification's page matches _pdfView.currentPage to avoid duplicates, using the
existing _hasSentInitialPage/_defaultPageSet flags and the same
invokeChannelMethod call on _controller to only emit when appropriate; also
ensure _hasSentInitialPage is set before emitting either path so both codepaths
share the same guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 34e81114-1f73-4eb5-be55-a6aaa21c7a2f
📒 Files selected for processing (4)
CHANGELOG.mdandroid/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.javaios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.mlib/flutter_pdfview.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| Float maxZoom = getFloat(params, "maxZoom"); | ||
| Float minZoom = getFloat(params, "minZoom"); | ||
| float effectiveMax = maxZoom != null ? maxZoom : DEFAULT_MAX_ZOOM; | ||
| float effectiveMin = minZoom != null ? minZoom : DEFAULT_MIN_ZOOM; | ||
| if (effectiveMin > effectiveMax) { | ||
| effectiveMin = effectiveMax; | ||
| } | ||
| pdfView.setMaxZoom(effectiveMax); | ||
| pdfView.setMinZoom(effectiveMin); |
There was a problem hiding this comment.
Zoom updates will fail after the first rebuild on Android.
This only applies maxZoom and minZoom during creation. PDFViewController._updateSettings() now forwards those keys on rebuild, but applySettings() still has no maxZoom or minZoom branches, so the first runtime change falls into the default case and throws IllegalArgumentException("Unknown PDFView setting").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.java`
around lines 120 - 128, The rebuild path is missing handling for maxZoom/minZoom
so applySettings() throws on runtime updates; update the applySettings(String
key, Object value) logic in FlutterPDFView to add branches for "maxZoom" and
"minZoom" (same parsing as used at creation via getFloat(params, "...")) and
call pdfView.setMaxZoom(effectiveMax) and pdfView.setMinZoom(effectiveMin)
accordingly, ensuring you coerce nulls to DEFAULT_MAX_ZOOM/DEFAULT_MIN_ZOOM and
enforce effectiveMin <= effectiveMax before calling setMinZoom/setMaxZoom; this
aligns applySettings() with PDFViewController._updateSettings() and prevents the
IllegalArgumentException for unknown settings.
| _maxScaleFactor = [args[@"maxZoom"] doubleValue]; | ||
| if (_maxScaleFactor <= 0) { | ||
| _maxScaleFactor = 4.0; | ||
| } | ||
| _minScaleFactor = [args[@"minZoom"] doubleValue]; | ||
|
|
||
| _pdfView.maxScaleFactor = _maxScaleFactor; |
There was a problem hiding this comment.
Zoom bounds are still init-only on iOS.
lib/flutter_pdfview.dart now forwards maxZoom and minZoom through updateSettings(), but this code only reads them during creation. Because onUpdateSettings: still does nothing, rebuilding PDFView with new zoom limits leaves the old bounds active until the platform view is recreated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m` around lines
174 - 180, The zoom min/max are only applied at init (_maxScaleFactor,
_minScaleFactor and _pdfView.maxScaleFactor) so updates via updateSettings are
ignored; modify the view's update handler (onUpdateSettings: or whichever method
processes updated args) to read args[@"maxZoom"] and args[@"minZoom"], validate
them (fall back to defaults if <=0), assign to _maxScaleFactor and
_minScaleFactor, and set the platform view's pdfView.maxScaleFactor and
pdfView.minScaleFactor accordingly (ensure UI updates happen on the main
thread).
| this.maxZoom = 4.0, | ||
| this.minZoom = 1.0, | ||
| }) : assert(filePath != null || pdfData != null), | ||
| assert(maxZoom > 0, 'maxZoom must be greater than 0'), | ||
| assert(minZoom > 0, 'minZoom must be greater than 0'), | ||
| assert(maxZoom >= minZoom, 'maxZoom must be >= minZoom'), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Apple's PDFKit, is setting PDFView.minScaleFactor = 1.0equivalent to fit-to-page, or is fit-to-page determined byPDFView.scaleFactorForSizeToFit?
💡 Result:
No.
minScaleFactoris just a lower bound for zoom (scaleFactor). SettingpdfView.minScaleFactor = 1.0means “don’t allow zooming out below 100%,” not “fit the page.” SettingminScaleFactor/maxScaleFactoralso implicitly turns offautoScales. [1]- The “fit” zoom is computed as
scaleFactorForSizeToFit—i.e., the scale factor that would fit the current page(s) in the view / whatautoScaleswould use as its “size to fit” scale. [2][3]
So “fit-to-page” (or whatever fit behavior applies for the current display mode) corresponds to pdfView.scaleFactor = pdfView.scaleFactorForSizeToFit (or autoScales = true), not minScaleFactor = 1.0. [2][3]
Sources
[1] Stack Overflow: Swift PDF viewer zoom? (includes the PDFView doc comments about minScaleFactor/maxScaleFactor and autoScales) (stackoverflow.com)
[2] Microsoft Learn (Xamarin.iOS): PdfView.ScaleFactorForSizeToFit summary (“fit … in the view”) (learn.microsoft.com)
[3] Stack Overflow quoting Apple docs for scaleFactorForSizeToFit (“size to fit” scale factor that autoScales would use) (techqa.club)
Citations:
- 1: https://stackoverflow.com/questions/66014055/swift-pdf-viewer-zoom?utm_source=openai
- 2: https://learn.microsoft.com/en-us/dotnet/api/pdfkit.pdfview?view=xamarin-ios-sdk-12&utm_source=openai
- 3: https://www.techqa.club/v/q/pdfkit-s-scalefactorforsizetofit-isn-t-working-to-set-zoom-in-swiftui-uiviewrepresentable-72064180?utm_source=openai
🏁 Script executed:
# First, check the exact code at lines 46-51 in lib/flutter_pdfview.dart
cat -n lib/flutter_pdfview.dart | head -60 | tail -20Repository: endigo/flutter_pdfview
Length of output: 892
🏁 Script executed:
# Search for minZoom/maxZoom initialization and defaults
rg "minZoom|maxZoom" lib/flutter_pdfview.dart -A 2 -B 2Repository: endigo/flutter_pdfview
Length of output: 1478
🏁 Script executed:
# Check iOS implementation that maps minZoom to PDFView.minScaleFactor
rg "minZoom|minScaleFactor" ios/ -A 3 -B 1Repository: endigo/flutter_pdfview
Length of output: 2279
🏁 Script executed:
# Verify the _PDFViewSettings.toMap() behavior mentioned in the review
ast-grep --pattern 'toMap() { $$$ minZoom $$$ }'Repository: endigo/flutter_pdfview
Length of output: 48
🏁 Script executed:
# Check git history to see if minZoom was previously optional/nullable
git log --oneline -p lib/flutter_pdfview.dart 2>/dev/null | head -200 | grep -A5 -B5 "minZoom" | head -50Repository: endigo/flutter_pdfview
Length of output: 893
🏁 Script executed:
# Check if _PDFViewSettings defines minZoom as nullable in its class
rg "class _PDFViewSettings" lib/flutter_pdfview.dart -A 20 | grep -E "(minZoom|maxZoom|double\?)"Repository: endigo/flutter_pdfview
Length of output: 101
Make minZoom nullable to preserve fit-to-page behavior.
The minZoom: 1.0 default forces a literal lower scale bound of 1.0×, but in Apple's PDFKit, minScaleFactor is only a lower bound—fit-to-page is determined by scaleFactorForSizeToFit. This breaks PDFs where the fit scale is below 1.0 (they open zoomed in instead of fit-to-page).
The iOS code already supports this: it uses (_minScaleFactor > 0) ? _minScaleFactor : fitScale, so passing null or a non-positive value as minZoom would preserve fit-to-page. Change minZoom to nullable with default null, update the assertion to allow null, and correct the doc comment (it is not "fit to page"—it's a zoom lower bound).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/flutter_pdfview.dart` around lines 46 - 51, Make minZoom nullable and
default to null so iOS can preserve fit-to-page: change the constructor
parameter type to allow null (minZoom = null), update the assert that currently
enforces minZoom > 0 to only assert when minZoom is non-null (e.g.,
assert(minZoom == null || minZoom > 0, ...)), and update the assert comparing
maxZoom >= minZoom to only run when minZoom is non-null (e.g., assert(minZoom ==
null || maxZoom >= minZoom, ...)). Also update the parameter/doc comment to
describe minZoom as an optional lower zoom bound (not "fit to page"). Ensure
references to _minScaleFactor/_minZoom in platform code remain compatible with a
null/non-positive value logic.
1 participant
Summary
onErrorasynchronously so Dart handler is ready (iOS onError is never called #211)onPageChangedafter initial render completes (onPageChanged is not triggered first time in iOS. #66)maxScaleFactorinlayoutSubviews([iOS] landscape PDF wrong initial zoom #247)maxZoomandminZoomparameters toPDFViewwidget (Feature equest: Configure Zoom Level #296)nightModeBackgroundColorparameterTest plan
flutter analyzereports no errorsonErrorfires for invalid file pathsonPageChangedtriggers on first loadmaxZoom/minZoomparameters work on both platforms🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Maintenance
Other