D12320: [RFC] add ability to read embedded cover files

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> embeddedimagedata.cpp:69
> +
> +        if (types == EmbeddedImageData::FrontCover) {
> +            imageData.insert(EmbeddedImageData::FrontCover,d->getFrontCover(fileUrl,mimeType));

should be `types & EmbeddedImageData::FrontCover`

> embeddedimagedata.cpp:149
> +                if (pictureFlac->type() == pictureFlac->FrontCover) {
> +                    return QByteArray(pictureFlac->data().data(),pictureFlac->data().size());
> +                }

missing space after `,`

> embeddedimagedata.cpp:170
> +        // Attached Picture.
> +        if (!lstPic.isEmpty()) {
> +            for (TagLib::List<TagLib::FLAC::Picture *>::Iterator it = lstPic.begin(); it != lstPic.end(); ++it) {

This is identical to the "audio/flac" case, can you use the same variable names etc. in both places?

> astippich wrote in embeddedimagedata.h:40
> std::make_unique is C++14, right? According to https://community.kde.org/Frameworks/Policies#Frameworks_Qt_requirements
> C++11 is the currently minimum version, so I left it unchanged. This is also similar to the other classes in KFileMetaData

`std::make_unique` is just a convenience helper

`auto foo = std::make_unique<Foo>` is exactly the same as  `auto foo = std::unique_ptr<Foo>(new Foo());`
see http://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D12320

To: astippich, mgallien, michaelh, bruns
Cc: kde-frameworks-devel, #baloo, bruns, ashaposhnikov, michaelh, astippich, spoorun, ngraham
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180510/2bc457a3/attachment-0001.html>