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");