Provide properties methods on FileRef, make FileRef non-virtual

Use of FileRef::file() is discouraged, but was necessary to access the
properties. It was also not clear whether to access the properties via
tag() or file(). With the property methods on FileRef, it should become
the "simple usage" interface it was meant to be.
As the destructor was the only virtual method on FileRef, it is now made
non-virtual. Probably, it is not useful as a virtual base class.

examples/tagreader.cpp

@@ -55,7 +55,7 @@ int main(int argc, char *argv[])
       cout << "track   - \"" << tag->track()   << "\"" << endl;
       cout << "genre   - \"" << tag->genre()   << "\"" << endl;
 
-      TagLib::PropertyMap tags = f.file()->properties();
+      TagLib::PropertyMap tags = f.properties();
 
       unsigned int longest = 0;
       for(auto i = tags.cbegin(); i != tags.cend(); ++i) {
@@ -71,9 +71,9 @@ int main(int argc, char *argv[])
         }
       }
 
-      TagLib::StringList names = f.file()->complexPropertyKeys();
+      TagLib::StringList names = f.complexPropertyKeys();
       for(const auto &name : names) {
-        const auto& properties = f.file()->complexProperties(name);
+        const auto& properties = f.complexProperties(name);
         for(const auto &property : properties) {
           cout << name << ":" << endl;
           for(const auto &[key, value] : property) {

examples/tagwriter.cpp

@@ -122,10 +122,9 @@ int main(int argc, char *argv[])
       TagLib::String value = argv[i + 1];
       int numArgsConsumed = 2;
 
-      TagLib::List<TagLib::FileRef>::ConstIterator it;
-      for(it = fileList.cbegin(); it != fileList.cend(); ++it) {
+      for(auto &f : fileList) {
 
-        TagLib::Tag *t = (*it).tag();
+        TagLib::Tag *t = f.tag();
 
         switch (field) {
         case 't':
@@ -152,7 +151,7 @@ int main(int argc, char *argv[])
         case 'R':
         case 'I':
           if(i + 2 < argc) {
-            TagLib::PropertyMap map = (*it).file()->properties ();
+            TagLib::PropertyMap map = f.properties();
             if(field == 'R') {
               map.replace(value, TagLib::String(argv[i + 2]));
             }
@@ -160,16 +159,16 @@ int main(int argc, char *argv[])
               map.insert(value, TagLib::String(argv[i + 2]));
             }
             numArgsConsumed = 3;
-            checkForRejectedProperties((*it).file()->setProperties(map));
+            checkForRejectedProperties(f.setProperties(map));
           }
           else {
             usage();
           }
           break;
         case 'D': {
-          TagLib::PropertyMap map = (*it).file()->properties();
+          TagLib::PropertyMap map = f.properties();
           map.erase(value);
-          checkForRejectedProperties((*it).file()->setProperties(map));
+          checkForRejectedProperties(f.setProperties(map));
           break;
         }
         case 'p': {
@@ -190,7 +189,7 @@ int main(int argc, char *argv[])
               TagLib::String mimeType = data.startsWith("\x89PNG\x0d\x0a\x1a\x0a")
                 ? "image/png" : "image/jpeg";
               TagLib::String description(argv[i + 2]);
-              it->file()->setComplexProperties("PICTURE", {
+              f.setComplexProperties("PICTURE", {
                 {
                   {"data", data},
                   {"pictureType", "Front Cover"},
@@ -201,7 +200,7 @@ int main(int argc, char *argv[])
             }
             else {
               // empty value, remove pictures
-              it->file()->setComplexProperties("PICTURE", {});
+              f.setComplexProperties("PICTURE", {});
             }
           }
           else {
@@ -220,9 +219,8 @@ int main(int argc, char *argv[])
       usage();
   }
 
-  TagLib::List<TagLib::FileRef>::ConstIterator it;
-  for(it = fileList.cbegin(); it != fileList.cend(); ++it)
-    (*it).file()->save();
+  for(auto &f : fileList)
+    f.save();
 
   return 0;
 }

taglib/fileref.cpp

@@ -33,6 +33,9 @@
 #include <utility>
 
 #include "tfilestream.h"
+#include "tpropertymap.h"
+#include "tstringlist.h"
+#include "tvariant.h"
 #include "tdebug.h"
 #include "aifffile.h"
 #include "apefile.h"
@@ -322,6 +325,20 @@ public:
   FileRefPrivate(const FileRefPrivate &) = delete;
   FileRefPrivate &operator=(const FileRefPrivate &) = delete;
 
+  bool isNull() const
+  {
+    return !file || !file->isValid();
+  }
+
+  bool isNullWithDebugMessage(const String &methodName) const
+  {
+    if(isNull()) {
+      debug("FileRef::" + methodName + "() - Called without a valid file.");
+      return true;
+    }
+    return false;
+  }
+
   File *file { nullptr };
   IOStream *stream { nullptr };
 };
@@ -360,17 +377,63 @@ FileRef::~FileRef() = default;
 
 Tag *FileRef::tag() const
 {
-  if(isNull()) {
-    debug("FileRef::tag() - Called without a valid file.");
+  if(d->isNullWithDebugMessage(__func__)) {
     return nullptr;
   }
   return d->file->tag();
 }
 
+PropertyMap FileRef::properties() const
+{
+  if(d->isNullWithDebugMessage(__func__)) {
+    return PropertyMap();
+  }
+  return d->file->properties();
+}
+
+void FileRef::removeUnsupportedProperties(const StringList& properties)
+{
+  if(d->isNullWithDebugMessage(__func__)) {
+    return;
+  }
+  return d->file->removeUnsupportedProperties(properties);
+}
+
+PropertyMap FileRef::setProperties(const PropertyMap &properties)
+{
+  if(d->isNullWithDebugMessage(__func__)) {
+    return PropertyMap();
+  }
+  return d->file->setProperties(properties);
+}
+
+StringList FileRef::complexPropertyKeys() const
+{
+  if(d->isNullWithDebugMessage(__func__)) {
+    return StringList();
+  }
+  return d->file->complexPropertyKeys();
+}
+
+List<VariantMap> FileRef::complexProperties(const String &key) const
+{
+  if(d->isNullWithDebugMessage(__func__)) {
+    return List<VariantMap>();
+  }
+  return d->file->complexProperties(key);
+}
+
+bool FileRef::setComplexProperties(const String &key, const List<VariantMap> &value)
+{
+  if(d->isNullWithDebugMessage(__func__)) {
+    return false;
+  }
+  return d->file->setComplexProperties(key, value);
+}
+
 AudioProperties *FileRef::audioProperties() const
 {
-  if(isNull()) {
-    debug("FileRef::audioProperties() - Called without a valid file.");
+  if(d->isNullWithDebugMessage(__func__)) {
     return nullptr;
   }
   return d->file->audioProperties();
@@ -383,8 +446,7 @@ File *FileRef::file() const
 
 bool FileRef::save()
 {
-  if(isNull()) {
-    debug("FileRef::save() - Called without a valid file.");
+  if(d->isNullWithDebugMessage(__func__)) {
     return false;
   }
   return d->file->save();
@@ -445,7 +507,7 @@ StringList FileRef::defaultFileExtensions()
 
 bool FileRef::isNull() const
 {
-  return (!d->file || !d->file->isValid());
+  return d->isNull();
 }
 
 FileRef &FileRef::operator=(const FileRef &) = default;

taglib/fileref.h

@@ -190,7 +190,7 @@ namespace TagLib {
     /*!
      * Destroys this FileRef instance.
      */
-    virtual ~FileRef();
+    ~FileRef();
 
     /*!
      * Returns a pointer to represented file's tag.
@@ -205,6 +205,84 @@ namespace TagLib {
      */
     Tag *tag() const;
 
+    /*!
+     * Exports the tags of the file as dictionary mapping (human readable) tag
+     * names (uppercase Strings) to StringLists of tag values. Calls this
+     * method on the wrapped File instance.
+     * For each metadata object of the file that could not be parsed into the PropertyMap
+     * format, the returned map's unsupportedData() list will contain one entry identifying
+     * that object (e.g. the frame type for ID3v2 tags). Use removeUnsupportedProperties()
+     * to remove (a subset of) them.
+     * For files that contain more than one tag (e.g. an MP3 with both an ID3v1 and an ID3v2
+     * tag) only the most "modern" one will be exported (ID3v2 in this case).
+     */
+    PropertyMap properties() const;
+
+    /*!
+     * Removes unsupported properties, or a subset of them, from the file's metadata.
+     * The parameter \a properties must contain only entries from
+     * properties().unsupportedData().
+     */
+    void removeUnsupportedProperties(const StringList& properties);
+
+    /*!
+     * Sets the tags of the wrapped File to those specified in \a properties.
+     * If some value(s) could not be written to the specific metadata format,
+     * the returned PropertyMap will contain those value(s). Otherwise it will be empty,
+     * indicating that no problems occurred.
+     * With file types that support several tag formats (for instance, MP3 files can have
+     * ID3v1, ID3v2, and APEv2 tags), this function will create the most appropriate one
+     * (ID3v2 for MP3 files). Older formats will be updated as well, if they exist, but won't
+     * be taken into account for the return value of this function.
+     * See the documentation of the subclass implementations for detailed descriptions.
+     */
+    PropertyMap setProperties(const PropertyMap &properties);
+
+    /*!
+     * Get the keys of complex properties, i.e. properties which cannot be
+     * represented simply by a string.
+     * Because such properties might be expensive to fetch, there are separate
+     * operations to get the available keys - which is expected to be cheap -
+     * and getting and setting the property values.
+     * Calls the method on the wrapped File, which collects the keys from one
+     * or more of its tags.
+     */
+    StringList complexPropertyKeys() const;
+
+    /*!
+     * Get the complex properties for a given \a key.
+     * In order to be flexible for different metadata formats, the properties
+     * are represented as variant maps.  Despite this dynamic nature, some
+     * degree of standardization should be achieved between formats:
+     *
+     * - PICTURE
+     *   - data: ByteVector with picture data
+     *   - description: String with description
+     *   - pictureType: String with type as specified for ID3v2,
+     *     e.g. "Front Cover", "Back Cover", "Band"
+     *   - mimeType: String with image format, e.g. "image/jpeg"
+     *   - optionally more information found in the tag, such as
+     *     "width", "height", "numColors", "colorDepth" int values
+     *     in FLAC pictures
+     * - GENERALOBJECT
+     *   - data: ByteVector with object data
+     *   - description: String with description
+     *   - fileName: String with file name
+     *   - mimeType: String with MIME type
+     *   - this is currently only implemented for ID3v2 GEOB frames
+     *
+     * Calls the method on the wrapped File, which gets the properties from one
+     * or more of its tags.
+     */
+    List<VariantMap> complexProperties(const String &key) const;
+
+    /*!
+     * Set all complex properties for a given \a key using variant maps as
+     * \a value with the same format as returned by complexProperties().
+     * An empty list as \a value removes all complex properties for \a key.
+     */
+    bool setComplexProperties(const String &key, const List<VariantMap> &value);
+
     /*!
      * Returns the audio properties for this FileRef.  If no audio properties
      * were read then this will returns a null pointer.

taglib/tag.h

@@ -119,7 +119,7 @@ namespace TagLib {
     /*!
      * Set all complex properties for a given \a key using variant maps as
      * \a value with the same format as returned by complexProperties().
-     * An empty list as \a value to removes all complex properties for \a key.
+     * An empty list as \a value removes all complex properties for \a key.
      */
     virtual bool setComplexProperties(const String &key, const List<VariantMap> &value);
 

taglib/toolkit/tfile.h

@@ -122,7 +122,7 @@ namespace TagLib {
      * Sets the tags of this File to those specified in \a properties. Calls the
      * according specialization method in the subclasses of File to do the translation
      * into the format-specific details.
-     * If some value(s) could not be written imported to the specific metadata format,
+     * If some value(s) could not be written to the specific metadata format,
      * the returned PropertyMap will contain those value(s). Otherwise it will be empty,
      * indicating that no problems occurred.
      * With file types that support several tag formats (for instance, MP3 files can have

tests/test_complexproperties.cpp

@@ -88,8 +88,8 @@ public:
     if(zlib::isAvailable()) {
       FileRef f(TEST_FILE_PATH_C("compressed_id3_frame.mp3"), false);
       CPPUNIT_ASSERT_EQUAL(StringList(PICTURE_KEY),
-        f.file()->complexPropertyKeys());
-      auto pictures = f.file()->complexProperties(PICTURE_KEY);
+        f.complexPropertyKeys());
+      auto pictures = f.complexProperties(PICTURE_KEY);
       CPPUNIT_ASSERT_EQUAL(1U, pictures.size());
       auto picture = pictures.front();
       CPPUNIT_ASSERT_EQUAL(86414U,
@@ -131,8 +131,8 @@ public:
 
     FileRef f(TEST_FILE_PATH_C("has-tags.m4a"), false);
     CPPUNIT_ASSERT_EQUAL(StringList(PICTURE_KEY),
-      f.file()->complexPropertyKeys());
-    auto pictures = f.file()->complexProperties(PICTURE_KEY);
+      f.complexPropertyKeys());
+    auto pictures = f.complexProperties(PICTURE_KEY);
     CPPUNIT_ASSERT_EQUAL(2U, pictures.size());
     auto picture = pictures.front();
     CPPUNIT_ASSERT_EQUAL(expectedData1,
@@ -150,8 +150,8 @@ public:
   {
     FileRef f(TEST_FILE_PATH_C("lowercase-fields.ogg"), false);
     CPPUNIT_ASSERT_EQUAL(StringList(PICTURE_KEY),
-      f.file()->complexPropertyKeys());
-    auto pictures = f.file()->complexProperties(PICTURE_KEY);
+      f.complexPropertyKeys());
+    auto pictures = f.complexProperties(PICTURE_KEY);
     CPPUNIT_ASSERT_EQUAL(1U, pictures.size());
     auto picture = pictures.front();
     CPPUNIT_ASSERT_EQUAL(ByteVector("JPEG data"),
@@ -243,19 +243,19 @@ public:
 
     {
       FileRef f(copy.fileName().c_str(), false);
-      CPPUNIT_ASSERT(f.file()->complexPropertyKeys().isEmpty());
-      f.file()->setComplexProperties(PICTURE_KEY, {TEST_PICTURE, picture2});
-      f.file()->setComplexProperties(GEOB_KEY, {geob1, geob2});
-      f.file()->save();
+      CPPUNIT_ASSERT(f.complexPropertyKeys().isEmpty());
+      f.setComplexProperties(PICTURE_KEY, {TEST_PICTURE, picture2});
+      f.setComplexProperties(GEOB_KEY, {geob1, geob2});
+      f.save();
     }
     {
       FileRef f(copy.fileName().c_str(), false);
       CPPUNIT_ASSERT_EQUAL(StringList({PICTURE_KEY, GEOB_KEY}),
-        f.file()->complexPropertyKeys());
+        f.complexPropertyKeys());
       CPPUNIT_ASSERT(List<VariantMap>({TEST_PICTURE, picture2}) ==
-        f.file()->complexProperties(PICTURE_KEY));
+        f.complexProperties(PICTURE_KEY));
       CPPUNIT_ASSERT(List<VariantMap>({geob1, geob2}) ==
-        f.file()->complexProperties(GEOB_KEY));
+        f.complexProperties(GEOB_KEY));
     }
   }
 

tests/test_sizes.cpp

@@ -172,7 +172,7 @@ public:
         CPPUNIT_ASSERT_EQUAL(classSize(1, true), sizeof(TagLib::FLAC::Properties));
         CPPUNIT_ASSERT_EQUAL(classSize(1, true), sizeof(TagLib::FLAC::UnknownMetadataBlock));
         CPPUNIT_ASSERT_EQUAL(classSize(0, true), sizeof(TagLib::File));
-        CPPUNIT_ASSERT_EQUAL(classSize(1, true), sizeof(TagLib::FileRef));
+        CPPUNIT_ASSERT_EQUAL(classSize(1, false), sizeof(TagLib::FileRef));
         CPPUNIT_ASSERT_EQUAL(classSize(0, true), sizeof(TagLib::FileRef::FileTypeResolver));
         CPPUNIT_ASSERT_EQUAL(classSize(1, true), sizeof(TagLib::FileRef::StreamTypeResolver));
         CPPUNIT_ASSERT_EQUAL(classSize(1, true), sizeof(TagLib::FileStream));