GNOME Bugzilla – Bug 681062
Remove custom volume slider widget code and instead use Gtk.VolumeButton
Last modified: 2012-08-17 07:56:56 UTC
Created attachment 220139 [details] replace custom volume widget with gtk's stock volume button I've attached a patch that removes quite some code that implements a custom volume slider widget. see the patch commit message. This patch is just a cleanup. Since the VolumeSlider/Button of banshee is broken on OS X, I found that the custom widget implementation might be the cause, so I wrote the patch to test with Gtk.VolumeButton. Turned out, the Gtk.VolumeButton is broken exactly the same way on Quartz (but not on Linux or X11 backend). I guess because they were both ported from the Bacon library. I've reported that quartz bug with gtk's stock volumebutton with a testcase to the gtk/quartz bugtracker (almost a year ago) here: https://bugzilla.gnome.org/show_bug.cgi?id=660282 Although this patch does not fix the volume slider in banshee on OS X, I brought it to a submittable clean state to at lease remove some (unneeded) code.
Created attachment 220140 [details] [review] up-to-date version of the patch Sorry, forget the first patch, i mistakenly uploaded an older version. use this patch instead.
Thanks for the patch ! Your analysis seems right, Gtk.VolumeButton was only added in GTK 2.12. Before that it was in a C library called libbacon, and Aaron ported it to C# at that time. BUT, with this patch, if I go to "Now playing" and switch to fullscreen (thus having the cover art in fullscreen), Banshee eats all my RAM. I don't see how this patch could cause this... Does it happen for anyone else ? By the way, I wanted to test that because FullscreenControls is using a ConnectedVolumeButton with Classic=true, and I'm wondering how this patch impacts it.
Review of attachment 220140 [details] [review]: ::: src/Core/Banshee.ThickClient/Banshee.Gui.Widgets/ConnectedVolumeButton.cs @@ +63,3 @@ + // expose the gtk# protected Active field as public + public bool Active { get { return this.Active; } } There is no Active protected field, so this is probably wrong and may be the cause of my "eating RAM" issue ::: src/Extensions/Banshee.NowPlaying/Banshee.NowPlaying/FullscreenControls.cs @@ +53,3 @@ HBox box = new HBox (); + volume_button = new ConnectedVolumeButton (); The Classic property was just to always display the scale above the button, so that it doesn't go outof the screen when in fullscreen mode. This should now be handled automatically by the GTK widget, so this is fine.
Comment on attachment 220140 [details] [review] up-to-date version of the patch Committed, with several changes: http://git.gnome.org/browse/banshee/commit/?id=35a105ba9 Don't hesitate to ask if you have questions about those changes.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
yes, I wondered what the Active field could have been for, but as the Gtk.VolumeButton had a protected field for that (else the compile would have failed) I figured it might be good idea to pipe it back to parent. Turned out, it wasn't a good idea. However, glad the patch made it :)