From 74d05eede9b7353fb20b13165e8f8f9011cd34d8 Mon Sep 17 00:00:00 2001 From: Martchus Date: Wed, 2 Jan 2019 17:12:48 +0100 Subject: [PATCH] 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 | 47 +++++++++++++++++++++++++++----------------- cli/helper.h | 7 ++++--- cli/mainfeatures.cpp | 9 +++++++-- tests/cli.cpp | 28 +++++++++++++++++++++++--- 4 files changed, 65 insertions(+), 26 deletions(-) diff --git a/cli/helper.cpp b/cli/helper.cpp index efc1172..358422c 100644 --- a/cli/helper.cpp +++ b/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 -std::vector valuesForNativeField(const char *idString, std::size_t idStringSize, const Tag *tag, TagType tagType) +template +std::pair, bool> valuesForNativeField(const char *idString, std::size_t idStringSize, const Tag *tag, TagType tagType) { - if (tagType != ConcreteTag::tagType) { - return vector(); + auto res = make_pair, bool>({}, false); + if ((tagType & tagTypeMask) == TagType::Unspecified) { + return res; } - return static_cast(tag)->values(ConcreteTag::FieldType::fieldIdFromString(idString, idStringSize)); + res.first = static_cast(tag)->values(ConcreteTag::FieldType::fieldIdFromString(idString, idStringSize)); + res.second = true; + return res; } -template +template bool setValuesForNativeField(const char *idString, std::size_t idStringSize, Tag *tag, TagType tagType, const std::vector &values) { - if (tagType != ConcreteTag::tagType) { + if ((tagType & tagTypeMask) == TagType::Unspecified) { return false; } return static_cast(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 FieldId FieldId::fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize) +template FieldId FieldId::fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize) { - return FieldId(nativeFieldId, nativeFieldIdSize, bind(&valuesForNativeField, nativeFieldId, nativeFieldIdSize, _1, _2), - bind(&setValuesForNativeField, nativeFieldId, nativeFieldIdSize, _1, _2, _3)); + return FieldId(nativeFieldId, nativeFieldIdSize, bind(&valuesForNativeField, nativeFieldId, nativeFieldIdSize, _1, _2), + bind(&setValuesForNativeField, 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(denotation + 4, denotationSize - 4); } else if (!strncmp(denotation, "vorbis:", 7)) { - return FieldId::fromNativeField(denotation + 7, denotationSize - 7); + return FieldId::fromNativeField(denotation + 7, denotationSize - 7); } else if (!strncmp(denotation, "id3:", 7)) { return FieldId::fromNativeField(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 FieldId::values(const Tag *tag, TagType tagType) const +std::pair, bool> FieldId::values(const Tag *tag, TagType tagType) const { + auto res = make_pair, 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 &values) const diff --git a/cli/helper.h b/cli/helper.h index 41b19ab..0d195d5 100644 --- a/cli/helper.h +++ b/cli/helper.h @@ -63,15 +63,16 @@ class FieldId { const char *name() const; bool denotes(const char *knownDenotation) const; const std::string &denotation() const; - std::vector values(const Tag *tag, TagType tagType) const; + std::pair, bool> values(const Tag *tag, TagType tagType) const; bool setValues(Tag *tag, TagType tagType, const std::vector &values) const; private: - using GetValuesForNativeFieldType = std::function(const Tag *, TagType)>; + using GetValuesForNativeFieldType = std::function, bool>(const Tag *, TagType)>; using SetValuesForNativeFieldType = std::function &)>; FieldId(const char *nativeField, std::size_t nativeFieldSize, const GetValuesForNativeFieldType &valuesForNativeField, const SetValuesForNativeFieldType &setValuesForNativeField); - template static FieldId fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize); + template + static FieldId fromNativeField(const char *nativeFieldId, std::size_t nativeFieldIdSize); KnownField m_knownField; std::string m_denotation; diff --git a/cli/mainfeatures.cpp b/cli/mainfeatures.cpp index f6f5c27..00e3823 100644 --- a/cli/mainfeatures.cpp +++ b/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 &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) { diff --git a/tests/cli.cpp b/tests/cli.cpp index e1b5cb2..a101e88 100644 --- a/tests/cli.cpp +++ b/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())); } /*!