GNOME Bugzilla – Bug 534954
Add ability to download cover art for entire library
Last modified: 2008-06-02 23:45:40 UTC
It should be possible to download cover art for the entire library at once instead of just getting it when a track is played. Additionally, I think it would be nice if cover art was automatically fetched when new tracks were imported or if track information was changed.
Created attachment 111561 [details] [review] Patch which implements these features via an extension
Looks really good. Just a bunch of nitpicks: Fix up copyright header for CoverArtJob.cs. Use last_scan instead of lastScan for private members, and lastScan for public method attribute names (you have them flipped). Use @"my multiline string" instead of "" + "". Probably a good idea to capitalize the SQL (eg select => SELECT) - I know I gave you the lowercase version, sorry. Since you're now joining with CoreTracks, you should select distinct(CoreAlbums.AlbumID). Does it make sense to limit the query to just the MusicLibrary? If so, just need to add AND PrimarySourceID = ?", MusicLibrary.DbId. If tracks.Count == 0, you call Finish, but without the Application.Invoke you use later. We generally use Banshee.Base.ThreadAssist.ProxyToMain to execute delegates on the main loop, if that's what you're doing. I think instead of listening for services starting waiting for InterfaceActionService, you can just add a dependency in the addins.xml file. Ideally would be able to do that for the MusicLibrarySource too, but...I think the way things are setup, it would require changes to more Core stuff.
Created attachment 111572 [details] [review] Patch which hopefully includes all of Gabriel's suggestions
*** Bug 532264 has been marked as a duplicate of this bug. ***
Created attachment 111630 [details] [review] Another iteration of the patch This version of the patch fixes some minor bugs and formatting issues. It also fixes a problem with the Rhapsody metadata provider being awfully slow sometimes (it was leaking the WebResponse in cases where an invalid result was returned).
Created attachment 111636 [details] [review] Yet another iteration of the patch This time there are some performance improvements -- it only keeps 10 tracks at a time in memory, and uses the same TrackInfo instance for all of them. Previously every track was loaded into memory.
Looks great to me. Assuming abock is fine w/ it, go ahead and commit.
Created attachment 111665 [details] [review] Another couple of fixes
What is "CoverArtMenu.xml"? It's referenced as a resource but I can't find it and Banshee crashes on a TargetInvocationException: Exception has been thrown by the target of an invocation. System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.ArgumentException: resource must be a valid resource name of 'assembly'. at Gtk.UIManager.AddUiFromResource (System.String resource) [0x00000] at Banshee.CoverArt.CoverArtService.Initialize () [0x000a5] in /home/jon/Projects/gtk/banshee/src/Extensions/Banshee.CoverArt/CoverArtService.cs:116 Line 116 is ui_manager_id = action_service.UIManager.AddUiFromResource ("CoverArtMenu.xml"); There are references to it in the patch but the file isn't there.
I see this has been added to RC1. The problem seems to have been taken care of there.