Fix specifying custom fields

* Fix support for Vorbis comment and add test case
* Consider only fields for the current format
  when displaying tags

cli/helper.cpp

@@ -244,20 +244,25 @@ void printField(const FieldScope &scope, const Tag *tag, TagType tagType, bool s
         // parse field denotation
         const auto &values = scope.field.values(tag, tagType);
 
+        // skip values if format-specific field ID used which is not applicable for the current tag
+        if (!values.second) {
+            return;
+        }
+
         // skip empty values (unless prevented)
-        if (skipEmpty && values.empty()) {
+        if (skipEmpty && values.first.empty()) {
             return;
         }
 
         // print empty value (if not prevented)
-        if (values.empty()) {
+        if (values.first.empty()) {
             printFieldName(fieldName, fieldNameLen);
             cout << "none\n";
             return;
         }
 
         // print values
-        for (const auto &value : values) {
+        for (const auto &value : values.first) {
             printFieldName(fieldName, fieldNameLen);
             printTagValue(*value);
         }
@@ -485,7 +490,7 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly)
                     } else if (part == "itunes" || part == "mp4") {
                         tagType |= TagType::Mp4Tag;
                     } else if (part == "vorbis") {
-                        tagType |= TagType::VorbisComment;
+                        tagType |= TagType::VorbisComment | TagType::OggVorbisComment;
                     } else if (part == "matroska") {
                         tagType |= TagType::MatroskaTag;
                     } else if (part == "all" || part == "any") {
@@ -608,19 +613,22 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly)
     return fields;
 }
 
-template <class ConcreteTag>
+template <class ConcreteTag, TagType tagTypeMask = ConcreteTag::tagType>
-std::vector<const TagValue *> valuesForNativeField(const char *idString, std::size_t idStringSize, const Tag *tag, TagType tagType)
+std::pair<std::vector<const TagValue *>, bool> valuesForNativeField(const char *idString, std::size_t idStringSize, const Tag *tag, TagType tagType)
 {
-    if (tagType != ConcreteTag::tagType) {
+    auto res = make_pair<std::vector<const TagValue *>, bool>({}, false);
-        return vector<const TagValue *>();
+    if ((tagType & tagTypeMask) == TagType::Unspecified) {
+        return res;
     }
-    return static_cast<const ConcreteTag *>(tag)->values(ConcreteTag::FieldType::fieldIdFromString(idString, idStringSize));
+    res.first = static_cast<const ConcreteTag *>(tag)->values(ConcreteTag::FieldType::fieldIdFromString(idString, idStringSize));
+    res.second = true;
+    return res;
 }
 
-template <class ConcreteTag>
+template <class ConcreteTag, TagType tagTypeMask = ConcreteTag::tagType>
 bool setValuesForNativeField(const char *idString, std::size_t idStringSize, Tag *tag, TagType tagType, const std::vector<TagValue> &values)
 {
-    if (tagType != ConcreteTag::tagType) {
+    if ((tagType & tagTypeMask) == TagType::Unspecified) {
         return false;
     }
     return static_cast<ConcreteTag *>(tag)->setValues(ConcreteTag::FieldType::fieldIdFromString(idString, idStringSize), values);
@@ -636,10 +644,10 @@ inline FieldId::FieldId(const char *nativeField, std::size_t nativeFieldSize, co
 }
 
 /// \remarks This wrapper is required because specifying c'tor template args is not possible.
-template <class ConcreteTag> FieldId FieldId::fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize)
+template <class ConcreteTag, TagType tagTypeMask> FieldId FieldId::fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize)
 {
-    return FieldId(nativeFieldId, nativeFieldIdSize, bind(&valuesForNativeField<ConcreteTag>, nativeFieldId, nativeFieldIdSize, _1, _2),
+    return FieldId(nativeFieldId, nativeFieldIdSize, bind(&valuesForNativeField<ConcreteTag, tagTypeMask>, nativeFieldId, nativeFieldIdSize, _1, _2),
-        bind(&setValuesForNativeField<ConcreteTag>, nativeFieldId, nativeFieldIdSize, _1, _2, _3));
+        bind(&setValuesForNativeField<ConcreteTag, tagTypeMask>, nativeFieldId, nativeFieldIdSize, _1, _2, _3));
 }
 
 FieldId FieldId::fromTagDenotation(const char *denotation, size_t denotationSize)
@@ -650,7 +658,7 @@ FieldId FieldId::fromTagDenotation(const char *denotation, size_t denotationSize
     } else if (!strncmp(denotation, "mp4:", 4)) {
         return FieldId::fromNativeField<Mp4Tag>(denotation + 4, denotationSize - 4);
     } else if (!strncmp(denotation, "vorbis:", 7)) {
-        return FieldId::fromNativeField<VorbisComment>(denotation + 7, denotationSize - 7);
+        return FieldId::fromNativeField<VorbisComment, TagType::VorbisComment | TagType::OggVorbisComment>(denotation + 7, denotationSize - 7);
     } else if (!strncmp(denotation, "id3:", 7)) {
         return FieldId::fromNativeField<Id3v2Tag>(denotation + 4, denotationSize - 4);
     } else if (!strncmp(denotation, "generic:", 8)) {
@@ -671,13 +679,16 @@ FieldId FieldId::fromTrackDenotation(const char *denotation, size_t denotationSi
     return FieldId(KnownField::Invalid, denotation, denotationSize);
 }
 
-std::vector<const TagValue *> FieldId::values(const Tag *tag, TagType tagType) const
+std::pair<std::vector<const TagValue *>, bool> FieldId::values(const Tag *tag, TagType tagType) const
 {
+    auto res = make_pair<std::vector<const TagValue *>, bool>({}, false);
     if (!m_nativeField.empty()) {
-        return m_valuesForNativeField(tag, tagType);
+        res = m_valuesForNativeField(tag, tagType);
     } else {
-        return tag->values(m_knownField);
+        res.first = tag->values(m_knownField);
+        res.second = true;
     }
+    return res;
 }
 
 bool FieldId::setValues(Tag *tag, TagType tagType, const std::vector<TagValue> &values) const

cli/helper.h

@@ -63,15 +63,16 @@ public:
     const char *name() const;
     bool denotes(const char *knownDenotation) const;
     const std::string &denotation() const;
-    std::vector<const TagValue *> values(const Tag *tag, TagType tagType) const;
+    std::pair<std::vector<const TagValue *>, bool> values(const Tag *tag, TagType tagType) const;
     bool setValues(Tag *tag, TagType tagType, const std::vector<TagValue> &values) const;
 
 private:
-    using GetValuesForNativeFieldType = std::function<std::vector<const TagValue *>(const Tag *, TagType)>;
+    using GetValuesForNativeFieldType = std::function<std::pair<std::vector<const TagValue *>, bool>(const Tag *, TagType)>;
     using SetValuesForNativeFieldType = std::function<bool(Tag *, TagType, const std::vector<TagValue> &)>;
     FieldId(const char *nativeField, std::size_t nativeFieldSize, const GetValuesForNativeFieldType &valuesForNativeField,
         const SetValuesForNativeFieldType &setValuesForNativeField);
-    template <class ConcreteTag> static FieldId fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize);
+    template <class ConcreteTag, TagType tagTypeMask = ConcreteTag::tagType>
+    static FieldId fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize);
 
     KnownField m_knownField;
     std::string m_denotation;

cli/mainfeatures.cpp

@@ -814,9 +814,14 @@ void extractField(
                 // iterate through all tags
                 for (const Tag *tag : tags) {
                     const TagType tagType = tag->type();
-                    for (const pair<FieldScope, FieldValues> &fieldDenotation : fieldDenotations) {
+                    for (const auto &fieldDenotation : fieldDenotations) {
                         try {
-                            for (const TagValue *value : fieldDenotation.first.field.values(tag, tagType)) {
+                            const auto valuesForField = fieldDenotation.first.field.values(tag, tagType);
+                            // skip if field ID is format specific and not relevant for the current format
+                            if (!valuesForField.second) {
+                                continue;
+                            }
+                            for (const TagValue *value : valuesForField.first) {
                                 values.emplace_back(value, joinStrings({ tag->typeName(), numberToString(values.size()) }, "-", true));
                             }
                         } catch (const ConversionException &e) {

tests/cli.cpp

@@ -240,7 +240,12 @@ void CliTests::testSpecifyingNativeFieldIds()
     const string mkvFileBackup(mkvFile + ".bak");
     const string mp4File(workingCopyPath("mtx-test-data/aac/he-aacv2-ps.m4a"));
     const string mp4FileBackup(mp4File + ".bak");
-    const char *const args1[] = { "tageditor", "set", "mkv:FOO=bar", "mp4:©foo=bar", "mp4:invalid", "-f", mkvFile.data(), mp4File.data(), nullptr };
+    const string vorbisFile(workingCopyPath("mtx-test-data/ogg/qt4dance_medium.ogg"));
+    const string vorbisFileBackup(vorbisFile + ".bak");
+    const string opusFile(workingCopyPath("mtx-test-data/opus/v-opus.ogg"));
+    const string opusFileBackup(opusFile + ".bak");
+    const char *const args1[] = { "tageditor", "set", "mkv:FOO=bar", "mp4:©foo=bar", "mp4:invalid", "vorbis:BAR=foo", "-f", mkvFile.data(),
+        mp4File.data(), vorbisFile.data(), opusFile.data(), nullptr };
     TESTUTILS_ASSERT_EXEC(args1);
     CPPUNIT_ASSERT(stderr.empty());
     // FIXME: provide a way to specify raw data type
@@ -248,13 +253,14 @@ void CliTests::testSpecifyingNativeFieldIds()
         stdout, { "making MP4 tag field ©foo: It was not possible to find an appropriate raw data type id. UTF-8 will be assumed." }));
     CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "Unable to parse denoted field ID \"invalid\": MP4 ID must be exactly 4 chars" }));
 
-    const char *const args2[] = { "tageditor", "get", "mkv:FOO", "mp4:©foo", "generic:year", "-f", mkvFile.data(), nullptr };
+    const char *const args2[] = { "tageditor", "get", "mkv:FOO", "mp4:©foo", "vorbis:BAR", "generic:year", "-f", mkvFile.data(), nullptr };
     TESTUTILS_ASSERT_EXEC(args2);
     CPPUNIT_ASSERT(stderr.empty());
     CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "Year              2010" }));
     CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "FOO               bar" }));
 
-    const char *const args3[] = { "tageditor", "get", "mkv:FOO", "mp4:©foo", "mp4:invalid", "generic:year", "-f", mp4File.data(), nullptr };
+    const char *const args3[]
+        = { "tageditor", "get", "mkv:FOO", "mp4:©foo", "vorbis:BAR", "mp4:invalid", "generic:year", "-f", mp4File.data(), nullptr };
     TESTUTILS_ASSERT_EXEC(args3);
     CPPUNIT_ASSERT(stderr.empty());
     CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "test" }));
@@ -263,10 +269,26 @@ void CliTests::testSpecifyingNativeFieldIds()
     CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "©foo             bar" }));
     CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "invalid           unable to parse - MP4 ID must be exactly 4 chars" }));
 
+    const char *const args4[] = { "tageditor", "get", "mkv:FOO", "mp4:©foo", "vorbis:BAR", "generic:year", "-f", vorbisFile.data(), nullptr };
+    TESTUTILS_ASSERT_EXEC(args4);
+    CPPUNIT_ASSERT(stderr.empty());
+    CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "Year              none" }));
+    CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "BAR               foo" }));
+
+    const char *const args5[] = { "tageditor", "get", "mkv:FOO", "mp4:©foo", "vorbis:BAR", "generic:year", "-f", opusFile.data(), nullptr };
+    TESTUTILS_ASSERT_EXEC(args5);
+    CPPUNIT_ASSERT(stderr.empty());
+    CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "Year              none" }));
+    CPPUNIT_ASSERT(testContainsSubstrings(stdout, { "BAR               foo" }));
+
     CPPUNIT_ASSERT_EQUAL(0, remove(mkvFile.data()));
     CPPUNIT_ASSERT_EQUAL(0, remove(mkvFileBackup.data()));
     CPPUNIT_ASSERT_EQUAL(0, remove(mp4File.data()));
     CPPUNIT_ASSERT_EQUAL(0, remove(mp4FileBackup.data()));
+    CPPUNIT_ASSERT_EQUAL(0, remove(vorbisFile.data()));
+    CPPUNIT_ASSERT_EQUAL(0, remove(vorbisFileBackup.data()));
+    CPPUNIT_ASSERT_EQUAL(0, remove(opusFile.data()));
+    CPPUNIT_ASSERT_EQUAL(0, remove(opusFileBackup.data()));
 }
 
 /*!