GNOME Bugzilla – Bug 574400
Resizes album art before sending it to notify-osd
Last modified: 2009-05-14 23:12:01 UTC
Please describe the problem: Hi! Someone noticed that the album art from Banshee was worse than it used to be when using Ubuntu's new notify-osd: https://bugs.launchpad.net/ubuntu/+source/banshee/+bug/338695 From testing it locally with art and asking Gabriel it seems Banshee resizes the art to 42 px here: http://svn.gnome.org/viewvc/banshee/trunk/banshee/src/Extensions/Banshee.NotificationArea/Banshee.NotificationArea/NotificationAreaService.cs?revision=5084&view=markup According to Macslow Banshee should probably just hand off the artwork to notify-osd untouched and let it resize the art instead. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
<https://wiki.ubuntu.com/NotificationDevelopmentGuidelines#blurry> has example code showing how to scale the icon only if it's being sent to notification-daemon, leaving it unscaled for other notification servers.
Created attachment 132961 [details] [review] only_resize_with_notification_daemon.patch Is this ok for you?
Nicolò, looks pretty good. Can you fix the indenting to use spaces instead of tabs, and put {} around your if/else (even though one line) - see HACKING for the full style guide.
Also, please change the IconThemeUtils.LoadIcon (48, "audio-x-generic"); fallback code and the IconThemeUtils.LoadIcon (48, Banshee.ServiceStack.Application.IconName); bits. Thanks!
I'm quite stupid... I completely forgot to do the things you asked! I will do them now
Created attachment 134584 [details] [review] notify_osd.patch
Sorry, I meant change both of those lookups in the same way you changed the ArtworkId lookup - to only scale if Name == "notification-daemon". Would probably be good to pull that logic out into a private GetPixbuf (string name) method that can we called in all three places.
Created attachment 134650 [details] [review] notify_osd.patch did you intend like this? I preferred to add a method with a Gdk.Pixbuf as a parameter instead of a string. So I could use the method for both the album art and the banshee icon. Can I further improve this patch?
Created attachment 134651 [details] [review] check_for_notifications_support.patch I also attached this patch from ubuntu, to check if notifications are supported.
Nicolò, thanks for your patience...I think I incorrectly assumed we could somehow get larger versions of the Banshee and audio-generic icons - but that's really not straightforward to do. So, I committed your original patch (and the upstream patch). :) Sorry for the wasted work, thanks a lot for the patch and for being so responsive on this. Note that my daemon name was not "notification-daemon", but "Notification Daemon" - so I modified the patch to check for both.
It's ok, no problem! But my first patch does not correct the banshee fallback icon! did you add it by yourself?
I actually never intended to change the fallback.
But the fallback is resized to 42x42: image.ScaleSimple (42, 42, Gdk.InterpType.Bilinear);
Oh I think I see - you're saying we can avoid keep those at 48x48 if !IsNotificationDaemon?
6 pixels are not so much better, but at least a little bit better. Anyway, thanks for committing this! Good night