Fix `ToolboxBitmapAttribute` to improve error tolerance and better support `PNG` based `ICO` embedded resources

[elachlan]

Changes to ToolboxBitmapAttribute were to avoid errors when a bitmap was expected but the icon file was a png based icon. The try catch method was used elsewhere in the class to avoid similar errors.

Used the method outlined in the linked issue and wrote the following python script to automate:

import os
import subprocess
import glob

# Get a list of all .ico files in the current directory
ico_files = [f for f in os.listdir('.') if f.endswith('.ico')]

# Process each .ico file
for ico_file in ico_files:
    try:
        # Convert the .ico file to .png files
        base_name = ico_file.replace('.ico', '')
        subprocess.run(['magick', ico_file, f'{base_name}-%d.png'], check=True)

        # Get the list of created .png files
        png_files = glob.glob(f'{base_name}-*.png')

        if not png_files:
            print(f"No .png files were created for {ico_file}. Skipping.")
            continue

        # Overwrite the original .ico file with the new one
        icotool_command = ['icotool', '-c', '-o', ico_file] + [f'-r{png}' for png in png_files]
        subprocess.run(icotool_command, check=True)

        # Clean up the temporary .png files
        for png in png_files:
            if os.path.isfile(png):
                os.remove(png)

    except subprocess.CalledProcessError:
        print(f"Error processing {ico_file}. Skipping.")
    except Exception as e:
        print(f"Unexpected error processing {ico_file}: {e}. Skipping.")

print("Processing complete.")

Microsoft Reviewers: Open in CodeFlow

[elachlan]

@MichalStrehovsky here is a PR with all the ico files converted. It resulted in errors in ToolboxBitmapAttribute, this pr just adds try catch to swallow those as there was a fallback mechanism already in place. But I suspect we can improve the code to check the file header to check its format to avoid the exception.

Winforms team might be able to help there.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 82.60870% with 12 lines in your changes missing coverage. Please review.

Project coverage is 74.36247%. Comparing base (0aa3a4d) to head (3da25ab). Report is 110 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #11375 +/- ## =================================================== + Coverage 74.26256% 74.36247% +0.09991% =================================================== Files 3025 3028 +3 Lines 626861 627537 +676 Branches 46742 46765 +23 =================================================== + Hits 465523 466652 +1129 + Misses 157993 157531 -462 - Partials 3345 3354 +9 ``` | [Flag](https://app.codecov.io/gh/dotnet/winforms/pull/11375/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | Coverage Δ | | |---|---|---| | [Debug](https://app.codecov.io/gh/dotnet/winforms/pull/11375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `74.36247% <82.60870%> (+0.09991%)` | :arrow_up: | | [integration](https://app.codecov.io/gh/dotnet/winforms/pull/11375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `17.98035% <0.00000%> (-0.01558%)` | :arrow_down: | | [production](https://app.codecov.io/gh/dotnet/winforms/pull/11375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `47.16010% <82.60870%> (+0.17419%)` | :arrow_up: | | [test](https://app.codecov.io/gh/dotnet/winforms/pull/11375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `96.98684% <ø> (ø)` | | | [unit](https://app.codecov.io/gh/dotnet/winforms/pull/11375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `44.14017% <82.60870%> (+0.17398%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet#carryforward-flags-in-the-pull-request-comment) to find out more.

[elachlan]

@merriemcgaw Are embedded resources considered a part of the public API? Would a change like this be a breaking change?

[RussKie]

Are embedded resources considered a part of the public API? Would a change like this be a breaking change?

No, it shouldn't be considered a breaking change. These may be helpful:

• https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-definitions.md
• https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-changes.md

[MichalStrehovsky]

It resulted in errors in ToolboxBitmapAttribute, this pr just adds try catch to swallow those as there was a fallback mechanism already in place. But I suspect we can improve the code to check the file header to check its format to avoid the exception.

What were the errors? It feels like the code this is touching should already handle PNG. Icon.ToBitmap used to throw but support for this was added long time ago (there was even a breaking change notice because the compat bar on .NET Framework is so high that even "something throwing a non-sensical exception" to "something working" is a breaking change, but fortunately we don't have to worry about that here).

[elachlan]

@LeafShi1 / @Olina-Zhang can your team please test this? The main test would be to open the new icon and compare it to the old icon. Also if you can trigger the CI to run the tests again that would be great.

[LeafShi1]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[lonitra]

Thanks for taking this on @elachlan! I did a look through and compared the icons before/after change and AFAICT visually the icons look the same. I did, however, spot some differences when comparing sizes and perhaps some of them are expected:

1. There were some icons which had a change in only the bit size e.g. originally 32 bits -> 4 bits. Is this expected? I saw this for icons: colorDialog.ico, RichEdit.ico, RichTextBox.ico, ShieldIcon.ico, Close_left.ico, Open_left.ico, DummyNodeImage.ico
2. Some icons had extra sizes that the original did not have e.g. 148x148 did not exist, but now does. I saw this for icons: 256_1.ico and 256_2.ico
3. Some icons had completely different sizes from the original e.g. original was 33x61 but new is 33x33. I saw this for icons: overflowButton.ico, ToolBarGrip.ico, OrderImages.ico

[elachlan]

@lonitra thank you so much for reviewing these. How are you checking the bit depth and sizing?

[lonitra]

@lonitra thank you so much for reviewing these. How are you checking the bit depth and sizing?

I had opened the icons in Visual Studio and there was a bar on the side that showed the sizing and bits

[elachlan]

Thanks! When I run the ImageMagick Identify command I get different results. It says RichEdit.ico is strangley 8bit before and after the conversion.

here is what it looks like from my VS Before: After:

VS seems to have issues displaying the original?

256_1.ico is strange, 148x148 and 47x47 look corrupted when viewed in VS for me. The meta data exists in the original ico file, but with zero file size. 202x123 has a zero size but somehow works in the original and converted versions.

Before:

256_1.ico[0] PNG 505x308 505x308+0+0 8-bit sRGB 13030B 0.000u 0:00.009
256_1.ico[1] ICO 202x123 202x123+0+0 8-bit sRGB 0.000u 0:00.009
256_1.ico[2] PNG 707x431 707x431+0+0 8-bit sRGB 16711B 0.000u 0:00.005
256_1.ico[3] ICO 148x148 148x148+0+0 8-bit sRGB 0.000u 0:00.004
256_1.ico[4] PNG 606x370 606x370+0+0 8-bit sRGB 13120B 0.000u 0:00.002
256_1.ico[5] ICO 47x47 47x47+0+0 8-bit sRGB 0.000u 0:00.002
256_1.ico[6] PNG 808x492 808x492+0+0 8-bit sRGB 805702B 0.000u 0:00.000

After:

256_1.ico[0] PNG 505x308 505x308+0+0 8-bit sRGB 7339B 0.000u 0:00.004
256_1.ico[1] PNG 202x123 202x123+0+0 8-bit sRGB 2277B 0.000u 0:00.003
256_1.ico[2] PNG 707x431 707x431+0+0 8-bit sRGB 10520B 0.000u 0:00.003
256_1.ico[3] PNG 148x148 148x148+0+0 8-bit sRGB 1466B 0.000u 0:00.002
256_1.ico[4] PNG 606x370 606x370+0+0 8-bit sRGB 7745B 0.000u 0:00.001
256_1.ico[5] PNG 47x47 47x47+0+0 8-bit sRGB 449B 0.000u 0:00.001
256_1.ico[6] PNG 808x492 808x492+0+0 8-bit sRGB 40338B 0.000u 0:00.000

If I use Icon tool on the orginal:

--icon --index=1 --width=505 --height=308 --bit-depth=32 --palette-size=0
--icon --index=2 --width=202 --height=123 --bit-depth=32 --palette-size=0
--icon --index=3 --width=707 --height=431 --bit-depth=32 --palette-size=0
--icon --index=4 --width=404 --height=246 --bit-depth=32 --palette-size=0
--icon --index=5 --width=606 --height=370 --bit-depth=32 --palette-size=0
--icon --index=6 --width=303 --height=185 --bit-depth=32 --palette-size=0
--icon --index=7 --width=808 --height=492 --bit-depth=32 --palette-size=0

Edit: ImageMagick is misinterpreting the resolution, which is causing issues.

I'll do some more work on identifying making the conversion script more error tolerant and add some verification using icontool.

[elachlan]

I've raised an ImageMagick issue at: https://github.com/ImageMagick/ImageMagick/issues/7341

[elachlan]

icotool can extract some of the icons, but errors on a few. I'll put together a new script and push through the changes when I have it sorted.

[Olina-Zhang]

Hi @elachlan, we tested this PR by checking all ico files in your repo: Icon-Resize branch and Winforms repo: Main branch to compare them, found following result:

1. Some icons in your branch display more clearly than Winforms: main branch in Visual Studio, see following screenshot(they are not all we collected, just a part)
2. Some icons Size is changed(@lonitra mentioned in above)
3. Some icons Bit is changed(@lonitra mentioned in above)
4. Some icons display are changed with extra pixels around icons, see following screenshot(they are not all we collected, just a part)
5. An exception pops up when adding ToolStrip/MenuStrip/StatusStrip/ContextMenuStrip control from toolBox to form designer Log:

[elachlan]

@Olina-Zhang I think the designer issue will need to be looked at by the team. I don't have access to the source code for the designer.

ImageMagick are fixing one of the issues which should resolve some of the sizing corruption.

[elachlan]

@merriemcgaw changing the underlying BMP format to PNG in the ICO files is a breaking change if a user is creating a bitmap directly instead of first creating an Icon. Are we sure that changing resources is not a breaking change?

[Olina-Zhang]

@Olina-Zhang I think the designer issue will need to be looked at by the team. I don't have access to the source code for the designer.

If we didn't replace the binaries built from your PR, that issue doesn't repro.

[elachlan]

ImageMagick fixed the sizing issue. So now the icons have appropriate sizes.

The bit depth is changed on purpose by ImageMagick to save space when there are limited colors in an image.

The artifacts we are seeing look like they are caused by icotool. I am investigating how to use ImageMagick to combine the png files to the ico without converting back to bmp.

[elachlan]

Looks like ImageMagick might not support PNG in ICO files because of specification reasons: https://github.com/ImageMagick/ImageMagick/issues/2394

@MichalStrehovsky could you live with bitmap based ICO files? Bitmap: ToolStripDropDown.ico 54kb->22kb PNG: 54kb->5kb

[MichalStrehovsky]

Looks like ImageMagick might not support PNG in ICO files because of specification reasons: ImageMagick/ImageMagick#2394

@MichalStrehovsky could you live with bitmap based ICO files? Bitmap: ToolStripDropDown.ico 54kb->22kb PNG: 54kb->5kb

In a blank WinForms app, we currently have 48 icons that take over 51 kB each (plus some smaller ones I'm not counting). That's 2.4 MB. A blank WinForms app with AOT and a couple size savings options is 9.2 MB. So these icons are more than 25% of the blank app. If we make them 15% of the blank app, it's certainly an improvement. Still feels a bit excessive. In other places (ASP.NET, Blazor, etc.) we jump on and try to fix anything above 1%.

First thing I'm getting from this is that the VS icon designer cannot be trusted (it shows various artifacts in both before and after versions). I'd ignore the VS icon designer completely and focus on differences that matter - e.g. load the icons as a .NET Icon and save everything as a Bitmap. Compare the Bitmaps. Then look if we can fix the bug where someone tries to open an icon as a Bitmap (the bug you're working around in the code changes here) - can Bitmap detect trying to open an icon and instead open it as Icon and call ToBitmap on it (i.e. move the workaround from there to Bitmap - I assume that would fix the designer issue)?

If the above fails or we don't want to go in that direction - can we instead get the icons out of the app? Then it doesn't matter how big they are. E.g. are these Designer icons? Could they be moved to the Designer assembly? Or could we instruct trimming to drop them based on some condition? What are they used for?

15% leaves a lot on the table. Last week I converted one of my WinForms apps to Native AOT. It's a fully functional and useful app. These icons would still constitute 10% of the app even if we cut their size in half because the whole app size was a little over 10 MB.

[MichalStrehovsky]

Thinking about it more, even in the best case 4+% of the executable size would be icons. It might be the best to look for ways to trim these out of the app. A 4% size saving is still pretty interesting. Assuming these are actually not normally needed - understanding what they are used for would be the best.

[elachlan]

@Olina-Zhang could your team please report the VS artifact issue?

I'll whip up a quick program to do the icon to bitmap conversion and see what before/after renders out to.

With regards to trimming out resources, I am unsure how the trimmer works or how the compiler handles resources. Someone much more skilled them myself would need to investigate that.

[Olina-Zhang]

@Olina-Zhang could your team please report the VS artifact issue?

You mean this problem I listed: An exception pops up when adding ToolStrip/MenuStrip/StatusStrip/ContextMenuStrip control from toolBox to form designer? I'm not quite sure what you mean. Do you request a details about how to repro it or others?

[elachlan]

Sorry about that. Can you please report the issue when viewing the existing icons in visual studio? Visual Studio should display the icons properly.

[Olina-Zhang]

You mean open a Winforms designer bug in Winforms designer repo? That issue just reproduces when replaced these three binaries: System.Drawing.Common.dll, System.Windows.Forms.Design.dll and System.Windows.Forms.dll built from this PR, I am not sure if our testing is a valid validation.

[elachlan]

The icon display issue in visual studio. Not the exception. I am unsure if winforms designer handles viewing/editing icon files.

[Olina-Zhang]

Haha, I see the problem you're referring to. I will open an issue for Visual Studio.

[Olina-Zhang]

Opened an internal VS artifact issue: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2067266.

[MichalStrehovsky]

With regards to trimming out resources, I am unsure how the trimmer works or how the compiler handles resources. Someone much more skilled them myself would need to investigate that.

A couple options, not sure which one is applicable.

• The compiler doesn't analyze which embedded resources are needed, it will keep them all if an assembly is needed. This PR is changing all ico files in the repo. We mostly care about those that end up in System.Windows.Forms. Can some be moved to System.Windows.Forms.Design (normal apps don't reference that assembly)? What are the icons used for? What if we scope this PR down to just the icons from System.Windows.Forms?
• The compiler can accept a list of resource names to trim and then it will trim them, even if the assembly is used. This trimming can be made conditional based on some property. It again begs the question of what are the icons in System.Windows.Forma assembly used for - is there a theme - a feature that might not be needed by everyone?

[elachlan]

Did another quick test, these artifacts are VS specific and don't show if using a browser to display the ico file:

Additionally, if I convert the ico to bmp via .NET and open in VS it displays fine:

[KlausLoeffelmann]

@merriemcgaw changing the underlying BMP format to PNG in the ICO files is a breaking change if a user is creating a bitmap directly instead of first creating an Icon. Are we sure that changing resources is not a breaking change?

I am concerned, to be honest. @merriemcgaw.

[elachlan]

@KlausLoeffelmann I think we can adjust Bitmap to check the streams leading bytes to establish what file type it is then create the bitmap accordingly.