hardcodet / wpf-notifyicon

NotifyIcon (aka system tray icon or taskbar icon) for the WPF platform
Other
810 stars 125 forks source link

Fixed #75: Pick the correct size of the icon from a .ico file. #76

Closed devpelux closed 2 years ago

devpelux commented 2 years ago

As explained in #75, if a .ico file with multiple icons is used, the wrong size of the icon is picked and downscaled to the requested size.

This PR fixed that problem by sending the requested icon size when creating the icon.

Downscaled wrong icon:
tray

Right icon:
tray2

Tested on Windows 11 with .NET 6.0.

devpelux commented 2 years ago

The reference assemblies for .NETFramework,Version=v4.5 were not found.

I think is because net 4.5 is not supported anymore by microsoft and azure... It also gave me problems with visual studio 2022.

Lakritzator commented 2 years ago

Hey, thank you for your contribution!

I had a quick look, mainly looking at the change and ignoring the 4.5 error (I think we can drop the support). What I am afraid of, is that this change partly fixes the issue, from what I found it doesn't work when a user has different DPI settings.

Instead of using the SystemParameters.SmallIconWidth and SystemParameters.SmallIconHeight it might make more sense to query them via: GetSystemMetricsForDpi using SM_CXSMICON

This also means we need to pass the DPI for the screen where the icon is visible, I currently do not recall if we have that information at hand. I know I did do some DPI stuff, but I jump back and forth between projects...

devpelux commented 2 years ago

Maybe something likes that? https://github.com/devpelux/fullcontrols/blob/fluent/FullControls/Core/Services/SysParams.cs

Lakritzator commented 2 years ago

Not exactly, as this doesn't use the Windows 10 APIs to get the current DPI. But to be honest, I already added DPI information to the SystemInfo class. See: https://github.com/hardcodet/wpf-notifyicon/blob/master/src/NotifyIconWpf/Interop/SystemInfo.cs#L76 I think a new ScaleWithDpi method for a Size would be good to have, than one could just use:

var iconSize = new Size((int)SystemParameters.SmallIconWidth, (int)SystemParameters.SmallIconHeight)).ScaleWithDpi();
return new Icon(streamInfo.Stream, iconSize.Width, iconSize.Height);

(untested code, don't have access to VS right now)

devpelux commented 2 years ago

I've added a way to get from CXSMICON and CYSMICON. I don't know how to test for different DPI.

For me both the ways give me the same value

SystemInfo: {Width=16, Height=16}
SystemParameters: {Width=16, Height=16}
Lakritzator commented 2 years ago

I just noticed that you are targeting the master branch, which is not your fault but I guess we should make develop the default.

It doesn't make sense to target master (which reminds me that I wanted to rename it) please target develop, so you will not need to bring the code from develop over to master.

Lakritzator commented 2 years ago

Ah yes, I cannot change the default branch, there is that 🤷‍♂️

Hi @hardcodet, can you please change the default branch to develop, so the PRs we get do not go to older branches?

hardcodet commented 2 years ago

@Lakritzator Done :)

devpelux commented 2 years ago

I don't have noticed the wrong branch, thanks for fixing.