Skip to content

Fix icon cropping bugs, harden batch processing, add test suite - #66

Merged
matt-edmondson merged 1 commit into
mainfrom
fix/crop-bounds-and-batch-resilience
Aug 5, 2026
Merged

Fix icon cropping bugs, harden batch processing, add test suite#66
matt-edmondson merged 1 commit into
mainfrom
fix/crop-bounds-and-batch-resilience

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Summary

Fixes four defects in the image pipeline, stops one bad input from killing a batch, makes exit codes meaningful, and adds the repository's first test suite.

Bug fixes

  • Crop off-by-one. The crop used right - left, but right and bottom are the inclusive indices of the last opaque pixel, so the span needs + 1. Every icon was losing its rightmost column and bottom row.
  • Fully transparent input crashed the batch. With nothing opaque to measure, the bounds stayed inverted and produced a negative crop width, throwing an unhandled ArgumentOutOfRangeException that aborted the run and lost every icon queued behind it. ProcessImage now emits a fully transparent square instead.
  • The output directory was not created if it did not already exist.
  • Output extensions were wrong. Output is always PNG-encoded but reused the input file name verbatim, so a .jpg input produced a .jpg-named file containing PNG bytes. The extension is now rewritten to .png.

Batch resilience

A per-file failure of any type is now reported with its exception type name and skipped, so a locked or malformed file costs one icon rather than the whole run. Previously only three ImageSharp decode exceptions were caught and anything else propagated. ProcessDirectory returns a BatchResult of written and failed counts, and the summary line reports both.

Exit codes

Code Meaning
0 All files processed, also --help and --version
1 Arguments unusable
2 Batch completed but at least one file failed

Both a failing batch and a missing required option previously exited 0.

Tests

Adds IconHelper.Test with 48 MSTest tests covering argument validation, the pixel pipeline, the file I/O layer and the exit code mapping.

The gold master tests run committed fixtures through the real ProcessDirectory path and compare pixel-for-pixel against committed expectations. regenerate.ps1 updates them, and two guard tests fail if a fixture or expectation goes unreferenced. Comparison is pixel-based rather than byte-based so an ImageSharp version bump does not cause false failures, with PNG bit depth and colour type asserted separately.

Supporting changes

Run is split into internal ProcessDirectory and ProcessImage, reached via InternalsVisibleTo, so the logic is testable. Output was verified byte-identical before and after that split. Also removes a dead local and its unused PointF alias, drops the unused ktsu.CaseConverter reference, and disposes the per-file Image that was leaking one instance per file.

Documents the recolouring algorithm inline, including the ImageSharp BlackWhite colour matrix it depends on. Rewrites README.md, which described an API (IconLoader, IconConverter, IconResolution, IconColorizer) that has never existed in this repo, and adds CLAUDE.md.

Test plan

  • dotnet build -c Release clean, 0 warnings with warnings as errors enabled
  • dotnet test -c Release passes 48 of 48
  • Gold master proven to detect regressions by temporarily perturbing the algorithm, which failed 6 of 7 cases, then restoring
  • All seven exit code paths verified against the real binary, not only unit tests
  • End to end run over a directory mixing a valid PNG, a PNG locked by another process, a text file, a fully transparent PNG and a JPEG. All three valid files written, both failures reported, exit code 2

Note for reviewers

Every gold master expectation changed, because the crop fix alters the output of every icon. Sizes grew by one pixel where the result was not already clamped to the maximum. That is expected and the images are worth a look.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ruh9nAQiuU5VGHV8uJCXtu

Fixes four defects in the image pipeline:

- The crop used `right - left` on bounding-box indices that are inclusive, so
  every icon lost its rightmost column and bottom row. Now `right - left + 1`.
- A fully transparent input left the bounds inverted, producing a negative crop
  width and an unhandled ArgumentOutOfRangeException that aborted the whole
  batch. ProcessImage now emits a fully transparent square instead.
- The output directory was not created if it did not already exist.
- Output was always PNG-encoded but reused the input file name verbatim, so a
  .jpg input produced a .jpg-named file containing PNG bytes. The extension is
  now rewritten to .png.

Batch processing no longer aborts on unexpected failures. A per-file error of
any type is reported with its exception type name and skipped, so a locked or
malformed file costs one icon rather than the whole run. ProcessDirectory
returns a BatchResult carrying the written and failed counts, and the summary
line reports both.

Exit codes now reflect the outcome: 0 for success, 1 for unusable arguments,
2 when the batch completed with failures. Both a failed batch and a missing
required option previously exited 0.

Adds IconHelper.Test with 48 MSTest tests covering argument validation, the
pixel pipeline, the file I/O layer, and the exit code mapping. Includes gold
master characterization tests that run committed fixtures through the real
ProcessDirectory path and compare pixel-for-pixel, with regenerate.ps1 to
update them and guard tests that fail if a fixture or expectation goes
unreferenced.

Restructuring needed to make the code testable: Run is split into internal
ProcessDirectory and ProcessImage, reached via InternalsVisibleTo. Verified
byte-identical output before and after the split. Also removes a dead local
and its unused PointF alias, drops the unused ktsu.CaseConverter reference,
and disposes the per-file Image that was leaking one instance per file.

Documents the recolouring algorithm inline, including the ImageSharp
BlackWhite colour matrix it depends on. Rewrites README.md, which previously
described an API (IconLoader, IconConverter, IconResolution, IconColorizer)
that has never existed in this repo, and adds CLAUDE.md.

Claude-Session: https://claude.ai/code/session_01Ruh9nAQiuU5VGHV8uJCXtu
@sonarqubecloud

sonarqubecloud Bot commented Aug 5, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
D Reliability Rating on New Code (required ≥ A)
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@matt-edmondson
matt-edmondson merged commit 3fd6873 into main Aug 5, 2026
4 of 5 checks passed
@matt-edmondson
matt-edmondson deleted the fix/crop-bounds-and-batch-resilience branch August 5, 2026 08:52
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.

1 participant