Render by PDFKit on iOS #14
Conversation
WalkthroughThis pull request introduces two main changes. A new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as SDImagePDFCoder
participant P as PDFKit
C->>S: createVectorPDFWithData(data, pageNumber)
S->>P: Initialize PDFDocument with data
P-->>S: Return PDFDocument
S->>S: Validate document & check page bounds
alt Valid Page
S->>P: Retrieve PDFPage for pageNumber
P-->>S: Return PDFPage
S->>S: Apply drawing transformation and render page
S->>C: Return rendered image
else Invalid Page
S->>C: Return error/nil
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
SDWebImagePDFCoder/Classes/SDImagePDFCoder.m (4)
11-11: Ensure PDFKit availability on targeted platforms.
Make sure the deployment target for this project includes iOS 11+ (or the corresponding OS version that supports PDFKit). If older platforms must be supported, consider falling back to Core Graphics when PDFKit is unavailable.
155-157: Consider enhanced error handling or logging.
WhenPDFDocumentinitialization fails, returningnilmight obscure the reason. Adding a warning log or custom error could help troubleshoot invalid PDF data.
160-163: Provide fallback or clear feedback on out-of-rangepageNumber.
Currently, ifpageNumberexceedspageCount, the method returnsnil. You could throw an exception, revert to the last page, or log a warning to let callers know they requested an invalid page index.
168-176: Evaluate bridging overhead and consistency.
You retrievedocumentRefandpageReffromPDFDocumentandPDFPage, which already provide high-level methods. If performance is critical, consider measuring the overhead of bridging to Core Graphics and see if sticking to PDFKit’s higher-level APIs (like[page drawWithBox:toContext:]) is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
SDWebImagePDFCoder.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist(1 hunks)SDWebImagePDFCoder/Classes/SDImagePDFCoder.m(2 hunks)
🔇 Additional comments (2)
SDWebImagePDFCoder/Classes/SDImagePDFCoder.m (1)
178-181:❓ Verification inconclusive
Confirm the correct PDF bounding box.
UsingkCGPDFMediaBoxhas broad coverage, but certain documents usekCGPDFCropBoxor other boxes for visible content. Verify thatkCGPDFMediaBoxis always the intended display area.
🏁 Script executed:
#!/bin/bash # Search the codebase for references to PDF bounding boxes to verify usage. rg -A 3 "kCGPDF\w+Box"Length of output: 722
Action Required: Confirm the intended display box
It appears that the code consistently uses
kCGPDFMediaBox(as shown by the grep results) without any fallback or alternative for documents that might use another box (e.g.,kCGPDFCropBox). Please verify that usingkCGPDFMediaBoxis indeed the intended display area for all supported PDFs. If there’s a chance that some documents rely on a different box for their visible content, consider whether additional handling or tests are needed.SDWebImagePDFCoder.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist (1)
1-8: Plist addition acknowledged.
The newIDEWorkspaceChecks.plistwithIDEDidComputeMac32BitWarningset totrueis standard for Xcode workspace checks. No issues found here.
| SDGraphicsBeginImageContextWithOptions(targetRect.size, NO, 0); | ||
| CGContextRef context = SDGraphicsGetCurrentContext(); | ||
|
|
||
| #if SD_UIKIT || SD_WATCH | ||
| // Core Graphics coordinate system use the bottom-left, UIKit use the flipped one | ||
| CGContextTranslateCTM(context, 0, rect.size.height); | ||
| CGContextScaleCTM(context, 1, -1); | ||
| #endif | ||
|
|
||
| CGContextConcatCTM(context, transform); | ||
| [page drawWithBox:kPDFDisplayBoxMediaBox toContext:context]; | ||
|
|
||
| image = SDGraphicsGetImageFromCurrentImageContext(); | ||
| SDGraphicsEndImageContext(); | ||
|
|
There was a problem hiding this comment.
Investigate the undefined targetRect.
SDGraphicsBeginImageContextWithOptions(targetRect.size, NO, 0); relies on targetRect, which is never declared or assigned before use. This likely causes undefined behavior or a compilation error.
Apply this diff to correct usage by substituting rect:
- SDGraphicsBeginImageContextWithOptions(targetRect.size, NO, 0);
+ SDGraphicsBeginImageContextWithOptions(rect.size, NO, 0);📝 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.
| SDGraphicsBeginImageContextWithOptions(targetRect.size, NO, 0); | |
| CGContextRef context = SDGraphicsGetCurrentContext(); | |
| #if SD_UIKIT || SD_WATCH | |
| // Core Graphics coordinate system use the bottom-left, UIKit use the flipped one | |
| CGContextTranslateCTM(context, 0, rect.size.height); | |
| CGContextScaleCTM(context, 1, -1); | |
| #endif | |
| CGContextConcatCTM(context, transform); | |
| [page drawWithBox:kPDFDisplayBoxMediaBox toContext:context]; | |
| image = SDGraphicsGetImageFromCurrentImageContext(); | |
| SDGraphicsEndImageContext(); | |
| SDGraphicsBeginImageContextWithOptions(rect.size, NO, 0); | |
| CGContextRef context = SDGraphicsGetCurrentContext(); | |
| #if SD_UIKIT || SD_WATCH | |
| // Core Graphics coordinate system use the bottom-left, UIKit use the flipped one | |
| CGContextTranslateCTM(context, 0, rect.size.height); | |
| CGContextScaleCTM(context, 1, -1); | |
| #endif | |
| CGContextConcatCTM(context, transform); | |
| [page drawWithBox:kPDFDisplayBoxMediaBox toContext:context]; | |
| image = SDGraphicsGetImageFromCurrentImageContext(); | |
| SDGraphicsEndImageContext(); |
|
It's welcomed to see your PR! This lib should support iOS 9+ (at least currently). But PDFKit needs iOS 11+ In the past 3 years, I already have a try for this and has a custom branch. If you can provide a fallback to use PDFKit on iOS 11+, and use CoreGraphics CGPDFContext on iOS 9, it's OK to merge now. Or, maybe we still need to wait for some day when I decided to drop iOS 9 support. |
Summary by CodeRabbit
Refactor
Chore
This update contributes to a smoother, more stable experience when viewing PDFs.