From 53a80ba1664b61859b03fe2225f26aa3f8433f16 Mon Sep 17 00:00:00 2001 From: Ryan Francesconi <2917795+ryanfrancesconi@users.noreply.github.com> Date: Wed, 28 Oct 2026 09:29:16 -0700 Subject: [PATCH] ID3v2: Keep frames whose combined values are unchanged in setProperties (#1472) properties() merges the values of all frames mapping to the same key, in frame order, so a key can be spread over several frames, e.g. two comments without a description in different languages give COMMENT=["hello", "hallo"]. setProperties() only kept a frame if its own values equal the whole requested list, so none of these frames was kept, and the list was then written as a single TXXX:COMMENT frame. This made setProperties(properties()) lossy: the COMM frames and their languages were replaced by a frame no comment reader looks for. Before deciding which frames to delete, merge the values of the frames holding a single key in frameList() order, as properties() does. If the requested values for a key equal the merged ones, keep all of its frames and do not create a new frame for it. TIPL and TMCL frames, and every key whose values changed, are handled as before. testSetPropertiesKeepsSpreadFrames covers the identity round trip on two COMM frames, a request matching only one of them (kept as before), a changed list (still a single TXXX), a key held by a TXXX and a COMM frame (compared in frame order, not frame ID order), and a save and reread of an MPEG file. --- taglib/mpeg/id3v2/id3v2tag.cpp | 19 ++++++ tests/test_id3v2.cpp | 102 +++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/taglib/mpeg/id3v2/id3v2tag.cpp b/taglib/mpeg/id3v2/id3v2tag.cpp index 6fc7ade1..bfe75288 100644 --- a/taglib/mpeg/id3v2/id3v2tag.cpp +++ b/taglib/mpeg/id3v2/id3v2tag.cpp @@ -433,6 +433,23 @@ PropertyMap ID3v2::Tag::setProperties(const PropertyMap &origProps) PropertyMap tiplProperties; PropertyMap tmclProperties; Frame::splitProperties(origProps, singleFrameProperties, tiplProperties, tmclProperties); + // A key can be spread over several frames (e.g. comments in different + // languages), whose values properties() merges in frame order. Keep all of + // them if the requested values are the merged ones. + PropertyMap spreadProperties; + for(const auto &frame : std::as_const(frameList())) { + if(const ByteVector id = frame->frameID(); id != "TIPL" && id != "TMCL") { + if(PropertyMap frameProperties = frame->asProperties(); frameProperties.size() == 1) + spreadProperties.merge(frameProperties); + } + } + StringList keptKeys; + for(const auto &[key, values] : std::as_const(spreadProperties)) { + if(singleFrameProperties.contains(key) && singleFrameProperties[key] == values) { + keptKeys.append(key); + singleFrameProperties.erase(key); + } + } for(const auto &[tag, frames] : std::as_const(frameListMap())) { for(const auto &frame : frames) { PropertyMap frameProperties = frame->asProperties(); @@ -448,6 +465,8 @@ PropertyMap ID3v2::Tag::setProperties(const PropertyMap &origProps) else tmclProperties.erase(frameProperties); } + else if(frameProperties.size() == 1 && keptKeys.contains(frameProperties.begin()->first)) + continue; else if(!singleFrameProperties.contains(frameProperties)) framesToDelete.append(frame); else diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index fc90f4aa..502a755e 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -130,6 +130,7 @@ class TestID3v2 : public CppUnit::TestFixture CPPUNIT_TEST(testPropertyInterface2); CPPUNIT_TEST(testPropertiesMovement); CPPUNIT_TEST(testPropertyGrouping); + CPPUNIT_TEST(testSetPropertiesKeepsSpreadFrames); CPPUNIT_TEST(testDeleteFrame); CPPUNIT_TEST(testSaveAndStripID3v1ShouldNotAddFrameFromID3v1ToId3v2); CPPUNIT_TEST(testParseChapterFrame); @@ -1384,6 +1385,107 @@ public: tag.addFrame(parsedFrameGrp1); } + void testSetPropertiesKeepsSpreadFrames() + { + const auto addComment = [](ID3v2::Tag &tag, const ByteVector &language, const String &text) { + auto frame = new ID3v2::CommentsFrame; + frame->setLanguage(language); + frame->setText(text); + tag.addFrame(frame); + return frame; + }; + + { + // The combined values of both frames are unchanged, so both are kept. + ID3v2::Tag tag; + const auto eng = addComment(tag, "eng", "hello"); + const auto deu = addComment(tag, "deu", "hallo"); + CPPUNIT_ASSERT_EQUAL(StringList("hello").append("hallo"), + tag.properties()["COMMENT"]); + + tag.setProperties(tag.properties()); + const ID3v2::FrameList comments = tag.frameList("COMM"); + CPPUNIT_ASSERT_EQUAL(2u, comments.size()); + CPPUNIT_ASSERT_EQUAL(static_cast(eng), comments[0]); + CPPUNIT_ASSERT_EQUAL(static_cast(deu), comments[1]); + CPPUNIT_ASSERT_EQUAL(ByteVector("eng"), eng->language()); + CPPUNIT_ASSERT_EQUAL(String("hello"), eng->text()); + CPPUNIT_ASSERT_EQUAL(ByteVector("deu"), deu->language()); + CPPUNIT_ASSERT_EQUAL(String("hallo"), deu->text()); + CPPUNIT_ASSERT(tag.frameList("TXXX").isEmpty()); + } + { + // A single frame whose value is the whole list is kept, as before. + ID3v2::Tag tag; + const auto eng = addComment(tag, "eng", "hello"); + addComment(tag, "deu", "hallo"); + + PropertyMap properties; + properties["COMMENT"] = StringList("hello"); + tag.setProperties(properties); + CPPUNIT_ASSERT_EQUAL(1u, tag.frameList("COMM").size()); + CPPUNIT_ASSERT_EQUAL(static_cast(eng), tag.frameList("COMM").front()); + CPPUNIT_ASSERT(tag.frameList("TXXX").isEmpty()); + } + { + // A changed list is still written as one frame. + ID3v2::Tag tag; + addComment(tag, "eng", "hello"); + addComment(tag, "deu", "hallo"); + + PropertyMap properties; + properties["COMMENT"] = StringList("a").append("b"); + tag.setProperties(properties); + CPPUNIT_ASSERT(tag.frameList("COMM").isEmpty()); + CPPUNIT_ASSERT_EQUAL(1u, tag.frameList("TXXX").size()); + CPPUNIT_ASSERT(ID3v2::UserTextIdentificationFrame::find(&tag, "COMMENT")); + CPPUNIT_ASSERT_EQUAL(properties["COMMENT"], tag.properties()["COMMENT"]); + } + { + // Values are compared in the order properties() returns them, which + // follows the frames rather than their IDs. + ID3v2::Tag tag; + tag.addFrame(new ID3v2::UserTextIdentificationFrame("COMMENT", StringList("first"))); + addComment(tag, "eng", "second"); + CPPUNIT_ASSERT_EQUAL(StringList("first").append("second"), + tag.properties()["COMMENT"]); + + tag.setProperties(tag.properties()); + CPPUNIT_ASSERT_EQUAL(1u, tag.frameList("TXXX").size()); + CPPUNIT_ASSERT_EQUAL(1u, tag.frameList("COMM").size()); + CPPUNIT_ASSERT_EQUAL(StringList("first").append("second"), + tag.properties()["COMMENT"]); + } + + ScopedFileCopy copy("xing", ".mp3"); + { + MPEG::File f(copy.fileName().c_str()); + ID3v2::Tag *tag = f.ID3v2Tag(true); + addComment(*tag, "eng", "hello"); + addComment(*tag, "deu", "hallo"); + f.save(); + } + { + MPEG::File f(copy.fileName().c_str()); + f.setProperties(f.properties()); + f.save(); + } + { + MPEG::File f(copy.fileName().c_str()); + const ID3v2::FrameList comments = f.ID3v2Tag()->frameList("COMM"); + CPPUNIT_ASSERT_EQUAL(2u, comments.size()); + auto eng = dynamic_cast(comments[0]); + auto deu = dynamic_cast(comments[1]); + CPPUNIT_ASSERT(eng); + CPPUNIT_ASSERT(deu); + CPPUNIT_ASSERT_EQUAL(ByteVector("eng"), eng->language()); + CPPUNIT_ASSERT_EQUAL(String("hello"), eng->text()); + CPPUNIT_ASSERT_EQUAL(ByteVector("deu"), deu->language()); + CPPUNIT_ASSERT_EQUAL(String("hallo"), deu->text()); + CPPUNIT_ASSERT(f.ID3v2Tag()->frameList("TXXX").isEmpty()); + } + } + void testDeleteFrame() { ScopedFileCopy copy("rare_frames", ".mp3");