fix: resolve SonarCloud issues in core renderers and extensions#84
Conversation
Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| float iconY = (bmp.Height - iconDestHeight) / 2; | ||
| var centerDest = new SKRect(iconX - iconBorderWidth, iconY - iconBorderWidth, iconX - iconBorderWidth + iconDestWidth + iconBorderWidth * 2, iconY - iconBorderWidth + iconDestHeight + iconBorderWidth * 2); | ||
| var iconDestRect = new SKRect(iconX, iconY, iconX + iconDestWidth, iconY + iconDestHeight); | ||
| var iconBgBrush = iconBackgroundSKColor != null ? new SKPaint { Color = (SKColor)iconBackgroundSKColor } : lightBrush; |
There was a problem hiding this comment.
🟡 Custom background paint for the icon overlay is never disposed, leaking native resources
The custom background paint object is allocated but never released (new SKPaint { ... } at QRCoder.Core/Renderers/QRCode.cs:225) when a background color is provided, so each icon-rendering call leaks a native SkiaSharp resource.
Impact: Repeated QR code generation with a custom icon background color accumulates undisposed native paint objects, increasing memory pressure.
Trigger condition and code-level mechanism
When iconBackgroundSKColor != null, the ternary at line 225 creates a new SKPaint and assigns it to iconBgBrush. This paint is used at line 230 but the DrawIcon method returns without ever calling Dispose() on it. When iconBackgroundSKColor is null, the existing lightBrush is reused (which is fine — it's disposed by the caller). But the newly-allocated paint in the non-null branch is orphaned.
The repository rule at rules/csharp-qrcoder.instructions.md:18 explicitly requires: "Sempre fazer Dispose() de objetos SKBitmap, SKCanvas, SKPaint" and "Usar using statements para recursos gráficos".
Prompt for agents
In the DrawIcon method at QRCode.cs:217-237, when iconBackgroundSKColor is not null, a new SKPaint is created at line 225 but never disposed. The fix needs to conditionally dispose the paint only when it was newly created (not when it's the shared lightBrush). One approach: wrap the iconBgBrush in a using block, but only when it's a new instance. For example, create a nullable SKPaint variable for the owned brush, use a try/finally or a separate using scope, and dispose it at the end of the method. Be careful not to dispose the lightBrush that was passed in from the caller.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| return string.Empty; | ||
| throw new InvalidOperationException("Unsupported one-time password auth type."); |
There was a problem hiding this comment.
📝 Info: Returning empty string replaced with throwing exception — breaking behavioral change
The ToString() override at QRCoder.Core/Generators/PayloadGenerator.cs:3109 was changed from return string.Empty to throw new InvalidOperationException(...). While the new behavior is arguably more correct (fail-fast for invalid enum values), this is a breaking change for any caller that previously relied on getting an empty string for an unsupported OneTimePasswordAuthType. Since OneTimePasswordAuthType is a public enum with only two values (TOTP, HOTP), the fallthrough case should only be reachable via casting an invalid integer to the enum (e.g., (OneTimePasswordAuthType)99). The risk is low but non-zero for existing consumers.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private static SKBitmap MakeDotPixel(int pixelsPerModule, int pixelSize, SKPaint brush) | ||
| { | ||
| // draw a dot | ||
| var bitmap = new SKBitmap(pixelSize, pixelSize); |
There was a problem hiding this comment.
🔍 Intermediate SKBitmap in MakeDotPixel is never disposed (pre-existing)
At QRCoder.Core/Renderers/ArtQRCode.cs:254, var bitmap = new SKBitmap(pixelSize, pixelSize) is created as an intermediate drawing surface. After its content is drawn onto cropped (line 268), bitmap is never disposed. This leaks a native pixel buffer on every call. The method is called twice per RenderGraphicCore invocation (once for dark, once for light module pixels). This is a pre-existing issue not introduced by this PR, but the method was modified (made static, DrawCircle params changed to float), so it's worth noting for a follow-up fix.
Was this helpful? React with 👍 or 👎 to provide feedback.
| using (var graphics = new SKCanvas(bitmap)) | ||
| { | ||
| graphics.DrawCircle(pixelSize / 2, pixelSize / 2, pixelSize / 2, brush); | ||
| graphics.DrawCircle(pixelSize / 2f, pixelSize / 2f, pixelSize / 2f, brush); |
There was a problem hiding this comment.
📝 Info: DrawCircle integer-to-float division change subtly alters rendering for odd pixel sizes
At QRCoder.Core/Renderers/ArtQRCode.cs:257, the DrawCircle call was changed from pixelSize / 2 (integer division, truncates) to pixelSize / 2f (float division, exact). For odd pixelSize values (e.g., 11), the old code would place the center at 5 and use radius 5, while the new code places it at 5.5 with radius 5.5. This is a correctness improvement — the circle is now properly centered — but it does change the rendered output for odd pixel sizes. If there are reference-image-based tests comparing pixel-exact output, they may need updating.
Was this helpful? React with 👍 or 👎 to provide feedback.
| { | ||
| return RenderGraphic(pixelsPerModule, darkSKColor, lightSKColor, icon, iconSizePercent, iconBorderWidth, drawQuietZones, iconBackgroundSKColor); | ||
| float iconDestWidth = iconSizePercent * bmp.Width / 100f; | ||
| float iconDestHeight = iconDestWidth * icon.Height / icon.Width; |
There was a problem hiding this comment.
📝 Info: Redundant ternary removed in DrawIcon extraction is a correct simplification
The old inline code at the former line 195 had float iconDestHeight = drawIconFlag ? iconDestWidth * icon.Height / icon.Width : 0;, but this expression was always inside an if (drawIconFlag) block, making the ternary always true. The new DrawIcon method at QRCoder.Core/Renderers/QRCode.cs:220 correctly simplifies this to float iconDestHeight = iconDestWidth * icon.Height / icon.Width;. This is not a bug — it's a correct cleanup of dead code.
Was this helpful? React with 👍 or 👎 to provide feedback.
2b7d3a1
into
feature/devin-20260712-sonar-quality
1 participant
All Submissions:
Changes to Core Features:
Summary
This PR resolves SonarCloud issues in the core SkiaSharp renderers and extension helpers (
QRCode,ArtQRCode,BitmapByteQRCode,SKColorExtensions,StringValueAttribute), plus a follow-up regression fix forOneTimePassword.ToString().Key changes
QRCode.csGetGraphicoverloads together (resolvesS4136).DrawIconto reduce the icon-render path complexity (resolvesS3776inRenderGraphicwith icon).SuppressMessageforS107on the legacyGetGraphicoverload andQRCodeHelper.GetQRCode.CreateRoundedSKRectIPathstatic(resolvesS2325).ArtQRCode.csSuppressMessageforS107/S1133on the obsoleteGetGraphicandS107/S3776onRenderGraphicCore.MakeDotPixel,IsPartOfQuietZone, andIsPartOfFinderPatternstatic(resolvesS2325).pixelSize / 2fto avoid integer division inDrawCircle(resolvesS2184).SuppressMessageforS107on the convenienceGetQRCodehelper.BitmapByteQRCode.csSuppressMessageforS3776on the byte-render method andS107on theGetQRCodehelper.HexSKColorToByteArrayandIntTo4Bytestatic(resolvesS2325).StartsWith("#")toStartsWith('#')(resolvesS6610).ExtensionsSKColorExtensions.FromHex:StartsWith('#')(resolvesS6610).StringValueAttribute: added[AttributeUsage(AttributeTargets.Field)](resolvesS3993).PayloadGenerator.cs(follow-up from fix: resolve SonarCloud issues in PayloadGenerator #82)OneTimePassword.ToString()by throwingInvalidOperationExceptionfor unsupportedOneTimePasswordAuthTypevalues, while suppressingS3877/S3928.Tests:
dotnet build QRCoder.Core.sln --configuration Releasepasses.dotnet test QRCoder.Core.Tests/ -f net10.0 --configuration Releasepasses (502 tests).net48tests are skipped on Linux due to missingmono.Link to Devin session: https://app.devin.ai/sessions/20c92a1bad724f63b8be89a9e195e664
Requested by: @afonsoft