bruns added inline comments.
INLINE COMMENTS
> embeddedimagedata.cpp:134
> TagLib::FLAC::File flacFile(&stream,
> TagLib::ID3v2::FrameFactory::instance(), true);
> - TagLib::List<TagLib::FLAC::Picture *> lstPic =
> flacFile.pictureList();
> -
> - if (!lstPic.isEmpty()) {
> - for (TagLib::List<TagLib::FLAC::Picture *>::Iterator it =
> lstPic.begin(); it != lstPic.end(); ++it) {
> - TagLib::FLAC::Picture *picture = *it;
> - if (picture->type() == picture->FrontCover) {
> - return QByteArray(picture->data().data(),
> picture->data().size());
> - }
> - }
> - }
> + return getFrontCoverFromFlacPicture(flacFile.pictureList());
> +
Does Taglib signal an error when it fails to parse the file? Or is calling
pictureList() always safe?
> embeddedimagedata.cpp:148
> + TagLib::Ogg::Opus::File opusFile(&stream, true);
> + if (opusFile.tag()) {
> + return
> getFrontCoverFromFlacPicture(opusFile.tag()->pictureList());
Not sure if this is an issue, but the old code only called
`tag()->pictureList()` after `!tag()->isEmpty()` - is this safe without?
Dito for oggFile above.
> astippich wrote in embeddedimagedata.cpp:204
> The code also works if only the picture data is there without the name.
> I could return an empty QByteArray if dataPosition == -1 and pictureData.size
> == 0.
> Do you prefer if I do that separately?
Is it safe to call find on an empty ByteVector - most likely yes ...
Then return QByteArray on dataPosition == -1, so it is obvious the error case
is handled, and keep the rest as is.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D16671
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
