From f4e7a742c322b2d50e1e1eaef4741273ae6edc75 Mon Sep 17 00:00:00 2001 From: Urs Fleisch Date: Fri, 2 Jan 2026 09:17:12 +0100 Subject: [PATCH] Improve Matroska API Make AttachedFile immutable. This is consistent with SimpleTag and Chapter and avoids using attached files which do not have all required attributes. Provide methods to insert and remove a single simple tag, so that they can be modified without setting all of them while still not exposing internal lists to the API. Use DATE_RECORDED instead of DATE_RELEASED for year() and the "DATE" property. This is more consistent with other tag formats, e.g. for ID3v2 "TDRC" is used, which is the recording time. --- examples/matroskawriter.cpp | 8 +-- taglib/matroska/ebml/ebmlmkattachments.cpp | 14 +---- taglib/matroska/matroskaattachedfile.cpp | 37 +++--------- taglib/matroska/matroskaattachedfile.h | 30 ++-------- taglib/matroska/matroskafile.cpp | 10 +--- taglib/matroska/matroskasimpletag.cpp | 4 +- taglib/matroska/matroskatag.cpp | 33 +++++++++- taglib/matroska/matroskatag.h | 15 +++++ taglib/toolkit/propertymapping.dox | 2 +- tests/test_matroska.cpp | 70 ++++++++++------------ 10 files changed, 99 insertions(+), 124 deletions(-) diff --git a/examples/matroskawriter.cpp b/examples/matroskawriter.cpp index 70ad8a80..211cfe3d 100644 --- a/examples/matroskawriter.cpp +++ b/examples/matroskawriter.cpp @@ -36,12 +36,8 @@ int main(int argc, char *argv[]) TagLib::FileStream image(argv[2]); auto data = image.readBlock(image.length()); auto attachments = file.attachments(true); - TagLib::Matroska::AttachedFile attachedFile; - attachedFile.setFileName("cover.jpg"); - attachedFile.setMediaType("image/jpeg"); - attachedFile.setData(data); - //attachedFile.setUID(5081000385627515072ull); - attachments->addAttachedFile(attachedFile); + attachments->addAttachedFile(TagLib::Matroska::AttachedFile( + data, "cover.jpg", "image/jpeg")); file.save(); diff --git a/taglib/matroska/ebml/ebmlmkattachments.cpp b/taglib/matroska/ebml/ebmlmkattachments.cpp index f57c9492..34941833 100644 --- a/taglib/matroska/ebml/ebmlmkattachments.cpp +++ b/taglib/matroska/ebml/ebmlmkattachments.cpp @@ -73,17 +73,9 @@ std::unique_ptr EBML::MkAttachments::parse() const if(!(filename && data)) continue; - Matroska::AttachedFile file; - file.setFileName(*filename); - file.setData(*data); - if(description) - file.setDescription(*description); - if(mediaType) - file.setMediaType(*mediaType); - if(uid) - file.setUID(uid); - - attachments->addAttachedFile(file); + attachments->addAttachedFile(Matroska::AttachedFile( + *data, *filename, mediaType ? *mediaType : String(), + uid, description ? *description : String())); } return attachments; } diff --git a/taglib/matroska/matroskaattachedfile.cpp b/taglib/matroska/matroskaattachedfile.cpp index f9fdf9bf..7957e86e 100644 --- a/taglib/matroska/matroskaattachedfile.cpp +++ b/taglib/matroska/matroskaattachedfile.cpp @@ -19,7 +19,6 @@ ***************************************************************************/ #include "matroskaattachedfile.h" -#include "tstring.h" #include "tbytevector.h" using namespace TagLib; @@ -27,7 +26,10 @@ using namespace TagLib; class Matroska::AttachedFile::AttachedFilePrivate { public: - AttachedFilePrivate() = default; + AttachedFilePrivate(const ByteVector &data, const String &fileName, + const String &mediaType, UID uid, const String &description) : + fileName(fileName), description(description), mediaType(mediaType), + data(data), uid(uid) {} ~AttachedFilePrivate() = default; String fileName; String description; @@ -40,8 +42,10 @@ public: // public members //////////////////////////////////////////////////////////////////////////////// -Matroska::AttachedFile::AttachedFile() : - d(std::make_unique()) +Matroska::AttachedFile::AttachedFile(const ByteVector &data, + const String &fileName, const String &mediaType, UID uid, + const String &description) : + d(std::make_unique(data, fileName, mediaType, uid, description)) { } @@ -69,51 +73,26 @@ void Matroska::AttachedFile::swap(AttachedFile &other) noexcept swap(d, other.d); } -void Matroska::AttachedFile::setFileName(const String &fileName) -{ - d->fileName = fileName; -} - const String &Matroska::AttachedFile::fileName() const { return d->fileName; } -void Matroska::AttachedFile::setDescription(const String &description) -{ - d->description = description; -} - const String &Matroska::AttachedFile::description() const { return d->description; } -void Matroska::AttachedFile::setMediaType(const String &mediaType) -{ - d->mediaType = mediaType; -} - const String &Matroska::AttachedFile::mediaType() const { return d->mediaType; } -void Matroska::AttachedFile::setData(const ByteVector &data) -{ - d->data = data; -} - const ByteVector &Matroska::AttachedFile::data() const { return d->data; } -void Matroska::AttachedFile::setUID(UID uid) -{ - d->uid = uid; -} - Matroska::AttachedFile::UID Matroska::AttachedFile::uid() const { return d->uid; diff --git a/taglib/matroska/matroskaattachedfile.h b/taglib/matroska/matroskaattachedfile.h index 7df5f5e7..2b7a2964 100644 --- a/taglib/matroska/matroskaattachedfile.h +++ b/taglib/matroska/matroskaattachedfile.h @@ -22,6 +22,7 @@ #define TAGLIB_MATROSKAATTACHEDFILE_H #include +#include "tstring.h" #include "taglib_export.h" namespace TagLib { @@ -39,7 +40,9 @@ namespace TagLib { /*! * Construct an attached file. */ - AttachedFile(); + AttachedFile(const ByteVector &data, const String &fileName, + const String &mediaType, UID uid = 0, + const String &description = String()); /*! * Construct an attached file as a copy of \a other. @@ -71,51 +74,26 @@ namespace TagLib { */ void swap(AttachedFile &other) noexcept; - /*! - * Set the \a fileName of the attached file. - */ - void setFileName(const String &fileName); - /*! * Returns the filename of the attached file. */ const String &fileName() const; - /*! - * Set a human-friendly \a description for the attached file. - */ - void setDescription(const String &description); - /*! * Returns the human-friendly description for the attached file. */ const String &description() const; - /*! - * Set the \a mediaType of the attached file. - */ - void setMediaType(const String &mediaType); - /*! * Returns the media type of the attached file. */ const String &mediaType() const; - /*! - * Set the data of the attached file. - */ - void setData(const ByteVector &data); - /*! * Returns the data of the attached file. */ const ByteVector &data() const; - /*! - * Set the \a uid representing the file, as random as possible. - */ - void setUID(UID uid); - /*! * Returns the UID of the attached file. */ diff --git a/taglib/matroska/matroskafile.cpp b/taglib/matroska/matroskafile.cpp index e27f389d..e21278be 100644 --- a/taglib/matroska/matroskafile.cpp +++ b/taglib/matroska/matroskafile.cpp @@ -335,13 +335,9 @@ bool Matroska::File::setComplexProperties(const String &key, const List()); - attachedFile.setFileName(fileName); - attachedFile.setUID(uid); - d->attachments->addAttachedFile(attachedFile); + d->attachments->addAttachedFile(AttachedFile( + data, fileName, mimeType, uid, + property.value("description").value())); } } return true; diff --git a/taglib/matroska/matroskasimpletag.cpp b/taglib/matroska/matroskasimpletag.cpp index a76833bd..1eb9a381 100644 --- a/taglib/matroska/matroskasimpletag.cpp +++ b/taglib/matroska/matroskasimpletag.cpp @@ -28,12 +28,12 @@ using namespace TagLib; class Matroska::SimpleTag::SimpleTagPrivate { public: - explicit SimpleTagPrivate(const String &name, const String &value, + SimpleTagPrivate(const String &name, const String &value, TargetTypeValue targetTypeValue, const String &language, bool defaultLanguage, unsigned long long trackUid) : value(value), name(name), language(language), trackUid(trackUid), targetTypeValue(targetTypeValue), defaultLanguageFlag(defaultLanguage) {} - explicit SimpleTagPrivate(const String &name, const ByteVector &value, + SimpleTagPrivate(const String &name, const ByteVector &value, TargetTypeValue targetTypeValue, const String &language, bool defaultLanguage, unsigned long long trackUid) : value(value), name(name), language(language), trackUid(trackUid), diff --git a/taglib/matroska/matroskatag.cpp b/taglib/matroska/matroskatag.cpp index c9d260e7..eaec2da7 100644 --- a/taglib/matroska/matroskatag.cpp +++ b/taglib/matroska/matroskatag.cpp @@ -89,6 +89,35 @@ void Matroska::Tag::addSimpleTag(const SimpleTag &tag) setNeedsRender(true); } +void Matroska::Tag::addSimpleTags(const SimpleTagsList& simpleTags) +{ + d->tags.append(simpleTags); + setNeedsRender(true); +} + +void Matroska::Tag::insertSimpleTag(unsigned int index, const SimpleTag &tag) +{ + if(index < d->tags.size()) { + auto it = d->tags.begin(); + std::advance(it, index); + d->tags.insert(it, tag); + } + else { + d->tags.append(tag); + } + setNeedsRender(true); +} + +void Matroska::Tag::removeSimpleTag(unsigned int index) +{ + if(index < d->tags.size()) { + auto it = d->tags.begin(); + std::advance(it, index); + d->tags.erase(it); + setNeedsRender(true); + } +} + void Matroska::Tag::removeSimpleTag(const String &name, SimpleTag::TargetTypeValue targetTypeValue, unsigned long long trackUid) @@ -323,7 +352,7 @@ namespace std::tuple("DISCNUMBER", "PART_NUMBER", Matroska::SimpleTag::TargetTypeValue::Album, true), std::tuple("TRACKTOTAL", "TOTAL_PARTS", Matroska::SimpleTag::TargetTypeValue::Track, false), std::tuple("DISCTOTAL", "TOTAL_PARTS", Matroska::SimpleTag::TargetTypeValue::Album, true), - std::tuple("DATE", "DATE_RELEASED", Matroska::SimpleTag::TargetTypeValue::Album, false), + std::tuple("DATE", "DATE_RECORDED", Matroska::SimpleTag::TargetTypeValue::Track, false), std::tuple("TITLESORT", "TITLESORT", Matroska::SimpleTag::TargetTypeValue::Track, false), std::tuple("ALBUMSORT", "TITLESORT", Matroska::SimpleTag::TargetTypeValue::Album, true), std::tuple("ARTISTSORT", "ARTISTSORT", Matroska::SimpleTag::TargetTypeValue::Track, false), @@ -334,7 +363,7 @@ namespace std::tuple("DJMIXER", "MIXED_BY", Matroska::SimpleTag::TargetTypeValue::Track, false), std::tuple("REMIXER", "REMIXED_BY", Matroska::SimpleTag::TargetTypeValue::Track, false), std::tuple("INITIALKEY", "INITIAL_KEY", Matroska::SimpleTag::TargetTypeValue::Track, false), - std::tuple("RELEASEDATE", "DATE_RELEASED", Matroska::SimpleTag::TargetTypeValue::Track, false), + std::tuple("RELEASEDATE", "DATE_RELEASED", Matroska::SimpleTag::TargetTypeValue::Album, false), std::tuple("ENCODINGTIME", "DATE_ENCODED", Matroska::SimpleTag::TargetTypeValue::Track, false), std::tuple("TAGGINGDATE", "DATE_TAGGED", Matroska::SimpleTag::TargetTypeValue::Track, false), std::tuple("ENCODEDBY", "ENCODER", Matroska::SimpleTag::TargetTypeValue::Track, false), diff --git a/taglib/matroska/matroskatag.h b/taglib/matroska/matroskatag.h index 57aba7d7..21696cc0 100644 --- a/taglib/matroska/matroskatag.h +++ b/taglib/matroska/matroskatag.h @@ -99,6 +99,21 @@ namespace TagLib { */ void addSimpleTag(const SimpleTag &tag); + /*! + * Add multiple tag attributes. + */ + void addSimpleTags(const SimpleTagsList &simpleTags); + + /*! + * Insert a tag attribute at position \a index. + */ + void insertSimpleTag(unsigned int index, const SimpleTag &tag); + + /*! + * Remove a tag attribute at position \a index. + */ + void removeSimpleTag(unsigned int index); + /*! * Remove a tag attribute. */ diff --git a/taglib/toolkit/propertymapping.dox b/taglib/toolkit/propertymapping.dox index b3fcc68e..43f32bd3 100644 --- a/taglib/toolkit/propertymapping.dox +++ b/taglib/toolkit/propertymapping.dox @@ -34,7 +34,7 @@ | CONDUCTOR | TPE3 | | \----:com.apple.iTunes:CONDUCTOR | | WM/Conductor | | | COPYRIGHT | TCOP | ICOP | cprt | | | | | COPYRIGHTURL | WCOP | | | | | | -| DATE | TDRC | ICRD | ©day | YEAR | WM/Year | DATE_RELEASED/50 | +| DATE | TDRC | ICRD | ©day | YEAR | WM/Year | DATE_RECORDED/30 | | DISCNUMBER | TPOS | | disk | DISC | WM/PartOfSet | PART_NUMBER/50 | | DISCSUBTITLE | TSST | PRT1 | \----:com.apple.iTunes:DISCSUBTITLE | | WM/SetSubTitle | | | DISCTOTAL | | | | | | TOTAL_PARTS/50 | diff --git a/tests/test_matroska.cpp b/tests/test_matroska.cpp index f932039f..b3fd23d7 100644 --- a/tests/test_matroska.cpp +++ b/tests/test_matroska.cpp @@ -104,7 +104,7 @@ * * - Set simple tag with target type in a Matroska file: * examples/tagwriter -C PART_NUMBER \ - * name=PART_NUMBER,targetTypeValue=20,value=2 file.mka + * 'name=PART_NUMBER,targetTypeValue=20,value="2"' file.mka * * - Set simple tag with binary value in a Matroska file: * examples/tagwriter -C BINARY \ @@ -218,23 +218,22 @@ public: CPPUNIT_ASSERT(!f.attachments(false)); auto tag = f.tag(true); CPPUNIT_ASSERT(tag->isEmpty()); - tag->addSimpleTag(Matroska::SimpleTag( - "Test Name 1", String("Test Value 1"), - Matroska::SimpleTag::TargetTypeValue::Track, "en")); tag->addSimpleTag(Matroska::SimpleTag( "Test Name 2", String("Test Value 2"), Matroska::SimpleTag::TargetTypeValue::Album)); + tag->insertSimpleTag(0, Matroska::SimpleTag( + "Test Name 1", String("Test Value 1"), + Matroska::SimpleTag::TargetTypeValue::Track, "en")); + tag->insertSimpleTag(1, Matroska::SimpleTag( + "Test Name 3", String("Test Value 3"))); + tag->removeSimpleTag(1); tag->setTitle("Test title"); tag->setArtist("Test artist"); tag->setYear(1969); auto attachments = f.attachments(true); - Matroska::AttachedFile attachedFile; - attachedFile.setFileName("cover.jpg"); - attachedFile.setMediaType("image/jpeg"); - attachedFile.setDescription("Cover"); - attachedFile.setData(ByteVector("JPEG data")); - attachedFile.setUID(5081000385627515072ULL); - attachments->addAttachedFile(attachedFile); + attachments->addAttachedFile(Matroska::AttachedFile( + "JPEG data", "cover.jpg", "image/jpeg", 5081000385627515072ULL, + "Cover")); CPPUNIT_ASSERT(f.save()); } { @@ -285,17 +284,17 @@ public: CPPUNIT_ASSERT_EQUAL(Matroska::SimpleTag::ValueType::StringType, simpleTags[2].type()); CPPUNIT_ASSERT_EQUAL(String("und"), simpleTags[3].language()); - CPPUNIT_ASSERT_EQUAL(String("Test Name 2"), simpleTags[3].name()); - CPPUNIT_ASSERT_EQUAL(String("Test Value 2"), simpleTags[3].toString()); + CPPUNIT_ASSERT_EQUAL(String("DATE_RECORDED"), simpleTags[3].name()); + CPPUNIT_ASSERT_EQUAL(String("1969"), simpleTags[3].toString()); CPPUNIT_ASSERT_EQUAL(ByteVector(), simpleTags[3].toByteVector()); CPPUNIT_ASSERT_EQUAL(true, simpleTags[3].defaultLanguageFlag()); - CPPUNIT_ASSERT_EQUAL(Matroska::SimpleTag::TargetTypeValue::Album, simpleTags[3].targetTypeValue()); + CPPUNIT_ASSERT_EQUAL(Matroska::SimpleTag::TargetTypeValue::Track, simpleTags[3].targetTypeValue()); CPPUNIT_ASSERT_EQUAL(0ULL, simpleTags[3].trackUid()); CPPUNIT_ASSERT_EQUAL(Matroska::SimpleTag::ValueType::StringType, simpleTags[3].type()); CPPUNIT_ASSERT_EQUAL(String("und"), simpleTags[4].language()); - CPPUNIT_ASSERT_EQUAL(String("DATE_RELEASED"), simpleTags[4].name()); - CPPUNIT_ASSERT_EQUAL(String("1969"), simpleTags[4].toString()); + CPPUNIT_ASSERT_EQUAL(String("Test Name 2"), simpleTags[4].name()); + CPPUNIT_ASSERT_EQUAL(String("Test Value 2"), simpleTags[4].toString()); CPPUNIT_ASSERT_EQUAL(ByteVector(), simpleTags[4].toByteVector()); CPPUNIT_ASSERT_EQUAL(true, simpleTags[4].defaultLanguageFlag()); CPPUNIT_ASSERT_EQUAL(Matroska::SimpleTag::TargetTypeValue::Album, simpleTags[4].targetTypeValue()); @@ -383,7 +382,8 @@ public: Matroska::File f(newname.c_str(), true, AudioProperties::Accurate); CPPUNIT_ASSERT(f.isValid()); CPPUNIT_ASSERT(!f.tag(false)); - f.attachments(true)->addAttachedFile(Matroska::AttachedFile()); + f.attachments(true)->addAttachedFile(Matroska::AttachedFile( + ByteVector(), "", "")); CPPUNIT_ASSERT(f.save()); } { @@ -487,7 +487,7 @@ public: PropertyMap initialProps; initialProps["ARTIST"] = StringList("Actors"); - initialProps["DATE"] = StringList("2023"); + initialProps["RELEASEDATE"] = StringList("2023"); initialProps["DESCRIPTION"] = StringList("Description"); initialProps["DIRECTOR"] = StringList("Director"); initialProps["ENCODEDBY"] = StringList("Lavf59.27.100"); @@ -562,7 +562,6 @@ public: CPPUNIT_ASSERT_EQUAL(String("handbrake"), tag->title()); CPPUNIT_ASSERT_EQUAL(String("Actors"), tag->artist()); - CPPUNIT_ASSERT_EQUAL(2023U, tag->year()); CPPUNIT_ASSERT_EQUAL(String(""), tag->album()); CPPUNIT_ASSERT_EQUAL(String(""), tag->comment()); CPPUNIT_ASSERT_EQUAL(String("Genre"), tag->genre()); @@ -695,15 +694,14 @@ public: } const StringList expectedSimpleTagNames { "BINARY", "TITLE", "ARTIST", "ARTISTSORT", "TITLESORT", - "DATE_RELEASED", "PART_NUMBER", "TOTAL_PARTS", - "MUSICBRAINZ_ALBUMARTISTID", "MUSICBRAINZ_ALBUMID", - "MUSICBRAINZ_RELEASEGROUPID", "ARTIST", "ARTISTS", "ARTISTSORT", + "PART_NUMBER", "TOTAL_PARTS", "MUSICBRAINZ_ALBUMARTISTID", "MUSICBRAINZ_ALBUMID", + "MUSICBRAINZ_RELEASEGROUPID", "DATE_RELEASED", "ARTIST", "ARTISTS", "ARTISTSORT", "ASIN", "BARCODE", "CATALOG_NUMBER", "CATALOG_NUMBER", "COMMENT", - "MIXED_BY", "ENCODER", "ENCODER_SETTINGS", "DATE_ENCODED", "GENRE", - "INITIAL_KEY", "ISRC", "LABEL_CODE", "LABEL_CODE", + "DATE_RECORDED", "MIXED_BY", "ENCODER", "ENCODER_SETTINGS", "DATE_ENCODED", + "GENRE", "INITIAL_KEY", "ISRC", "LABEL_CODE", "LABEL_CODE", "ORIGINAL_MEDIA_TYPE", "MUSICBRAINZ_ARTISTID", "MUSICBRAINZ_RELEASETRACKID", "MUSICBRAINZ_TRACKID", "ORIGINALDATE", - "PURCHASE_OWNER", "RELEASECOUNTRY", "DATE_RELEASED", "RELEASESTATUS", + "PURCHASE_OWNER", "RELEASECOUNTRY", "RELEASESTATUS", "RELEASETYPE", "REMIXED_BY", "SCRIPT", "DATE_TAGGED", "TITLE", "PART_NUMBER", "TOTAL_PARTS" }; @@ -960,14 +958,10 @@ public: CPPUNIT_ASSERT(f.tag(false)); CPPUNIT_ASSERT(!f.attachments(false)); auto attachments = f.attachments(true); - Matroska::AttachedFile attachedFile; - attachedFile.setFileName("cover.jpg"); - attachedFile.setMediaType("image/jpeg"); - attachedFile.setDescription("Cover"); // Large enough for emitSizeChanged() from Matroska::Segment::render() - attachedFile.setData(ByteVector(20000, 'x')); - attachedFile.setUID(5081000385627515072ULL); - attachments->addAttachedFile(attachedFile); + attachments->addAttachedFile(Matroska::AttachedFile( + ByteVector(20000, 'x'), "cover.jpg", "image/jpeg", + 5081000385627515072ULL, "Cover")); CPPUNIT_ASSERT(f.save()); } { @@ -979,11 +973,11 @@ public: CPPUNIT_ASSERT(attachments); CPPUNIT_ASSERT(PropertyMap(SimplePropertyMap{ {"ARTIST", {"Actors"}}, - {"DATE", {"2023"}}, {"DESCRIPTION", {"Description"}}, {"DIRECTOR", {"Director"}}, {"ENCODEDBY", {"Lavf59.27.100"}}, {"GENRE", {"Genre"}}, + {"RELEASEDATE", {"2023"}}, {"SUMMARY", {"Comment"}}, {"SYNOPSIS", {"Plot"}} }) == tag->properties()); @@ -1161,13 +1155,9 @@ public: chapters->addChapterEdition(edition1); chapters->addChapterEdition(edition2); - Matroska::AttachedFile attachedFile; - attachedFile.setFileName("folder.png"); - attachedFile.setMediaType("image/png"); - attachedFile.setDescription("Cover"); - attachedFile.setData(ByteVector("PNG data")); - attachedFile.setUID(1763187649ULL); - f.attachments(true)->addAttachedFile(attachedFile); + f.attachments(true)->addAttachedFile(Matroska::AttachedFile( + ByteVector("PNG data"), "folder.png", "image/png", 1763187649ULL, + "Cover")); CPPUNIT_ASSERT(f.save()); } {