feat: full MusicXML key-mode support in mx::api

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 <mode> 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 <mode>none</mode>. It is a mode value like any other, and it says nothing about
+// how the signature is drawn.
+//
+// MusicXML's <mode> 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 <mode> element. KeyMode::none
+    // is a stated mode, and it writes <mode>none</mode>.
     KeyMode mode;
 
     // Supports changing the key somewhere other than at the start of a measure.

src/private/mx/impl/Converter.cpp

@@ -1366,6 +1366,16 @@ const Converter::EnumMap<core::CancelLocation, api::CancelLocation> Converter::c
     {core::CancelLocation::beforeBarline(), api::CancelLocation::beforeBarline},
 };
 
+// The standard <mode> vocabulary. api::KeyMode::unspecified and api::KeyMode::unsupported are absent
+// because they have no wire spelling.
+const Converter::EnumMap<core::Mode, api::KeyMode> 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<core::TimeSymbol, api::TimeSignatureSymbol> 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);

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;
 
+    // <mode> 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 <mode> 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<core::SoundID, api::SoundID> instrumentMap;
     const static EnumMap<core::KindValue, api::ChordKind> kindMap;
     const static EnumMap<core::CancelLocation, api::CancelLocation> cancelLocationMap;
+    const static EnumMap<core::Mode, api::KeyMode> keyModeMap;
     const static EnumMap<core::TimeSymbol, api::TimeSignatureSymbol> simpleTimeSymbolMap;
     const static EnumMap<core::TimeSymbol, api::ComplexTimeSymbol> complexTimeSymbolMap;
     const static EnumMap<core::TimeSeparator, api::TimeSeparator> timeSeparatorMap;

src/private/mx/impl/MeasureReader.cpp

@@ -713,20 +713,7 @@ std::optional<api::TransposeData> 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;
 

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 <mode> 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));

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"(<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<score-partwise version="4.0">
+  <part-list>
+    <score-part id="P1">
+      <part-name>P</part-name>
+    </score-part>
+  </part-list>
+  <part id="P1">
+    <measure number="1">
+      <attributes>
+        <divisions>1</divisions>
+        <key>
+)" + inKeyChildren +
+           R"(
+        </key>
+      </attributes>
+      <note>
+        <pitch>
+          <step>C</step>
+          <octave>4</octave>
+        </pitch>
+        <duration>1</duration>
+        <type>quarter</type>
+      </note>
+    </measure>
+  </part>
+</score-partwise>
+)";
+}
+
+/// 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("<mode>major</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::minor).find("<mode>minor</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::dorian).find("<mode>dorian</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::phrygian).find("<mode>phrygian</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::lydian).find("<mode>lydian</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::mixolydian).find("<mode>mixolydian</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::aeolian).find("<mode>aeolian</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::ionian).find("<mode>ionian</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::locrian).find("<mode>locrian</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::none).find("<mode>none</mode>") != std::string::npos);
+
+    // unspecified and unsupported have no spelling, so no <mode> element is written
+    CHECK(keyModeXml(0, KeyMode::unspecified).find("<mode>") == std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::unsupported).find("<mode>") == std::string::npos);
+}
+
+TEST(ModeDeserializationAllValues, KeyData)
+{
+    CHECK_EQUAL(KeyMode::major, keyFromXml("<fifths>0</fifths><mode>major</mode>").mode);
+    CHECK_EQUAL(KeyMode::minor, keyFromXml("<fifths>0</fifths><mode>minor</mode>").mode);
+    CHECK_EQUAL(KeyMode::dorian, keyFromXml("<fifths>0</fifths><mode>dorian</mode>").mode);
+    CHECK_EQUAL(KeyMode::phrygian, keyFromXml("<fifths>0</fifths><mode>phrygian</mode>").mode);
+    CHECK_EQUAL(KeyMode::lydian, keyFromXml("<fifths>0</fifths><mode>lydian</mode>").mode);
+    CHECK_EQUAL(KeyMode::mixolydian, keyFromXml("<fifths>0</fifths><mode>mixolydian</mode>").mode);
+    CHECK_EQUAL(KeyMode::aeolian, keyFromXml("<fifths>0</fifths><mode>aeolian</mode>").mode);
+    CHECK_EQUAL(KeyMode::ionian, keyFromXml("<fifths>0</fifths><mode>ionian</mode>").mode);
+    CHECK_EQUAL(KeyMode::locrian, keyFromXml("<fifths>0</fifths><mode>locrian</mode>").mode);
+    CHECK_EQUAL(KeyMode::none, keyFromXml("<fifths>0</fifths><mode>none</mode>").mode);
+}
+
+TEST(ModeAbsentIsUnspecified, KeyData)
+{
+    // <mode> is optional; when it is absent the key states no mode
+    const auto key = keyFromXml("<fifths>-3</fifths>");
+    CHECK_EQUAL(KeyMode::unspecified, key.mode);
+    CHECK_EQUAL(-3, key.fifths);
+}
+
+TEST(ModeOutsideVocabularyIsUnsupported, KeyData)
+{
+    // <mode> is an open vocabulary in MusicXML; a value we do not model reads as unsupported
+    CHECK_EQUAL(KeyMode::unsupported, keyFromXml("<fifths>0</fifths><mode>banana</mode>").mode);
+}
+
+TEST(ModeNoneRoundTrip, KeyData)
+{
+    // a keyless signature: zero fifths with <mode>none</mode>
+    KeyData key;
+    key.fifths = 0;
+    key.mode = KeyMode::none;
+
+    const auto xml = mxtest::toXml(putKeyInScore(key));
+    CHECK(xml.find("<fifths>0</fifths>") != std::string::npos);
+    CHECK(xml.find("<mode>none</mode>") != 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<KeyMode> 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("<mode>major</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::minor).find("<mode>minor</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::none).find("<mode>none</mode>") != std::string::npos);
+    CHECK(keyModeXml(0, KeyMode::unspecified).find("<mode>") == 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;

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