Implement large notification icon support for windows #85
Conversation
There was a problem hiding this comment.
Thank you for your contribution!
I have added a few comments. My main issue with this feature is the class-wide configuration of large notification icons.
Is the issue you are trying to resolve---small notification icons---mainly an issue on HiDPI monitors, or are notifications scaled appropriately in that case?
| @@ -66,6 +66,10 @@ class Icon(object): | |||
| #: notification. | |||
| HAS_NOTIFICATION = True | |||
|
|
|||
| #: Whether this particular implementation forces displaying a | |||
| #: large notification icon | |||
| FORCE_LARGE_NOTIFICATION_ICON = False | |||
There was a problem hiding this comment.
These class-level constants have previously served as an indication that the backend supports a particular feature whereas this new field serves as application-wide configuration.
I understand why you have implemented it this way, but I think a more suitable approach would be to specify this with a constructor argument, but there is currently no way to pass a constructor argument to one implementation only. What do you think of a backend specific options system like this?
There was a problem hiding this comment.
When we set NIIF_ICON_MASK flag to dwInfoFlags parameter, toast notification adds a text that is the file description of the executable. In our case it is python.exe. It's not a problem for me because I will create an executable via pyinstaller and set a version file for my application. For other library users may not want to see ugly pyhon text.
lib/pystray/_win32.py
Outdated
| 0, | ||
| 0, | ||
| 128 if Icon.FORCE_LARGE_NOTIFICATION_ICON else 0, | ||
| 128 if Icon.FORCE_LARGE_NOTIFICATION_ICON else 0, |
There was a problem hiding this comment.
Is there any reason you would not always want to do this? Are there cases where this causes regressions?
There was a problem hiding this comment.
My screen dpi value is 240 (scaled 250%). I tested it for different dpi. But the result was similar for all. I setted it 128, because my screen resolution 4k and 64x64 is stil a bit blurry. I calculated icon size for the current dpi (96 -> 32x32, 240 -> 80x80) but the blur did not changed. I also tested my code with 1080p resolution (scaled 100%), no difference for 128x128 or default icon size.
Comparison:
moses-palmer
force-pushed
the
master
branch
2 times, most recently
from
02eb7d9
to
01e848d
Compare
On Windows 10, toast notification icon seems very small.
We can force it to show large icon by setting dwInfoFlags parameter to NIIF_ICON_MASK for NOTIFYICONDATA.
But this change that is not enough to get good result. We need to change LoadImage function cx,cy parameters to load the icon image with like 128x128 pixels. Otherwise it seems blurry.
After changes, toast notification icon seems as it's supposed to be.