GNOME Bugzilla – Bug 692107
Should show embedded cover art even if being a single (album name == "")
Last modified: 2014-07-25 12:12:08 UTC
I've just realised that Banshee is the only media player I have (compared to Totem and Rhythmbox) which completely ignores embedded cover art if there is no album name stored in the ID3tags of the song. This is not such a big deal when having just a few singles inside your huge collection (because most songs have albums); in this case some cover art is simply not shown in the album grid (and it makes sense because the album grid is cool to group many songs together under clickable groups; each album being a group). But the experience is horrible if, for example, you open some music folder in a USB stick, and find some songs which are singles: even nautilus shows the embedded cover art, but if the user double-clicks in the single to listen to it in Banshee, no cover art shows up.
Created attachment 233908 [details] [review] proposed patch Most MetadataProvider classes cannot obviously retrieve cover art if there is no album name information in the track. However, there is a provider which doesn't retrieve cover art from the internet: EmbeddedMetadataProvider. In this case, the restriction about album name being not unknown doesn't really apply, so this commit moves the restriction to the online providers and removes it from the more generic MetadataService that manages them all.
Hi Andrés, Could you send me a binary with this patch as discussed by mail? I'll try it out and report the results.
(In reply to comment #2) > Could you send me a binary with this patch as discussed by mail? I'll try it > out and report the results. Hi Michiel, thanks for offering your help for testing the fix! To provide you with a binary, first I need to know what version of Banshee you currently have installed, please.
(In reply to comment #3) > (In reply to comment #2) > > Could you send me a binary with this patch as discussed by mail? I'll try it > > out and report the results. > > Hi Michiel, thanks for offering your help for testing the fix! To provide you > with a binary, first I need to know what version of Banshee you currently have > installed, please. I'm running Banshee 2.6.1 on Linux, x86-64 architecture.
(In reply to comment #4) > I'm running Banshee 2.6.1 on Linux, x86-64 architecture. Here you have a set of 2.6.1 binaries with the patch applied. (I've sent you the compressed file by email because it's too big to upload to bugzilla.) Normally, you would only need to replace 3 files (Banshee.Core.dll, Banshee.Services.dll, and Banshee.CoverArt.dll), however, depending on what distro you are using, it may happen that your Banshee is compiled with the .NET 3.5 *or* the .NET 4.0 profile. If replacing those 3 files doesn't work, then you may need to replace *all* binaries. (Of course, you may want to do a backup before replacing files in your /usr.)
Comment on attachment 233908 [details] [review] proposed patch Hey Michiel, did you have a chance to test my binaries? BTW, I'm marking this patch as needs-work, because Michiel actually sent me some interesting comments by email, which can actually qualify as a review of this patch, which I'm going to quote here to remind myself of improving the patch later: > I've browsed through the code a bit, out of curiosity. > > First of all, it looks like there are indeed unit-tests, though they > are too simple to detect any breaking changes. Even so, your patch > should probably include some tests there for completeness sake: > > https://git.gnome.org/browse/banshee/tree/src/Core/Banshee.Core/Banshee.Base/Tests/CoverArtSpecTests.cs > > I also came across some lines that you probably missed in your patch: > > https://git.gnome.org/browse/banshee/tree/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs#n124 > https://git.gnome.org/browse/banshee/tree/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs#n254 > > There may be more; I don't know. > > You know, the fact that the code needs to be changed in so many > locations just for this fix indicates that it could use some serious > refactoring. There are other indications too, like a string-comparison > to "Unknown Artist" to determine if the artist is unknown (I'll make > sure not to name my kids that). And the implementation of the > `ArtistAlbumEqual` method made me shiver. ;-) One last note: please be a bit more explicit about what's wrong with AlbumArtistEqual? I'm most likely not the author of that method (hundreds of devs have contributed to banshee), and we all have different backgrounds so that can be interpreted in many ways :)
(In reply to comment #6) > (From update of attachment 233908 [details] [review]) > Hey Michiel, did you have a chance to test my binaries? Sorry, no. I've been pretty busy. I'll probably still do it this week. > BTW, I'm marking this patch as needs-work, because Michiel actually sent me > some interesting comments by email, which can actually qualify as a review of > this patch, which I'm going to quote here to remind myself of improving the > patch later: Everything up until "There may be more" was a (limited) review of your patch. The rest is a comment on what I've observed of the Banshee code base as a whole (also quite limited). >> And the implementation of the >> `ArtistAlbumEqual` method made me shiver. ;-) > > One last note: please be a bit more explicit about what's wrong with > AlbumArtistEqual? I'm most likely not the author of that method (hundreds of > devs have contributed to banshee), and we all have different backgrounds so > that can be interpreted in many ways :) I'm sorry, I thought it was obvious (no offense meant). It's important for your patch. The name and signature of the method suggest that it will test whether `this` and `track` have the same Artist and the same Album. Fair enough. But the implementation is comparing ArtworkId! It just so happens that ArtworkId is an MD5 hash which *currently* always consists of Artist and Album, so in effect the method does what it's supposed to do (for the sake of argument, let's rule out the possibility of collisions). But up comes another developer years later who thinks that an ArtworkId should also sometimes consist of Artist and Track. :-) Such a developer would inadvertently break the ArtistAlbumEqual method, and who could blame him? (By the way, please note the name of the method is ArtistAlbumEqual, not AlbumArtistEqual.) Anyway, I see this kind of antipattern all over the place. I can wholeheartedly recommend the book "Refactoring" by Martin Fowler. It's an excellent read. Until then, Google for "Shotgun Surgery". Cheers!
I tried to replace the binaries (all of them), but no luck. Here's the error message (the significant bit, anyway): Missing method System.Reflection.Assembly::op_Equality(Assembly,Assembly) in assembly /usr/lib64/mono/2.0/mscorlib.dll, referenced in assembly /usr/lib64/banshee/Banshee.Services.dll Then I tried to patch the source-code and recompile, but my source-code is version 2.6.1, and your diff was probably against a more recent version, so no luck there either. I'm giving up on this for now. Truth be told, I doubt your patch would have solved my bug anyway, because the tracks that aren't showing any cover-art for me had full meta-data filled in properly, including Album, Artist, Album-Artist, Title, etc. And as I understand your patch, it only falls back to using the track name when the album name could not be found. I might try again in the future, if and when the code has seen more comprehensive changes. Or, I suppose, if your patch finds its way into the official build and the Gentoo Portage tree. Good luck!
Sorry for the super-late reply! (In reply to comment #8) > I tried to replace the binaries (all of them), but no luck. Here's the error > message (the significant bit, anyway): > > Missing method System.Reflection.Assembly::op_Equality(Assembly,Assembly) in > assembly /usr/lib64/mono/2.0/mscorlib.dll, referenced in assembly > /usr/lib64/banshee/Banshee.Services.dll Oh, that's actually a very usual problem that is easy to avoid: http://orangesquash.org.uk/~laney/blog/posts/2011/10/mono-gotcha/ In banshee master we have already committed a fix. > Then I tried to patch the source-code and recompile, but my source-code is > version 2.6.1, and your diff was probably against a more recent version, so no > luck there either. Sorry! > I'm giving up on this for now. Truth be told, I doubt your patch would have > solved my bug anyway, because the tracks that aren't showing any cover-art for > me had full meta-data filled in properly, including Album, Artist, > Album-Artist, Title, etc. And as I understand your patch, it only falls back to > using the track name when the album name could not be found. Oh I see, then maybe you're right. > I might try again in the future, if and when the code has seen more > comprehensive changes. Or, I suppose, if your patch finds its way into the > official build and the Gentoo Portage tree. I just committed an updated fix for this problem ( https://github.com/GNOME/banshee/commit/b2f8cbb14cdd8f99ef06cd59d2025c842d182d85 ), so you can test it when we release banshee version 2.9.2. BTW, in regards to the code: > Anyway, I see this kind of antipattern all over the place. I can > wholeheartedly recommend the book "Refactoring" by Martin Fowler. It's an > excellent read. Until then, Google for "Shotgun Surgery". I usually read Martin's ramblings in his blog, I think he has good ideas. However, he's mostly a guy working on proprietary software, so the workflow of opensource, being slightly different, doesn't always match to his recommendations. For example in this case "shotgun surgery"-style changes could cause a lot of regressions (given that we don't have near 100% code coverage by our unit tests), and would remove very valuable git-blame info per line of code. In particular the ArtistAlbumEqual thing you mention, I agree it looks strange, but it was not written by me (but by a previous maintainer who has contributed way more than me, and which I have in high regard). So I prefer to modify his code when I need to modify because of a change I want to make, not because I want to blindly & subjectively "improve the code". At the end of the day, in opensource, most of the time our regression testing are our users, and I don't want to overflow them with so much responsibility for my mistakes ;) (This problem has been fixed in the development version. The fix will be available in the next major software release.)