dotnet / winforms

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

[API Proposal] Ability to set the default icon for Forms. #8905

Open stcpottdav opened 1 year ago

stcpottdav commented 1 year ago

Is your feature request related to a problem? Please describe

Currently the only way to set the default icon is via reflection. This is risky as it is looking at the internals of the object. It would be best to expose an API to set it.

Example of a way to set the icon currently:

typeof(Form).GetField("defaultIcon", BindingFlags.NonPublic | BindingFlags.Static).SetValue(null, SystemIcons.Shield);

Describe the solution you'd like and alternatives you've considered

namespace System.Windows.Forms;

public class Form
{
    /// <summary>
    ///  Gets / sets the default icon used for a <see cref="System.Windows.Forms.Form"/>.
    /// </summary>
    /// <exception cref="System.ArgumentNullException">value is null.</exception>
    public static System.Drawing.Icon DefaultIcon { get; set; }
}

Will this feature affect UI controls?

Designer should not have to change for this. It should not impact accessibility. It should not need to be localized.

elachlan commented 1 year ago

It would be cool if we can set it via the proj file via an xml tag as well.

Currently I think I just have a "base" form that all other forms inherit and the icon is set on that.

JeremyKuhne commented 1 year ago

I think this is a reasonable ask, but it probably should stay on Form as it only applies to it. If it is in Application it might be confused with the program icon. There is no reason to not have a getter either, imo.

namespace System.Windows.Forms;

public class Form
{
    /// <summary>
    ///  Gets / sets the default icon used for a <see cref="System.Windows.Forms.Form"/>.
    /// </summary>
    /// <exception cref="System.ArgumentNullException">value is null.</exception>
    public static System.Drawing.Icon DefaultIcon { get; set; }
}

This would, of course, have the existing default icon as the default value.

@stcpottdav does this design meet your requirements? @elachlan, pulling from the project could be looked at as a separate thing, but I'm not sure how broadly useful that would be.

stcpottdav commented 1 year ago

Yes @JeremyKuhne this meets my requirements. The main reason I suggested the Application object is because this is where SetDefaultFont is and it seemed similar in concept.

elachlan commented 1 year ago

I'd lean towards Application.DefaultFormIcon. But I can see a static property on Form also making sense. I think there is more similar settings in Application though.

RussKie commented 1 year ago

I'm not sure I fully understand the business need behind this request. A form's icon can be set via the Icon property. The DefaultIcon is an implementation detail that ensures there's a default icon set. I would also struggle to buy in to a "we have tens/hundreds of forms in which we need to set Icon property over and over" situation - as it would denote architectural issues with the application.

elachlan commented 1 year ago

Yes, which is why I suggested setting the default icon from the ApplicationIcon in the proj file. It's already used to set the applications executable icon.

JeremyKuhne commented 1 year ago

Yes, which is why I suggested setting the default icon from the ApplicationIcon in the proj file. It's already used to set the applications executable icon.

The difference here is that the application icon is a native resource and has nothing to do with the WinForms runtime.

JeremyKuhne commented 1 year ago

t sure I fully understand the business need behind this request. A form's icon can be set via the Icon property. The DefaultIcon is an implementation detail that ensures there's a default icon set.

@RussKie this is trivial to implement with little risk. The fact that it isn't a publicly exposed detail isn't a reason to not expose it.

@stcpottdav we do need some more details on why setting individual Form icons is undesirable to help justify this when we put it to .NET API review.

elachlan commented 1 year ago

@JeremyKuhne could we read the Application Icon in as the default icon? A quick google has this to get the AppIcon:

Icon appIcon = Icon.ExtractAssociatedIcon(Assembly.GetExecutingAssembly().Location);

Apparently that doesn't work if the file is on a network path. But Pinvoke could work: http://www.pinvoke.net/default.aspx/shell32/ExtractIconEx.html

Or: http://www.pinvoke.net/default.aspx/shell32.SHGetFileInfo

That way the default icon for an app is what is set as the ApplicationIcon and not the winforms default icon.

Business case wise, it reduces any chance of forms falling through the cracks. It would be nice if the designer would read this in as well. Although I am unsure how that would work in practice.

RussKie commented 1 year ago

could we read the Application Icon in as the default icon?

I find this a significantly more compelling enhancement. For instance, it could be implemented as an opt-out MSBuild property (i.e., on by default) that embeds the icon specified by ApplicationIcon property as an embedded resource, and then emit an assignment via the application bootstrap. This way it would be transparent to developers, i.e., "just work". [EDIT] Or extract it and assign it with a pinvoke suggested by @elachlan.

elachlan commented 1 year ago

Only issue I see with the ApplicationIcon approach is it's "App Wide" so this would also apply to any libraries used by the system. Which could cause some interesting behaviour where the ApplicationIcon is used in a third party library. But that's the whole point of it I guess.

I guess we could have add static Icon applicationDefaultIcon and have it be the "top level" and then have a dictionary to hold the icon for each library's Assembly. Then its just a change to DefaultIcon to use applicationDefaultIcon when there isn't an ApplicationIcon set on the library? https://github.com/dotnet/winforms/blob/ee9fe7b0e30fd80fbd938e47a15bac97212068be/src/System.Windows.Forms/src/System/Windows/Forms/Form.cs#L886-L906

I feel like it might be too complicated though, there is a lot to be said for simply using the ApplicationIcon.

elachlan commented 1 year ago

I created a test in #8918 and it works!

JeremyKuhne commented 1 year ago

I think the other ideas being floated are worth pursuing and I don't think doing what I've proposed here blocks any of them. We spoke as a team yesterday and plan to go ahead with the surface I proposed (pending a response from @stcpottdav). We can create additional API/feature proposals.

@elachlan / @RussKie I mentioned in #8919 that we should extend Icon to allow loading from PE file resource ids. I'll create the API proposal shortly.

stcpottdav commented 1 year ago

I assume the last post is referring to this: https://github.com/dotnet/winforms/issues/8905#issuecomment-1487535838

This seems good to me.

Thank You.

JeremyKuhne commented 1 year ago

@stcpottdav What I need from you is an explanation of why manually setting Icon on every form isn't desirable/possible. If you can give a concrete example of your scenario, it will help me with the formal API review.

elachlan commented 1 year ago

@JeremyKuhne one of our apps has a concept of plugins. Which allows a customer to bring their own "DLL". They might not inherit from our base classes and as such won't have the apps branding. By setting the default icon we can set branding application wide without having to worry about third party DLLs.

In practice this hasn't happened yet. But it could.

stcpottdav commented 1 year ago

My scenario is the upgrade from .NET Framework 4.x to .NET 6. In this scenario it the default icon changed and or end users noticed this. Setting the default icon allows a application specific icon to be set without having to edit the forms.

terrajobst commented 1 year ago

Video

namespace System.Windows.Forms;

public partial class Form
{
    public static System.Drawing.Icon DefaultIcon { get; set; }
}
JeremyKuhne commented 1 year ago

I didn't consider the fact that there are multiple sizes needed for Form. (WM_SETICON)

There is a presumption that Icon is going to have all of the size data in it. You'll get undesirable results when extracting a given HICON to create Icon.

I'm not sure the best way to handle this. Going to sit on this for a bit while I think about various options.

elachlan commented 1 year ago

Maybe we can add the idea of MultiIcon icons into System.Drawing?

janseris commented 1 year ago

I'd lean towards Application.DefaultFormIcon. But I can see a static property on Form also making sense. I think there is more similar settings in Application though.

Application.DefaultFormIcon would be very useful and very elegant. Currently, we have to set icon manually in every window in the designer or pass it from parent to child form etc. (unnecessary complexity) For example I have a universal reusable fullscreen image displayer component (but is a Form). When used in multiple projects, I always have to pass the current app icon in it.