GNOME Bugzilla – Bug 407672
Banshee doesn't share its own playlists via DAAP
Last modified: 2007-04-20 17:17:18 UTC
This is a spin off of comments on: Bug #369647 – DAAP playlists don't show up in Banshee A colleague and I noticed that we couldn't see each other's playlists in Banshee. I checked out the trunk, did a bit of investigating and noticed that in fact Banshee doesn't currently advertise its own playlists but it does display them for other remote DAAP shares. I opened this new issue because I think this is a different problem to that originally raised in #369647. I'm new to hacking Banshee but I've implemented a solution. I've created a patch against revision 2092 of the trunk which adds playlist sharing (includes SmartPlaylists) which I'll add in a comment below.
Created attachment 82496 [details] [review] Playlist advertising patch v1 Patch attached, I look forward to any feedback. Cheers
Cool - thanks for the initiative. It's always nice to receive new bugs with accompanying patches! As 0.11.x is feature/string frozen, I'll mark this for further review and inclusion in the next series (tentatively 0.12.x, but the versioning may yet change). Setting priority to high as well. I've done a quick scan of the patch to look for anything that stands out to me off the top of my head, but have not tested it. It looks pretty sane and honestly a lot smaller than I expected from the summary, so that's good news. When I can focus on new features, I'll take a closer look at this. I'm going to leave it with status "None" in case others may have time to do a better review and so it stands out in the patches page. Thanks!
I've taken a look at the code too (no testing yet). Can you explain why you added the track_map dictionary? Am I right in assuming that this is both a performance and a memory enhancement?
Also, does this handle playlist updates? Anyway, I haven't tested it yet (but will do so when I get home), if it works (which I highly suspect to be the case) it has my vote for the accepted-commit_after_freeze flag.
Hi Thanks for the comments. There were in fact three reasons for track_map: 1. Performance 2. So as not to waste memory 3. And because if I let duplicate tracks get in there the DAAP server would just crash when another client tried to connect to it :D It doesn't handle live playlist updates because it seemed to me (looking at the code and at the way the app behaves) that the only time the DAAP database is ever updated to reflect Banshee's model is when the plugin is unloaded and reloaded (e.g. stopping and restarting banshee, or disabling/enabling the plugin). The only time DaapCore registers to listen for library reload events is if the library hasn't finshed loading before the plugin is initialized. If this delayed loading route is taken DaapCore unregisters itself for that event when it finally does load (in OnLibraryReloaded).
Created attachment 84220 [details] Proposed changelog entry
Finally managed to test this, works like a charm. While in the long run, DAAP server support should go, it's a good addition for now. Thanks!
Yeah, the DAAP server will be removed from Banshee proper in favor of interfacing with Tangerine, but good patch. Sorry it took so long to go in. It's now committed.
Thanks very much! If someone has time and the perms could you possibly update the svn log for that commit to credit my name as "Munckfish" not "Munkfish"? Or even better use my real name as stated in the attached change log. svn pe -r2227 --revprop svn:log . I wouldn't pester about this but it's the first actual code I've contributed to an Open Source project so it's a small moment of pride for me. :D Cheers
I didn't see your ChangeLog entry, so was just going off your bugzilla account name. It'd be very helpful if you just changed your bugzilla account name to your real name. I've updated the ChangeLog and your name should show up in the release notes for the next release.
Thx Aaron.