D16283: implement more tags for asf metadata
This revision was automatically updated to reflect the committed changes. Closed by commit R286:02e8cab069ff: implement more tags for asf metadata (authored by astippich). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D16283?vs=43885=44366#toc REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16283?vs=43885=44366 REVISION DETAIL https://phabricator.kde.org/D16283 AFFECTED FILES autotests/samplefiles/test.wma autotests/taglibextractortest.cpp autotests/taglibextractortest.h src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16283: implement more tags for asf metadata
bruns accepted this revision.
bruns added a comment.
This revision is now accepted and ready to land.
Add the comment, otherwise its fine.
INLINE COMMENTS
> taglibextractor.cpp:843
> +} else if (mimeType == QLatin1String("audio/x-ms-wma")) {
> +TagLib::ASF::Tag* asfTags = dynamic_cast(tags);
> +if (asfTags) {
Can you add a comment here for why the dynamic_cast is used here?
REPOSITORY
R286 KFileMetaData
BRANCH
extract_asf
REVISION DETAIL
https://phabricator.kde.org/D16283
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D16283: implement more tags for asf metadata
astippich added inline comments.
INLINE COMMENTS
> bruns wrote in taglibextractor.cpp:739
> this makes me always wonder why we don't do:
>
> // decltype(data.albumArtists) == QStringList
> for (attribute : lstAsf) {
> QString t = TStringToQString(attribute.toString());
> data.albumArtists << contactsFromString(t);
> }
Yeah, there is a lot of unneccesary stuff in there that must be cleaned up. But
before I get to that, I would like to add tests for multiple entries, and
before that we have to agree how the output of mutliple entries shall look
like, which brings me to D12950 :)
> bruns wrote in taglibextractor.h:28
> Side note - I think `tfilestream.h` is no longer needed.
Is it okay if I push this directly?
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D16283
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D16283: implement more tags for asf metadata
astippich updated this revision to Diff 43885. astippich marked 3 inline comments as done. astippich added a comment. - fix comment and simplify iterating - cleanup REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16283?vs=43822=43885 BRANCH extract_asf REVISION DETAIL https://phabricator.kde.org/D16283 AFFECTED FILES autotests/samplefiles/test.wma autotests/taglibextractortest.cpp autotests/taglibextractortest.h src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16283: implement more tags for asf metadata
bruns added inline comments.
INLINE COMMENTS
> taglibextractor.cpp:720
> +//map the rating values of WMP to Baloo rating
> +//0->0, 1->2, 25->4, 50->6,7 5->8, 99->10
> +if (rating == 0) {
`// 0->0, 1->2, 25->4, 50->6, 75->8, 99->10`
> taglibextractor.cpp:736
> +lstASF = asfTags->attribute("WM/AlbumArtist");
> +if (!lstASF.isEmpty()) {
> +for (TagLib::ASF::AttributeList::ConstIterator it = lstASF.begin();
> it != lstASF.end(); ++it) {
you don't need the `isEmpty()` here, as you iterate over it.
> taglibextractor.cpp:737
> +if (!lstASF.isEmpty()) {
> +for (TagLib::ASF::AttributeList::ConstIterator it = lstASF.begin();
> it != lstASF.end(); ++it) {
> +if (!data.albumArtists.isEmpty()) {
I think you can write instead:
`for (const auto& attribute : qAsConst(lstAsf)) { ... }`,
as `TagLib::List` has `begin()` and `end()`
> taglibextractor.cpp:739
> +if (!data.albumArtists.isEmpty()) {
> +data.albumArtists += ", ";
> +}
this makes me always wonder why we don't do:
// decltype(data.albumArtists) == QStringList
for (attribute : lstAsf) {
QString t = TStringToQString(attribute.toString());
data.albumArtists << contactsFromString(t);
}
> taglibextractor.h:28
> #include
>
> namespace TagLib
Side note - I think `tfilestream.h` is no longer needed.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D16283
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D16283: implement more tags for asf metadata
astippich added a comment. for some reason the TagLib::ASF::File tag() function returns only an invalid pointer, so I decided to use the dynamic_cast here. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16283 To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16283: implement more tags for asf metadata
astippich created this revision. astippich added reviewers: mgallien, bruns. Herald added projects: Frameworks, Baloo. Herald added subscribers: Baloo, kde-frameworks-devel. astippich requested review of this revision. REVISION SUMMARY add some more tags for asf audio files REPOSITORY R286 KFileMetaData BRANCH extract_asf REVISION DETAIL https://phabricator.kde.org/D16283 AFFECTED FILES autotests/samplefiles/test.wma autotests/taglibextractortest.cpp autotests/taglibextractortest.h src/extractors/taglibextractor.cpp src/extractors/taglibextractor.h To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
