dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.41k stars 984 forks source link

[API Proposal]: Add ability to get modern Windows icons to System.Drawing.SystemIcons. #8802

Closed JeremyKuhne closed 1 year ago

JeremyKuhne commented 1 year ago

Background and motivation

The existing SystemIcons come from an older API that has icons that don't get updated with the OS. The new API SHGetStockIconInfo is the way to get the current version of icons used in dialogs, etc.

Providing a new method in SystemIcons would allow getting updated versions of icons and allow access to a number of additional icons. It additionally allows getting small icon versions, highlighted versions, link versions, etc.

API Proposal

namespace System.Drawing;

public static class SystemIcons
{
    /// <summary>
    ///  Gets the specified stock shell icon.
    /// </summary>
    public static Icon GetStockIcon(StockIcon stockIcon);
    public static Icon GetStockIcon(StockIcon stockIcon, StockIconOptions options);
}

[Flags]
public enum StockIconOptions
{
    SmallIcon = 0x000000001,
    ShellIconSize = 0x000000004,
    LinkOverlay = 0x000008000,
    Selected = 0x000010000,
}

// Matching https://learn.microsoft.com/windows/win32/api/shellapi/ne-shellapi-shstockiconid with
// Pascal casing and no abbreviations ("Assoc" to "Associations"), will still keep acronyms (CD, not CompactDisc).
public enum StockIcon
{
    DocumentNoAssociation = 0,
    DocumentWithAssociation = 1,
    Application = 2,
    Folder = 3,
    OpenFolder = 4,
    Drive525 = 5,
    Drive35 = 6,
    DriveRemovable = 7,
    DriveFixed = 8,
    DriveNetwork = 9,
    DriveNetworkDisabled = 10,
    DriveCD = 11,
    DriveRAM = 12,
    World = 13,
    Server = 15,
    Printer = 16,
    // ...
}

API Usage

using Icon icon = SystemIcons.GetStockIcon(StockIcon.Shield);

Alternative Designs

We could update the existing methods to use the current icons but:

We could also put the static method in StockIcons, but that would make discoverability more difficult. Could potentially add to 'Icon'?

Risks

No response

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-drawing See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation The existing `SystemIcons` come from an older API that has icons that don't get updated with the OS. The new API [`SHGetStockIconInfo`](https://learn.microsoft.com/windows/win32/api/shellapi/nf-shellapi-shgetstockiconinfo) is the way to get the _current_ version of icons used in dialogs, etc. Providing a new method in `SystemIcons` would allow getting updated versions of icons and allow access to a number of additional icons. ### API Proposal ```csharp namespace System.Drawing; public static class SystemIcons { /// /// Gets the specified stock shell icon. /// public static Icon GetStockIcon(StockIcon stockIcon); } // Matching https://learn.microsoft.com/windows/win32/api/shellapi/ne-shellapi-shstockiconid with // Pascal casing and no abbreviations ("Assoc" to "Associations"), will still keep acronyms (CD, not CompactDisc). public enum StockIcon { DocumentNoAssociation = 0, DocumentWithAssociation = 1, Application = 2, Folder = 3, OpenFolder = 4, Drive525 = 5, Drive35 = 6, DriveRemovable = 7, DriveFixed = 8, DriveNetwork = 9, DriveNetworkDisabled = 10, DriveCD = 11, DriveRAM = 12, World = 13, Server = 15, Printer = 16, // ... } ``` ### API Usage ```csharp using Icon icon = SystemIcons.GetStockIcon(StockIcon.Shield); ``` ### Alternative Designs We could update the existing methods to use the current icons but: - Some applications will depend on the legacy icon look - There are over 90 stock icons (and growing) We could also put the static method in `StockIcons`, but that would make discoverability more difficult. Could potentially add to 'Icon'? ### Risks _No response_
Author: JeremyKuhne
Assignees: -
Labels: `api-suggestion`, `area-System.Drawing`
Milestone: -
RussKie commented 1 year ago

Overall it looks good. If there's a new icon, which isn't defined in StockIcon enum, one would still be able to use it. 👍

There are over 90 stock icons (and growing)

Are we planning to continue to add new values to StockIcon enum? Would it be a breaking change?

JeremyKuhne commented 1 year ago

Are we planning to continue to add new values to StockIcon enum? Would it be a breaking change?

As required, sure. I would not plan to block any requested value either.

koszeggy commented 1 year ago

@JeremyKuhne Quoting from the previous thread:

I'll implement unless someone else is particularly eager. :)

Feel free to get inspiration from here. Results on different OS versions are illustrated here (see System* properties).

Once we've moved System.Drawing to the WinForms repo

Is it already decided? Today System.Drawing can be used independently from WinForms and I think this should remain this way. And the platform-independent System.Drawing primitives such as Color, Rectangle, Size, etc. are part of .NET Standard 2.0 that can be used literally anywhere.

Anyway, until this feature gets into .NET my library is available here. May be useful also when targeting older frameworks or non-Windows platforms (though on non-Windows platforms it defaults to use some default icons).

JeremyKuhne commented 1 year ago

Is it already decided? Today System.Drawing can be used independently from WinForms and I think this should remain this way. And the platform-independent System.Drawing primitives such as Color, Rectangle, Size, etc. are part of .NET Standard 2.0 that can be used literally anywhere.

System.Drawing.Common will remain a separate package, it will just build from the Windows Forms repository and we (WinForms) will manage it. System.Drawing.Primitives (exchange types such as Point, Color, etc.) will continue to be built in the runtime repo.

bartonjs commented 1 year ago

We added a None=0 value to StockIconOptions.

Assuming that the shorter overload is passing StockIconOptions.None, consider collapsing into one overload with a defaulted parameter.

Consider renaming StockIcon to StockIconId, just in case there would be a future situation warranting something like public class StockIcon : Icon.

namespace System.Drawing;

public static class SystemIcons
{
    /// <summary>
    ///  Gets the specified stock shell icon.
    /// </summary>
    public static Icon GetStockIcon(StockIcon stockIcon);
    public static Icon GetStockIcon(StockIcon stockIcon, StockIconOptions options);
}

[Flags]
public enum StockIconOptions
{
    None = 0,
    SmallIcon = 0x000000001,
    ShellIconSize = 0x000000004,
    LinkOverlay = 0x000008000,
    Selected = 0x000010000,
}

// Matching https://learn.microsoft.com/windows/win32/api/shellapi/ne-shellapi-shstockiconid with
// Pascal casing and no abbreviations ("Assoc" to "Associations"), will still keep acronyms (CD, not CompactDisc).
public enum StockIcon
{
    DocumentNoAssociation = 0,
    DocumentWithAssociation = 1,
    Application = 2,
    Folder = 3,
    OpenFolder = 4,
    Drive525 = 5,
    Drive35 = 6,
    DriveRemovable = 7,
    DriveFixed = 8,
    DriveNetwork = 9,
    DriveNetworkDisabled = 10,
    DriveCD = 11,
    DriveRAM = 12,
    World = 13,
    Server = 15,
    Printer = 16,
    // ...
}
KalleOlaviNiemitalo commented 1 year ago

When documenting this, please make it clear whether the caller should eventually Dispose(). The SystemIcons.Application etc. properties cache the results and return Icon instances that ignore Dispose(). If GetStockIcon works differently, that will cause confusion.

JeremyKuhne commented 1 year ago

Discussed further with the API review board, I'll collapse to a single API with a default parameter and rename StockIconOptions.None to StockIconOptions.Default with a description of what that means.

JeremyKuhne commented 1 year ago

When documenting this, please make it clear whether the caller should eventually Dispose().

@KalleOlaviNiemitalo I will. Given the large number of icon types and options caching them doesn't seem plausible, so disposing them will be clearly called out.

JeremyKuhne commented 1 year ago

I've got the basic implementation in place, need to polish it up.

Amy-Li03 commented 1 year ago

Verified this on .NET 8.0 latest build: 8.0.100-preview.4.23179.4, saved the generated Icons using Icon.Save(), it behaves as expected.