Rewrite the taglib writer to use taglib's
unified property interface, which is more extensible.
Details
tests pass
Diff Detail
- Repository
- R286 KFileMetaData
- Branch
- rewrite_taglib2
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 7772 Build 7790: arc lint + arc unit
src/writers/taglibwriter.cpp | ||
---|---|---|
67–68 | use a smart pointer (unique_ptr, QScopedPtr) here, and get rid of the delete below. |
I think this becomes better structured when you:
- Create one function per file type, with parameters (Taglib::Filestream, KFM::PropertyMap)
- From this function, call a generic updateProperties(oldProperties, newProperties) -> mergedProperties
- Call the type specific function from TaglibWriter::write(...)
Especially when taking the changes for writing the rating into account, this would make the code easier to read - handling of different types just once (not once for reading and once for writing), and no upcasting/dynamic_cast of Taglib::File*. It also saves the heap allocation of the concrete TagLib::File implementation.
Done. It requires a little bit more of boilerplate code, but I have not strong preference.
src/writers/taglibwriter.cpp | ||
---|---|---|
70 | When you make this dito for the specializations in D18604, just pass in Taglib::File*, and call auto tags = dynamic_cast<FooTag*>(file->tag()); there. |
src/writers/taglibwriter.cpp | ||
---|---|---|
70 | So iff the rating is updated at the same time as another property, taglib has to adjust some in-memory structure twice. Obviously, for most formats this poses no problem, but for Ape/Vorbis it does? Also, this no longer applies when you use the specific tag types for Ape/Ogg. Writing to disk only happens when calling file->save()... |
src/writers/taglibwriter.cpp | ||
---|---|---|
70 | Not sure I understand. I would like to avoid doing file->properties(); and file->setProperties(); twice for those formats for which only the PropertyMap is sufficient, e.g. Ape and Ogg, to avoid doing any unnecessary work. The beauty of Ape and Ogg in the TagLib implementation is that they solely work with the PropertyMap and do not require any special handling. See D18826, Ape and Ogg do not have any extra codepath at all. For writing tags, we have to be a little bit more careful, though. If writing Rating information is added to writeGenericProperties, for example Id3v2's "TXXX" tags will be polluted with these values. |
src/writers/taglibwriter.cpp | ||
---|---|---|
70 | I think for reading it as generic as possible is fine, but for writing being a little bit more explicit does not hurt. My proposal for Ape and Ogg is to split the writing for these to two different functions. Yes, it is possible to use the propertymap for both, as both use the same scale and the same tag name, but the implementations on the taglib side are completely different. Writing the "rating" for XiphComment (Ogg) and Ape in distinct functions (see my comment in D18604) hardly adds any code, but gets the Ape and Ogg code paths in line with the other file formats. You don't write the ASF/MP4 rating using the property interface although it would be possible, and IMHO thats the right thing to do, also for Ape and Ogg. As soon Ape and Ogg are split, you no longer rely on the PropertyMap for the rating, and you won't have to use properties()/setProperties() twice. |
src/writers/taglibwriter.cpp | ||
---|---|---|
70 | No, I cannot use the the PropertyMap for ASF/MP4, those atoms/attributes are unsupported in the PropertyMap and need to be handled separately. I would have done so if it is possible. |
src/writers/taglibwriter.cpp | ||
---|---|---|
70 | Taglib does not "handle this nicely". For APE and Xiph, it just accepts *any* unknown key and uses it verbatim, while for MP4 and ASF it rejects any unknown key. The setProperties() is also quite inconsistent, for APE and ASF it only removes items which have an empty value, while for Xiph, the properties are completely replaced. As soon as you add support for a property where APE and Xiph key naming differs, or is only supported by one, you will require two functions anyway. Using APE::Tag::setItem(...) is as efficient as manipulating the key/value in the Taglib::PropertyMap first and setting it by setProperties(...). Likewise for Xiph. If you want to squeeze out the last bit of efficiency, you would skip the setProperties(...) completely when no property is changed by writeGenericProperties(....). This happens e.g. if you only change the rating. If you want to avoid duplicate code, move the properties()/setProperties() into writeGenericProperties(), that saves 12*2 lines and adds 2. |
src/writers/taglibwriter.cpp | ||
---|---|---|
70 |
And so do APE::Tag::setItem and OGG::XiphComment::addField ...? btw, also perfectly legal, APE and Xiph allow writing arbitrary tags, while the others do not.
Xiph explicitly allows multiple entries per key, which need to be removed when writing.
TagLib automatically translates different keys from APE to "common names", e.g. DISC->DISCNUMBER etc. I would really like to hand off manual tag handling to TagLib as much as possible. The library solely responsible for reading tags usually knows better how to handle the tags than we do (with a few exceptions to the rule of course). |
src/writers/taglibwriter.cpp | ||
---|---|---|
70 | By using the type specific function you signal you are aware of the differences between the two, and supply the appropriate data. RATING is not handled by the properties interface, it just works by coincidence, not by design. In case Taglib properly handles a tag, I am not against using it, as said several times. This is the case for DISC, but not for RATING. |