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

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> embeddedimagedata.cpp:67
> +
> +        if (types & EmbeddedImageData::FrontCover || types & EmbeddedImageData::AllImages) {
> +            imageData.insert(EmbeddedImageData::FrontCover, d->getFrontCover(fileUrl,mimeType));

If you follow the suggestion above, you can drop the second or'ed statement.

> embeddedimagedata.cpp:68
> +        if (types & EmbeddedImageData::FrontCover || types & EmbeddedImageData::AllImages) {
> +            imageData.insert(EmbeddedImageData::FrontCover, d->getFrontCover(fileUrl,mimeType));
> +        }

missing space after `,`
you can also directly pass in fileMimeType.name()

> embeddedimagedata.h:45
> +        AllImages = 0x01,
> +        FrontCover = 0x02
> +    };

I would prefer `FrontCover = 0x1`, `AllImages = FrontCover`, where AllImages is extended later when other types are supported.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh, bruns
Cc: anthonyfieroni, 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/20180523/9bc6d3ee/attachment-0001.html>