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 ability to get modern Windows icons to System.Drawing.SystemIcons. #8802

Closed
JeremyKuhne opened this issue Mar 10, 2023 · 11 comments · Fixed by #8878
Closed
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented area-System.Drawing System.Drawing issues
Milestone

Comments

@JeremyKuhne
Copy link
Member

JeremyKuhne commented Mar 10, 2023

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:

  • 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

@JeremyKuhne JeremyKuhne added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Mar 10, 2023
@ghost ghost added the untriaged The team needs to look at this issue in the next triage label Mar 10, 2023
@ghost
Copy link

ghost commented Mar 10, 2023

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 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

namespace System.Drawing;

public static class SystemIcons
{
    /// <summary>
    ///  Gets the specified stock shell icon.
    /// </summary>
    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

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: -

@JeremyKuhne JeremyKuhne added the blocking Used by the API Review Board label Mar 10, 2023
@JeremyKuhne JeremyKuhne self-assigned this Mar 10, 2023
@RussKie
Copy link
Member

RussKie commented Mar 10, 2023

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
Copy link
Member Author

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
Copy link
Contributor

koszeggy commented Mar 12, 2023

@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).

@ViktorHofer ViktorHofer removed the untriaged The team needs to look at this issue in the next triage label Mar 13, 2023
@JeremyKuhne
Copy link
Member Author

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.

@JeremyKuhne JeremyKuhne transferred this issue from dotnet/runtime Mar 14, 2023
@JeremyKuhne JeremyKuhne added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner and removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation labels Mar 14, 2023
@bartonjs
Copy link
Member

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,
    // ...
}

@bartonjs bartonjs 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 blocking Used by the API Review Board labels Mar 16, 2023
@JeremyKuhne JeremyKuhne added the area-System.Drawing System.Drawing issues label Mar 16, 2023
@KalleOlaviNiemitalo
Copy link

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
Copy link
Member Author

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 JeremyKuhne added this to the .NET 8.0 milestone Mar 16, 2023
@JeremyKuhne
Copy link
Member Author

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
Copy link
Member Author

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

JeremyKuhne added a commit to JeremyKuhne/winforms that referenced this issue Mar 21, 2023
@ghost ghost added the 🚧 work in progress Work that is current in progress label Mar 21, 2023
JeremyKuhne added a commit that referenced this issue Mar 21, 2023
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Mar 21, 2023
@ghost ghost removed this from the .NET 8.0 milestone Mar 21, 2023
@Amy-Li03
Copy link
Contributor

Amy-Li03 commented Mar 23, 2023

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.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2023
@merriemcgaw merriemcgaw added this to the 8.0 Preview4 milestone May 3, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants