Skip to content

Commit

Permalink
Fix specifying custom fields
Browse files Browse the repository at this point in the history 
* Fix support for Vorbis comment and add test case
* Consider only fields for the current format
  when displaying tags
  • Loading branch information
@Martchus
Martchus committed Jan 2, 2019
1 parent 47e9290 commit 74d05ee
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 26 deletions.
47 changes: 29 additions & 18 deletions cli/helper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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") {
Expand Down Expand Up @@ -608,19 +613,22 @@ FieldDenotations parseFieldDenotations(const Argument &fieldsArg, bool readOnly)
return fields;
}

template <class ConcreteTag>
std::vector<const TagValue *> valuesForNativeField(const char *idString, std::size_t idStringSize, const Tag *tag, TagType tagType)
template <class ConcreteTag, TagType tagTypeMask = ConcreteTag::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) {
return vector<const TagValue *>();
auto res = make_pair<std::vector<const TagValue *>, bool>({}, false);
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);
Expand All @@ -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),
bind(&setValuesForNativeField<ConcreteTag>, nativeFieldId, nativeFieldIdSize, _1, _2, _3));
return FieldId(nativeFieldId, nativeFieldIdSize, bind(&valuesForNativeField<ConcreteTag, tagTypeMask>, nativeFieldId, nativeFieldIdSize, _1, _2),
bind(&setValuesForNativeField<ConcreteTag, tagTypeMask>, nativeFieldId, nativeFieldIdSize, _1, _2, _3));
}

FieldId FieldId::fromTagDenotation(const char *denotation, size_t denotationSize)
Expand All @@ -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)) {
Expand All @@ -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
Expand Down
7 changes: 4 additions & 3 deletions cli/helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,16 @@ class FieldId {
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;
Expand Down
9 changes: 7 additions & 2 deletions cli/mainfeatures.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
28 changes: 25 additions & 3 deletions tests/cli.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,21 +240,27 @@ 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
CPPUNIT_ASSERT(testContainsSubstrings(
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" }));
Expand All @@ -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()));
}

/*!
Expand Down

0 comments on commit 74d05ee

Please sign in to comment.