Embedded cover art support

[Mark Kretschmann]

On Fri, Feb 26, 2010 at 6:24 AM, Dan Meltzer
<parallelgrapefruit at gmail.com> wrote:
> On Thu, Feb 25, 2010 at 8:50 PM,  <ecroy at gmx.net> wrote:
>>> merge request will follow after some more testing!
>>
>> Well, finally came around to actually do it, so...
>>
>>   http://gitorious.org/amarok/amarok/merge_requests/134
>>
>> The proposed patch works fine for me and at last makes my music collection not look like a greyish dump yard anymore (now it's a colorful one :o)
>>
>
>
> Hi:
>
> A few things.
>
> 1) Please check the style you are using, it's a little different from
> what we use throughout Amarok elsewhere.  see
> HACKING/intro_and_style.txt
>
> 2) I am a little worried about how many places the code at the
> beginning of loadImageFromFile() has been copied to at this point.  I
> wonder if we should create a Meta::getFileRef( Meta::TrackPtr ) that
> centralizes this code, in case we want to change it at some point in
> the future.
>
> 3) It took me a few read throughs (even with the comment) to
> understand what isEmbeddedImage() is doing.  I wonder if it can be
> clarified a bit?  It also confuses me because we only deal with id3
> tags for embedded images, correct? And some of those formats wouldn't
> have id3 tags... (though in that case they probably shouldn't be in
> the images table.)
>
> 4) This should probably also be implemented for Meta::File.  Not
> necessarily in this patch, but it shouldn't be too miserable to do.
>
> 5) The collection scanner portion of this looks like it could be
> optimized to me... Gitorious just died so I can't look at the patch
> too closely, but you should a) be able to use 0/1 (I think qt will
> serialize booleans automatically) and b) default it to false, and
> check whether or not it's true, rather than only setting it to true if
> it has the frame.
>
> I think this is a very useful feature, and am glad you spent time
> working on it :)

Big thanks Andreas, I've merged your patch into mainline :)

But please also consider what Dan wrote. Possibly the patch could
still be improved a bit, maybe with another patch?

-- 
Mark Kretschmann
Amarok Developer, Software Engineer at Collabora Ltd
Fellow of the Free Software Foundation Europe
http://amarok.kde.org - http://www.fsfe.org - http://collabora.co.uk