mgallien requested changes to this revision.
mgallien added a comment.
This revision now requires changes to proceed.
Please fix the issues.
INLINE COMMENTS
> taglibextractor.cpp:363
> + if (!genresList.isEmpty()) {
> + result->add(Property::Genre, genresList);
> }
Is it really needed to push directly a list ?
You are ignoring the fact that the original developer intended add to be called
once for each value. I would prefer to keep doing that until we can be
absolutely sure that nothing breaks if we change that.
> taglibextractor.cpp:370
> + const QStringList artists = contactsFromString(artistString);
> + result->add(Property::Artist, artists);
>
idem
> taglibextractor.cpp:374
> + const QStringList composers = contactsFromString(composersString);
> + result->add(Property::Composer, composers);
>
idem
> taglibextractor.cpp:385
> + const QStringList albumArtists =
> contactsFromString(albumArtistsString);
> + result->add(Property::AlbumArtist, albumArtists);
> }
idem
> taglibextractor.cpp:389
> if (tags->track()) {
> - result->add(Property::TrackNumber, tags->track());
> + result->add(Property::TrackNumber,
> static_cast<int>(tags->track()));
> }
Are you sure we need this cast ? I do not think we will ever need to have
negative track numbers added. The original type is unsigned int and should
exactly convey the fact that we do not expect negative numbers.
> taglibextractor.cpp:393
> if (tags->year()) {
> - result->add(Property::ReleaseYear, tags->year());
> + result->add(Property::ReleaseYear,
> static_cast<int>(tags->year()));
> }
idem
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D11365
To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella,
ngraham, alexeymin
