Split QRCode and ArtQRCode to QRCoder.SystemDrawing #686
Conversation
Removed branch specification for pull request trigger.
…yload handling - Implemented abstract class AbstractQRCode and derived classes: ArtQRCode, AsciiQRCode, Base64QRCode, BitmapByteQRCode, PdfByteQRCode, PngByteQRCode, PostscriptQRCode, SvgQRCode, and QRCode. - Added helper classes for each QR code type to facilitate QR code generation. - Introduced PayloadGenerator with various payload types including BezahlCode, BitcoinAddress, CalendarEvent, ContactData, Geolocation, Girocode, and more. - Defined enums for various configurations and options within the QR code generation process. - Included exception handling classes for managing errors related to data size and specific payload types. - Established a comprehensive structure for QR code data management with QRCodeData and QRCodeGenerator classes.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR reorganizes QRCoder into separate projects: a core library and a new Changes
Sequence DiagramsequenceDiagram
participant Consumer
participant QRCoder as QRCoder<br/>(Core)
participant SystemDrawing as QRCoder.SystemDrawing<br/>(System.Drawing)
Consumer->>QRCoder: Generate QR data<br/>(QRCodeData, QRCodeGenerator)
QRCoder->>Consumer: Return QRCodeData
Consumer->>SystemDrawing: Create QRCode/ArtQRCode<br/>(Windows platform)
SystemDrawing->>SystemDrawing: Call GetGraphic<br/>(Color, Bitmap)
SystemDrawing->>Consumer: Return Bitmap
Consumer->>QRCoder: Create Base64QRCode<br/>(PNG-only)
QRCoder->>QRCoder: ColorTranslator.FromHtml<br/>(HTML hex → Color)
QRCoder->>QRCoder: PngByteQRCode.GetGraphic<br/>(default black/white or custom)
QRCoder->>Consumer: Return base64 PNG string
Consumer->>SystemDrawing: Request non-PNG format<br/>(JPEG, GIF)
SystemDrawing-->>Consumer: NotSupportedException
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The review requires understanding a significant architectural split: moving System.Drawing-dependent code to a separate project, updating cross-cutting dependencies, verifying API surface changes across multiple approval files, and ensuring test coverage remains correct after the reorganization. Multiple file categories demand separate reasoning despite some homogeneous patterns (project reference updates). Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
…intain consistency
… parameters for improved flexibility
…zation and maintain consistency
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
QRCoder/PngByteQRCode.cs (1)
54-56: Make the default-color fast path robust to Color naming.Comparing System.Drawing.Color with == can be tripped by named vs custom colors. Compare ARGB to be explicit.
Apply:
- => darkColor == System.Drawing.Color.Black && lightColor == System.Drawing.Color.White + => darkColor.ToArgb() == System.Drawing.Color.Black.ToArgb() + && lightColor.ToArgb() == System.Drawing.Color.White.ToArgb() ? GetGraphic(pixelsPerModule, drawQuietZones) : GetGraphic(pixelsPerModule, [darkColor.R, darkColor.G, darkColor.B, darkColor.A], [lightColor.R, lightColor.G, lightColor.B, lightColor.A], drawQuietZones);QRCoder.SystemDrawing/QRCoder.SystemDrawing.csproj (1)
3-15: Declare Windows support and enable Windows targeting for non-Windows builds.System.Drawing.Common is only supported on Windows in .NET 6+. Mark the package/platform to avoid CA1416 warnings and runtime surprises when consumers use this library cross‑platform.
Consider:
<PropertyGroup> <TargetFrameworks>netstandard2.0;netstandard2.1;net6.0</TargetFrameworks> <IsTrimmable Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))">true</IsTrimmable> <IsPackable>true</IsPackable> + <SupportedOSPlatform>windows</SupportedOSPlatform> + <EnableWindowsTargeting>true</EnableWindowsTargeting> </PropertyGroup>Optionally add assembly-level attributes for APIs in this project:
- [assembly: System.Runtime.Versioning.SupportedOSPlatform("windows")]
Also applies to: 17-26
QRCoderTests/Base64QRCodeRendererTests.cs (1)
46-56: Loosen assertion to message casing/wording changes.Exception text can vary. Assert case‑insensitively or match enum/name to reduce brittleness.
Example:
- ex.Message.ShouldContain("Only PNG format is supported"); - ex.Message.ShouldContain("Jpeg"); + ex.Message.ShouldContain("Only PNG format is supported", Case.Insensitive); + ex.Message.ShouldContain(nameof(Base64QRCode.ImageType.Jpeg), Case.Insensitive);QRCoder/Base64QRCode.cs (1)
45-46: Consider input validation for HTML hex color strings.
ColorTranslator.FromHtmlwill throw anArgumentExceptionif the color string is invalid. Consider adding validation or wrapping the call in a try-catch to provide clearer error messages to users.Example:
public string GetGraphic(int pixelsPerModule, string darkColorHtmlHex, string lightColorHtmlHex, bool drawQuietZones = true) - => GetGraphic(pixelsPerModule, ColorTranslator.FromHtml(darkColorHtmlHex), ColorTranslator.FromHtml(lightColorHtmlHex), drawQuietZones); +{ + if (string.IsNullOrWhiteSpace(darkColorHtmlHex)) + throw new ArgumentException("Dark color cannot be null or empty.", nameof(darkColorHtmlHex)); + if (string.IsNullOrWhiteSpace(lightColorHtmlHex)) + throw new ArgumentException("Light color cannot be null or empty.", nameof(lightColorHtmlHex)); + + try + { + return GetGraphic(pixelsPerModule, ColorTranslator.FromHtml(darkColorHtmlHex), ColorTranslator.FromHtml(lightColorHtmlHex), drawQuietZones); + } + catch (Exception ex) when (ex is ArgumentException or FormatException) + { + throw new ArgumentException($"Invalid color format. Expected HTML hex format (e.g., '#FF0000'). Dark: '{darkColorHtmlHex}', Light: '{lightColorHtmlHex}'", ex); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
QRCoderTests/Base64QRCodeRendererTests.can_render_base64_qrcode_jpeg.approved.gifis excluded by!**/*.gifQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap.embeddedLogo.approved.gifis excluded by!**/*.gifQRCoderTests/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_background.embeddedLogo.approved.gifis excluded by!**/*.gifQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_quietzones.approved.svgis excluded by!**/*.svgQRCoderTests/SvgQRCodeRendererTests.can_render_svg_qrcode_with_png_logo_bitmap_without_quietzones.embeddedLogo.approved.gifis excluded by!**/*.gif
📒 Files selected for processing (19)
QRCoder.SystemDrawing/QRCoder.SystemDrawing.csproj(1 hunks)QRCoder.sln(2 hunks)QRCoder/Base64QRCode.cs(4 hunks)QRCoder/Extensions/ColorTranslator.cs(1 hunks)QRCoder/PngByteQRCode.cs(1 hunks)QRCoder/QRCoder.csproj(0 hunks)QRCoder/SvgQRCode.cs(0 hunks)QRCoderApiTests/ApiApprovalTests.cs(1 hunks)QRCoderApiTests/QRCoder.approved.txt(2 hunks)QRCoderApiTests/QRCoderApiTests.csproj(1 hunks)QRCoderApiTests/net60/QRCoder.SystemDrawing.approved.txt(1 hunks)QRCoderApiTests/net60/QRCoder.approved.txt(0 hunks)QRCoderApiTests/netstandard20+netstandard21/QRCoder.SystemDrawing.approved.txt(1 hunks)QRCoderBenchmarks/QRCoderBenchmarks.csproj(1 hunks)QRCoderConsole/QRCoderConsole.csproj(1 hunks)QRCoderDemo/QRCoderDemo.csproj(1 hunks)QRCoderTests/Base64QRCodeRendererTests.cs(1 hunks)QRCoderTests/QRCoderTests.csproj(1 hunks)QRCoderTests/SvgQRCodeRendererTests.cs(0 hunks)
💤 Files with no reviewable changes (4)
- QRCoderTests/SvgQRCodeRendererTests.cs
- QRCoder/SvgQRCode.cs
- QRCoder/QRCoder.csproj
- QRCoderApiTests/net60/QRCoder.approved.txt
🔇 Additional comments (14)
QRCoder.sln (1)
34-35: LGTM! New project properly integrated into solution.The QRCoder.SystemDrawing project is correctly added with:
- Proper project GUID ({FEE6DC1D-BE5A-4498-A70D-35C516AD13F3})
- Complete build configurations for all platforms (Any CPU, ARM, x64, x86)
- Both Debug and Release configurations with ActiveCfg and Build.0 entries
Also applies to: 206-221
QRCoderApiTests/netstandard20+netstandard21/QRCoder.SystemDrawing.approved.txt (1)
1-54: LGTM! Well-structured API surface for System.Drawing integration.The public API is clean and follows consistent patterns:
- Proper inheritance from AbstractQRCode and IDisposable
- Helper classes for simplified usage
- Reasonable default parameters
- Maintains QRCoder namespace for compatibility
QRCoderApiTests/QRCoderApiTests.csproj (1)
9-9: LGTM! Project reference correctly updated.The reference change from QRCoder to QRCoder.SystemDrawing aligns with the PR objectives and enables testing of the new System.Drawing-based API surface.
QRCoderApiTests/ApiApprovalTests.cs (1)
47-47: LGTM! Proper test coverage for QRCode API surface.Adding the QRCode type to the API approval tests ensures that any changes to the public API of the System.Drawing-based QRCode class are tracked and reviewed.
QRCoderBenchmarks/QRCoderBenchmarks.csproj (1)
16-16: LGTM! Benchmarks correctly reference SystemDrawing.The project reference update ensures benchmarks can measure performance of the System.Drawing-based QR code rendering.
QRCoderTests/QRCoderTests.csproj (1)
43-43: LGTM! Test project correctly updated to reference SystemDrawing.The reference change enables testing of the System.Drawing-based QR code functionality while maintaining multi-target framework support.
QRCoderDemo/QRCoderDemo.csproj (1)
19-21: LGTM! Demo project simplified and updated correctly.The project reference has been updated to use QRCoder.SystemDrawing with a cleaner, simplified reference format (removing unnecessary GUID and Name metadata).
QRCoderConsole/QRCoderConsole.csproj (1)
31-31: LGTM! Console project correctly references SystemDrawing.The reference update aligns with the PR objectives. Note that the project targets both Windows-specific (net6.0-windows) and cross-platform (net6.0) frameworks, which may rely on different rendering paths.
QRCoder/Extensions/ColorTranslator.cs (1)
9-221: Solid fallback for HTML color parsing.Named/hex parsing looks correct and defensive. Guard + dictionary approach LGTM.
QRCoderApiTests/QRCoder.approved.txt (1)
33-41: API delta aligns with PNG‑only path and obsoleted imgType.Obsolete messages and overload shape look consistent with the new behavior.
Also applies to: 51-55
QRCoder/Base64QRCode.cs (3)
57-59: LGTM! Clear deprecation path.The obsolete attribute provides a clear migration message, and the delegation chain ensures consistent behavior.
70-79: LGTM! Appropriate handling of the breaking change.The method correctly throws
NotSupportedExceptionfor non-PNG formats while still supporting existing code that explicitly specifies PNG format. The obsolete attribute guides users to the new API.
122-142: LGTM! Well-implemented convenience method.The method properly disposes of all resources using
usingstatements and provides a clean, single-call API for generating base64-encoded QR codes.QRCoderApiTests/net60/QRCoder.SystemDrawing.approved.txt (1)
1-58: LGTM! Well-structured API surface for Windows-specific System.Drawing support.The API approval file correctly documents the public surface of the QRCoder.SystemDrawing assembly:
- All types are appropriately marked with
[SupportedOSPlatform("windows")]since System.Drawing.Bitmap is Windows-specific in .NET 6+- The API follows consistent naming conventions and patterns
- Both QRCode and ArtQRCode classes include corresponding helper classes for single-call convenience methods
- The enums are properly nested within their respective classes
This separation successfully isolates System.Drawing dependencies as intended by the PR.
Co-authored-by: Günther Foidl <[email protected]>
|
There's a newline at the bottom of QRCoderConsole.csproj and QRCoderDemo.csproj. Let me know if you were referring to something else. |
|
GitHub diff here showed me that the newline is missing. When you there's one, then it's OK. |
Actually, you're right. I don't know why GitHub didn't show me the missing newlines. I have their beta expierence enabled; perhaps that's why it's not showing up for me, but did for you. 🤷♂️ |
I have it also enabled. Maybe some other preview feature that shows it differently. |
2 participants
Removed constructor:
public SvgLogo(System.Drawing.Bitmap iconRasterized, int iconSizePercent = 15, bool fillLogoBackground = true) { }.Users will need to use the constructor with a
byte[]containing a PNG image, or the one with astringcontaining a SVG icon.Removed method from
Base64QRCode:public string GetGraphic(int pixelsPerModule, System.Drawing.Color darkColor, System.Drawing.Color lightColor, System.Drawing.Bitmap icon, int iconSizePercent = 15, int iconBorderWidth = 6, bool drawQuietZones = true, QRCoder.Base64QRCode.ImageType imgType = 2) { }Users will need to use a different renderer and convert the resulting byte array via
Convert.ToBase64StringSummary by CodeRabbit
New Features
Deprecation
Breaking Changes