Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Add Extract methods to Icon #8929

Closed
JeremyKuhne opened this issue Mar 31, 2023 · 24 comments · Fixed by #8983
Closed

[API Proposal]: Add Extract methods to Icon #8929

JeremyKuhne opened this issue Mar 31, 2023 · 24 comments · Fixed by #8983
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented area-System.Drawing System.Drawing issues blocking Used by the API Review Board
Milestone

Comments

@JeremyKuhne
Copy link
Member

JeremyKuhne commented Mar 31, 2023

Background and motivation

System.Drawing.Icon has methods for creating icons from managed resources and associated native icons. It doesn't, however, allow you to get all native resource icons, or always specify the size that you require.

The Icon constructors that take a path / stream only support .ico files. They do allow choosing a particular size, but also try to pick a color depth based on the current display settings. Color depth matching is of limited use and these constructors are costly as they manually load the entire file, parsing it to create the "best-fit" icon. The full stream is kept in memory (this is particularly painful if you're trying to load smaller icons).

System.Drawing.Icon.ExtractAssociatedIcon() has some overlap with these new APIs. It will load the first icon from the file it's given or fall back on whatever executable the file is associated with. The API used for this doesn't allow specifying a size. (We could spin our own by calling FindExecutable if we find a need in the future.)

API Proposal

namespace System.Drawing;

public class Icon
{
    // The existing Extract method.
    public static Icon ExtractAssociatedIcon(string! filePath);

    // The following work with .ico files as well as PE files (.exe, .dll, etc.). The id is an index when positive or a resource id when negative.

    // Retrieves the specified icon at the current system icon size setting (large by default).
    public static Icon? ExtractIcon(string! filePath, int id, bool smallIcon = false);

    // Allows retrieving the specified icon at a specific size (say 32x32). Icon sizes are always square.
    public static Icon? ExtractIcon(string! filePath, int id, ushort size);

    // Gets the icon count for the specified file.
    public static int GetIconCount(string! filePath);
}

public static class SystemIcons
{
    // Existing.
    public static unsafe Icon GetStockIcon(StockIconId stockIcon, StockIconOptions options = StockIconOptions.Default);

    // New. When asking for an explicit size (as opposed to just large/small) other options aren't available.
    public static unsafe Icon GetStockIcon(StockIconId stockIcon, ushort size);
}

API Usage

// Get the first icon in regedit.exe at 32x32 size.
using Icon icon = Icon.ExtractIcon("regedit.exe", id: 0, size: 32);

// Get the icon from devenv.exe with the 1200 resource id, at the default large size
using Icon icon = Icon.ExtractIcon("devenv.exe", id: -1200);

Alternative Designs

Could potentially add additional constructors to Icon, but behavior differences between string overloads would probably be confusing. For example, as the entire source file is not copied, resizing via Copy is scaled, instead of reparsed.

Risks

Nothing notable.

Notes

  • This API will scale to the requested size from available sizes. We'll use SHGetStockIconInfo- I believe it tries to scale down from larger sizes.
  • We will not track the original source or update the copy constructors. When the full data is kept in the other APIs the original data is parsed again to try and find a matching size when using the copy constructors. With Icons loaded through these APIs you'll just get the current backing bitmap scaled if you change sizes with the copy constructors (as you would with HICON constructed Icons and ExtractAssociatedIcon).
@JeremyKuhne JeremyKuhne added api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation untriaged The team needs to look at this issue in the next triage area-System.Drawing System.Drawing issues labels Mar 31, 2023
@JeremyKuhne JeremyKuhne added this to the .NET 8.0 milestone Mar 31, 2023
@JeremyKuhne
Copy link
Member Author

#8905, #8918, cc: @elachlan

@JeremyKuhne
Copy link
Member Author

Example of loading regedit.exe's icon explicitly at 256x256:

image

@JeremyKuhne
Copy link
Member Author

And the application stock icon at 256x256:

image

@elachlan
Copy link
Contributor

In the example API usage the filePath is not a full path. Will it find the executable or is it shortened for the example?

It will load the first icon from the file it's given or fall back on whatever executable the file is associated with.
Cool!

So presumably to get the icon of the current running application:

// get app icon
using Icon icon = Icon.ExtractIcon(Environment.ProcessPath, id: 0, size: 32);

@JeremyKuhne
Copy link
Member Author

It will load the first icon from the file it's given or fall back on whatever executable the file is associated with.

That's for the existing ExtractAssociatedIcon.

So presumably to get the icon of the current running application:

Yes, if it has one. You need to check for null and use some other icon if one isn't embedded.

@elachlan
Copy link
Contributor

I like this plan. So we don't need any new PInvokes? Have we migrated System.Drawing to CsWin32 yet?

@JeremyKuhne
Copy link
Member Author

I like this plan. So we don't need any new PInvokes? Have we migrated System.Drawing to CsWin32 yet?

Just a couple of new ones. We can't migrate System.Drawing to CsWin32 until the Win32 metadata includes the gdiplusflat header. There is an active issue for it, I haven't had time to try and get it working myself. There is also an issue with some of the shell APIs technically being platform specific. For our purposes we can use any definition, but there is no way to force pick one for AnyCPU in CsWin32.

@KlausLoeffelmann
Copy link
Member

Love it!

@harborsiem
Copy link

@JeremyKuhne : Did you think about an information for the possible Icon sizes? The information about the sizes are inside of a MultiIcon file in the iconheader.

// Gets the icon sizes for the specified file.
    public static int[] GetIconSizes(string! filePath);

There is a very old interesting article in codeproject.com for MultiIcons with sources. See MultiIcons.
I have done some bugfixes to the library in a Github project

@JeremyKuhne
Copy link
Member Author

Did you think about an information for the possible Icon sizes?

@harborsiem I did. I didn't address it here as I'd want to prototype it to make sure the proposed API is solid. I believe it would have to be hand-spun to match the semantics of the proposed APIs above. While we do have existing code for cruising the raw data for info, we'd need to handle resource id vs index specification. I'm guessing it is a fair amount of work and at the moment I don't have cycles. I'm happy to assist if someone else has some time to create another API request and do some prototyping as a part of it.

@JeremyKuhne JeremyKuhne added api-approved (4) API was approved in API review, it can be implemented api-ready-for-review (2) API is ready for formal API review; applied by the issue owner blocking Used by the API Review Board and removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation untriaged The team needs to look at this issue in the next triage api-approved (4) API was approved in API review, it can be implemented labels Apr 5, 2023
@JeremyKuhne JeremyKuhne changed the title API Proposal: Add Extract methods to Icon [API Proposal]: Add Extract methods to Icon Apr 5, 2023
@elachlan
Copy link
Contributor

@JeremyKuhne when is the next api review meeting?

@JeremyKuhne
Copy link
Member Author

@elachlan Hopefully tomorrow

@terrajobst
Copy link
Member

terrajobst commented Apr 11, 2023

Video

  • size should be of type int
  • Let's not expose GetIconCount(), folks who want to enumerate can use a while-loop plus a null check
    • We'd like to add a better API that returns all icons later, having a count method encourages a less performant pattern because enumeration repeatedly opens and closes the provided file.
namespace System.Drawing;

public partial class Icon
{
    // Existing:
    // public static Icon ExtractAssociatedIcon(string filePath);
    public static Icon? ExtractIcon(string filePath, int id, bool smallIcon = false);
    public static Icon? ExtractIcon(string filePath, int id, int size);
}

public partial class SystemIcons
{
    // Existing:
    // public static Icon GetStockIcon(StockIconId stockIcon, StockIconOptions options = StockIconOptions.Default);
    public static Icon GetStockIcon(StockIconId stockIcon, int size);
}

@terrajobst terrajobst added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Apr 11, 2023
@elachlan
Copy link
Contributor

@JeremyKuhne what PInvoke did you want to use?

@elachlan
Copy link
Contributor

elachlan commented Apr 12, 2023

According to the docs (https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-extracticonexa), we might be able to extract an array of icons? Are the docs wrong?

[out] phiconLarge

Type: HICON*

Pointer to an array of icon handles that receives handles to the large icons extracted from the file. If this parameter is NULL, no large icons are extracted from the file.

[out] phiconSmall

Type: HICON*

Pointer to an array of icon handles that receives handles to the small icons extracted from the file. If this parameter is NULL, no small icons are extracted from the file.

Edit: example usage, its possible. - http://www.jasinskionline.com/windowsapi/ref/e/extracticonex.html

@elachlan
Copy link
Contributor

This looks like it works. I only tested with one icon though.

internal static unsafe Icon[]? ExtractSmallIcons(string? lpszFile)
        {
            if (string.IsNullOrWhiteSpace(lpszFile))
            {
                return null;
            }

            HICON[] iconSmall;
            uint readIconCount = 0;
            fixed (char* lpszFileLocal = lpszFile)
            {
                uint numIcons = ExtractIconEx(lpszFile: lpszFileLocal, nIconIndex: -1, nIcons: 0);
                iconSmall = new HICON[numIcons];
                fixed (HICON* phiconSmall = iconSmall)
                {
                    readIconCount = ExtractIconEx(lpszFile: lpszFileLocal, nIconIndex: 0, phiconLarge: null, phiconSmall: phiconSmall, nIcons: numIcons);
                }
            }

            if (readIconCount == 0)
            {
                return null;
            }

            Icon[] iconArray = new Icon[readIconCount];
            for (int i = 0; i < readIconCount; i++)
            {
                HICON icon = iconSmall[i];
                if (!icon.IsNull)
                {
                    iconArray[i] = (Icon)Icon.FromHandle(icon).Clone();
                }
            }

            return iconArray;
        }

@JeremyKuhne
Copy link
Member Author

@elachlan I've prototyped this one already. I was planning to implement this tomorrow.

@elachlan
Copy link
Contributor

To be clear, my implementation is to get an array of icons without looping.

@JeremyKuhne
Copy link
Member Author

@elachlan The difficulty with that API is that there is no way to specify the icon size. You will be able to iterate through with this proposal, I'll have a test that validates that.

@elachlan
Copy link
Contributor

I was attempting to solve the issue raised by the API review. Which was the file being continuously opened and closed for each icon if you want a list of icons.

@JeremyKuhne
Copy link
Member Author

I was attempting to solve the issue raised by the API review. Which was the file being continuously opened and closed for each icon if you want a list of icons.

Understood. It isn't a terrible thing that the handle gets reopened multiple times. There is IO caching in the OS and it is probably a pretty rare scenario where users code is going to be significantly impacted by the overhead of multiple calls.

JeremyKuhne added a commit to JeremyKuhne/winforms that referenced this issue Apr 12, 2023
This adds Icon.Extracticon to allow extracting icons from icon files and native resources from PE files at any specified resolution.

Also adds an overload to SystemIcons.GetStockIcon() to get a specified size.

Fixes dotnet#8929
@ghost ghost added the 🚧 work in progress Work that is current in progress label Apr 12, 2023
JeremyKuhne added a commit that referenced this issue Apr 13, 2023
This adds Icon.Extracticon to allow extracting icons from icon files and native resources from PE files at any specified resolution.

Also adds an overload to SystemIcons.GetStockIcon() to get a specified size.

Fixes #8929
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Apr 13, 2023
@ghost ghost removed this from the .NET 8.0 milestone Apr 13, 2023
@Nora-Zhou01
Copy link
Member

Verified this issue in the latest .NET 8.0 Build: 8.0.100-preview4.23330.11, We can see that the new Icon.ExtractIcon method has been added.
dgdfgfdgf

@MelonWang1
Copy link
Contributor

Verified it in VS insertion PR with .Net 8 Preview 4 test pass build: 8.0.0-preview.4.23253.2, the Icon.ExtractIcon method has been added. Same test result as above.

@MelonWang1 MelonWang1 added this to the 8.0 Preview4 milestone May 5, 2023
@DJm00n
Copy link
Contributor

DJm00n commented May 20, 2023

Some missing APIs that I can think of:

  • return icon count in a file (currently doable with a loop as @terrajobst suggested)
  • return icon resource ids in a EXE/DLL file
  • return all icon sizes by number and/or resource id from a EXE/DLL/ICO file

@JeremyKuhne Is any work planned in this direction?

@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved (4) API was approved in API review, it can be implemented area-System.Drawing System.Drawing issues blocking Used by the API Review Board
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants