From 80e42828a8c1724bde2643a54a72f97a76a6c48f Mon Sep 17 00:00:00 2001 From: Ryan Francesconi <2917795+ryanfrancesconi@users.noreply.github.com> Date: Sat, 31 Oct 2026 22:29:52 -0700 Subject: [PATCH] ID3v2: Update an edited frame in place in setProperties (#1481) When exactly one frame holds an edited key and its type can hold the new value, setProperties() now updates that frame instead of deleting it and appending a new one. A COMM or USLT keeps its language, a TXXX its description's spelling, and each keeps its position and its stored text encoding, which is promoted at render when the new text needs it. --- taglib/mpeg/id3v2/id3v2tag.cpp | 59 ++++++++++++++++++++- tests/test_id3v2.cpp | 93 ++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 2 deletions(-) diff --git a/taglib/mpeg/id3v2/id3v2tag.cpp b/taglib/mpeg/id3v2/id3v2tag.cpp index bfe75288..aa7d7e04 100644 --- a/taglib/mpeg/id3v2/id3v2tag.cpp +++ b/taglib/mpeg/id3v2/id3v2tag.cpp @@ -58,6 +58,46 @@ namespace constexpr long MaxPaddingSize = 1024 * 1024; constexpr unsigned int MAX_ID3V2_FRAME_COUNT = 50000; + /*! + * Sets \a values as the value of \a frame, the only frame holding \a key, + * if its type can hold them, keeping its other fields. + * \return false, leaving \a frame unchanged, if it cannot. + */ + bool updateFrameInPlace(ID3v2::Frame *frame, const String &key, const StringList &values) + { + if(auto f = dynamic_cast(frame)) { + f->setText(values); + return true; + } + if(auto f = dynamic_cast(frame)) { + if(ID3v2::Frame::keyToFrameID(key) != frame->frameID()) + return false; + f->setText(values); + return true; + } + if(values.size() != 1) + return false; + if(auto f = dynamic_cast(frame)) { + f->setText(values.front()); + return true; + } + if(auto f = dynamic_cast(frame)) { + f->setText(values.front()); + return true; + } + if(auto f = dynamic_cast(frame)) { + f->setUrl(values.front()); + return true; + } + if(auto f = dynamic_cast(frame)) { + if(ID3v2::Frame::keyToFrameID(key) != frame->frameID()) + return false; + f->setUrl(values.front()); + return true; + } + return false; + } + /*! * Downgrade ID3v2.4 text \a encoding to value supported by ID3v2.3. */ @@ -437,9 +477,13 @@ PropertyMap ID3v2::Tag::setProperties(const PropertyMap &origProps) // languages), whose values properties() merges in frame order. Keep all of // them if the requested values are the merged ones. PropertyMap spreadProperties; + Map framesPerKey; 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) + PropertyMap frameProperties = frame->asProperties(); + for(const auto &[key, values] : std::as_const(frameProperties)) + framesPerKey[key] += 1; + if(frameProperties.size() == 1) spreadProperties.merge(frameProperties); } } @@ -467,8 +511,19 @@ PropertyMap ID3v2::Tag::setProperties(const PropertyMap &origProps) } else if(frameProperties.size() == 1 && keptKeys.contains(frameProperties.begin()->first)) continue; - else if(!singleFrameProperties.contains(frameProperties)) + else if(!singleFrameProperties.contains(frameProperties)) { + // The only frame holding an edited key keeps its other fields if it can hold the new value. + if(frameProperties.size() == 1) { + const String &key = frameProperties.begin()->first; + if(framesPerKey[key] == 1 && singleFrameProperties.contains(key) && + !singleFrameProperties[key].isEmpty() && + updateFrameInPlace(frame, key, singleFrameProperties[key])) { + singleFrameProperties.erase(key); + continue; + } + } framesToDelete.append(frame); + } else singleFrameProperties.erase(frameProperties); } diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index ba2127a4..b23ef414 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -131,6 +131,7 @@ class TestID3v2 : public CppUnit::TestFixture CPPUNIT_TEST(testPropertiesMovement); CPPUNIT_TEST(testPropertyGrouping); CPPUNIT_TEST(testSetPropertiesKeepsSpreadFrames); + CPPUNIT_TEST(testSetPropertiesUpdatesFrameInPlace); CPPUNIT_TEST(testDeleteFrame); CPPUNIT_TEST(testSaveAndStripID3v1ShouldNotAddFrameFromID3v1ToId3v2); CPPUNIT_TEST(testParseChapterFrame); @@ -1487,6 +1488,98 @@ public: } } + void testSetPropertiesUpdatesFrameInPlace() + { + { + // An edited value goes into the frame that held it, which keeps its + // language, description, position and encoding. + ID3v2::Tag tag; + auto title = new ID3v2::TextIdentificationFrame("TIT2", String::Latin1); + title->setText("Title"); + tag.addFrame(title); + auto comment = new ID3v2::CommentsFrame(String::Latin1); + comment->setLanguage("deu"); + comment->setText("hallo"); + tag.addFrame(comment); + auto lyrics = new ID3v2::UnsynchronizedLyricsFrame(String::Latin1); + lyrics->setLanguage("deu"); + lyrics->setText("la la"); + tag.addFrame(lyrics); + auto userText = new ID3v2::UserTextIdentificationFrame("MyTag", StringList("x"), + String::Latin1); + tag.addFrame(userText); + + PropertyMap properties = tag.properties(); + CPPUNIT_ASSERT(properties.contains("LYRICS")); + CPPUNIT_ASSERT(properties.contains("MYTAG")); + properties["TITLE"] = StringList("Title, edited"); + properties["COMMENT"] = StringList("tschuess"); + properties["LYRICS"] = StringList("la la la"); + properties["MYTAG"] = StringList("y"); + CPPUNIT_ASSERT(tag.setProperties(properties).isEmpty()); + + const ID3v2::FrameList frames = tag.frameList(); + CPPUNIT_ASSERT_EQUAL(4u, frames.size()); + CPPUNIT_ASSERT_EQUAL(static_cast(title), frames[0]); + CPPUNIT_ASSERT_EQUAL(static_cast(comment), frames[1]); + CPPUNIT_ASSERT_EQUAL(static_cast(lyrics), frames[2]); + CPPUNIT_ASSERT_EQUAL(static_cast(userText), frames[3]); + CPPUNIT_ASSERT_EQUAL(String("Title, edited"), title->toString()); + CPPUNIT_ASSERT_EQUAL(String::Latin1, title->textEncoding()); + CPPUNIT_ASSERT_EQUAL(ByteVector("deu"), comment->language()); + CPPUNIT_ASSERT_EQUAL(String("tschuess"), comment->text()); + CPPUNIT_ASSERT_EQUAL(ByteVector("deu"), lyrics->language()); + CPPUNIT_ASSERT_EQUAL(String("la la la"), lyrics->text()); + CPPUNIT_ASSERT_EQUAL(String("MyTag"), userText->description()); + CPPUNIT_ASSERT_EQUAL(StringList("y"), tag.properties()["MYTAG"]); + } + { + // A value the frame cannot hold, and a removed key, are handled as before. + ID3v2::Tag tag; + auto comment = new ID3v2::CommentsFrame; + comment->setLanguage("deu"); + comment->setText("hallo"); + tag.addFrame(comment); + tag.addFrame(new ID3v2::UserTextIdentificationFrame("MyTag", StringList("x"))); + + 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(!ID3v2::UserTextIdentificationFrame::find(&tag, "MyTag")); + } + + // A Latin-1 frame given text Latin-1 cannot hold is promoted when rendered. + ScopedFileCopy copy("xing", ".mp3"); + const String text("\xc5\x81\xc3\xb3" "d\xc5\xba \xe2\x9c\x93", String::UTF8); + { + MPEG::File f(copy.fileName().c_str()); + auto comment = new ID3v2::CommentsFrame(String::Latin1); + comment->setLanguage("deu"); + comment->setText("hallo"); + f.ID3v2Tag(true)->addFrame(comment); + f.save(MPEG::File::ID3v2, File::StripOthers, ID3v2::v3); + } + { + MPEG::File f(copy.fileName().c_str()); + PropertyMap properties = f.properties(); + properties["COMMENT"] = StringList(text); + f.setProperties(properties); + f.save(MPEG::File::ID3v2, File::StripOthers, ID3v2::v3); + } + { + MPEG::File f(copy.fileName().c_str()); + const ID3v2::FrameList comments = f.ID3v2Tag()->frameList("COMM"); + CPPUNIT_ASSERT_EQUAL(1u, comments.size()); + auto comment = dynamic_cast(comments.front()); + CPPUNIT_ASSERT(comment); + CPPUNIT_ASSERT_EQUAL(ByteVector("deu"), comment->language()); + CPPUNIT_ASSERT_EQUAL(text, comment->text()); + } + } + void testDeleteFrame() { ScopedFileCopy copy("rare_frames", ".mp3");