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

SystemIcons outdated? #8842

Closed
Rand-Random opened this issue Mar 23, 2022 · 20 comments
Closed

SystemIcons outdated? #8842

Rand-Random opened this issue Mar 23, 2022 · 20 comments
Labels
area-System.Drawing System.Drawing issues

Comments

@Rand-Random
Copy link

Rand-Random commented Mar 23, 2022

Is the SystemIcons outdated? Should the SystemIcons return the shell icons?

As described here:
https://stackoverflow.com/questions/24257506/how-can-i-get-messagebox-icons-in-windows-8-1
SHGetStockIconInfo vs LoadIcon

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged The team needs to look at this issue in the next triage label Mar 23, 2022
@Rand-Random
Copy link
Author

Rand-Random commented Mar 23, 2022

@dotnet/area-system-drawing

@danmoseley
Copy link
Member

@RussKie does Winforms require such icons?

@filipnavara filipnavara added the area-System.Drawing System.Drawing issues label Mar 23, 2022
@ghost
Copy link

ghost commented Mar 23, 2022

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

Issue Details

Is the SystemIcons outdated? Should the SystemIcons return the shell icons?

As described here:
https://stackoverflow.com/questions/24257506/how-can-i-get-messagebox-icons-in-windows-8-1
SHGetStockIconInfo vs LoadIcon

Author: Rand-Random
Assignees: -
Labels:

area-System.Drawing, untriaged

Milestone: -

@RussKie
Copy link
Member

RussKie commented Mar 24, 2022

does Winforms require such icons?

I am not aware of any asks in the Windows Forms SDK for these.
And in general the Windows Forms SDK tries to take what Windows provides.

@Rand-Random
Copy link
Author

@RussKie
so I assume this means, that yes SystemIcons are outdated, and it would be safe to update them?

@RussKie
Copy link
Member

RussKie commented Mar 24, 2022

Windows Forms SDK doesn't use SystemIcons at all - we don't have any of the referred interop definitions, and we call Windows MessageBox API directly.

[DllImport(Libraries.User32, ExactSpelling = true, CharSet = CharSet.Unicode)]
public static extern ID MessageBoxW(IntPtr hWnd, string lpText, string lpCaption, MB uType);
public static ID MessageBoxW(HandleRef hWnd, string lpText, string lpCaption, MB uType)
{
ID result = MessageBoxW(hWnd.Handle, lpText, lpCaption, uType);
GC.KeepAlive(hWnd.Wrapper);
return result;
}

private static DialogResult ShowCore(
IWin32Window owner,
string text,
string caption,
MessageBoxButtons buttons,
MessageBoxIcon icon,
MessageBoxDefaultButton defaultButton,
MessageBoxOptions options,
bool showHelp)
{
MB style = GetMessageBoxStyle(owner, buttons, icon, defaultButton, options, showHelp);
IntPtr handle = IntPtr.Zero;
if (showHelp || ((options & (MessageBoxOptions.ServiceNotification | MessageBoxOptions.DefaultDesktopOnly)) == 0))
{
if (owner is null)
{
handle = GetActiveWindow();
}
else
{
handle = Control.GetSafeHandle(owner);
}
}
IntPtr userCookie = IntPtr.Zero;
if (Application.UseVisualStyles)
{
// CLR4.0 or later, shell32.dll needs to be loaded explicitly.
if (Kernel32.GetModuleHandleW(Libraries.Shell32) == IntPtr.Zero)
{
if (Kernel32.LoadLibraryFromSystemPathIfAvailable(Libraries.Shell32) == IntPtr.Zero)
{
int lastWin32Error = Marshal.GetLastWin32Error();
throw new Win32Exception(lastWin32Error, string.Format(SR.LoadDLLError, Libraries.Shell32));
}
}
// Activate theming scope to get theming for controls at design time and when hosted in browser.
// NOTE: If a theming context is already active, this call is very fast, so shouldn't be a perf issue.
userCookie = ThemingScope.Activate(Application.UseVisualStyles);
}
Application.BeginModalMessageLoop();
try
{
return (DialogResult)MessageBoxW(handle, text, caption, style);
}
finally
{
Application.EndModalMessageLoop();
ThemingScope.Deactivate(userCookie);
// Right after the dialog box is closed, Windows sends WM_SETFOCUS back to the previously active control
// but since we have disabled this thread main window the message is lost. So we have to send it again after
// we enable the main window.
User32.SendMessageW(handle, User32.WM.SETFOCUS);
GC.KeepAlive(owner);
}
}

@danmoseley
Copy link
Member

@Rand-Random we're just calling LoadIcon. Do you have a pointer to what you propose we should do instead (or as well)?

@Rand-Random
Copy link
Author

Rand-Random commented Mar 24, 2022

@danmoseley - calling SHGetStockIconInfo seems to get more up-to-date icons, so that would be my proposal

if the switch is feasible I would also recommend extending systemicons with all possibilities of SHGetStockIconInfo as described here: https://docs.microsoft.com/en-us/windows/win32/api/shellapi/ne-shellapi-shstockiconid

though I can’t say if switching from LoadIcon to SHGetStockIconInfo is the better approach than to update LoadIcon returning up-to-date icons

as a side note:
SHGetStockIconInfo isn’t my idea, it origins from the post on stackoverflow I referred to in my original post

@danmoseley
Copy link
Member

There's a lot of such icons: https://docs.microsoft.com/en-us/windows/win32/api/shellapi/ne-shellapi-shstockiconid
It would be good to hear from others whether they need such icons, and which, and whether they should replace the existing ones returned. What do those look like today?

@carlossanlop carlossanlop added this to the Future milestone Jun 1, 2022
@ViktorHofer ViktorHofer removed the untriaged The team needs to look at this issue in the next triage label Jun 1, 2022
@ghost
Copy link

ghost commented Jun 1, 2022

This issue has been marked needs-author-action and may be missing some important information.

@Rand-Random
Copy link
Author

@ViktorHofer
What action is required on my part?

@ghost ghost added the needs-further-triage label Jun 1, 2022
@ViktorHofer
Copy link
Member

Referring to @danmoseley's previous comment:

It would be good to hear from others whether they need such icons, and which, and whether they should replace the existing ones returned. What do those look like today?

A good next step would be to discuss if these icons are required and loop some other people in as well.

@ViktorHofer
Copy link
Member

cc @JeremyKuhne

@JeremyKuhne
Copy link
Member

I think we should expose a new StockIcons to get at the set that are exposed via SHGetStockIconInfo. UI modernization is a tenet and this would help enable this for the WinForms runtime and WinForms apps. There isn't a good way to update the existing ones as some users will undoubtedly want the current behavior.

Something like the following:

namespace System.Drrawing;

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

public enum StockIcons
{
    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,
    // ...
}

Here is a current related WinForms runtime issue: #7390

@Rand-Random
Copy link
Author

@JeremyKuhne
Sorry, for the late reply.

I would argue against

There isn't a good way to update the existing ones as some users will undoubtedly want the current behavior.

Because this can be said, about every time Windows decided to update icons.

Lets assume Windows did update the Icons for every operating system, we would already have 11 different Icon versions.
To pick up your argument you could argue that someone would like to have Windows XP Icons on Windows 11.

I would argue against and say that "SystemIcons" should ALWAYS return the icons of the current System, and not Icons from an older system.

If you want to support Icons from older Systems you should fully commit to the idea and have a public API where you can define which SystemIcon you want and from which System.

eg.

Icon GetSystemIcons(SystemIcon systemIcon, string systemInfo);

//examples
var xpIcon = GetSystemIcons(SystemIcon.Question, "Window XP");
var sevenIcon = GetSystemIcons(SystemIcon.Question, "Window 7");

//with cross plattform in mind
var ubuntuIcon = GetSystemIcons(SystemIcon.Question, "Ubuntu 18");

@JeremyKuhne
Copy link
Member

Because this can be said, about every time Windows decided to update icons.

@Rand-Random yes, the difference here is that we never updated the existing ones. That would be the behavior change and the reason for the new API that always gets the current icons. If you use the new API it will be documented as being "current". There are also significantly more icons available, which also helps justify a new API.

@ViktorHofer two things here:

  1. We should probably have the new API return cached icons as well
  2. WinForms will use this API once it is available

@JeremyKuhne
Copy link
Member

Created an API proposal. #8802. @ViktorHofer Once it is approved I'll represent in API review. Once we've moved System.Drawing to the WinForms repo I'll implement unless someone else is particularly eager. :)

@JeremyKuhne
Copy link
Member

Resolved with #8802 & #8878

@DJm00n
Copy link
Contributor

DJm00n commented May 4, 2023

There is another approach to get the modern icons exist - so called "font icons":
https://learn.microsoft.com/en-us/windows/apps/design/style/segoe-fluent-icons-font

Segoe UI Symbol - Windows 8
Segoe MDL2 Assets - Windows 10
Segoe Fluent Icons - Windows 11

UPDATE:
Corresponding Microsoft.UI.Xaml.Controls.Symbol Enum and Microsoft.UI.Xaml.Controls.SymbolIcon Class in Windows App SDK.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 13, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Drawing System.Drawing issues
Projects
None yet
Development

No branches or pull requests

8 participants