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

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

Merged
merged 6 commits into from
May 21, 2022
Merged

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

merged 6 commits into from
May 21, 2022

Conversation

devpelux
Copy link
Contributor

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.

hardcodet and others added 2 commits April 22, 2021 13:22
Long-overdue contributor recognition.
@devpelux
Copy link
Contributor Author

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

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

@Lakritzator
Copy link
Collaborator

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

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

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

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

@Lakritzator Done :)

@Lakritzator Lakritzator changed the base branch from master to develop May 20, 2022 09:53
@devpelux
Copy link
Contributor Author

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

@Lakritzator Lakritzator merged commit 3672b4e into hardcodet:develop May 21, 2022
@punker76 punker76 added this to the 2.0.0 milestone Jun 11, 2024
@punker76 punker76 added the Fix label Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants