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

Update tray icons to modern Windows style. #29

Merged
merged 3 commits into from
Mar 23, 2016
Merged

Update tray icons to modern Windows style. #29

merged 3 commits into from
Mar 23, 2016

Conversation

aixxe
Copy link
Contributor

@aixxe aixxe commented Mar 14, 2016

Quick change to the graphics to look a bit better on Windows 8 and Windows 10.

New images

@mattock
Copy link
Member

mattock commented Mar 14, 2016

Thanks for the PR! I assume these are not really suitable for Windows 7 and earlier? If so, we need to either

  1. Load these icons at runtime on Win 8/10, and load the old ones on Win 7 and earlier
  2. Install the correct iconset during OpenVPN install

Based on a quick look at Makefile.am it seems that the icons are embedded into the openvpn-gui.exe binary. So option 2) would be the correct approach. That said, I have no idea how that can be done.

@leobasilio
Copy link
Contributor

Actually option 1 is the right one. It's just a matter of loading the correct resources at tray.c.

@selvanair
Copy link
Collaborator

I'm not that familiar with ico files and their windows7/vista compatibility, but this may be a good opportunity to bring the notification icons in line with microsoft's recommendations:

Quoting from: https://msdn.microsoft.com/en-us/library/ee330740(v=vs.85).aspx
"Notification area icons should be high-DPI aware. An application should provide both a 16x16 pixel icon and a 32x32 icon in its resource file, and then use LoadIconMetric to ensure that the correct icon is loaded and scaled appropriately."

We have only 16x16 icons and LoadLocalizedIcon does not check the system metric. Is it possible to extend this PR to do that?

@selvanair
Copy link
Collaborator

These work fine on Windows 7 and look somewhat better than the current ones. In the absence of additional resolutions in the .ico, I tested scaling these and look ok at the common 125% and 150% sizes as well.

Has anyone tested these on other versions of windows?

@leobasilio
Copy link
Contributor

They're better than the current ones. No doubt. But I feel like they're missing some contour. Here's how they look on Windows 8:

tray icons

@selvanair
Copy link
Collaborator

Yeah, it lacks a border unlike the current one. Yet the crisp lines look better, though the lack of border may not work well on light colored backgrounds.

@aixxe
Copy link
Contributor Author

aixxe commented Mar 22, 2016

Yeah, it's definitely not ideal. My original 10-style icons also don't look so great in the status window.

I've just finished some icons that better match the Windows 8 style.

Windows 8 Style

I've committed these new icons here.

@leobasilio
Copy link
Contributor

Perfect. If I may ask, would it be possible for these icons to have higher resolutions also? They're sort of pixelated when shown at the taskbar.

taskbar

@aixxe
Copy link
Contributor Author

aixxe commented Mar 22, 2016

Updated with 32x32 icons for the Windows 8-style icons.

New 32x32 Icons

@leobasilio
Copy link
Contributor

Great. Should we keep the current icons on Windows XP? In case we don't, there's no need for additional code and this PR is ready to be merged, in my opinion.

@aixxe needs to merge the "icons-win8" branch with his "master", so they can take part on this PR. Otherwise a new PR should be opened.

@selvanair
Copy link
Collaborator

The version in master doesn't support Windows XP anymore, so if it works on vista+ we are good. That said, just wondering why wouldn't these work on XP -- these are just 24 bit images with 8-bit alpha, aren't they?

@leobasilio
Copy link
Contributor

I think it's just a matter of style matching. No big deal.

@selvanair selvanair merged commit dfa1edf into OpenVPN:master Mar 23, 2016
@selvanair
Copy link
Collaborator

@aixxe : If you have the master files used for generating the 16x16 and 32x32 tray icons, could you please generate 20x20 and 24x24 sizes as well? That would help us support multiple dpi settings better: letting Windows scale the icons to match the dpi does not lead to good results. This was discussed in this Trac ticket

Also if you could add the high resolution master (or a vector format file if available) to the repo, anyone could generate additional sizes on the fly in future, should the need arise.

@aixxe
Copy link
Contributor Author

aixxe commented Dec 6, 2016

@selvanair: I've committed the updated icons to my fork here.

Unfortunately I don't have a vector version as each icon was edited by hand.

@selvanair
Copy link
Collaborator

エイミー: Thanks. I will test these out.

@selvanair
Copy link
Collaborator

@aixxe I tested the icons by fixing the icon loading logic. Now my 144 dpi monitor shows the 24x24 icon without need for scaling and looks excellent. There is a test executable here if you want to try it out.

Please submit it as a PR so that we can get it merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants