mirror/taglib
1
0
Fork 0
mirror of https://github.com/taglib/taglib.git synced 2026-10-30 20:08:51 -04:00
Code Issues Packages Projects Releases Wiki Activity

ID3v2: Keep frames whose combined values are unchanged in setProperties (#1472)

Browse Source 
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.
This commit is contained in:
Ryan Francesconi
2026-10-28 17:29:16 +01:00
committed by GitHub
parent 9d09f9b413
commit 53a80ba166
2 changed files with 121 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
taglib/mpeg/id3v2/id3v2tag.cpp
+19
View File
@@ -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
tests/test_id3v2.cpp
+102
View File
@@ -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<ID3v2::Frame *>(eng), comments[0]);
CPPUNIT_ASSERT_EQUAL(static_cast<ID3v2::Frame *>(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<ID3v2::Frame *>(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<ID3v2::CommentsFrame *>(comments[0]);
auto deu = dynamic_cast<ID3v2::CommentsFrame *>(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");