diff --git a/src/include/mx/api/KeyData.h b/src/include/mx/api/KeyData.h index 16cb446eb..f4551b371 100644 --- a/src/include/mx/api/KeyData.h +++ b/src/include/mx/api/KeyData.h @@ -10,12 +10,31 @@ namespace mx { namespace api { +// KeyMode is the mode of a traditional key signature, the MusicXML element. Together with +// fifths it tells you which key the signature spells: zero fifths with major is C major, with minor +// it is A minor, with dorian it is D dorian. The mode does not change the accidentals that are +// drawn; fifths alone determines those. +// +// `none` is the mode of music that has no tonal center. It is how a keyless staff is written: +// zero fifths with none. It is a mode value like any other, and it says nothing about +// how the signature is drawn. +// +// MusicXML's element is optional, so `unspecified` (a key signature that does not state a +// mode) is distinct from `none` (a key signature that states there is no mode). enum class KeyMode { - unspecified, // a mode value was not provided - unsupported, // a mode value was provided but is not supported + unspecified, // no mode is stated + unsupported, // a mode was stated, but it is outside the standard vocabulary below major, - minor + minor, + dorian, + phrygian, + lydian, + mixolydian, + aeolian, + ionian, + locrian, + none }; // CancelLocation represents the cancel element's optional location attribute. From MusicXML @@ -45,6 +64,11 @@ enum class CancelLocation // key.fifths = -2; // (i.e. 2 flats) // key.mode = KeyMode::minor; // (optional) // +// Example, a keyless signature (no tonal center): +// KeyData key; +// key.fifths = 0; // (i.e. no sharps or flats) +// key.mode = KeyMode::none; +// // If you want to create a custom time signature, you can do so like this. Here we are creating a // key where C's are sharp and D's are one-quarter-tone sharp. See KeyComponent for details. // @@ -78,7 +102,9 @@ struct KeyData // (i.e. unless a cancel element is present). CancelLocation cancelLocation; - // Mode specifies whether the key is major or minor. It is optional. + // The mode of the key signature (major, minor, dorian, none, and so on). It is optional: the + // default, KeyMode::unspecified, states no mode and writes no element. KeyMode::none + // is a stated mode, and it writes none. KeyMode mode; // Supports changing the key somewhere other than at the start of a measure. diff --git a/src/private/mx/impl/Converter.cpp b/src/private/mx/impl/Converter.cpp index f8334dfc6..cdd118fdd 100644 --- a/src/private/mx/impl/Converter.cpp +++ b/src/private/mx/impl/Converter.cpp @@ -1366,6 +1366,16 @@ const Converter::EnumMap Converter::c {core::CancelLocation::beforeBarline(), api::CancelLocation::beforeBarline}, }; +// The standard vocabulary. api::KeyMode::unspecified and api::KeyMode::unsupported are absent +// because they have no wire spelling. +const Converter::EnumMap Converter::keyModeMap = { + {core::Mode{"major"}, api::KeyMode::major}, {core::Mode{"minor"}, api::KeyMode::minor}, + {core::Mode{"dorian"}, api::KeyMode::dorian}, {core::Mode{"phrygian"}, api::KeyMode::phrygian}, + {core::Mode{"lydian"}, api::KeyMode::lydian}, {core::Mode{"mixolydian"}, api::KeyMode::mixolydian}, + {core::Mode{"aeolian"}, api::KeyMode::aeolian}, {core::Mode{"ionian"}, api::KeyMode::ionian}, + {core::Mode{"locrian"}, api::KeyMode::locrian}, {core::Mode{"none"}, api::KeyMode::none}, +}; + // core normal() folds into api unspecified: absent and "normal" are the same display. // The simple (narrow) symbol vocabulary: common/cut only. core normal() folds into api unspecified. const Converter::EnumMap Converter::simpleTimeSymbolMap = { @@ -1950,6 +1960,16 @@ api::CancelLocation Converter::convert(core::CancelLocation value) const return findApiItem(cancelLocationMap, api::CancelLocation::unspecified, value); } +core::Mode Converter::convert(api::KeyMode value) const +{ + return findCoreItem(keyModeMap, core::Mode{}, value); +} + +api::KeyMode Converter::convert(const core::Mode &value) const +{ + return findApiItem(keyModeMap, api::KeyMode::unsupported, value); +} + core::TimeSymbol Converter::convert(api::TimeSignatureSymbol value) const { return findCoreItem(simpleTimeSymbolMap, core::TimeSymbol::normal(), value); diff --git a/src/private/mx/impl/Converter.h b/src/private/mx/impl/Converter.h index 83a2e5114..d62f4b44f 100644 --- a/src/private/mx/impl/Converter.h +++ b/src/private/mx/impl/Converter.h @@ -39,6 +39,7 @@ #include "mx/core/generated/MeasureNumberingValue.h" #include "mx/core/generated/MembraneValue.h" #include "mx/core/generated/MetalValue.h" +#include "mx/core/generated/Mode.h" #include "mx/core/generated/NoteTypeValue.h" #include "mx/core/generated/NoteheadValue.h" #include "mx/core/generated/OrnamentsGroupChoice.h" @@ -187,6 +188,13 @@ class Converter core::CancelLocation convert(api::CancelLocation value) const; api::CancelLocation convert(core::CancelLocation value) const; + // is an open vocabulary, so a core Mode holds an arbitrary string. api::KeyMode::unspecified + // and api::KeyMode::unsupported have no wire spelling and convert to an empty core::Mode; callers + // write no element for an empty Mode. A core Mode outside the standard vocabulary (including + // the empty one) converts to api::KeyMode::unsupported. + core::Mode convert(api::KeyMode value) const; + api::KeyMode convert(const core::Mode &value) const; + // Simple (narrow) symbol: common/cut only. unspecified maps to core normal(); callers skip the // attribute entirely for unspecified (absent and normal mean the same thing on the wire). core::TimeSymbol convert(api::TimeSignatureSymbol value) const; @@ -271,6 +279,7 @@ class Converter const static EnumMap instrumentMap; const static EnumMap kindMap; const static EnumMap cancelLocationMap; + const static EnumMap keyModeMap; const static EnumMap simpleTimeSymbolMap; const static EnumMap complexTimeSymbolMap; const static EnumMap timeSeparatorMap; diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index f880e6417..03b1ab293 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -713,20 +713,7 @@ std::optional MeasureReader::parseAttributes(const core::Att if (traditionalKey.mode().has_value()) { - // TODO - support all modes, not just major/minor - const auto coreMode = traditionalKey.mode()->value(); - if (coreMode == "major") - { - keyData.mode = api::KeyMode::major; - } - else if (coreMode == "minor") - { - keyData.mode = api::KeyMode::minor; - } - else - { - keyData.mode = api::KeyMode::unsupported; - } + keyData.mode = myConverter.convert(*traditionalKey.mode()); } keyData.tickTimePosition = myCurrentCursor.tickTimePosition; diff --git a/src/private/mx/impl/PropertiesWriter.cpp b/src/private/mx/impl/PropertiesWriter.cpp index bd1ee6894..b80a5faf9 100644 --- a/src/private/mx/impl/PropertiesWriter.cpp +++ b/src/private/mx/impl/PropertiesWriter.cpp @@ -153,10 +153,13 @@ void PropertiesWriter::writeTraditionalKey(const api::KeyData &inKeyData, core:: tkg.setCancel(cancel); } - if (inKeyData.mode == api::KeyMode::major || inKeyData.mode == api::KeyMode::minor) + // an unspecified or unsupported mode converts to an empty core::Mode, which has no spelling to + // write; every other KeyMode, including none, writes its element + const Converter modeConverter; + const auto mode = modeConverter.convert(inKeyData.mode); + if (!mode.value().empty()) { - const auto modeStr = (inKeyData.mode == api::KeyMode::major) ? "major" : "minor"; - tkg.setMode(core::Mode{modeStr}); + tkg.setMode(mode); } ioKey.setChoice(core::KeyChoice::traditionalKey(tkg)); diff --git a/src/private/mxtest/api/KeyDataTest.cpp b/src/private/mxtest/api/KeyDataTest.cpp index b1f43f23b..779272164 100644 --- a/src/private/mxtest/api/KeyDataTest.cpp +++ b/src/private/mxtest/api/KeyDataTest.cpp @@ -61,6 +61,59 @@ const mx::core::Key &getFirstCoreKey(const mx::core::DocumentPtr &corePtr) REQUIRE(!keys.empty()); return keys.front(); } + +/// Helper: build a single-key score with the given fifths and mode, serialize it, and return the XML. +std::string keyModeXml(int fifths, KeyMode mode) +{ + KeyData key; + key.fifths = fifths; + key.mode = mode; + return mxtest::toXml(putKeyInScore(key)); +} + +/// Helper: wrap a key element's children in a minimal score-partwise document. +std::string keyXmlDocument(const std::string &inKeyChildren) +{ + return R"( + + + + P + + + + + + 1 + +)" + inKeyChildren + + R"( + + + + + C + 4 + + 1 + quarter + + + + +)"; +} + +/// Helper: parse a document whose key holds the given children, and return the first KeyData. +KeyData keyFromXml(const std::string &inKeyChildren) +{ + const auto score = mxtest::fromXml(keyXmlDocument(inKeyChildren)); + REQUIRE(!score.parts.empty()); + REQUIRE(!score.parts.at(0).measures.empty()); + const auto &keys = score.parts.at(0).measures.at(0).keys; + REQUIRE(!keys.empty()); + return keys.at(0); +} } // namespace TEST(EMajor, KeyData) @@ -561,6 +614,152 @@ TEST(CancelLocationFromXml, KeyData) CHECK_EQUAL(CancelLocation::right, keys.at(0).cancelLocation); } +TEST(ModeSerializationAllValues, KeyData) +{ + // every mode with a MusicXML spelling writes that exact spelling + CHECK(keyModeXml(0, KeyMode::major).find("major") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::minor).find("minor") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::dorian).find("dorian") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::phrygian).find("phrygian") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::lydian).find("lydian") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::mixolydian).find("mixolydian") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::aeolian).find("aeolian") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::ionian).find("ionian") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::locrian).find("locrian") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::none).find("none") != std::string::npos); + + // unspecified and unsupported have no spelling, so no element is written + CHECK(keyModeXml(0, KeyMode::unspecified).find("") == std::string::npos); + CHECK(keyModeXml(0, KeyMode::unsupported).find("") == std::string::npos); +} + +TEST(ModeDeserializationAllValues, KeyData) +{ + CHECK_EQUAL(KeyMode::major, keyFromXml("0major").mode); + CHECK_EQUAL(KeyMode::minor, keyFromXml("0minor").mode); + CHECK_EQUAL(KeyMode::dorian, keyFromXml("0dorian").mode); + CHECK_EQUAL(KeyMode::phrygian, keyFromXml("0phrygian").mode); + CHECK_EQUAL(KeyMode::lydian, keyFromXml("0lydian").mode); + CHECK_EQUAL(KeyMode::mixolydian, keyFromXml("0mixolydian").mode); + CHECK_EQUAL(KeyMode::aeolian, keyFromXml("0aeolian").mode); + CHECK_EQUAL(KeyMode::ionian, keyFromXml("0ionian").mode); + CHECK_EQUAL(KeyMode::locrian, keyFromXml("0locrian").mode); + CHECK_EQUAL(KeyMode::none, keyFromXml("0none").mode); +} + +TEST(ModeAbsentIsUnspecified, KeyData) +{ + // is optional; when it is absent the key states no mode + const auto key = keyFromXml("-3"); + CHECK_EQUAL(KeyMode::unspecified, key.mode); + CHECK_EQUAL(-3, key.fifths); +} + +TEST(ModeOutsideVocabularyIsUnsupported, KeyData) +{ + // is an open vocabulary in MusicXML; a value we do not model reads as unsupported + CHECK_EQUAL(KeyMode::unsupported, keyFromXml("0banana").mode); +} + +TEST(ModeNoneRoundTrip, KeyData) +{ + // a keyless signature: zero fifths with none + KeyData key; + key.fifths = 0; + key.mode = KeyMode::none; + + const auto xml = mxtest::toXml(putKeyInScore(key)); + CHECK(xml.find("0") != std::string::npos); + CHECK(xml.find("none") != std::string::npos); + + const auto score = mxtest::fromXml(xml); + REQUIRE(!score.parts.empty()); + REQUIRE(!score.parts.at(0).measures.empty()); + const auto &keys = score.parts.at(0).measures.at(0).keys; + REQUIRE(!keys.empty()); + const auto &deserializedKey = keys.at(0); + CHECK_EQUAL(0, deserializedKey.fifths); + CHECK_EQUAL(KeyMode::none, deserializedKey.mode); + CHECK(deserializedKey.nonTraditional.empty()); + CHECK_EQUAL(key, deserializedKey); +} + +TEST(ModeRoundTripAllValues, KeyData) +{ + // every mode with a spelling survives a write/read round trip, and fifths is untouched by it + const std::vector modes{KeyMode::major, KeyMode::minor, KeyMode::dorian, KeyMode::phrygian, + KeyMode::lydian, KeyMode::aeolian, KeyMode::ionian, KeyMode::locrian, + KeyMode::mixolydian, KeyMode::none}; + for (const auto mode : modes) + { + KeyData key; + key.fifths = 2; + key.mode = mode; + const auto score = mxtest::fromXml(mxtest::toXml(putKeyInScore(key))); + REQUIRE(!score.parts.empty()); + REQUIRE(!score.parts.at(0).measures.empty()); + const auto &keys = score.parts.at(0).measures.at(0).keys; + REQUIRE(!keys.empty()); + CHECK_EQUAL(mode, keys.at(0).mode); + CHECK_EQUAL(2, keys.at(0).fifths); + } +} + +TEST(ZeroFifthsModesAreDistinct, KeyData) +{ + // zero fifths does not imply any particular mode: C major, A minor, keyless, and mode-less are + // four different key signatures + KeyData cMajor; + cMajor.fifths = 0; + cMajor.mode = KeyMode::major; + + KeyData aMinor = cMajor; + aMinor.mode = KeyMode::minor; + + KeyData keyless = cMajor; + keyless.mode = KeyMode::none; + + KeyData modeless = cMajor; + modeless.mode = KeyMode::unspecified; + + CHECK(cMajor != aMinor); + CHECK(cMajor != keyless); + CHECK(cMajor != modeless); + CHECK(aMinor != keyless); + CHECK(aMinor != modeless); + CHECK(keyless != modeless); + + // and the distinction survives serialization + CHECK(keyModeXml(0, KeyMode::major).find("major") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::minor).find("minor") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::none).find("none") != std::string::npos); + CHECK(keyModeXml(0, KeyMode::unspecified).find("") == std::string::npos); +} + +TEST(ModeNoneIsNotNonTraditional, KeyData) +{ + // KeyMode::none is a mode value, not a nontraditional key; it writes a traditional key + KeyData key; + key.fifths = 0; + key.mode = KeyMode::none; + + const auto original = putKeyInScore(key); + auto &docMgr = DocumentManager::getInstance(); + const auto originalIdResult = docMgr.createFromScore(original); + REQUIRE(originalIdResult.ok()); + const int originalId = originalIdResult.value(); + const mx::core::DocumentPtr corePtr = docMgr.getDocument(originalId); + + const auto &coreKey = getFirstCoreKey(corePtr); + CHECK(coreKey.choice().isTraditionalKey()); + const auto &coreTraditionalKey = coreKey.choice().asTraditionalKey(); + CHECK_EQUAL(0, coreTraditionalKey.fifths().value()); + REQUIRE(coreTraditionalKey.mode().has_value()); + CHECK_EQUAL(std::string{"none"}, coreTraditionalKey.mode()->value()); + + docMgr.destroyDocument(originalId); +} + TEST(KeyDataEquality_change_cancelLocation, KeyData) { KeyData key1; diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 1fdc3c389..d113e78d2 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -41,6 +41,7 @@ lysuite/ly11c_TimeSignatures_CompoundSimple.xml lysuite/ly11g_TimeSignatures_SingleNumber.xml lysuite/ly12b_Clefs_NoKeyOrClef.xml lysuite/ly13a_KeySignatures.xml +lysuite/ly13b_KeySignatures_ChurchModes.xml lysuite/ly13d_KeySignatures_Microtones.xml lysuite/ly21a_Chord_Basic.xml lysuite/ly21b_Chords_TwoNotes.xml