Skip to content

[codex] Fix PNG IconSource conversion#232

Draft
HavenDV wants to merge 1 commit into
masterfrom
codex/fix-208-png-iconsource
Draft

[codex] Fix PNG IconSource conversion#232
HavenDV wants to merge 1 commit into
masterfrom
codex/fix-208-png-iconsource

Conversation

@HavenDV
Copy link
Copy Markdown
Owner

@HavenDV HavenDV commented Apr 11, 2026

Summary

  • normalize icon streams before constructing System.Drawing.Icon
  • convert raw PNG image data to ICO data for tray icon creation
  • cover both sync and async IconSource paths through the shared stream helper

Why

TaskbarIcon.OnIconSourceChanged uses ToIconAsync(), which previously passed raw PNG streams into System.Drawing.Icon. That works for .ico content but throws for BitmapImage sources backed by PNG files, which is the failure reported in #208.

Validation

  • dotnet build src/libs/H.NotifyIcon/H.NotifyIcon.csproj --configuration Release /p:GeneratePackageOnBuild=false
  • dotnet build src/libs/H.NotifyIcon.Wpf/H.NotifyIcon.Wpf.csproj --configuration Release /p:GeneratePackageOnBuild=false

Fixes #208

@claude
Copy link
Copy Markdown

claude Bot commented Apr 11, 2026

Code Review

This PR correctly addresses issue #208 where raw PNG streams were passed directly to new Icon(stream, iconSize), which only accepts ICO format.

What works well

  • The PNG magic-byte detection (IsPng) is correct — the 8-byte signature 89 50 4E 47 0D 0A 1A 0A matches the PNG spec exactly.
  • Using using var iconStream = stream.ToIconStream() is safe: new Icon(iconStream, iconSize) reads and copies the stream data during construction, so disposing the MemoryStream wrapper afterwards is fine.
  • The fix is appropriately scoped — the ToBitmap path is correctly left alone because System.Drawing.Bitmap natively handles PNG streams.

Issues to address

1. Width/Height overflow for images > 255 px (medium)

In PngToIcoConverter.ConvertPngToIco (pre-existing, but surfaced by this PR):

outStream.WriteByte((byte)metadata.Width);   // silently wraps on > 255
outStream.WriteByte((byte)metadata.Height);

The ICO directory entry encodes width/height as a single byte where 0 means 256. For any image wider or taller than 255 px the cast wraps silently:

Actual size Stored byte Interpreted as
256 0 256 ✓
257 1 1 ✗
512 0 256 ✗

Windows typically uses the embedded PNG dimensions rather than the directory-entry fields when rendering, so many cases will still render correctly — but tools that strictly parse the header will mis-report dimensions. Consider using 0 for the "256" special case:

outStream.WriteByte(metadata.Width >= 256 ? (byte)0 : (byte)metadata.Width);
outStream.WriteByte(metadata.Height >= 256 ? (byte)0 : (byte)metadata.Height);

2. Stream position not reset (low, but defensive)

ToIconStream calls stream.CopyTo(buffer) starting from the stream's current position. If a caller accidentally passes a partially-read stream the data will be truncated silently. A single guard at the top would prevent that class of bug:

if (stream.CanSeek)
    stream.Position = 0;

3. [SupportedOSPlatform] attribute inconsistency (nit)

ToSmallIcon is decorated with windows5.0 while ToIconStream uses windows. Either is fine, but they should be consistent across the file.

4. Redundant null-check (nit)

The if (stream == null) throw new ArgumentNullException guard inside the private ToIconStream helper is unreachable — the only call site (ToSmallIcon) does not allow a null stream. You can drop it, or replace it with ArgumentNullException.ThrowIfNull(stream) for the modern API style used in .NET libraries.


No issues with

  • The MemoryStream(iconData, writable: false) return — clearly signals the stream is read-only.
  • buffer.ToArray() after CopyTo — always returns the full backing array regardless of stream position.
  • The SkiaSharp path (Utilities/SkiaSharp/StreamExtensions.cs) is untouched, which is correct since SKBitmap.Decode handles PNG natively.

Overall this is a solid fix for a real crash. The width/height overflow in PngToIcoConverter is the only item worth fixing before merge; the rest are nits.

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.

Setting IconSource doesn't convert png to icon

1 participant