Implement support for writing rating information for taglib writer
ClosedPublic

Authored by astippich on Jan 29 2019, 6:30 PM.

Details

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

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich created this revision.Jan 29 2019, 6:30 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptJan 29 2019, 6:30 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Jan 29 2019, 6:30 PM
astippich retitled this revision from implement support for writing rating information for taglib writer to Implement support for writing rating information for taglib writer.Jan 29 2019, 6:30 PM
bruns added inline comments.Jan 31 2019, 6:35 PM
autotests/taglibwritertest.cpp
409

What are the differences between the various mp3 test cases?

src/writers/taglibwriter.cpp
123

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)); }
astippich updated this revision to Diff 50709.Feb 2 2019, 11:41 AM
  • rebase on latest parent revision
astippich added inline comments.Feb 2 2019, 11:45 AM
autotests/taglibwritertest.cpp
409

It tests all possible ratings, since the commonly used numbers are somewhat arbitrary, and I wanted to make sure writing and extracting works

src/writers/taglibwriter.cpp
123

We have to test the rest anyway, so I don't see much benefit in this honestly.

bruns added inline comments.Feb 4 2019, 9:39 PM
src/writers/taglibwriter.cpp
236

Is this the same as

auto id3Tags = dynamic_cast<TagLib::ID3v2::Tag*>(file.tag());
if (id3Tags) { ... }

?

298

leftover ...

astippich added inline comments.Feb 5 2019, 6:50 PM
src/writers/taglibwriter.cpp
236

No. Mp3 supports ID3v1, ID3v2 or Ape tags, so simply casting to ID3v2 is not possible.

bruns added inline comments.Feb 5 2019, 9:55 PM
src/writers/taglibwriter.cpp
236

but in case file.tag() returns an ApeTag, the dynamic_cast will return a nullptr. This is a dynamic_cast, not a static_cast.

astippich added inline comments.Feb 5 2019, 10:11 PM
src/writers/taglibwriter.cpp
236

It always returns a nullptr

bruns added inline comments.Feb 28 2019, 10:52 PM
src/writers/taglibwriter.cpp
80

This requires a range check here - and then you can use a plain array for lookup.

89

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.

bruns requested changes to this revision.Mar 6 2019, 8:57 PM
This revision now requires changes to proceed.Mar 6 2019, 8:57 PM
astippich updated this revision to Diff 55056.Mar 30 2019, 9:09 AM
  • split ape and vorbis tag writing functions

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?

bruns added a comment.Mar 30 2019, 2:44 PM

Can you change the id3v2 rating QHash with range checks and a plain array? Then its fine to go.

astippich updated this revision to Diff 55127.Mar 31 2019, 12:26 PM
  • rebase
  • simplify id3 rating to array
bruns added inline comments.Mar 31 2019, 9:13 PM
src/writers/taglibwriter.cpp
45

Remove ...

71

missing spaces

131

please add an end of anonymous namespace comment here, it is hard to spot otherwise

astippich updated this revision to Diff 55298.Apr 2 2019, 6:40 PM
  • cleanup and formatting
bruns accepted this revision.Apr 2 2019, 10:28 PM
This revision is now accepted and ready to land.Apr 2 2019, 10:28 PM
This revision was automatically updated to reflect the committed changes.