Change PdfByteQRCode to draw rectangles instead of embed JPGs #635
Conversation
📝 WalkthroughWalkthroughImplements in-memory PDF construction for PdfByteQRCode by drawing QR modules as PDF path commands, adds internal helpers for path/color/formatting, removes the jpgQuality parameter from a GetGraphic overload, removes Windows-only platform attributes, and adds unit tests for PDF QR rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Helper as PdfByteQRCodeHelper
participant Renderer as PdfByteQRCode
participant PDF as In-memory PDF Builder
Caller->>Helper: GetQRCode(plainText, pixelsPerModule, colors?, ecc...)
Helper->>Renderer: new PdfByteQRCode(qrCodeData)
Helper->>Renderer: GetGraphic(pixelsPerModule, darkHex, lightHex, dpi)
Renderer->>PDF: Compute layout, transform, background rect
Renderer->>PDF: CreatePathFromModules -> emit path commands
Renderer->>PDF: Set colors (ColorToPdfRgb) and fill (even-odd)
PDF-->>Renderer: byte[] PDF bytes
Renderer-->>Helper: byte[] PDF bytes
Helper-->>Caller: byte[] PDF bytes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Shane32
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
QRCoder/PdfByteQRCode.cs (2)
62-131: Handle #RGB hex inputs without throwing.
HexColorToByteArraycollapses shorthand HTML colors such as#FFFdown to a single byte, soColorToPdfRgbimmediately throws because it insists on exactly three components. The prior System.Drawing-based pipeline accepted CSS shorthand viaColorTranslator.FromHtml, so this regresses existing callers. Please normalize 3-digit inputs (and reject other unsupported lengths) before converting to bytes.if (colorString.StartsWith("#")) colorString = colorString.Substring(1); - byte[] byteColor = new byte[colorString.Length / 2]; + + if (colorString.Length == 3) + { + colorString = new string(new[] + { + colorString[0], colorString[0], + colorString[1], colorString[1], + colorString[2], colorString[2] + }); + } + else if (colorString.Length != 6) + { + throw new ArgumentException("HTML hex colors must contain exactly 3 or 6 digits.", nameof(colorString)); + } + + byte[] byteColor = new byte[colorString.Length / 2]; for (int i = 0; i < byteColor.Length; i++) byteColor[i] = byte.Parse(colorString.Substring(i * 2, 2), NumberStyles.HexNumber, CultureInfo.InvariantCulture); return byteColor;
15-18: Drop the Windows-only annotation.The renderer no longer touches System.Drawing, yet the new build still tags
PdfByteQRCode(and its helper) with[SupportedOSPlatform("windows")]. That keeps CA1416 warnings alive—and many callers treat those warnings as errors—so the advertised cross-platform gain never lands. Please remove the attribute blocks so the API is available without Windows gating and update the snapshots accordingly.-#if NET6_0_OR_GREATER -[System.Runtime.Versioning.SupportedOSPlatform("windows")] -#endif // ReSharper disable once InconsistentNaming public class PdfByteQRCode : AbstractQRCode, IDisposable @@ -#if NET6_0_OR_GREATER -[System.Runtime.Versioning.SupportedOSPlatform("windows")] -#endif /// <summary> /// Provides static methods for creating PDF byte array QR codes.Also applies to: 305-307
🧹 Nitpick comments (1)
QRCoderTests/PdfByteQRCodeRendererTests.cs (1)
14-17: Dispose QR resources in the tests.
QRCodeGenerator,QRCodeData, andPdfByteQRCodeare disposable; leaving them undisposed in tight test loops leaks handles and can taint later assertions. Please wrap each inusing var(or helper) in the test bodies.- var gen = new QRCodeGenerator(); - var data = gen.CreateQrCode("This is a quick test! 123#?", QRCodeGenerator.ECCLevel.L); - var pdfCodeGfx = new PdfByteQRCode(data).GetGraphic(5); + using var gen = new QRCodeGenerator(); + using var data = gen.CreateQrCode("This is a quick test! 123#?", QRCodeGenerator.ECCLevel.L); + using var renderer = new PdfByteQRCode(data); + var pdfCodeGfx = renderer.GetGraphic(5);Also applies to: 26-27
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
QRCoderTests/PdfByteQRCodeRendererTests.can_render_pdfbyte_qrcode_blackwhite.approved.pdfis excluded by!**/*.pdfQRCoderTests/PdfByteQRCodeRendererTests.can_render_pdfbyte_qrcode_color.approved.pdfis excluded by!**/*.pdfQRCoderTests/PdfByteQRCodeRendererTests.can_render_pdfbyte_qrcode_custom_dpi.approved.pdfis excluded by!**/*.pdfQRCoderTests/PdfByteQRCodeRendererTests.can_render_pdfbyte_qrcode_from_helper.approved.pdfis excluded by!**/*.pdfQRCoderTests/PdfByteQRCodeRendererTests.can_render_pdfbyte_qrcode_from_helper_2.approved.pdfis excluded by!**/*.pdf
📒 Files selected for processing (6)
QRCoder/PdfByteQRCode.cs(2 hunks)QRCoderApiTests/net35+net40+net50+net50-windows+netstandard20+netstandard21/QRCoder.approved.txt(1 hunks)QRCoderApiTests/net60-windows/QRCoder.approved.txt(1 hunks)QRCoderApiTests/net60/QRCoder.approved.txt(1 hunks)QRCoderApiTests/netstandard13/QRCoder.approved.txt(1 hunks)QRCoderTests/PdfByteQRCodeRendererTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
QRCoder/PdfByteQRCode.cs (2)
QRCoder/PngByteQRCode.cs (2)
GetGraphic(37-49)GetGraphic(60-61)QRCoder/BitmapByteQRCode.cs (1)
HexColorToByteArray(143-151)
QRCoderTests/PdfByteQRCodeRendererTests.cs (1)
QRCoder/PdfByteQRCode.cs (8)
PdfByteQRCode(18-302)PdfByteQRCode(41-41)PdfByteQRCode(47-47)GetGraphic(54-55)GetGraphic(141-274)PdfByteQRCodeHelper(310-351)GetQRCode(325-334)GetQRCode(343-350)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: additional-tests
|
For better readability of the GitHub diff, please enable 'hide whitespace'. |
gfoidl
left a comment
gfoidl
left a comment
There was a problem hiding this comment.
Really nice 👍🏻
And thanks for all the comments, to make it easier to understand and for better future maintenance.
Co-authored-by: Günther Foidl <gue@korporal.at>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoder/PdfByteQRCode.cs (1)
60-60: Verify the XML doc comment phrasing.The comment states "obsolete, no longer used" but the parameter has not been marked with
[Obsolete]attribute. Consider rephrasing to clarify that the parameter is retained for backward compatibility but is not used internally.Suggested phrasing:
- /// <param name="jpgQuality">The JPEG quality parameter (obsolete, no longer used).</param> + /// <param name="jpgQuality">This parameter is no longer used but is retained for backward compatibility.</param>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
QRCoder/PdfByteQRCode.cs(1 hunks)QRCoderApiTests/net60-windows/QRCoder.approved.txt(0 hunks)QRCoderApiTests/net60/QRCoder.approved.txt(1 hunks)QRCoderApiTests/netstandard13/QRCoder.approved.txt(1 hunks)QRCoderTests/PdfByteQRCodeRendererTests.cs(1 hunks)
💤 Files with no reviewable changes (1)
- QRCoderApiTests/net60-windows/QRCoder.approved.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- QRCoderApiTests/netstandard13/QRCoder.approved.txt
- QRCoderTests/PdfByteQRCodeRendererTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/PdfByteQRCode.cs (4)
QRCoder/PngByteQRCode.cs (3)
GetGraphic(37-49)GetGraphic(60-61)Dispose(176-180)QRCoder/BitmapByteQRCode.cs (1)
HexColorToByteArray(143-151)QRCoder/QRCodeData.cs (1)
Dispose(226-231)QRCoder/AbstractQRCode.cs (1)
Dispose(42-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: additional-tests
- GitHub Check: build
🔇 Additional comments (10)
QRCoder/PdfByteQRCode.cs (10)
64-73: LGTM!The dimension calculations and color parsing logic are correct. The conversion from DPI to PDF points (72 DPI standard) is handled appropriately.
75-96: LGTM!The platform-conditional StreamWriter initialization correctly handles the differences between framework versions. The NETFRAMEWORK target doesn't have the
leaveOpenparameter, so the writer is not explicitly disposed (lines 186-188), which is appropriate since closing it would also close the underlying stream before we can extract the bytes.
99-126: LGTM!The PDF catalog and pages structure is correctly formed according to PDF specification. The inline page dictionary is a valid approach and the extensive comments are helpful for understanding the PDF structure.
128-140: LGTM!The content stream construction is correct. The transformation matrix properly handles the Y-axis flip (PDF's origin is bottom-left, while the module matrix origin is top-left), and the drawing sequence (background fill, then dark modules fill) produces the correct result.
141-151: LGTM!The content stream object is correctly structured with the required length dictionary entry and proper stream markers.
153-180: LGTM!The cross-reference table and trailer are correctly formatted according to PDF specification. The checked cast on line 154 appropriately guards against overflow, and the xref entries use the required 10-digit zero-padded format.
184-191: LGTM!The cleanup logic correctly handles the platform-specific StreamWriter disposal, and the stream remains accessible for the final
ToArray()call.
199-219: LGTM!The path creation logic correctly generates rectangle commands for each dark module. The implementation is straightforward and correct.
221-237: LGTM!The color conversion correctly normalizes RGB byte values to the PDF color space (0.0-1.0 range). The implementation uses the multiplication optimization suggested in the past review comment, which improves performance compared to division. The input validation ensures the color array has exactly 3 bytes.
Based on learnings
239-251: LGTM!The
ToStroverloads correctly useInvariantCulturefor consistent PDF formatting across different locales. The float format pattern"0.######"provides appropriate precision while avoiding unnecessary trailing zeros.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
QRCoder/PdfByteQRCode.cs (2)
39-47: Consider extracting hex color parsing to a common utility.This method is duplicated in
BitmapByteQRCode.cs(lines 141-149). Extracting this logic to a shared helper class would improve maintainability and reduce code duplication.Example:
+// In a shared utility class (e.g., ColorHelper.cs) +internal static class ColorHelper +{ + internal static byte[] HexColorToByteArray(string colorString) + { + if (colorString.StartsWith("#")) + colorString = colorString.Substring(1); + byte[] byteColor = new byte[colorString.Length / 2]; + for (int i = 0; i < byteColor.Length; i++) + byteColor[i] = byte.Parse(colorString.Substring(i * 2, 2), NumberStyles.HexNumber, CultureInfo.InvariantCulture); + return byteColor; + } +}Then use
ColorHelper.HexColorToByteArray()in both classes.
150-163: Consider usinglongfor xref positions.The checked casts from
longtointon lines 150 and 163 guard against overflow but will throw if the PDF exceeds ~2GB. While unlikely for QR codes, usinglongconsistently would be more robust.Apply this diff:
writer.Flush(); - var startxref = checked((int)stream.Position); + var startxref = stream.Position; // Cross-reference table - maps object numbers to byte offsets writer.Write( "xref\r\n" + // Cross-reference table keyword "0 " + ToStr(xrefs.Count + 1) + "\r\n" + // First object number (0) and count of entries "0000000000 65535 f\r\n" // Entry 0: always free, generation 65535, 'f' = free ); // Write byte offset for each object foreach (var refValue in xrefs) { // Write each entry as a 10-digit zero-padded byte offset, 5-digit zero-padded generation number (0), and 'n' = in use - writer.Write(checked((int)refValue).ToString("0000000000", CultureInfo.InvariantCulture) + " 00000 n\r\n"); + writer.Write(refValue.ToString("0000000000", CultureInfo.InvariantCulture) + " 00000 n\r\n"); }Note: The PDF format itself uses 10-digit fields for offsets, which can accommodate values up to 9,999,999,999 bytes (~9.3GB), so
longis appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/PdfByteQRCode.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/PdfByteQRCode.cs (2)
QRCoder/PngByteQRCode.cs (3)
GetGraphic(35-47)GetGraphic(58-59)Dispose(174-178)QRCoder/BitmapByteQRCode.cs (1)
HexColorToByteArray(142-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: additional-tests
- GitHub Check: build
🔇 Additional comments (7)
QRCoder/PdfByteQRCode.cs (7)
58-59: Backward compatibility approach is appropriate.The
jpgQualityparameter is retained but unused, which is clearly documented as obsolete in the XML comments. This approach maintains backward compatibility for the v1.x line while transitioning to the new rectangle-based rendering, as mentioned in the PR objectives.
82-147: Excellent PDF structure documentation.The inline comments thoroughly explain each PDF command and object structure, making the code highly maintainable and educational. This is particularly valuable for a complex format like PDF.
195-215: LGTM!The path generation logic is correct and efficient. Using
StringBuilderfor accumulating path commands is appropriate, and the PDF rectangle syntax (x y width height re) is correct.
222-233: Optimization from past review implemented correctly.The use of
const float inv255 = 1 / 255f;implements the performance optimization suggested in the past review by gfoidl. The color validation and conversion logic are correct.Based on past review comment.
240-247: Correct use of invariant culture formatting.Using
CultureInfo.InvariantCulturefor bothintandfloatformatting ensures consistent PDF generation regardless of system locale, which is essential for PDF compatibility.
253-293: LGTM!The helper methods follow proper resource management patterns with
usingstatements and provide convenient static APIs for generating QR codes. The implementation is consistent with similar helpers in the codebase.
240-247: Float precision is sufficient; no changes needed.The 6-decimal format ("0.######") covers all current uses (color components, coordinate transforms, page dimensions) with sub-point precision far beyond PDF requirements.
This changes the PdfByteQRCode algorithm to draw rectangles rather than to embed an image. This results in a PDF that has no compression artifacts and is about 50% the size of the previous JPG-embedded image (at default compression settings). It also allows the renderer to be compatible with any OS.
As an added benefit, the previous rendering process would convert to PNG first (using PngByteQRCode), then read it via GDI+, then save it via GDI+ to JPEG. All of that has been eliminated, making this code much faster. I did not perform any string optimizations within this PR as it was not within scope.
Extensive comments to the PDF commands were added so it is understood what the code actually does.
The jpgQuality parameter is now unused. While I could add a method without the parameter and mark the old method as obsolete, this would technically be a source-breaking change (not binary-breaking) in certain cases. So for v1.x I did not change it.
Summary by CodeRabbit
New Features
Changes
Tests