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

[Matthieu Gallien]

mgallien added a comment.


  Thanks for your work. Please find a few remarks inline.

INLINE COMMENTS

> embeddedimagedata.h:25
> +#include "kfilemetadata_export.h"
> +#include <QMimeDatabase>
> +

You can do a forward declaration of QMimeDatabase because it is only referred through a reference.

> embeddedimagedata.h:40
> +    class Private;
> +    Private *d;
> +

You should be using a std::unique_ptr instead of a raw pointer. You also should take care of either forbidding copy (operator= and copy constructor) or implement them to perform a deep copy including duplicating the private object.
Currently, it will be implicitly shared between instances.
In many classes in KDE repository you do not need to do it because QObject inheritance gives you exactly that.

> embeddedimagedata.h:42
> +
> +    QByteArray getFrontCover(const QString &fileUrl, const QString &mimeType) const;
> +

The private method can go to the private class.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180502/970c0419/attachment.html>