Skip to content

Add QR-safe canvas frames (card/sticker)#41

Merged
PrzemyslawKlys merged 3 commits intomasterfrom
feature/canvas-frame-20260127
Jan 27, 2026
Merged

Add QR-safe canvas frames (card/sticker)#41
PrzemyslawKlys merged 3 commits intomasterfrom
feature/canvas-frame-20260127

Conversation

@PrzemyslawKlys
Copy link
Member

@PrzemyslawKlys PrzemyslawKlys commented Jan 27, 2026

Summary

Adds QR-safe canvas frames plus decorative badges/tabs/ribbons that render entirely outside the QR bounds, with new style-board presets and tests.

Changes

  • Add QrPngCanvasFrameOptions (frame ring) and QrPngCanvasBadgeOptions (badge/tab/ribbon).
  • Extend QrPngCanvasOptions validation and cloning.
  • Scale frame/badge options during supersampling.
  • Render frames as true rings (preserving canvas effects) with canvas rounded-rect clipping.
  • Render badge/tab/ribbon shapes outside the QR bounds with optional ribbon tails.
  • Add style-board presets: Card Frame, Sticker Frame, Top Badge, Ribbon Tab.
  • Add renderer tests for frame and badge/ribbon placement safety.

Safety

  • All decorations are positioned relative to QR bounds and clamped to available padding.
  • Rendering is clipped to the canvas rounded-rect silhouette.

Validation

  • dotnet build -c Release
  • dotnet test CodeGlyphX.Tests/CodeGlyphX.Tests.csproj -c Release --filter "FullyQualifiedName~QrPngRendererTests|FullyQualifiedName~QrArtAutoTuneTests|FullyQualifiedName~QrArtSafetyPresetsTests|FullyQualifiedName~RendererFormatTests"
  • dotnet run -c Release --project CodeGlyphX.Examples/CodeGlyphX.Examples.csproj

Copilot AI review requested due to automatic review settings January 27, 2026 17:26
@claude
Copy link

claude bot commented Jan 27, 2026

Claude finished @PrzemyslawKlys's task —— View job


Code Review Analysis

I've completed a comprehensive review of this PR that adds QR-safe canvas frames. Here's my detailed feedback:

Todo List

  • Read and analyze changed files
  • Review frame rendering implementation
  • Check validation and error handling
  • Assess test coverage
  • Evaluate performance and security
  • Provide comprehensive feedback

🔍 Code Quality Assessment

Strengths

  1. Excellent QR Safety: Frame clamping logic in DrawCanvasFrame:1791-1814 correctly ensures frames stay outside QR bounds
  2. Clean Architecture: Well-structured options classes with proper validation
  3. Good Test Coverage: Comprehensive tests validate frame bounds and clamping behavior
  4. Proper Scaling Support: Frame options correctly scale during supersampling
  5. Intuitive API: New style presets demonstrate clean usage patterns

⚠️ Critical Issues Found

1. Canvas Effects Being Overwritten (High Priority)

Location: QrPngRenderer.Internal.A.cs:1816-1835

The current DrawCanvasFrameRing implementation has a fundamental design flaw identified by both automated reviews:

// Current problematic approach (conceptual):
FillRoundedRect(outer_area, frame_color);      // Draw outer frame
DrawCanvasFill(inner_area, background);       // Overwrites inner area

Problem: When the frame is drawn, it fills the outer rounded rectangle, then calls canvas background fill to "cut out" the ring. This overwrites previously-rendered canvas effects (grain, vignette, halo, splash) in the gap area between the QR and frame.

Impact: Enabling frames silently removes other canvas effects instead of being purely additive.

Solution: Render as a true ring by only painting pixels inside the outer bounds AND outside the inner bounds.

2. Missing Canvas Clipping (Medium Priority)

Location: QrPngRenderer.Internal.A.cs:1880-1924

Frame rendering lacks clipping to canvas rounded corners. Other canvas effects use InsideRounded checks, but frames don't.

Impact: Frames can paint into transparent rounded corners when pushed to canvas edges, changing the canvas silhouette.

3. Incomplete Supersampling Integration (Low Priority)

Location: QrPngRenderer.Internal.B.cs:1394-1405

The ScaleFrame method correctly scales thickness/gap/radius but lacks validation that scaled values remain within supersampled canvas bounds.


🔧 Implementation Issues

Validation Logic

  • ✅ Good: Parameter validation in QrPngCanvasFrameOptions.Validate()
  • ✅ Good: Runtime clamping to available padding
  • ❌ Missing: Canvas boundary validation for scaled values

Error Handling

  • ✅ Good: Graceful degradation when insufficient padding
  • ✅ Good: Proper null checks throughout
  • ⚠️ Concern: No validation that frames work with all canvas corner radius values

📊 Test Coverage Assessment

Excellent Coverage:

  • ✅ Frame bounds validation (QrPngRendererTests.cs:1213-1265)
  • ✅ Clamping behavior testing (QrPngRendererTests.cs:1268-1319)
  • ✅ QR safety verification (frames never overlap QR area)

Missing Coverage (as noted by Copilot):

  • ❌ Canvas effect interaction tests
  • ❌ Rounded canvas corner clipping tests
  • ❌ Inner frame rendering tests

Performance Considerations

Positive:

  • Efficient ring rendering algorithm
  • Proper early returns for degenerate cases
  • Good memory usage (no unnecessary allocations)

Concerns:

  • Double pixel iteration (outer loop + canvas effects) could be optimized
  • No pixel-level caching for complex rounded corner calculations

🔒 Security Assessment

Low Risk: No security concerns identified. All user inputs are properly validated and clamped.


🚀 Recommendations

  1. Fix Canvas Effects Overwriting: Implement true ring rendering to preserve existing canvas effects
  2. Add Canvas Clipping: Use InsideRounded checks similar to other canvas effects
  3. Expand Test Coverage: Add tests for canvas effect interactions and edge cases
  4. Documentation: Consider adding performance notes about frame rendering order

📝 Minor Issues

  • Code style is consistent with existing patterns
  • API design follows established conventions
  • Error messages are appropriate
  • No unused code or obvious optimizations missed

Overall Assessment: This is a well-implemented feature with good architectural decisions and safety measures. The main issue is the canvas effects overwriting problem, which should be addressed before merging to maintain the expected additive behavior of canvas effects.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1da49a6ca6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a QR-safe decorative canvas frame that renders outside the QR bounds (incl. quiet zone), plus new style-board presets demonstrating “card” and “sticker” framed looks.

Changes:

  • Introduces QrPngCanvasFrameOptions and wires it into QrPngCanvasOptions validation.
  • Adds cloning + supersampling scaling support for the new frame options.
  • Implements PNG rendering for the frame ring and adds two style-board presets using it.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
CodeGlyphX/Rendering/Png/QrPngRenderer.Internal.B.cs Scales Canvas.Frame during background supersampling.
CodeGlyphX/Rendering/Png/QrPngRenderer.Internal.A.cs Renders the canvas frame ring around the QR bounds.
CodeGlyphX/Rendering/Png/QrPngCanvasOptions.cs Adds Frame property and validates it.
CodeGlyphX/Rendering/Png/QrPngCanvasFrameOptions.cs New options type + validation for frame configuration.
CodeGlyphX/QrEasy.Helpers.cs Deep-clones frame options in CloneOptions.
CodeGlyphX.Examples/QrStyleBoardExample.cs Adds “Card Frame” and “Sticker Frame” presets using the new frame feature.

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@PrzemyslawKlys PrzemyslawKlys merged commit 1fe059a into master Jan 27, 2026
6 of 7 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the feature/canvas-frame-20260127 branch January 27, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants