More progress in ID3 ... setProperties() will get messy :(

taglib/mpeg/id3v2/frames/textidentificationframe.cpp

@@ -57,6 +57,17 @@ TextIdentificationFrame::TextIdentificationFrame(const ByteVector &data) :
   setData(data);
 }
 
+TextIdentificationFrame *TextIdentificationFrame::createTIPLFrame(const PropertyMap &properties) // static
+{
+  TextIdentificationFrame* frame = TextIdentificationFrame("TIPL");
+  StringList l;
+  for(PropertyMap::ConstIterator it = properties.begin(); it != properties.end(); ++it){
+    l.append(it->first);
+    l.append(it->second.toString(",")); // comma-separated list of names
+  }
+  frame->setText(l);
+  return frame;
+}
 TextIdentificationFrame::~TextIdentificationFrame()
 {
   delete d;
@@ -92,6 +103,25 @@ void TextIdentificationFrame::setTextEncoding(String::Type encoding)
   d->textEncoding = encoding;
 }
 
+// array of allowed TIPL prefixes and their corresponding key value
+static const uint involvedPeopleSize = 5;
+static const char* involvedPeople[2] = {
+    {"ARRANGER", "ARRANGER"},
+    {"ENGINEER", "ENGINEER"},
+    {"PRODUCER", "PRODUCER"},
+    {"DJ-MIX", "DJMIXER"},
+    {"MIX", "MIXER"}
+};
+
+const KeyConversionMap &TextIdentificationFrame::involvedPeopleMap() // static
+{
+  static KeyConversionMap m;
+  if(m.isEmpty())
+    for(uint i = 0; i < involvedPeopleSize; ++i)
+      m.insert(involvedPeople[i][1], involvedPeople[i][0]);
+  return m;
+}
+
 PropertyMap TextIdentificationFrame::asProperties() const
 {
   if(frameID() == "TIPL")
@@ -204,16 +234,6 @@ TextIdentificationFrame::TextIdentificationFrame(const ByteVector &data, Header
   parseFields(fieldData(data));
 }
 
-// array of allowed TIPL prefixes and their corresponding key value
-static const uint involvedPeopleSize = 5;
-static const char* involvedPeople[2] = {
-    {"ARRANGER", "ARRANGER"},
-    {"ENGINEER", "ENGINEER"},
-    {"PRODUCER", "PRODUCER"},
-    {"DJ-MIX", "DJMIXER"},
-    {"MIX", "MIXER"}
-};
-
 PropertyMap TextIdentificationFrame::makeTIPLProperties() const
 {
   PropertyMap map;
@@ -249,14 +269,14 @@ PropertyMap TextIdentificationFrame::makeTMCLProperties() const
     return map;
   }
   for(StringList::ConstIterator it = fieldList().begin(); it != fieldList().end(); ++it) {
-    String key = PropertyMap::prepareKey(*it);
-    if(key.isNull()) {
+    String instrument = PropertyMap::prepareKey(*it);
+    if(instrument.isNull()) {
       // instrument is not a valid key -> frame unsupported
       map.clear();
       map.unsupportedData().append(frameID());
       return map;
     }
-    map.insert(key, (++it).split(","));
+    map.insert(L"PERFORMER:" + instrument, (++it).split(","));
   }
   return map;
 }

taglib/mpeg/id3v2/frames/textidentificationframe.h

@@ -36,6 +36,7 @@ namespace TagLib {
   namespace ID3v2 {
 
     class Tag;
+    typedef Map<String, String> KeyConversionMap;
 
     //! An ID3v2 text identification frame implementation
 
@@ -123,6 +124,13 @@ namespace TagLib {
        */
       explicit TextIdentificationFrame(const ByteVector &data);
 
+      /*!
+       * This is a special factory method to create a TIPL (involved people list)
+       * frame from the given \a properties. Will parse key=[list of values] data
+       * into the TIPL format as specified in the ID3 standard.
+       */
+      static TextIdentificationFrame *createTIPLFrame(const PropertyMap &properties);
+
       /*!
        * Destroys this TextIdentificationFrame instance.
        */
@@ -173,6 +181,11 @@ namespace TagLib {
        */
       StringList fieldList() const;
 
+      /*!
+       * Returns a KeyConversionMap mapping a role as it would be  used in a PropertyMap
+       * to the corresponding key used in a TIPL ID3 frame to describe that role.
+       */
+      static const KeyConversionMap &involvedPeopleMap();
       PropertyMap asProperties() const;
 
     protected:

taglib/mpeg/id3v2/frames/urllinkframe.cpp

@@ -153,10 +153,12 @@ void UserUrlLinkFrame::setDescription(const String &s)
 
 PropertyMap UserUrlLinkFrame::asProperties() const
 {
-  String key = PropertyMap::prepareKey(description());
   PropertyMap map;
-  if(key.isEmpty())
+  String key;
+  if(description().isEmpty())
      key = "URL";
+  else
+    key = PropertyMap::prepareKey(description());
   if(key.isNull())
     map.unsupportedData().append(L"WXXX/" + description());
   else

taglib/mpeg/id3v2/id3v2frame.cpp

@@ -263,7 +263,7 @@ String::Type Frame::checkTextEncoding(const StringList &fields, String::Type enc
   return checkEncoding(fields, encoding, header()->version());
 }
 
-static const uint frameTranslationSize = 53;
+static const uint frameTranslationSize = 50;
 static const char *frameTranslation[][2] = {
   // Text information frames
   { "TALB", "ALBUM"},
@@ -323,10 +323,10 @@ static const char *frameTranslation[][2] = {
   { "WORS", "RADIOSTATIONWEBPAGE" },
   { "WPAY", "PAYMENTWEBPAGE" },
   { "WPUB", "PUBLISHERWEBPAGE" },
-  { "WXXX", "URL"},
+  //{ "WXXX", "URL"}, handled specially
   // Other frames
-  { "COMM", "COMMENT" },
-  { "USLT", "LYRICS" },
+  //{ "COMM", "COMMENT" }, handled specially
+  //{ "USLT", "LYRICS" }, handled specially
 };
 
 Map<ByteVector, String> &idMap()

taglib/mpeg/id3v2/id3v2tag.cpp

@@ -342,27 +342,161 @@ PropertyMap ID3v2::Tag::properties() const
   return properties;
 }
 
-PropertyMap ID3v2::Tag::setProperties(const PropertyMap &properties)
+PropertyMap ID3v2::Tag::setProperties(const PropertyMap &origProps)
 {
   FrameList toRemove;
+  PropertyMap properties = origProps;
   // first find out what frames to remove; we do not remove in-place
   // because that would invalidate FrameListMap iterators.
-  //
-  for (FrameListMap::ConstIterator it = frameListMap().begin(); it != frameListMap().end(); ++it)
-    // Remove all frames which are not ignored
-    if (it->second.size() == 0 || !ignored(it->first) )
-      toRemove.append(it->second);
+  // At the moment, we remove _all_ frames that don't contain unsupported data,
+  // and create new ones in the next step; this is to avoid clumsy technicalities
+  // arising when trying to do this more efficient. For example, if the current tag
+  // contains one URL attribute stored in an WXXX frame, but the given \a properties
+  // contain two URL values, we would need to remove the WXXX frame (which supports
+  // only one value), and create a TXXX frame with description=URL.
+  // The same may happen with COMM and USLT. Additionally, handling of TIPL and TMCL is
+  // complicated.
+  // In the future, someone might come up with a more clever sync algorithm. :-)
+  for(FrameListMap::ConstIterator it = frameListMap().begin(); it != frameListMap().end(); ++it) {
+    String key = Frame::frameIDToKey(it->first);
+    // for unsupported (binary) frame types, as well as frames that need special treatment
+    // (TXXX, WXXX, COMM, TMCL, TIPL, USLT), key will be String::null
+    if(key.isNull())
+      continue;
+    // else: non-user text or url frame -> there should be only one frame of this type,
+    // and it's asProperties() method should return a PropertyMap with exactly one key
+    // and empty unsupportedData().
+    if(it->second.size() != 1)
+      debug("invalid ID3 tag: found more than one " + it->first + " frame");
+    PropertyMap frameMap = it->second[0]->asProperties();
+    if(properties.contains(key) && frameMap[key] == properties[key])
+      properties.erase(key);
+    else
+      toRemove.append(it->second[0]);
+  }
 
-  for (FrameList::ConstIterator it = toRemove.begin(); it != toRemove.end(); it++)
+  // now handle the special cases
+  // first: TXXX frames
+  FrameList &userTextFrames = frameList("TXXX");
+  for(FrameList::ConstIterator it = userTextFrames.begin(); it != userTextFrames.end(); ++it) {
+    PropertyMap frameMap = (*it)->asProperties();
+    if(!frameMap.unsupportedData().isEmpty())
+      // don't touch unsupported frames
+      continue;
+    // TXXX frames yield only one key, so it must be begin()->first
+    String &key = frameMap.begin()->first;
+    if(!Frame::keyToFrameID(key).isNull())
+      // TXXX frame which a description (=key) for which there is a dedicated frame.
+      // We don't want this, so remove the frame, the appropriate T*** or W*** frame
+      // will be created later on.
+      toRemove.append(*it);
+    if(key.find(":") > 0)
+      // colon-separated key: this should be inside a TMCL frame.
+      toRemove.append(*it);
+    // More (ugly) exceptions: If the user provides more than one COMMENT, LYRICS, or URL
+    // tag, we store all of these in a TXXX, because COMM, USLT and WXXX. Otherwise there
+    // should not be such a TXXX frame.
+    if(key == "COMMENT") {
+      if(properties.contains("COMMENT")
+          && properties["COMMENT"].size() >= 2
+          && properties["COMMENT"] == frameMap.begin()->second)
+        properties.erase("COMMENT");
+      else
+        toRemove.append(*it);
+    }else if(key == "LYRICS") {
+      if(properties.contains("LYRICS")
+          && properties["LYRICS"].size() >= 2
+          && properties["LYRICS"] == frameMap.begin()->second)
+        properties.erase("LYRICS");
+      else
+        toRemove.append(*it);
+    }else if(key == "URL") {
+       if(properties.contains("URL")
+           && properties["URL"].size() >= 2
+           && properties["URL"] == frameMap.begin()->second)
+         properties.erase("URL");
+       else
+         toRemove.append(*it);
+    }
+  }
+
+  // next: WXXX frames
+  FrameList &userUrlFrames = frameList("WXXX");
+  for(FrameList::ConstIterator it = userUrlFrames.begin(); it != userUrlFrames.end(); ++it) {
+    PropertyMap frameMap = (*it)->asProperties();
+    if(!frameMap.unsupportedData().isEmpty())
+      // don't touch unsupported frames
+      continue;
+    // WXXX frames yield only one key, so it must be begin()->first
+    String &key = frameMap.begin()->first;
+    if(!Frame::keyToFrameID(key).isNull())
+      // WXXX frame which a description (=key) for which there is a dedicated frame.
+      // We don't want this, so remove the frame, the appropriate T*** or W*** frame
+      // will be created later on.
+      toRemove.append(*it);
+    else if(key.find(":") > 0)
+      // colon-separated key: this should be inside a TMCL frame.
+      toRemove.append(*it);
+    // More exceptions: we don't allow COMMENT and LYRICS in WXXX; they should be in COMM and USLT
+    // (or TXXX, see above).
+    else if(key == "COMMENT" || key == "LYRICS")
+      toRemove.append(*it);
+    // now, the key is either URL or some other string that neither has a dedicated text frame, nor
+    // a colon. We accept the frame if it's contents match the values in properties. However, if
+    // key != URL and the values are changed, they will be stored inside a TXXX frame instead, since
+    // we can't distinguish free-form text from free-form URL keys (possible fix: use URL:REASON like
+    // in TMCL / TIPL?).
+    else if(properties.contains(key) && properties[key] == frameMap.begin()->second)
+      properties.erase(key);
+    else
+      toRemove.append(*it);
+  }
+  for(FrameList::ConstIterator it = toRemove.begin(); it != toRemove.end(); ++it)
     removeFrame(*it);
 
-  // now create new frames from the TagDict and add them.
-  for (TagDict::ConstIterator it = dict.begin(); it != dict.end(); ++it) {
-    Frame *newFrame = createFrame(it->first, it->second);
-    if (newFrame)
-      addFrame(newFrame);
-    else
-      debug("ERROR: Don't know how to translate tag " + it->first + " to ID3v2!");
+  // next: TIPL
+  PropertyMap existingTipl;
+  if(!frameList("TIPL").isEmpty())
+    existingTipl = frameList("TIPL").front()->asProperties();
+  PropertyMap requestedTipl;
+  KeyConversionMap::ConstIterator it = TextIdentificationFrame::involvedPeopleMap().begin();
+  bool rebuildTipl = false;
+  for(; it != TextIdentificationFrame::involvedPeopleMap().end(); ++it) {
+    if(properties.contains(it->first)){
+      requestedTipl.insert(it->first, properties[it->first]);
+      properties.erase(it->first); // it's ensured now that this key gets handled correctly
+      if(!existingTipl.contains(it->first) || existingTipl[it->first] != requestedTipl[it->first])
+        rebuildTipl = true;
+    } else if(existingTipl.contains(it->first))
+      rebuildTipl = true;
+  }
+  if(rebuildTipl){
+    removeFrames("TIPL");
+    addFrame(TextIdentificationFrame::createTIPLFrame(requestedTipl));
+  }
+
+  // next: create frames for everything still in properties except for TMCL ("PERFORMER:<instrument>")
+  // keys, which are collected in a dedicated map
+  PropertyMap requestedTmcl;
+  for(PropertyMap::ConstIterator it = properties.begin(); it != properties.end(); ++it){
+    if(it->first.find(":") != -1)
+      requestedTmcl.insert(it->first, it->second);
+    else{
+      // phew. Now we are in the simple case that our key=<value list> pair can be represented by a
+      // single frame, either a T*** (not TIPL, TMCL) or W*** frame.
+      ByteVector id = Frame::keyToFrameID(it->first);
+    }
+  }
+
+    // next: TMCL
+  PropertyMap existingTmcl;
+  if(!frameList("TMCL").isEmpty())
+    existingTmcl = frameList("TMCL").front()->asProperties();
+  bool rebuildTmcl = false;
+  // search for TMCL keys ("PERFORMER:<instrument>") in properties
+  for(PropertyMap::ConstIterator it = properties.begin(); it != properties.end(); ++it){
+    if(it->first.find(":") != -1)
+      requestedTmcl.insert(it->first, it->second);
   }
 }