Conversation
…setpagedevice from EPS output
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSelects PS vs EPS templates at runtime when generating PostScript/EPS output, adds EPS-specific header/functions/footer constants, updates AppendFormat arguments (including viewBox width and points-per-module), and normalizes approved PostScript/EPS test fixtures’ whitespace and DSC blocks. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@QRCoder/PostscriptQRCode.cs`:
- Around line 113-115: The header currently mixes viewBox.Width and
pointsPerModule causing mismatched geometry when input is non-square; in the
sb.AppendFormat call that formats header (the line using
sb.AppendFormat(CultureInfo.InvariantCulture, header, [...]) in
PostscriptQRCode.cs), replace the first argument CleanSvgVal(viewBox.Width) with
CleanSvgVal(pointsPerModule) so both /sz and %%BoundingBox use the square side
length (pointsPerModule) consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 621116e7-c479-4dca-bf0d-d26d995304ed
📒 Files selected for processing (6)
QRCoder/PostscriptQRCode.csQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txtQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txtQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txtQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txtQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size_no_quiet_zones.approved.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@QRCoder/PostscriptQRCode.cs`:
- Around line 228-256: The EPS prolog currently does "5 dict begin" but defines
seven symbols (csquare, f, b, background, nl, sz, sc), causing dictfull on Level
1 interpreters; change the dictionary capacity to at least 7 (e.g., replace "5
dict begin" with "7 dict begin" or otherwise allocate a dict large enough before
defining csquare, f, b, background, nl and later sz/sc) so all defs (csquare, f,
b, background, nl, sz, sc) can be stored in the same dictionary without raising
dictfull.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68a9fb5c-646d-4d97-8513-91d5d381f933
📒 Files selected for processing (2)
QRCoder/PostscriptQRCode.csQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
QRCoder/PostscriptQRCode.cs (1)
117-122: Verify format string placeholders align with argument count.The
functionstemplate now receives 9 arguments (indices 0-8), butPS_FUNCTIONSonly uses placeholders{0}through{6}. The extra arguments{7}and{8}(viewBox.Width,pointsPerModule) are only consumed byEPS_FUNCTIONSfor/szand/sc. While this works correctly (unused placeholders are simply ignored), it creates a subtle coupling between the two templates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@QRCoder/PostscriptQRCode.cs` around lines 117 - 122, The AppendFormat call passes 9 arguments for the variable `functions` even though `PS_FUNCTIONS` only expects placeholders {0}..{6}, creating a hidden coupling with `EPS_FUNCTIONS`; fix by ensuring `functions` is formatted with exactly the arguments it needs—either build separate argument lists for `PS_FUNCTIONS` and `EPS_FUNCTIONS` or conditionally call sb.AppendFormat with the correct subset—adjust the call site using `sb.AppendFormat` and the `functions` variable (and related values CleanSvgVal(...), drawableModulesCount, viewBox.Width, pointsPerModule) so placeholders and arguments align exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt`:
- Around line 8-9: The test fixture contains "5 dict begin" but the PostScript
output defines seven names (csquare, f, b, background, nl, sz, sc); update the
fixture to match the corrected dictionary capacity by replacing "5 dict begin"
with "7 dict begin" (or regenerate the fixture from PostscriptQRCode.cs after
you apply the dictionary-size fix) so the dictionary can hold all seven
definitions.
---
Nitpick comments:
In `@QRCoder/PostscriptQRCode.cs`:
- Around line 117-122: The AppendFormat call passes 9 arguments for the variable
`functions` even though `PS_FUNCTIONS` only expects placeholders {0}..{6},
creating a hidden coupling with `EPS_FUNCTIONS`; fix by ensuring `functions` is
formatted with exactly the arguments it needs—either build separate argument
lists for `PS_FUNCTIONS` and `EPS_FUNCTIONS` or conditionally call
sb.AppendFormat with the correct subset—adjust the call site using
`sb.AppendFormat` and the `functions` variable (and related values
CleanSvgVal(...), drawableModulesCount, viewBox.Width, pointsPerModule) so
placeholders and arguments align exactly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b16c5df5-89e5-4959-b7f8-fc93ca9e8e63
📒 Files selected for processing (2)
QRCoder/PostscriptQRCode.csQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
QRCoder/PostscriptQRCode.cs (1)
98-136: Consider adding test coverage forepsFormat=truewithdrawQuietZones=false.The existing tests cover:
epsFormat=true+drawQuietZones=true(default)epsFormat=false+drawQuietZones=falseThe combination
epsFormat=true+drawQuietZones=falseis untested. This would exercise the EPS-specificgsave/0 0 movetologic with the non-zerooffsetpath, verifying coordinate handling in constrained spaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@QRCoder/PostscriptQRCode.cs` around lines 98 - 136, Add a unit test that calls GetGraphic with epsFormat=true and drawQuietZones=false to exercise the EPS-specific branch; specifically construct a QrCodeData instance (or use the existing test helper) and invoke GetGraphic(viewBox, darkColor, lightColor, drawQuietZones: false, epsFormat: true) and assert the returned string contains the EPS header/functions patterns (e.g., EPS_HEADER/EPS_FUNCTIONS markers such as "gsave" and "0 0 moveto") and the expected module output for the reduced drawableModulesCount (verify presence of "f "/ "b " sequences and that no extra quiet-zone modules are rendered), ensuring the same helpers/assertions used in the other tests are reused for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@QRCoder/PostscriptQRCode.cs`:
- Around line 98-136: Add a unit test that calls GetGraphic with epsFormat=true
and drawQuietZones=false to exercise the EPS-specific branch; specifically
construct a QrCodeData instance (or use the existing test helper) and invoke
GetGraphic(viewBox, darkColor, lightColor, drawQuietZones: false, epsFormat:
true) and assert the returned string contains the EPS header/functions patterns
(e.g., EPS_HEADER/EPS_FUNCTIONS markers such as "gsave" and "0 0 moveto") and
the expected module output for the reduced drawableModulesCount (verify presence
of "f "/ "b " sequences and that no extra quiet-zone modules are rendered),
ensuring the same helpers/assertions used in the other tests are reused for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65c0f5a7-d4f8-41b1-9e9e-7b0c8be5e458
📒 Files selected for processing (2)
QRCoder/PostscriptQRCode.csQRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt
|
Final review from ChatGPT regarding the created EPS file:
|
1 participant
Fix EPS protocol violations in
PostscriptQRCodeProblem
When
epsFormat = true, the generated output violated the EPS specification (Adobe Technical Note #5002) in multiple ways. Previously only the magic comment on line 1 changed between PS and EPS modes; all other content — including forbidden operators and non-standard structure — was shared.Changes
Hard violations fixed
showpagein EPS — forbidden; causes host document to eject a page when the EPS is placedsetpagedevicein EPS — forbidden device-control operator; EPS must be device-independent%%BeginFeature/%%EndFeatureblock)0 0 movetooutsidegsave— leaks current-point state into the surrounding documentgsavein EPS outputdictfullerrors when embedded7 dict begin/endto isolate all 7 definitions (csquare,f,b,background,nl,sz,sc) in a private dictionaryInappropriate DSC comments removed from EPS
%%DocumentMedia%%Pages/%%Page%%OriginNon-standard DSC structure replaced in EPS
%%BeginConstants/%%EndConstants%%BeginSetup/%%EndSetup%%BeginFunctions/%%EndFunctions%%BeginProlog/%%EndProlog(standard DSC)%%BeginBody/%%EndBodyMinor PS fixes
%%LanguageLevel: 2,%%BeginFunctions, andshowpagein the PS templates%!PS-Adobe-3.0magic commentImplementation
A separate
EPS_HEADER,EPS_FUNCTIONS, andEPS_FOOTERconstant set was introduced. The PS path (PS_HEADER,PS_FUNCTIONS,PS_FOOTER) is unchanged in behaviour.GetGraphic()selects the correct set based onepsFormat.The resulting EPS structure is:
Summary by CodeRabbit
New Features
Bug Fixes