GNOME Bugzilla – Bug 536132
Lastfm MetadataProvider (Cover Fetcher)
Last modified: 2008-07-20 20:48:31 UTC
This patch adds 2 things: 1. It extends Lastfm.Data with LastfmAlbumData. It gives acces to albumdata, like: - Tracklisting - ReleaseDate - Lastfm Url - Coverart 2. It provides a working Lastfm MetadataProvider for fetching covers. A few things definitely require fixing: - caching of the fragments in LastfmAlbumData doesn't work. - the "try" statement in LastFMQueryJob Run(), that covers the whole fetching. I am sure there are a lot of other things in this patch that can be done better, but that's something I would love to hear. I am just starting with C#. Succesfully tested with latest svn.
Created attachment 111919 [details] [review] Lastfm.Data AlbumData + Banshee.Metadata.LastFM
Thanks for the patch ! It compiled fine and fetched two additional covers that the other providers didn't find. A few comments : - I think you can just remove the "try/catch" statement in LastFMQueryJob.Run(). Any exception will be caught in MetadataServiceJob.Run() - Cosmetic : there are a few indentation issues, mostly in LastFMQueryJob.cs - Detail : LastfmAlbumData.cs has the wrong filename in the header
Created attachment 112430 [details] [review] Lastfm.Data AlbumData + Banshee.Metadata.LastFM (version 2) Fixed all the issues + improved upon the first version: Fixed: - Caching works now based on fragment+xpath - Removed try/catch statement - Fixed all indentation issues and code formatting so it complies to HACKING - fixed the filename in the LastfmAlbumData.cs header Added: - Hack for getting 300x300 album covers instead of 130x130 from lastfm - Hack for getting large size album coverart for lastfm links to Amazon cover art - Workaround for the newly introduced api nonsense where albums with no album art have a "noimage.jpg"-like coverart url.
I have the following errors when I try to compile after applying your new patch : make[4]: Entering directory `/home/lorentz/Projets/banshee/src/Core/Banshee.Services' Compiling Banshee.Services.dll... ./Banshee.Metadata.LastFM/LastFMQueryJob.cs(77,17): error CS0029: Cannot implicitly convert type `Lastfm.Data.LastfmAlbumData' to `bool' ./Banshee.Metadata.LastFM/LastFMQueryJob.cs(77,17): error CS0023: The `!' operator cannot be applied to operand of type `Lastfm.Data.LastfmAlbumData' Compilation failed: 2 error(s), 0 warnings
Created attachment 112592 [details] [review] Lastfm.Data AlbumData + Banshee.Metadata.LastFM (version 2.01) Oops :) Encountered it yesterday, but forgot to update it here. Now it should compile and work properly.
I guess you went a bit overboard with the parentheses in your fix, but it works. I don't have any more covers to fetch, and I can't remember which covers where only on Last.fm, so someone else will have to do some more testing.
I just tested this patch against current svn, and it works well. Of the albums in my collection that the existing metadata providers couldn't find covers for, the lastfm provider managed to grab covers for about 50% of them. So it's definitely a worthwhile addition!
I can also confirm this patch against current svn on Hardy.
Created attachment 113123 [details] [review] Lastfm.Data.AlbumData + Banshee.Metadata.LastFM (version 2.02) Now uses Track.AlbumArtist instead of Track.ArtistName. Needs svn 4117+ to function.
It appears that this patch is the cause of the error messages I experience. Since I also experience a lot of timeouts when fetching coverart, maybe that is related. Could link the strange 404 errors to this patch when I added a start/stop log message when starting a metadataprovider in MetaDataServiceJob.Run(). Exception is raised in the lastfm.data section of the patch, when an album is not present on lastfm. From the --debug: CoverFetcher: Banshee.Metadata.LastFM.LastFMMetadataProvider Started - geoffbullock-asymphonyofhope [Warn 00:36:57.666] Caught an exception - The remote server returned an error: (404) Not Found. (in `System') at System.Net.HttpWebRequest.CheckFinalStatus (System.Net.WebAsyncResult result) [0x002c7] in /usr/src/packages/BUILD/mono-1.9.1/mcs/class/System/System.Net/HttpWebRequest.cs:1310 at System.Net.HttpWebRequest.SetResponseData (System.Net.WebConnectionData data) [0x00102] in /usr/src/packages/BUILD/mono-1.9.1/mcs/class/System/System.Net/HttpWebRequest.cs:1179 [Debug 00:36:57.666] CoverFetcher: Banshee.Metadata.LastFM.LastFMMetadataProvider Ended - geoffbullock-asymphonyofhope Testing on hold until fix.
Created attachment 113417 [details] [review] Lastfm.Data.AlbumData + Banshee.Metadata.LastFM (version 2.04) Patch rewritten, exception handling added, double-url encoding added and now it finds 90% of my albums if it is the only provider enabled. A worthy extra provider. Tested around 50 times past days with complete deletion of .cache/albumart and .config/banshee-1 and then importing my whole library. No crashes occurred, all exceptions handled gracefully. diff against svn 4199
The following part in the LastfmAlbumData constructor looks strange to me : //Return Exception if the album is not found on Lastfm. try { if (AlbumData != null) { return; } } catch { throw; } Are you just evaluating the AlbumData property to check if the album can be found on Last.fm ?
Betrand: Yes. When creating a LastfmAlbumData object, it should fail if the album is not found on lastfm. I initially created a extra method for it, but I realised I could just call the AlbumData, which always fails if the info.xml file isn't found. So your conclusion is correct.
Thanks for the patch and your patience, Peter. Committed to trunk (and added you to the contributors list)!