D18604: Implement support for writing rating information for taglib writer
This revision was automatically updated to reflect the committed changes. Closed by commit R286:8e79f7ce2b7c: Implement support for writing rating information for taglib writer (authored by astippich). REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18604?vs=55298=55428 REVISION DETAIL https://phabricator.kde.org/D18604 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns accepted this revision. This revision is now accepted and ready to land. REPOSITORY R286 KFileMetaData BRANCH rating_write2 REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich updated this revision to Diff 55298. astippich added a comment. - cleanup and formatting REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18604?vs=55127=55298 BRANCH rating_write2 REVISION DETAIL https://phabricator.kde.org/D18604 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns added inline comments.
INLINE COMMENTS
> taglibwriter.cpp:45
> +
> +#include
>
Remove ...
> taglibwriter.cpp:71
> +int id3v2RatingTranslation[11] = {
> +0,1,13,54,64,118,128,186,196,242,255
> +};
missing spaces
> taglibwriter.cpp:131
> +
> +}
> +
please add an end of anonymous namespace comment here, it is hard to spot
otherwise
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh,
astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich updated this revision to Diff 55127. astippich added a comment. - rebase - simplify id3 rating to array REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18604?vs=55056=55127 BRANCH rating_write2 REVISION DETAIL https://phabricator.kde.org/D18604 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns added a comment. Can you change the id3v2 rating QHash with range checks and a plain array? Then its fine to go. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich added a comment. After some thinking, I have split up the function for writing specific Ape and Vorbis tags, as many more tags like "Opus" etc. are in use for Vorbis tags and not for Ape, and can be easily added this way. I am still using the property map, though. Are you fine with this? REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich updated this revision to Diff 55056. astippich added a comment. - split ape and vorbis tag writing functions REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18604?vs=50709=55056 BRANCH rating_write2 REVISION DETAIL https://phabricator.kde.org/D18604 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns added inline comments.
INLINE COMMENTS
> taglibwriter.cpp:80
> +if (newProperties.contains(Property::Rating)) {
> +int rating = newProperties.value(Property::Rating).toInt();
> +id3Tags->removeFrames("POPM");
This requires a range check here - and then you can use a plain array for
lookup.
> taglibwriter.cpp:89
> +
> +void writeVorbisAndApeTags(TagLib::PropertyMap , const
> PropertyMap )
> +{
I think this should be split in
`writeApeTags(Taglib::APE::Tag *tags, PropertyMap) { ...; tags->setItem(...) }`
and
`writeXiphTags(Taglib::Ogg::XiphComment *tags, PropertyMap) {...;
tags->addField(..., replace) }`
Thats much more in line with the other calls.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, domson, ashaposhnikov, michaelh, astippich,
spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich added inline comments. INLINE COMMENTS > bruns wrote in taglibwriter.cpp:229 > but in case file.tag() returns an ApeTag, the dynamic_cast will return a > nullptr. This is a dynamic_cast, not a static_cast. It always returns a nullptr REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns added inline comments. INLINE COMMENTS > astippich wrote in taglibwriter.cpp:229 > No. Mp3 supports ID3v1, ID3v2 or Ape tags, so simply casting to ID3v2 is not > possible. but in case file.tag() returns an ApeTag, the dynamic_cast will return a nullptr. This is a dynamic_cast, not a static_cast. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich added inline comments.
INLINE COMMENTS
> bruns wrote in taglibwriter.cpp:229
> Is this the same as
>
> auto id3Tags = dynamic_cast(file.tag());
> if (id3Tags) { ... }
>
> ?
No. Mp3 supports ID3v1, ID3v2 or Ape tags, so simply casting to ID3v2 is not
possible.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns added inline comments.
INLINE COMMENTS
> taglibwriter.cpp:229
> +writeID3v2Tags(file.ID3v2Tag(), properties);
> +}
> file.save();
Is this the same as
auto id3Tags = dynamic_cast(file.tag());
if (id3Tags) { ... }
?
> taglibwriter.cpp:279
> +if (mp4Tags) {
> +qDebug() << QStringLiteral("enter");
> +writeMp4Tags(mp4Tags, properties);
leftover ...
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich added a dependent revision: D18665: Cleanup taglib writer test. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich added inline comments.
INLINE COMMENTS
> bruns wrote in taglibwritertest.cpp:409
> What are the differences between the various mp3 test cases?
It tests all possible ratings, since the commonly used numbers are somewhat
arbitrary, and I wanted to make sure writing and extracting works
> bruns wrote in taglibwriter.cpp:153
> This should probably go into a separate file, togheter with the inverse
> transform.
>
> This can then be tested independently, i.e.
>
> for rating in {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10} {
> r = wmpRating(rating);
> Q_ASSERT(rating = balooRating(r)); }
We have to test the rest anyway, so I don't see much benefit in this honestly.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich updated this revision to Diff 50709. astippich added a comment. - rebase on latest parent revision REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18604?vs=50505=50709 BRANCH rating_write2 REVISION DETAIL https://phabricator.kde.org/D18604 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
bruns added inline comments.
INLINE COMMENTS
> taglibwritertest.cpp:409
> +QTest::addRow("mp3_0")
> +<< QStringLiteral("mp3")
> +<< QStringLiteral("audio/mpeg3")
What are the differences between the various mp3 test cases?
> taglibwriter.cpp:153
> +//map the rating values of WMP to Baloo rating
> +//0->0, 1->2, 25->4, 50->6, 75->8, 99->10
> +int rating = properties.value(Property::Rating).toInt();
This should probably go into a separate file, togheter with the inverse
transform.
This can then be tested independently, i.e.
for rating in {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10} {
r = wmpRating(rating);
Q_ASSERT(rating = balooRating(r)); }
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D18604
To: astippich, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich added a dependency: D18603: Implement more tags for taglib writer. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: Implement support for writing rating information for taglib writer
astippich retitled this revision from "implement support for writing rating information for taglib writer" to "Implement support for writing rating information for taglib writer". REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D18604 To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D18604: implement support for writing rating information for taglib writer
astippich created this revision. astippich added reviewers: bruns, mgallien. Herald added projects: Frameworks, Baloo. Herald added subscribers: Baloo, kde-frameworks-devel. astippich requested review of this revision. REVISION SUMMARY Add support for writing the rating tag to all supported mime types. Since the implementations quite differ, implement mime type specific paths when necessary. TEST PLAN tests pass REPOSITORY R286 KFileMetaData BRANCH rating_write REVISION DETAIL https://phabricator.kde.org/D18604 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, bruns, mgallien Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
