Embedded cover art support

[Dan Meltzer]

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 :)

Dan,
> Testers and feedback would be very welcome!
>
> Thanks,
>  Andreas
> --
> GRATIS für alle GMX-Mitglieder: Die maxdome Movie-FLAT!
> Jetzt freischalten unter http://portal.gmx.net/de/go/maxdome01
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>