Add path-based SVG rendering with RLE encoding to reduce file size and memory usage #655
Conversation
📝 WalkthroughWalkthroughAdds a parameterless SvgQRCode.GetGraphic(), consolidates SVG rendering into a unified hex-based path, introduces explicit SizingMode values, adds color/alpha and logo-placement helpers, switches module rendering to path-based run-length encoding, adjusts SVG attribute emission, and updates tests and API snapshots. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant SvgQRCode
participant Matrix as ModuleMatrix
participant PathBuilder
participant LogoHelper
Caller->>SvgQRCode: GetGraphic(...) or GetGraphic()
SvgQRCode->>SvgQRCode: Normalize inputs (viewBox, colors -> hex, sizingMode)
alt WidthHeightAttribute
SvgQRCode->>SvgQRCode: Emit width & height attributes
else ViewBoxAttribute
SvgQRCode->>SvgQRCode: Emit viewBox attribute only
end
opt non-embedded logo
SvgQRCode->>SvgQRCode: Add xmlns:xlink
end
opt background (non-transparent light color)
SvgQRCode->>SvgQRCode: Emit background <rect>
end
SvgQRCode->>Matrix: Read QR module matrix
SvgQRCode->>PathBuilder: Build run-length encoded dark-module path (skip modules blocked by logo)
PathBuilder-->>SvgQRCode: Return path data
opt logo provided
SvgQRCode->>LogoHelper: Compute logo attributes & blocked modules
LogoHelper-->>SvgQRCode: logo bounds & attributes
SvgQRCode->>SvgQRCode: Render embedded `<svg>` or `<image>` with proper layering
end
SvgQRCode-->>Caller: Return assembled SVG string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)QRCoder/PdfByteQRCode.cs (1)
⏰ 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). (6)
🔇 Additional comments (2)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
QRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_from_helper.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_simple.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_viewbox_mode.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_viewbox_mode_viewboxattr.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_background.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_quietzones.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bytearray.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_svg_logo_embedded.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_svg_logo_image_tag.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_without_quietzones.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_without_quietzones_hex.approved.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (1)
CleanSvgVal(139-139)
⏰ 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). (6)
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: additional-tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (1)
CleanSvgVal(139-139)
⏰ 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). (6)
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: additional-tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
QRCoder/SvgQRCode.cs (1)
154-169: Critical: Prevent NaN/Infinity logo coordinates when viewBox is zero.This is a repeat of an unresolved issue from a previous review. When
sizingMode == ViewBoxAttribute, the documentation states that theviewBoxargument is unused, and callers can passSize.Empty. However, lines 160-161 still divide byviewBox.Width/Height, producingInfinitywhen dimensions are zero. This causes:
NaN/Infinityvalues in the generated<image>attributes- Broken
IsBlockedByLogologic, preventing modules from being cleared behind the logoApply this diff to guard against zero dimensions:
+ var logoViewBox = viewBox; + if (logoViewBox.Width <= 0 || logoViewBox.Height <= 0) + { + logoViewBox = new Size(drawableModulesCount, drawableModulesCount); + } - var viewBoxLogoAttr = GetLogoAttributes(logo, viewBox); + var viewBoxLogoAttr = GetLogoAttributes(logo, logoViewBox); // Convert from viewBox coordinates to module coordinates - double scaleX = drawableModulesCount / (double)viewBox.Width; - double scaleY = drawableModulesCount / (double)viewBox.Height; + double scaleX = drawableModulesCount / (double)logoViewBox.Width; + double scaleY = drawableModulesCount / (double)logoViewBox.Height;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (1)
CleanSvgVal(139-139)
⏰ 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). (6)
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: additional-tests
🔇 Additional comments (9)
QRCoder/SvgQRCode.cs (9)
29-106: LGTM!The documentation updates correctly describe the new
sizingModeandlogoparameters, and clarify whenpixelsPerModuleandviewBoxare applicable.
112-137: LGTM!The SVG header construction is well-structured, conditionally including namespace declarations and sizing attributes based on the logo presence and sizing mode.
139-152: LGTM!The transparency-aware rendering strategy is well-designed, optimizing for the common case where dark color is fully opaque by drawing light color as a single background rectangle.
171-181: LGTM!The conditional path drawing logic correctly handles transparency cases, skipping unnecessary draws when colors are fully transparent.
218-280: LGTM!The RLE path encoding implementation is well-designed, combining consecutive horizontal modules into single path commands for optimal file size. The logo blocking logic correctly ensures light-colored background behind logos.
296-306: LGTM!The
IsBlockedByLogoimplementation correctly converts module coordinates to viewBox space and checks for rectangle overlap.
308-329: LGTM!The
GetLogoAttributeshelper andImageAttributesstruct correctly calculate centered logo positioning. Note that the zero viewBox issue is already flagged in a separate comment.
332-335: LGTM! Consistent with PostscriptQRCode.The format change from
G15toG7maintains consistency withPostscriptQRCode.csand follows the .NET Framework vs Core compatibility guidance.Based on relevant code snippets.
337-399: LGTM!The transparency helper methods correctly handle:
- The literal
"transparent"keyword- Hex colors with alpha channels (
#RRGGBBAAand#RGBA)- 4-bit to 8-bit alpha conversion (multiply by 17)
- Default to fully opaque (255) for colors without alpha
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
QRCoder/SvgQRCode.cs (2)
158-168: Prevent NaN/Infinity logo coordinates when viewBox is zero.This issue was previously flagged: When
sizingMode == ViewBoxAttribute, callers passSize.Emptyfor the viewBox, but the code still divides byviewBox.Width/Height, producingInfinity/NaNthat corrupts the<image>attributes and breaksIsBlockedByLogologic.Apply the previously suggested fix to guard against zero dimensions:
- var viewBoxLogoAttr = GetLogoAttributes(logo, viewBox); - // Convert from viewBox coordinates to module coordinates - double scaleX = drawableModulesCount / (double)viewBox.Width; - double scaleY = drawableModulesCount / (double)viewBox.Height; + var logoViewBox = viewBox; + if (logoViewBox.Width <= 0 || logoViewBox.Height <= 0) + { + logoViewBox = new Size(drawableModulesCount, drawableModulesCount); + } + var viewBoxLogoAttr = GetLogoAttributes(logo, logoViewBox); + // Convert from viewBox coordinates to module coordinates + double scaleX = drawableModulesCount / (double)logoViewBox.Width; + double scaleY = drawableModulesCount / (double)logoViewBox.Height;
283-294: Critical: Fix double-append bug in Append helper.This issue was previously flagged: When
HAS_SPANis defined, the number is appended twice—once from the buffer (line 289) and again fromToString(line 293), corrupting the SVG output by duplicating all numeric values.Apply the previously suggested fix:
void Append(int num) { #if HAS_SPAN Span<char> buffer = stackalloc char[16]; if (num.TryFormat(buffer, out int charsWritten, default, CultureInfo.InvariantCulture)) { svgFile.Append(buffer.Slice(0, charsWritten)); + return; } #endif svgFile.Append(num.ToString(CultureInfo.InvariantCulture)); }
🧹 Nitpick comments (1)
QRCoder/SvgQRCode.cs (1)
343-384: Consider validation for malformed color strings.The
GetTransparencymethod returns 255 (fully opaque) for malformed hex colors. While this is defensive programming, it may silently hide user errors in color specification (e.g.,"#foo"or"#12345").Consider adding explicit validation or logging for invalid color formats to help developers catch mistakes during testing, especially since this is a new transparency feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (1)
CleanSvgVal(139-139)
⏰ 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). (6)
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: additional-tests
- GitHub Check: Test .NET Core 2.1
🔇 Additional comments (2)
QRCoder/SvgQRCode.cs (2)
410-415: LGTM: Explicit enum values.Adding explicit values to the
SizingModeenum is good practice and maintains backward compatibility since they match the implicit default values.
218-280: Verify path rendering logic with transparency tests.The
DrawModulesPathlocal function implements RLE-based path generation with transparency-aware module selection. The logic for light modules (!isDarkModule || isBlockedByLogo) intentionally draws light color behind the logo whenfillLogoBackgroundis true, which aligns with the PR objective "Draws the light color behind logos."However, per the PR's TODO, transparency tests are still needed to confirm this behavior works correctly across different color combinations and logo configurations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (7)
GetGraphic(31-35)GetGraphic(46-50)GetGraphic(61-65)GetGraphic(74-75)GetGraphic(86-87)GetGraphic(98-132)CleanSvgVal(139-139)
🪛 GitHub Check: additional-tests
QRCoder/SvgQRCode.cs
[failure] 271-271:
No overload for method 'IsBlockedByLogo' takes 4 arguments
🪛 GitHub Check: Test .NET 5.0 Windows
QRCoder/SvgQRCode.cs
[failure] 271-271:
No overload for method 'IsBlockedByLogo' takes 4 arguments
⏰ 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). (4)
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 2.1
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
QRCoder/SvgQRCode.cs (2)
267-278: Verify logo background fill logic per PR objectives.The
ShouldDrawModulelogic for light modules draws the light color behind the entire logo area, regardless of whether the underlying QR modules are dark or light (line 277:!isDarkModule || isBlockedByLogo). This means dark modules behind the logo are overridden with the light color before the logo is placed.While this may be intentional to ensure a consistent background behind logos, it differs from typical QR code rendering where modules behind logos are simply cleared/removed. The PR objectives explicitly list "Verify that the light color should be drawn behind logos" as a TODO item.
Consider adding tests to verify this behavior works correctly when:
- The logo has transparency (verifying that the light background shows through)
- The logo is small and centered (ensuring dark modules around it aren't affected)
- Comparing output with/without
FillLogoBackground()enabledDo you want me to help generate test cases for these scenarios, or would you prefer to open an issue to track this verification task?
320-324: Consider parameter type consistency with PostscriptQRCode.
CleanSvgValacceptsfloat, while the similar method inPostscriptQRCode.cs(line ~138) acceptsdouble. Althoughfloatis appropriate here sinceRectangleFproperties are floats, consider either:
- Using
doublefor consistency across QR code generators- Documenting why
floatprecision is sufficient for SVG outputBased on relevant code snippets
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (7)
GetGraphic(31-35)GetGraphic(46-50)GetGraphic(61-65)GetGraphic(74-75)GetGraphic(86-87)GetGraphic(98-132)CleanSvgVal(139-139)
⏰ 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). (6)
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: additional-tests
🔇 Additional comments (8)
QRCoder/SvgQRCode.cs (8)
26-33: LGTM: Parameterless convenience overload added.The new parameterless
GetGraphic()method provides a sensible default configuration. The use ofSize.EmptywithSizingMode.ViewBoxAttributeis appropriate since the viewBox parameter is unused in that mode (as documented), and the logo coordinate calculations at line 167 correctly usedrawableModulesCountinstead of the viewBox dimensions.
121-146: LGTM: Clean SVG tag construction.The refactored SVG opening tag construction efficiently handles optional attributes:
- Conditionally includes
xmlns:xlinkonly when needed (non-embedded logos)- Properly omits
widthandheightattributes forViewBoxAttributemode- Uses inline integer formatting via the local
AppendhelperThis reduces SVG file size and improves maintainability.
148-161: LGTM: Efficient background rendering optimization.The logic correctly determines when to draw the light color as a simple background rectangle versus as a path:
- When dark color is fully opaque, light can be a single
<rect>(lines 152-161)- When dark has transparency, light modules must be drawn as a path (line 173)
This optimization significantly reduces SVG file size for the common opaque-colors case.
282-293: LGTM: Efficient integer formatting with proper fallback.The
Appendhelper efficiently formats integers usingTryFormatwith stack allocation on modern frameworks, with a properreturnstatement (line 289) preventing the double-append bug that was flagged in previous reviews. The fallback toToStringensures compatibility with older frameworks.
326-372: Verify error handling strategy for invalid color formats.
GetTransparencydefaults to 255 (fully opaque) for any color string that doesn't match expected formats (line 371). This means invalid hex colors like"#GG0000"or typos in the hex string will silently be treated as opaque, potentially masking user errors.Consider whether to:
- Keep the current lenient behavior (fail-safe to opaque)
- Throw an exception for malformed hex colors
- Add validation in public
GetGraphicmethods that acceptstringcolorsThe current behavior is fail-safe but might hide bugs. Document the intended error handling strategy.
390-422: LGTM: Clean color-to-hex conversion with optimizations.The
ColorToHexmethod includes thoughtful optimizations:
- Shorthand notation for common colors (
#000and#FFF)- Omits alpha channel for fully opaque colors
- Uses the
transparentkeyword for fully transparent colors- Includes alpha channel (
#RRGGBBAA) only when neededThis contributes to the ~70% file size reduction mentioned in the PR objectives.
163-168: LGTM: Logo coordinate calculations fixed.The logo attribute calculation now correctly uses
new Size(drawableModulesCount, drawableModulesCount)instead of the potentially zero-valuedviewBoxparameter. This addresses the NaN/Infinity issue flagged in previous reviews whenSizingMode.ViewBoxAttributewas used withSize.Empty.
216-264: LGTM: Efficient RLE path generation.The
DrawModulesPathlocal function implements run-length encoding (RLE) to combine horizontal runs of adjacent modules into single path commands. This optimization:
- Significantly reduces the number of path commands (one per run vs. one per module)
- Uses compact relative path notation (
hfor horizontal,vfor vertical)- Correctly handles module boundaries and run detection
This is a key contributor to the ~70% file size reduction mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
QRCoder/SvgQRCode.cs (2)
320-324: Update comment to reflect float signatureThe comment references "double values" but the method signature is now
float. This is a minor documentation inconsistency.Update the comment:
- //Clean double values for international use/formats + //Clean float values for international use/formats //We use explicitly "G7" to avoid differences between .NET full and Core platforms //https://stackoverflow.com/questions/64898117/tostring-has-a-different-behavior-between-net-462-and-net-core-3-1 private static string CleanSvgVal(float input) => input.ToString("G7", CultureInfo.InvariantCulture);
137-144: Validate viewBox dimensions for WidthHeightAttributeThrow an exception if
viewBox.WidthorviewBox.Height≤ 0 to avoid emittingwidth="0" height="0":if (sizingMode == SizingMode.WidthHeightAttribute) { + if (viewBox.Width <= 0 || viewBox.Height <= 0) + throw new ArgumentException("ViewBox must have positive width and height when using WidthHeightAttribute sizing mode.", nameof(viewBox)); svgFile.Append(@" width="""); Append(viewBox.Width); // … }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (7)
GetGraphic(31-35)GetGraphic(46-50)GetGraphic(61-65)GetGraphic(74-75)GetGraphic(86-87)GetGraphic(98-132)CleanSvgVal(139-139)
⏰ 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). (1)
- GitHub Check: additional-tests
🔇 Additional comments (7)
QRCoder/SvgQRCode.cs (7)
26-33: LGTM: Convenient parameterless overloadThe new parameterless
GetGraphic()method provides a simple default path for generating scalable SVG QR codes. The defaults (ViewBoxAttribute mode with black/white colors) are sensible.
104-104: LGTM: Clean consolidation through ColorToHexRouting the Color-based overload through
ColorToHexunifies the rendering path and ensures consistent hex-based color handling.
273-277: Verify the light color behind logos behaviorThe current logic fills the logo area with light color when
fillLogoBackgroundis true:
- Dark modules: drawn only where not blocked by logo
- Light modules: drawn everywhere except where blocked by dark modules, PLUS the entire logo area
This means the logo sits on a light background. The PR objectives include "Todo: Verify that the light color should be drawn behind logos."
Please confirm this is the intended behavior. Alternative approaches:
- Current: Light background behind logo (current implementation)
- Transparent: No fill behind logo (would require
(!isDarkModule && !isBlockedByLogo)instead)- Conditional: Different behavior based on logo transparency
If the current behavior is correct, please update the PR objectives to remove the TODO.
282-293: LGTM: Efficient integer appending with proper flow controlThe
Appendhelper correctly handles both Span-based and fallback paths with the return statement preventing the double-append issue flagged in previous reviews.
326-388: LGTM: Comprehensive transparency handlingThe transparency helpers correctly parse alpha channels from multiple hex formats:
#RRGGBBAA: 8-bit alpha (lines 343-354)#RGBA: 4-bit alpha converted to 8-bit via multiplication by 17 (lines 356-367)"transparent"keyword: returns 0 (lines 337-338)- Default: returns 255 for opaque colors without alpha (line 371)
The math for 4-bit to 8-bit conversion (alpha * 17) is correct, mapping 0-15 to 0-255.
390-422: LGTM: Efficient color-to-hex conversion with shorthandsThe
ColorToHexhelper optimizes common cases with shorthand formats (#000 for black, #FFF for white) while correctly handling transparency:
- Fully opaque:
#RRGGBBformat- Fully transparent:
"transparent"keyword- Partially transparent:
#RRGGBBAAformatThis contributes to the file size reduction mentioned in the PR objectives.
296-318: LGTM: Correct logo placement and collision detectionThe helper methods implement correct logic:
IsBlockedByLogo: Proper rectangle overlap detection using edge comparisonsGetLogoAttributes: Correctly centers logo within viewBox using float arithmetic
Shane32
changed the title
Shane32
added
enhancement
|
@gfoidl Some PRs are small ... some PRs are big .... (lol) |
QRCoder/SvgQRCode.cs
Outdated
| #if HAS_SPAN | ||
| Span<char> buffer = stackalloc char[16]; | ||
| if (num.TryFormat(buffer, out int charsWritten, default, CultureInfo.InvariantCulture)) | ||
| { | ||
| svgFile.Append(buffer.Slice(0, charsWritten)); | ||
| return; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
| #if HAS_SPAN | |
| Span<char> buffer = stackalloc char[16]; | |
| if (num.TryFormat(buffer, out int charsWritten, default, CultureInfo.InvariantCulture)) | |
| { | |
| svgFile.Append(buffer.Slice(0, charsWritten)); | |
| return; | |
| } | |
| #endif | |
| #if NET6_0_OR_GREATER | |
| svgFile.Append(CultureInfo.InvariantCulture, $"{num}"); | |
| #endif |
Should also (w/o measuring) be a little bit faster, as it's formatted to the SB's buffer directly, and not via the span detour.
For < .NET 6 where span is available I wouldn't bother and don't have a separate #ifdef for this.
There was a problem hiding this comment.
I pushed this code into StringExtensions, with a fast path for .NET 6+, a fallback for HAS_SPAN, and fallback for anything else. Your argument is completely valid; we really don't need to optimize for older targets....but why not - so long as it doesn't impede reading the code? I think what I'd like to do is for v2 create an Append(IFormatProvider, FormattableString) method for pre-.NET6 environments and combine a bunch of these multi-line Append commands into a interpolated string for better readability and more concise code.
There was a problem hiding this comment.
I'd also like to consider a dependency on System.Memory for .NET Standard 2.0 support in v2 so that Span support is available throughout the codebase. We can review a PR and see if it makes sense or not. It may not help much if the framework (like int.Parse) does not support Span.
There was a problem hiding this comment.
if the framework (like int.Parse) does not support Span.
I think that could be polyfilled with C#'s extension members. So a new compiler creaets code that runs on old targets.
Thus I also think that System.Memory makes the code base cleaner.
There was a problem hiding this comment.
Well this savings are impressive 👍🏻
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
QRCoder/SvgQRCode.cs (2)
128-128: Consider verbatim string for readability.As noted in past review, this line has many quote characters. Using a verbatim string could improve readability:
- svgFile.Append("\" shape-rendering=\"crispEdges\""); + svgFile.Append(@"\" shape-rendering=\""crispEdges\""");Or with raw string literals (C# 11+):
- svgFile.Append("\" shape-rendering=\"crispEdges\""); + svgFile.Append("""" shape-rendering="crispEdges"""");Based on learnings (past review comment by gfoidl)
282-293: Consider StringBuilder.AppendFormat for .NET 6+ performance.As suggested in past review, for .NET 6+, using the interpolated string overload could be slightly faster as it formats directly to the StringBuilder's buffer:
+#if NET6_0_OR_GREATER + svgFile.Append(CultureInfo.InvariantCulture, $"{num}"); +#elif HAS_SPAN -#if HAS_SPAN Span<char> buffer = stackalloc char[16]; if (num.TryFormat(buffer, out int charsWritten, default, CultureInfo.InvariantCulture)) { svgFile.Append(buffer.Slice(0, charsWritten)); return; } #endif svgFile.Append(num.ToString(CultureInfo.InvariantCulture));However, the current implementation is also performant and reasonable.
Based on learnings (past review comment by gfoidl)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/SvgQRCode.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/SvgQRCode.cs (1)
QRCoder/PostscriptQRCode.cs (1)
CleanSvgVal(139-139)
⏰ 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). (6)
- GitHub Check: additional-tests
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 3.1
🔇 Additional comments (4)
QRCoder/SvgQRCode.cs (4)
26-33: LGTM! Clean parameterless overload.The new parameterless
GetGraphic()method provides a sensible default for generating scalable SVGs (black on white, no width/height attributes). The delegation to the main method is clean and appropriate.
116-294: Excellent path-based rendering implementation!The main rendering logic is well-designed:
Correct transparency handling: The code properly distinguishes between fully opaque, partially transparent, and fully transparent colors, drawing light modules as either a background rect (when dark is opaque) or as a path (when dark is transparent) to ensure proper layering.
Efficient RLE encoding: The run-length encoding for combining adjacent modules reduces file size significantly.
Proper logo obstruction: The
ShouldDrawModulelogic correctly clears space for logos while maintaining QR code readability.Safe numeric formatting: The
Appendhelper with stackalloc is efficient and thereturnstatement on line 289 correctly prevents double-appending.All previously reported critical issues (NaN/Infinity coordinates, double-append bug, extra argument) have been successfully addressed.
296-422: LGTM! Well-implemented helper methods.The helper methods are well-designed and correct:
- IsBlockedByLogo: Proper rectangle overlap detection for logo obstruction.
- GetLogoAttributes: Correctly centers the logo based on percentage size.
- CleanSvgVal: Uses "G7" format for cross-platform consistency (good catch on the .NET Core vs Full Framework difference).
- GetTransparency: Robustly handles multiple hex formats (#RRGGBBAA, #RGBA) and the "transparent" keyword with proper bounds checking.
- ColorToHex: Efficiently uses shorthand notation for common colors and properly handles transparency.
427-438: LGTM! Explicit enum values improve stability.Adding explicit values to the
SizingModeenum is good practice for API stability and serialization. The updated documentation clearly explains each mode's behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
QRCoder/SvgQRCode.cs (1)
121-146: Consider verbatim string for better readability.The escaped quotes in line 128 can make the string harder to read. As suggested in past review comments, consider using a verbatim string literal for clarity.
- svgFile.Append("\" shape-rendering=\"crispEdges\""); + svgFile.Append(@"\" shape-rendering=\"crispEdges""");Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/Extensions/StringExtensions.cs(1 hunks)QRCoder/PdfByteQRCode.cs(2 hunks)QRCoder/SvgQRCode.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
QRCoder/PdfByteQRCode.cs (1)
QRCoder/Extensions/StringExtensions.cs (2)
AppendInvariant(64-79)AppendInvariant(86-101)
QRCoder/SvgQRCode.cs (2)
QRCoder/PostscriptQRCode.cs (1)
CleanSvgVal(139-139)QRCoder/Extensions/StringExtensions.cs (3)
AppendInvariant(64-79)AppendInvariant(86-101)ToString(55-56)
QRCoder/Extensions/StringExtensions.cs (6)
QRCoder/PayloadGenerator/BitcoinLikeCryptoCurrencyAddress.cs (1)
ToString(46-65)QRCoder/PayloadGenerator/Girocode.cs (1)
ToString(75-95)QRCoder/PayloadGenerator/MoneroTransaction.cs (1)
ToString(40-48)QRCoder/PayloadGenerator/SlovenianUpnQr.cs (1)
ToString(142-161)QRCoder/PayloadGenerator/BezahlCode.cs (1)
ToString(247-328)QRCoder/PayloadGenerator/SwissQrCode.cs (3)
ToString(293-294)ToString(480-490)ToString(544-598)
⏰ 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). (6)
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 2.1
- GitHub Check: additional-tests
- GitHub Check: Test .NET Core 3.1
🔇 Additional comments (8)
QRCoder/PdfByteQRCode.cs (1)
207-212: LGTM!The migration to
AppendInvariantensures consistent invariant culture formatting for numeric values in PDF path commands.QRCoder/SvgQRCode.cs (7)
26-33: LGTM!The parameterless overload provides a convenient default for generating scalable SVG without explicit sizing, routing through the unified rendering path with sensible defaults (black on white, ViewBoxAttribute mode).
148-180: LGTM!The transparency-aware rendering logic correctly handles all cases:
- Fully opaque dark with opaque/transparent light: light as background rect
- Partially transparent dark: light modules drawn as path
- Fully transparent colors: skipped entirely
This ensures proper layering and avoids unnecessary SVG elements.
216-280: LGTM!The path-based rendering with RLE encoding is well-implemented:
- Correctly combines adjacent modules into single rectangles
- Uses efficient SVG path commands (M for move, h/v for relative lines, z for close)
ShouldDrawModulelogic properly handles both dark/light modules and logo obstructionThis achieves the stated goal of ~70% file size reduction.
282-292: LGTM!The rectangle overlap logic correctly detects when a module at (x,y) intersects with the logo area. The conditions properly check all four edges for overlap.
312-375: LGTM!The transparency helpers correctly handle:
- The "transparent" keyword → alpha 0
- #RRGGBBAA format (9 chars) → extract alpha from last 2 hex digits
- #RGBA format (5 chars) → extract alpha from last digit, multiply by 17 for 8-bit
- Default → fully opaque (alpha 255)
The
IsPartiallyTransparentandIsFullyTransparentpredicates provide clear intent.
381-408: LGTM!The
ColorToHexconversion is well-optimized:
- Uses shorthand #000 and #FFF for common black/white
- Uses "transparent" keyword for fully transparent (better SVG compatibility than #RRGGBB00)
- Uses #RRGGBB for opaque colors
- Uses #RRGGBBAA only when necessary for partial transparency
This keeps the SVG output concise while maintaining correctness.
413-424: LGTM!Explicitly assigning enum values (0 and 1) ensures stability for serialization and prevents potential breaking changes if new values are added.
| // Create a single rectangle for the entire run of dark modules | ||
| // Format: x y width height re | ||
| pathCommands.Append(ToStr(startX)); | ||
| pathCommands.AppendInvariant(startX); | ||
| pathCommands.Append(' '); | ||
| pathCommands.Append(ToStr(y)); | ||
| pathCommands.AppendInvariant(y); | ||
| pathCommands.Append(' '); | ||
| pathCommands.Append(ToStr(x - startX)); | ||
| pathCommands.AppendInvariant(x - startX); | ||
| pathCommands.Append(" 1 re\r\n"); |
There was a problem hiding this comment.
Hopefully for v2 this can all be cleaned up into an interpolated string, fully optimized for .NET Standard 2.1+, but compatible with .NET Standard 2.0 as well.
2 participants
Changes:
GetGraphic()overload with no arguments for a scalable SVG (no width/height set) with black on whitestringBuilder.ToString()stackallocand copied directly into the StringBuildertransparent)Todo:
Notes:
viewBoxargument no longer affects the actual SVG view box. According to online documentation, the view box has no bearing whatsoever on the rendered size of the SVG. The view box just tells the viewer (e.g. browser) what portion of an infinitely large 2-dimensional coordinate space within the SVG to display. Only thewidthandheightSVG tag parameters affect the displayed size. The only way this would affect a user/developer is if they were altering the SVG content after its generation. It should not affect embedding the SVG inside another SVG, as the embedded SVG coordinate system is its own.Also, the RLE encoding in a single dimension replaced a 2-dimensional rectangle detection algorithm previously in use. I find the previous algorithm to have very little usefulness beyond the finder boxes. In the future, it would be best to have an algorithm that would encode adjoining modules of any shape, which is certainly possible using even-odd fill. (Applies to PDF writing as well.) But it would come at the cost of additional memory requirements.
The rendered SVG within the transposition tests still passes, since the SVG when rendered to a bitmap has not changed with this PR. Visual inspection of all prior tests indicate that the SVG produces identical output visually.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests