GNOME Bugzilla – Bug 363548
Master list of shared songs (and other enhancements)
Last modified: 2006-10-23 14:37:26 UTC
There are at least 50 DAAP servers running on my dorm LAN at any given time. This presents a number of issues: * 50+ shared libraries in my Banshee source list is an eyesore. Scrolling can also be difficult when libraries are constantly coming and going from the list. * Many servers are inaccessible because iTunes limits the number of connections per day. Successfully connecting to available servers is a tedious game that I'd rather not play. * People have lots of great music but if there's ONE SONG that I really want to check out then I have to connect to person after person, searching their libraries, until I find it (if I find it at all). I wrote a patch to address these issues. Specifically... * DaapSources are added as children of a master "Shared Music" source. - This allow the user to collapse all shared libraries, solving my first problem. - DaapSource now inherits from ChildSource. - The master source is an instance of the new class DaapAllSource. * ChildSources can now have children. - This allows DaapSources to have playlist children. - SourceView.FindSource is now recursive (addresses a FIXME in the code). - SourceView.AddSource is recursive as well. * The master "Shared Music" source contains the tracks of all connected libraries. - This allows the user to search all shared music, solving my third problem. - Tracks are added and removed when servers connect and disconnect. - New events were added to DaapSource to facilitate this. * The master list has a "Connect to Everyone" button which attempts a connection with every DAAP server. - This solves my second problem. - A public method ConnectAll was added to DaapCore. - DaapSouce.Activate is overloaded with a bool to control silent connection failure. Since many dorm/office environments have lots of shared music, I think these changes will be helpful to a lot of people. I've ironed out all of the bugs I could in a day of testing, but there are still some problems I haven't been able to nail (segfaults, UI freezes, etc.). I don't know if they're bugs in my code or the rest of the DAAP plugin, but I'm hoping that others can help me sort them out.
Created attachment 75050 [details] [review] The Patch!
Hi Scott. This looks like a very interesting and nice patch. I'd agree with most of the features here. I like having the shares under one master "share." I'm not typically in an environment with dozens of shares, but I can imagine such a situation is rather irritating. That said, I would like to see two things to begin with: a) Your code style is inconsistent with the guidelines in the HACKING file. Some things here: make sure you use spaces (4) for tab, everything should line up nicely regarding indentation; opening braces go on the same line, with the exception of namespaces, classes, or methods (your lock lines have the brace on the next line) This may sound like nit-picking, but it is very important to have consistent style throughout the application. It also makes it easier for me to review as a patch before reviewing in a merge. b) I'd like to see this bug and patch split into two separate bugs/patches: 1. The first two points in your patch - making DAAP sources a child of the master DAAP source and the recursive ChildSources. I also do not like the name DaapAllSource. I would propose something like DaapContainerSource. 2. The rest of the patch should be separate - connect to all, show all tracks in the parent source, etc. I'd like two separate bugs/patches because the first part (1) looks mostly good to me, and I'd like to commit that very soon, but the second part would need some more discussion, testing, and review, and this process will end up delaying the first part of the patch. Sound reasonable?
An excellent idea Scott. I also work in a dorm environment from time to time and, while I don't have 50 shares to play with, I have seen it in the 20+ from time to time. I hope it won't take much to get things in line with Aaron's recomendations because I'd like to see it in place real soon. :-)
Created attachment 75114 [details] [review] A patch that just addresses the first two problems Thanks for the tips, abock. OK, this patches just the first two items (shared libraries are children of a DaapContainerSource, and ChildSources can have children). I'll start another bug for the other things. If this is going to be committed soon, I'll hold off on that patch since many of the same files are involved.
Bug 363813 for the other stuff.
Created attachment 75139 [details] [review] An updated patch Added the virtual bool AutoExpand to the Source class (default is true). If a Source's AutoExpand is true, it is expanded in the treeview after it is added. Made DaapSource and DaapContainerSource's AutoExpand false.
Looking good. I'm going to test this out shortly and if there aren't any problems I'd like to get it in today for 0.11.2 tomorrow. Because of time, I'm going to let it slide/fix it on my own, but there are still lots of tabs in the patch. Try looking carefully at it in your web browser... you'll see things don't quite line up: http://bugzilla.gnome.org/attachment.cgi?id=75139&action=view If you can fix it, fine, if not, just be aware of it for future patches. Thanks!
Created attachment 75145 [details] [review] Without tabs (I hope) Yeah, sorry about that. I must have a different monospace font because the tabs line up fine for me so I can't really see where they are. Anyway, I ran the patch through a quick python script to replace the tabs, so it should be fine now. And I've set Monodevelop to use spaces, so it shouldn't be a issue anymore ;)
Okay, the first part of this patch is in: 2006-10-21 Aaron Bockover <abock@gnome.org> * src/Banshee.Base/Sources/TestSource.cs: Test source for testing the functionality of the source base class, source manager, and source view; tests can be invoked using the new interactive remote debugger * src/Banshee.Base/Makefile.am: * src/Banshee.Base/Banshee.Base.mdp: Added TestSource.cs * src/Banshee.Base/ChildSource.cs: Child sources can now be parents * src/Banshee.Base/Source.cs: * src/Banshee.Base/SourceManager.cs: * src/Banshee.Base/Gui/SourceView.cs: Patched by Scott Peterson to allow recursive sources (BGO #363548) * src/Banshee.Base/SourceManager.cs: Added RemoveSource(Type), use generics for the source list
Scott: I said "first part" because: a) AutoExpand doesn't seem to work. Today I started working on a remote debugger/tester. Some of the tests I added were to test your patch, so here's how to run them: 1. Remove your patches from your build and update to the latest CVS (as the first part of your patch is in). Rebuild, install. 2. Open two terminals. Launch banshee like normal in one and wait for it to be loaded. 3. In the second terminal run "banshee --debug-client" 4. That will dump you into the new command line remote test thingy. At the prompt: banshee> test-source-add-single 5. That command will start running the source tests on the running instance. Watch your source view and be amazed :) This test shows your patch is doing the right thing with adding the sources recursively, but note they are not autoexpanded. This needs fixing. Be warned: the command line client is a bit fragile do to just using Console.ReadLine currently - bad input (backspace) can possibly crash the client (shouldn't crash the server/banshee instance) - if that happens, just restart the client.
Oh, you don't want test-source-add-single. (part 4 of my last comment). You want: banshee> test-source-add-recursive You can see other available remote methods by running 'show-methods' This continue my other comment... b) I only committed the source part of your patch because I need that tested and working perfectly first (as I'm very close to releasing). c) I'm in a hotel right now and don't see any DAAP shares to test d) Because of (c) I see a small bug that you probably didn't see. The parent DAAP source is always shown. It should not ever be shown if there are zero children. Please address this. Let's keep tracking the rest (DAAP-specific) part of this patch/bug here. Patches from now on should be based on CVS. Sound good? Thanks Scott! Keep it up :)
Created attachment 75158 [details] [review] This patch just addresses the AutoExpand bug OK, AutoExpand should work now. I'll have a patch for DaapCore in a bit to take care of the DaapContainerSource when no servers are present.
Created attachment 75161 [details] [review] DAAP patch For others following along - here's the remainder of the patch (just the DAAP part).
AutoExpand patch committed. Thanks Scott.
Created attachment 75162 [details] [review] And this takes care of the other thing This patch ensures that the DaapContainerSource is only added if there are Daap servers (and that it goes away if all of the servers do).
Looks good to me, I've asked James to test/commit as I am DAAP serverless until Monday.
Created attachment 75171 [details] [review] Fixes a bug in Source.ClearChildSources() TreeIters must be removed from a TreeStore in reverse order or else the tree is corrupted. That means that ClearChildSources needs to remove children on a LIFO basis.
Note: I'm putting that last patch in this bug because it's a one-line fix and it's relevant to this bug.
Created attachment 75173 [details] [review] Fixes ClearChildSources() and makes RemoveChildSource recursive Almost forgot about recursive child removal.
All patches committed. Thanks Scott!
The patch is up on http://bugzilla.gnome.org/show_bug.cgi?id=363813, addressing the rest of my original issues.