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

[Alexander Stippich]

astippich marked 8 inline comments as done.

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

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

[Alexander Stippich]

astippich updated this revision to Diff 35934.
astippich added a comment.


  - implement feedback
  - rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12320?vs=34829&id=35934

BRANCH
  cover_read

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/embeddedimagedatatest.cpp
  autotests/embeddedimagedatatest.h
  autotests/samplefiles/test.mpc
  src/CMakeLists.txt
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

To: astippich, mgallien, michaelh, bruns
Cc: anthonyfieroni, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham

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

[Stefan Brüns]

bruns added a comment.


  as the remaining issues are formatting only, this is an `accept` by me save 
the requested changes.
  @mgallien - if i am late to accept, can you do it?

INLINE COMMENTS

> embeddedimagedata.cpp:59
> +
> +QMap 
> EmbeddedImageData::imageData(const QString &fileUrl, const 
> EmbeddedImageData::ImageTypes types) const
> +{

nitpick - excessively long line

  QMap
  EmbeddedImageData::imageData(const QString &fileUrl, 
   const EmbeddedImageData::ImageTypes types) const
  {

> embeddedimagedata.cpp:73
> +
> +QByteArray EmbeddedImageData::Private::getFrontCover(const QString &fileUrl, 
> const QString &mimeType) const
> +{

dito, long line

> embeddedimagedata.cpp:95
> +for (TagLib::ID3v2::FrameList::ConstIterator it = 
> lstID3v2.begin(); it != lstID3v2.end(); ++it) {
> +TagLib::ID3v2::AttachedPictureFrame *frontCoverFrame = 
> static_cast(*it);
> +if (frontCoverFrame->type() == frontCoverFrame->FrontCover) {

long line, just make it `auto`, the type is obvious from the `static_cast`

> embeddedimagedata.h:35
> + * \brief The EmbeddedImageData is a class which extracts the images
> + * stored in the metadata tags of files as byte arrays.
> + *

Can you add one or two more lines what format to expect inside the byte array?

> embeddedimagedata.h:51
> + * Extracts the images stored in the metadata tags from a file.
> + * By default, the front covers are extracted.
> + */

nitpick - singular, front cover

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

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

[Matthieu Gallien]

mgallien added a comment.


  In D12320#272370 , @bruns wrote:
  
  > as the remaining issues are formatting only, this is an `accept` by me save 
the requested changes.
  >  @mgallien - if i am late to accept, can you do it?
  
  
  Yes sure. I have just seen your message. I also believe Alex is mostly away 
from keyboard this weekend.

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

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

[Alexander Stippich]

astippich added a comment.


  Sorry, I meant Kf 5.47, so deadline on Saturday.

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

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

[Stefan Brüns]

bruns added a comment.


  This looks good now, but haven't checked to thoroughly.
  Did you mean 5.47 or 5.48 - 5.48 is almost a month away ...

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

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

[Matthieu Gallien]

mgallien added a comment.


  In D12320#271300 , @astippich 
wrote:
  
  > any more comments? would be nice to ship it with Kf 5.48
  
  
  @bruns you requested changes. Can you have a look when you have time ?

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

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

[Alexander Stippich]

astippich added a comment.


  any more comments? would be nice to ship it with Kf 5.48

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

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

[Alexander Stippich]

astippich updated this revision to Diff 34829.
astippich added a comment.


  - simplify code

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12320?vs=34012&id=34829

BRANCH
  cover_read

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/embeddedimagedatatest.cpp
  autotests/embeddedimagedatatest.h
  autotests/samplefiles/test.mpc
  src/CMakeLists.txt
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

To: astippich, mgallien, michaelh, bruns
Cc: anthonyfieroni, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham

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

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

[Alexander Stippich]

astippich added a comment.


  any more comments?

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

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

[Alexander Stippich]

astippich added a comment.


  @anthonyfieroni 
  The preview is actually working perfectly fine, my configuration was wrong. I 
swear I checked it before :) sorry for the noise

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

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

[Alexander Stippich]

astippich added a comment.


  In D12320#260789 , @anthonyfieroni 
wrote:
  
  > In D12320#260768 , @astippich 
wrote:
  >
  > > Unfortunately, found it only after I wrote all that myself :/
  >
  >
  > Sorry i don't see RFC earlier.
  >
  > In D12320#260768 , @astippich 
wrote:
  >
  > > Off topic: The file preview does only work for folder previews, but for 
the individual files the previews are not shown. Is that intended behavior?
  >
  >
  > It *should* work for files as well as folders. Recently was added a patch 
that disable file preview for small sizes, can you increase icon size by slider?
  
  
  Does not work here, I will report a bug.
  
  In D12320#261315 , @mgallien wrote:
  
  > I am away from keyboard. Can we move forward ?
  >  Does it make sense to aim for having only one copy of this code and makes 
the kio code depends on KFileMetaData ?
  
  
  That would make sense, I think. Btw, no need to rush here, Kf 5.47 is still 
some weeks away. I missed the 5.46 deadline unfortunately.
  
  I still have one question: In case there is no cover found, should the QMap 
contain an entry with an empty bytearray or should it contain no corresponding 
entry at all? In the first case, an application has to test if the byte array 
is empty, in the latter it has to test if the map contains an entry. What's the 
better solution?
  Currently it is doing the first.

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

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

[Alexander Stippich]

astippich updated this revision to Diff 34012.
astippich added a comment.


  - add an AllImages flag
  - add simple documentation

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12320?vs=33955&id=34012

BRANCH
  cover_read

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/embeddedimagedatatest.cpp
  autotests/embeddedimagedatatest.h
  autotests/samplefiles/test.mpc
  src/CMakeLists.txt
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

To: astippich, mgallien, michaelh, bruns
Cc: anthonyfieroni, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham

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

[Matthieu Gallien]

mgallien added a comment.


  I am away from keyboard. Can we move forward ?
  Does it make sense to aim for having only one copy of this code and makes the 
kio code depends on KFileMetaData ?

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

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

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D12320#260768 , @astippich 
wrote:
  
  > Unfortunately, found it only after I wrote all that myself :/
  
  
  Sorry i don't see RFC earlier.
  
  In D12320#260768 , @astippich 
wrote:
  
  > Off topic: The file preview does only work for folder previews, but for the 
individual files the previews are not shown. Is that intended behavior?
  
  
  It *should* work for files as well as folders. Recently was added a patch 
that disable file preview for small sizes, can you increase icon size by slider?

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

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

[Alexander Stippich]

astippich added a comment.


  In D12320#260761 , @anthonyfieroni 
wrote:
  
  > I do *same* thing in KIO-Extras 
https://phabricator.kde.org/source/kio-extras/browse/master/thumbnail/audiocreator.cpp
  
  
  Yeah, I found that when I was looking into what's needed for a preview for 
audio files and was quite surprised.
  Unfortunately, found it only after I wrote all that myself :/
  Anyways, KFIleMetaData needs such a thing.
  Off topic: The file preview does only work for folder previews, but for the 
individual files the previews are not shown. Is that intended behavior?

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

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

[Anthony Fieroni]

anthonyfieroni added a comment.


  I do *same* thing in KIO-Extras 
https://phabricator.kde.org/source/kio-extras/browse/master/thumbnail/audiocreator.cpp

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

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

[Alexander Stippich]

astippich marked 4 inline comments as done.
astippich added a comment.


  Thanks a lot!

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

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

[Alexander Stippich]

astippich updated this revision to Diff 33955.
astippich added a comment.


  - fix usage of qflags
  - use unique_ptr

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12320?vs=33934&id=33955

BRANCH
  cover_read

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/embeddedimagedatatest.cpp
  autotests/embeddedimagedatatest.h
  autotests/samplefiles/test.mpc
  src/CMakeLists.txt
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

To: astippich, mgallien, michaelh, bruns
Cc: kde-frameworks-devel, #baloo, bruns, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham

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::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` is exactly the same as  `auto foo = 
std::unique_ptr(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

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

[Alexander Stippich]

astippich marked 2 inline comments as done.
astippich added inline comments.

INLINE COMMENTS

> mgallien wrote in embeddedimagedata.h:40
> 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.

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

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, bruns, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham

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

[Alexander Stippich]

astippich retitled this revision from "{RFC] add ability to read embedded cover 
files" to "[RFC] add ability to read embedded cover files".

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, bruns, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham

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

[Alexander Stippich]

astippich updated this revision to Diff 33934.
astippich added a comment.
Restricted Application edited subscribers, added: Baloo, kde-frameworks-devel; 
removed: Frameworks.


  - adjust to feedback

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12320?vs=33375&id=33934

BRANCH
  cover_read

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/embeddedimagedatatest.cpp
  autotests/embeddedimagedatatest.h
  autotests/samplefiles/test.mpc
  src/CMakeLists.txt
  src/embeddedimagedata.cpp
  src/embeddedimagedata.h

To: astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, bruns, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, #frameworks

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 
> +

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

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

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> embeddedimagedata.h:36
> +};
> +QByteArray imageData(const QString &fileUrl, const QMimeDatabase 
> &mimeDatabase, const ImageType imageType = FrontCover) const;
> +

return type QMap?

And instead of an enum imageType a QFlags imagetype?

The mimeDatabase should not be part of the method signature, its an 
implementation detail

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

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

[Alexander Stippich]

astippich added a comment.


  So, this is a next attempt trying to create a solution that works for 
everyone. It is not yet perfect, but I'd like to get some feedback if such an 
approach is favorable.
  A new class is created that will handle the cover art. This way, there won't 
be any interference with other applications such as baloo, but applications 
relying on KFileMetaData can still query the cover art separately if desired.
  
  Currently missing is documentation. I did the Cmake changes to my best 
knowledge, but some feedback here is very welcome. Same goes for the API.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

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

[Alexander Stippich]

astippich retitled this revision from "add ability to read embedded cover 
files" to "{RFC] add ability to read embedded cover files".

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun