Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,12 @@ help text and exited 0.

Do not "fix" these silently, they are documented in the README as limitations:

- Output extensions are rewritten to `.png`, so two inputs sharing a base name (`a.png`, `a.jpg`)
collide and the later one wins.
- A run that fails every single file still exits `2`, the same as a run that failed one file. The
exit status says "something failed", the summary line says how much.

### Fixed Bugs Worth Knowing About

Both are covered by regression tests. Do not reintroduce them.
Each is covered by regression tests. Do not reintroduce them.

- **Bounding-box off-by-one.** The crop used `right - left`, but `right` and `bottom` are *inclusive*
indices of the last opaque pixel, so the span needs `+ 1`. Every icon used to lose its rightmost
Expand All @@ -181,6 +179,15 @@ Both are covered by regression tests. Do not reintroduce them.
`PaddingThatFitsIsAppliedExactlyAndNotClamped` and
`ProcessDirectoryTests.SmallArtworkWithValidatedPaddingDoesNotFailTheBatch`.

- **Inputs overwrote each other, and the output could overwrite the input.** Every output is
`<base name>.png`, so `save.png` and `save.bmp` wrote the same file: the later one won while the
summary counted both, and `--output` equal to `--input` replaced the source icons with their masks
and exited 0. `ProcessDirectory` now remembers the names it has written in a run and counts a
collision as a failed file, and `Validate` rejects an output directory that is the input directory
(`Arguments.PathComparison` decides case sensitivity per platform). Pinned by
`ProcessDirectoryTests.InputsSharingABaseNameDoNotOverwriteEachOther` and
`ArgumentsTests.ValidateRejectsAnOutputThatIsTheInputDirectory`.

## Testing

`IconHelper.Test` is an MSTest project (`MSTest.Sdk`, Microsoft Testing Platform). Both it and the
Expand Down
16 changes: 16 additions & 0 deletions IconHelper.Test/ArgumentsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,22 @@ public void ValidateAcceptsAnOutputDirectoryThatDoesNotExistYet()
Assert.IsTrue(args.Validate(out _), "The output directory is created on demand, so it need not exist yet.");
}

[TestMethod]
public void ValidateRejectsAnOutputThatIsTheInputDirectory()
{
// Every output is <base name>.png, so writing into the input directory replaces the .png
// sources with their own masks.
using TempDirectory temp = new();
Arguments args = ValidArguments(temp);
args.OutputPath = args.InputPath + Path.DirectorySeparatorChar;

bool valid = args.Validate(out Collection<string> errors);

Assert.IsFalse(valid);
Assert.HasCount(1, errors);
Assert.Contains("--output must not be the --input directory", errors[0]);
}

[TestMethod]
public void ValidateRejectsEmptyPaths()
{
Expand Down
25 changes: 25 additions & 0 deletions IconHelper.Test/ProcessDirectoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,31 @@ public void RewritesTheOutputExtensionToPng()
Assert.IsFalse(File.Exists(Path.Combine(output, "photo.jpg")), "The original extension should not be reused.");
}

[TestMethod]
public void InputsSharingABaseNameDoNotOverwriteEachOther()
{
// Both map to save.png. Writing both left only the later file on disk while the summary
// counted two, so the second is reported as failed instead and the first is kept.
using TempDirectory temp = new();
string input = temp.Combine("in");
string output = temp.Combine("out");
Directory.CreateDirectory(input);
WritePng(Path.Join(input, "save.png"), 64);

using (Image<Rgba32> bitmap = TestImages.Blank(64, 64))
{
TestImages.FillRect(bitmap, 16, 16, 32, 32, new Rgba32(255, 255, 255, 255));
bitmap.SaveAsBmp(Path.Join(input, "save.bmp"));
}

BatchResult result = IconHelper.ProcessDirectory(ArgumentsFor(input, output), NamedColors.White);

Assert.AreEqual(1, result.Written, "Only one output file can exist for the shared name.");
Assert.AreEqual(1, result.Failed, "The colliding input must be reported, not silently overwritten.");
Assert.HasCount(result.Written, Directory.GetFiles(output), "The written count must match the files on disk.");
Assert.AreEqual(IconHelper.ExitSomeFilesFailed, IconHelper.ExitCodeFor(result));
}

[TestMethod]
public void SkipsFilesAlreadyMarkedAsGenerated()
{
Expand Down
30 changes: 26 additions & 4 deletions IconHelper/Arguments.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,23 @@ internal bool Validate(out Collection<string> errors)
errors.Add("Padding must be less than half the size of the image.");
}

if (!TryResolveInput(out _, out string? inputError))
bool inputResolved = TryResolveInput(out AbsoluteDirectoryPath? input, out string? inputError);
if (!inputResolved)
{
errors.Add(inputError);
errors.Add(inputError!);
}

if (!TryResolveOutput(out _, out string? outputError))
bool outputResolved = TryResolveOutput(out AbsoluteDirectoryPath? output, out string? outputError);
if (!outputResolved)
{
errors.Add(outputError);
errors.Add(outputError!);
}

// Every output is a .png named after its input, so writing into the input directory replaces
// any .png source with its own recoloured mask, and the original artwork is gone.
if (inputResolved && outputResolved && IsSameDirectory(input!, output!))
{
errors.Add($"--output must not be the --input directory, or the source icons are overwritten: {(string)output!}");
}

if (!ColorParser.TryParse(Color, out _))
Expand All @@ -73,6 +82,19 @@ internal bool Validate(out Collection<string> errors)
return errors.Count == 0;
}

/// <summary>
/// How file and directory names compare on this platform. Windows and macOS file systems are case
/// insensitive by default, so <c>Icons</c> and <c>icons</c> name the same directory there.
/// </summary>
internal static StringComparison PathComparison { get; } =
OperatingSystem.IsWindows() || OperatingSystem.IsMacOS() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;

private static bool IsSameDirectory(AbsoluteDirectoryPath first, AbsoluteDirectoryPath second) =>
string.Equals(
Path.TrimEndingDirectorySeparator((string)first),
Path.TrimEndingDirectorySeparator((string)second),
PathComparison);

/// <summary>
/// Resolves <see cref="InputPath"/> to an absolute directory that must already exist.
/// </summary>
Expand Down
16 changes: 15 additions & 1 deletion IconHelper/IconHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ internal static BatchResult ProcessDirectory(Arguments args, Color color)

int processed = 0;
int failed = 0;

// Output name to the input that produced it. Every output is <base name>.png, so inputs that
// share a base name (save.png, save.bmp) map to one file, and writing both would leave only
// the last while counting two.
Dictionary<string, string> written = new(StringComparer.FromComparison(Arguments.PathComparison));
System.Collections.ObjectModel.Collection<string> files = Directory.GetFiles(inputDirectory, "*").ToCollection();
foreach (string? file in files)
{
Expand All @@ -125,6 +130,14 @@ internal static BatchResult ProcessDirectory(Arguments args, Color color)
continue;
}

string outputName = $"{Path.GetFileNameWithoutExtension(file)}.png";
if (written.TryGetValue(outputName, out string? earlierInput))
{
Console.WriteLine($"Failed to process {file}: its output {outputName} was already written from {earlierInput}.");
failed++;
continue;
}

try
{
Console.WriteLine($"Processing {file}...");
Expand All @@ -135,10 +148,11 @@ internal static BatchResult ProcessDirectory(Arguments args, Color color)
// Always write a .png extension, since the encoder always writes PNG data. FileName
// rejects anything carrying a directory separator, and the / operator composes the
// two into an absolute file path.
FileName outputFileName = FileName.Create<FileName>($"{Path.GetFileNameWithoutExtension(file)}.png");
FileName outputFileName = FileName.Create<FileName>(outputName);
AbsoluteFilePath outputFilePath = outputDirectory / outputFileName;

image.SaveAsPng(outputFilePath, Encoder);
written.Add(outputName, file);
processed++;
}
catch (Exception e)
Expand Down
6 changes: 4 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,10 @@ for how many succeeded.
## Notes and Limitations

- Output is always PNG, and the extension is rewritten to match, so `logo.jpg` becomes `logo.png`. If
the input directory holds two files with the same base name but different extensions, the later one
overwrites the earlier.
the input directory holds two files with the same base name but different extensions, the first one
processed is written and the other is reported as failed, so the run exits `2`. Rename one of them.
- `--output` must not be the `--input` directory, because the outputs would replace the `.png`
sources. That is rejected with exit code `1`.
- Input formats are whatever ImageSharp can decode (PNG, JPEG, BMP, GIF, TGA, TIFF, WebP, PBM, QOI).
Vector formats such as SVG are not supported.
- The tool only ever shrinks artwork. Passing a `--size` larger than the source icon leaves it at its
Expand Down
Loading