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

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

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=35934 BRANCH cover_read REVISION DETAIL https://phabricator.kde.org/D12320

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

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 , const >

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

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

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

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,

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

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,

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

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

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

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,

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

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=34829 BRANCH cover_read REVISION DETAIL https://phabricator.kde.org/D12320 AFFECTED FILES

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

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

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

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

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,

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

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

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

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=34012 BRANCH cover_read REVISION DETAIL

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

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,

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

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 ,

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

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

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

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:

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

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,

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

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=33955 BRANCH cover_read REVISION DETAIL

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

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

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

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

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

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,

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

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

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

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

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

bruns added inline comments. INLINE COMMENTS > embeddedimagedata.h:36 > +}; > +QByteArray imageData(const QString , const QMimeDatabase > , const ImageType imageType = FrontCover) const; > + return type QMap? And instead of an enum imageType a QFlags imagetype?

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

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

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

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,