feat:Add spacing parameter to PDF viewer for customizable page spacing#335
feat:Add spacing parameter to PDF viewer for customizable page spacing#335ma-mobile wants to merge 1 commit intoendigo:masterfrom
Conversation
WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
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: 3
🧹 Nitpick comments (1)
test/flutter_pdfview_test.dart (1)
170-177: Assert the platform config, not just the Dart field.This only proves the constructor stored
spacingon the widget. It would still pass ifspacingwere dropped before the platform view is created. Please add a widget test that pumpsPDFViewand verifies the mocked platform-view/method-channel payload includesspacing: 15.Based on learnings: Add tests for new features in 'test/flutter_pdfview_test.dart' including widget creation, configuration, error handling, and mock method channels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/flutter_pdfview_test.dart` around lines 170 - 177, The test currently only asserts the Dart widget fields (customPdfView.enableSwipe, swipeHorizontal, pageFling, pageSnap, spacing) but must assert the platform-view/method-channel payload; update the test in flutter_pdfview_test.dart to pump a PDFView widget (PDFView) into the widget tester, ensure the mock platform view/method-channel is registered/attached, then capture the platform creation payload or the method-channel call used to create/configure the native view and assert it includes "spacing": 15; reference the PDFView widget and the mock method channel or platform view registry used in this test to locate where to intercept and assert the payload.
🤖 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 159-160: The inter-page spacing isn't visible because
pageBreakMargins on _pdfView only works when _pdfView.displaysPageBreaks is
enabled and horizontal spacing requires left/right insets; update the
initialization for _pdfView to set _pdfView.displaysPageBreaks = YES and compute
pageBreakMargins based on displayDirection (use left/right insets of _spacing
when displayDirection == kPDFDisplayDirectionHorizontal, otherwise use
top/bottom insets) so pageBreakMargins is effective for both horizontal and
vertical paging.
In `@lib/flutter_pdfview.dart`:
- Around line 254-255: The PDFView spacing prop is currently create-only: update
the Dart side to include spacing in updatesMap() so changes to the
PDFView(spacing: ...) prop are emitted after creation, and extend the native
updateSettings handling code paths (the platform view update/updateSettings
handlers) to accept and apply spacing updates so rebuilding the widget actually
updates the native view; reference the spacing constructor param, the
updatesMap() method, and the native updateSettings handlers when making the
changes.
- Line 35: Reject negative spacing at the API boundary by validating the public
spacing value before it reaches native renderers: in the constructor where
spacing is assigned (the spacing parameter / field) add an assert or clamp
(e.g., spacing = spacing < 0 ? 0 : spacing) and likewise guard any other public
setter or alternate constructor usage referenced near the second occurrence (the
code around the other spacing assignment at lines 103-104) to ensure spacing is
non-negative; update any places that forward spacing to native calls (renderers)
to use the validated/clamped value and include an assert to catch negative
inputs in debug builds.
---
Nitpick comments:
In `@test/flutter_pdfview_test.dart`:
- Around line 170-177: The test currently only asserts the Dart widget fields
(customPdfView.enableSwipe, swipeHorizontal, pageFling, pageSnap, spacing) but
must assert the platform-view/method-channel payload; update the test in
flutter_pdfview_test.dart to pump a PDFView widget (PDFView) into the widget
tester, ensure the mock platform view/method-channel is registered/attached,
then capture the platform creation payload or the method-channel call used to
create/configure the native view and assert it includes "spacing": 15; reference
the PDFView widget and the mock method channel or platform view registry used in
this test to locate where to intercept and assert the payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 555cf2d1-bc71-42cb-98e2-cc8a30340038
📒 Files selected for processing (7)
README.mdandroid/src/main/java/io/endigo/plugins/pdfviewflutter/FlutterPDFView.javaexample/lib/main.dartexample/pubspec.yamlios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.mlib/flutter_pdfview.darttest/flutter_pdfview_test.dart
| if (@available(iOS 11.0, *)) { | ||
| _pdfView.pageBreakMargins = UIEdgeInsetsMake(_spacing, 0, _spacing, 0); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Apple's PDFKit, how does PDFView.pageBreakMargins affect inter-page spacing when displayDirection is horizontal? Do left/right insets control the visible gap in horizontal paging modes?
💡 Result:
PDFView.pageBreakMargins is only used for the page-break “spacing” layout when displaysPageBreaks == true: it defines extra inset around each page’s frame (top/left/bottom/right), and PDFKit uses that to create the visible separation between adjacent pages. By default, when page breaks are shown, the margins are non‑zero; if page breaks are off, the margins are effectively zero. Negative values are clamped to 0. [1][2]
So when displayDirection == .horizontal in a scroll/continuous layout, the visible gap between pages horizontally is produced by the trailing margin of the left page (right) plus the leading margin of the next page (left) (top/bottom mainly affect vertical padding around the page, not the left↔right inter-page gap). [1]
In contrast, in “paging” setups that aren’t using PDFKit’s page-break spacing (commonly when you enable usePageViewController(true)), pageBreakMargins typically won’t control the inter-page gap because pages aren’t laid out in the same continuous “page breaks” container. (There’s no public API in PDFKit to set UIPageViewController’s internal inter-page spacing.) [1]
Sources:
[1] ComPDFKit CPDFView docs describing displaysPageBreaks and pageBreakMargins semantics (mirrors Apple PDFKit behavior) (developers.compdf.com)
[2] Monkeybread PDFViewMBS.pageBreakMargins notes on defaults + clamping (monkeybreadsoftware.net)
Citations:
- 1: https://developers.compdf.com/guides/pdf-sdk/ios/api-reference/Classes/CPDFView.html
- 2: https://www.monkeybreadsoftware.net/pdfkit-pdfviewmbs-property.shtml?utm_source=openai
🏁 Script executed:
# Search for displaysPageBreaks and usePageViewController in iOS code
rg -t objc -t swift "displaysPageBreaks|usePageViewController" ios/Repository: endigo/flutter_pdfview
Length of output: 333
🏁 Script executed:
# Read the full FlutterPDFView.m to understand initialization and context
wc -l ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.mRepository: endigo/flutter_pdfview
Length of output: 130
🏁 Script executed:
# Look for where _pdfView is initialized and configured
rg -t objc "PDFView|_pdfView\s*=" ios/flutter_pdfview/Sources/flutter_pdfview/ -A 3Repository: endigo/flutter_pdfview
Length of output: 7515
🏁 Script executed:
# Read context around usePageViewController calls - find line numbers first
rg -t objc "usePageViewController" ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m -n -B 5 -A 5Repository: endigo/flutter_pdfview
Length of output: 793
🏁 Script executed:
# Search for pageFling variable and displayDirection to understand the full layout logic
rg -t objc "pageFling|displayDirection" ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.m -n -B 2 -A 2Repository: endigo/flutter_pdfview
Length of output: 1270
🏁 Script executed:
# Read the full initialization section (roughly lines 80-160) to understand context
sed -n '80,160p' ios/flutter_pdfview/Sources/flutter_pdfview/FlutterPDFView.mRepository: endigo/flutter_pdfview
Length of output: 2745
🏁 Script executed:
# Check if displaysPageBreaks is ever set to true
rg -t objc "displaysPageBreaks" ios/Repository: endigo/flutter_pdfview
Length of output: 48
Inter-page spacing will not be visible without enabling page breaks on iOS.
pageBreakMargins only affects spacing when displaysPageBreaks is true. The current code never enables page breaks, so the spacing parameter has no effect regardless of inset direction. Even with the conditional inset fix for horizontal/vertical paging, the feature won't work until _pdfView.displaysPageBreaks = YES is set during initialization. Additionally, when using horizontal paging (displayDirection = kPDFDisplayDirectionHorizontal), the visible inter-page gap is controlled by left/right insets, not top/bottom—so UIEdgeInsetsMake(_spacing, 0, _spacing, 0) produces no horizontal spacing.
Possible fix
_pdfView.autoScales = _autoSpacing;
+ if (`@available`(iOS 11.0, *)) {
+ _pdfView.displaysPageBreaks = YES;
+ }
+
if (`@available`(iOS 11.0, *)) {
- _pdfView.pageBreakMargins = UIEdgeInsetsMake(_spacing, 0, _spacing, 0);
+ _pdfView.pageBreakMargins = swipeHorizontal
+ ? UIEdgeInsetsMake(0, _spacing, 0, _spacing)
+ : UIEdgeInsetsMake(_spacing, 0, _spacing, 0);
}🤖 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
159 - 160, The inter-page spacing isn't visible because pageBreakMargins on
_pdfView only works when _pdfView.displaysPageBreaks is enabled and horizontal
spacing requires left/right insets; update the initialization for _pdfView to
set _pdfView.displaysPageBreaks = YES and compute pageBreakMargins based on
displayDirection (use left/right insets of _spacing when displayDirection ==
kPDFDisplayDirectionHorizontal, otherwise use top/bottom insets) so
pageBreakMargins is effective for both horizontal and vertical paging.
| this.password, | ||
| this.nightMode = false, | ||
| this.autoSpacing = true, | ||
| this.spacing = 0, |
There was a problem hiding this comment.
Reject negative spacing at the API boundary.
spacing is public now, but nothing prevents callers from passing a negative value. Add an assert or clamp here before that value reaches the native renderers.
Small guard
- }) : assert(filePath != null || pdfData != null),
+ }) : assert(filePath != null || pdfData != null),
+ assert(spacing >= 0),
super(key: key);Also applies to: 103-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/flutter_pdfview.dart` at line 35, Reject negative spacing at the API
boundary by validating the public spacing value before it reaches native
renderers: in the constructor where spacing is assigned (the spacing parameter /
field) add an assert or clamp (e.g., spacing = spacing < 0 ? 0 : spacing) and
likewise guard any other public setter or alternate constructor usage referenced
near the second occurrence (the code around the other spacing assignment at
lines 103-104) to ensure spacing is non-negative; update any places that forward
spacing to native calls (renderers) to use the validated/clamped value and
include an assert to catch negative inputs in debug builds.
| this.spacing, | ||
| this.pageFling, |
There was a problem hiding this comment.
spacing is create-only right now.
These additions carry spacing into the initial creation params, but updatesMap() still never emits spacing changes, and the native updateSettings paths in this PR don't handle it either. Rebuilding PDFView(spacing: ...) with a new value will silently do nothing after the platform view exists.
Also applies to: 274-275, 313-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/flutter_pdfview.dart` around lines 254 - 255, The PDFView spacing prop is
currently create-only: update the Dart side to include spacing in updatesMap()
so changes to the PDFView(spacing: ...) prop are emitted after creation, and
extend the native updateSettings handling code paths (the platform view
update/updateSettings handlers) to accept and apply spacing updates so
rebuilding the widget actually updates the native view; reference the spacing
constructor param, the updatesMap() method, and the native updateSettings
handlers when making the changes.
2 participants
1 - Add custom spacing option to PDFView
2 - Added
spacingparameter to PDFView widget (defaults to 0)3 - Implemented
spacingon Android usingcom.github.barteksc.pdfviewer4 - Implemented
spacingon iOS using PDFKitpageBreakMargins5 - Updated README and example app to demonstrate the new feature
6 - Added unit test for spacing parameter
Summary by CodeRabbit
New Features
Documentation
Chores
Tests