fix(api): round-trip the key cancel location attribute

src/include/mx/api/KeyData.h

@@ -18,6 +18,19 @@ enum class KeyMode
     minor
 };
 
+// CancelLocation represents the cancel element's optional location attribute. From MusicXML
+// Specification: The cancel-location type is used to indicate where a key signature
+// cancellation appears relative to a new key signature: to the left, to the right, or before
+// the barline and to the left. It is left by default. For mid-measure key elements, a
+// cancel-location of before-barline should be treated like a cancel-location of left.
+enum class CancelLocation
+{
+    unspecified, // a location value was not provided
+    left,
+    right,
+    beforeBarline
+};
+
 // KeyData represents a key signature. It can be in one of two configurations. Either you specify
 // 'fifths' and 'mode', or you can create a custom key signature by adding items to the customKey
 // vector. If anything is found in the customKey vector, then fifths and mode will be ignored.
@@ -61,6 +74,10 @@ struct KeyData
     // appears relative to the new key signature.
     int cancel;
 
+    // The cancel element's optional location attribute. It is ignored unless cancel is non-zero
+    // (i.e. unless a cancel element is present).
+    CancelLocation cancelLocation;
+
     // Mode specifies whether the key is major or minor. It is optional.
     KeyMode mode;
 
@@ -79,15 +96,16 @@ struct KeyData
     std::vector<KeyComponent> nonTraditional;
 
     KeyData()
-        : fifths{0}, cancel{0}, mode{KeyMode::unspecified}, tickTimePosition{0}, staffIndex{INDEX_UNSPECIFIED},
-          nonTraditional{}
+        : fifths{0}, cancel{0}, cancelLocation{CancelLocation::unspecified}, mode{KeyMode::unspecified},
+          tickTimePosition{0}, staffIndex{INDEX_UNSPECIFIED}, nonTraditional{}
     {
     }
 };
 
 MXAPI_EQUALS_BEGIN(KeyData)
 MXAPI_EQUALS_MEMBER(fifths)
 MXAPI_EQUALS_MEMBER(cancel)
+MXAPI_EQUALS_MEMBER(cancelLocation)
 MXAPI_EQUALS_MEMBER(mode)
 MXAPI_EQUALS_MEMBER(tickTimePosition)
 MXAPI_EQUALS_MEMBER(staffIndex)

src/private/mx/impl/Converter.cpp

@@ -1332,6 +1332,12 @@ const Converter::EnumMap<core::KindValue, api::ChordKind> Converter::kindMap = {
     {core::KindValue::none(), api::ChordKind::none},
 };
 
+const Converter::EnumMap<core::CancelLocation, api::CancelLocation> Converter::cancelLocationMap = {
+    {core::CancelLocation::left(), api::CancelLocation::left},
+    {core::CancelLocation::right(), api::CancelLocation::right},
+    {core::CancelLocation::beforeBarline(), api::CancelLocation::beforeBarline},
+};
+
 api::Step Converter::convert(core::Step inStep) const
 {
     return findApiItem(stepMap, api::Step::c, inStep);
@@ -1654,6 +1660,16 @@ api::ChordKind Converter::convert(core::KindValue value) const
     return findApiItem(kindMap, api::ChordKind::unspecified, value);
 }
 
+core::CancelLocation Converter::convert(api::CancelLocation value) const
+{
+    return findCoreItem(cancelLocationMap, core::CancelLocation::left(), value);
+}
+
+api::CancelLocation Converter::convert(core::CancelLocation value) const
+{
+    return findApiItem(cancelLocationMap, api::CancelLocation::unspecified, value);
+}
+
 double Converter::convertToAlter(int semitones, double cents)
 {
     double alter = 0.0;

src/private/mx/impl/Converter.h

@@ -6,6 +6,7 @@
 
 #include "mx/api/BarlineData.h"
 #include "mx/api/ClefData.h"
+#include "mx/api/KeyData.h"
 #include "mx/api/MarkData.h"
 #include "mx/api/NoteData.h"
 #include "mx/api/PositionData.h"
@@ -17,6 +18,7 @@
 #include "mx/core/generated/BarStyle.h"
 #include "mx/core/generated/BeamValue.h"
 #include "mx/core/generated/CSSFontSize.h"
+#include "mx/core/generated/CancelLocation.h"
 #include "mx/core/generated/ClefSign.h"
 #include "mx/core/generated/DynamicsChoice.h"
 #include "mx/core/generated/FermataShape.h"
@@ -155,6 +157,9 @@ class Converter
     core::KindValue convert(api::ChordKind value) const;
     api::ChordKind convert(core::KindValue value) const;
 
+    core::CancelLocation convert(api::CancelLocation value) const;
+    api::CancelLocation convert(core::CancelLocation value) const;
+
     static double convertToAlter(int semitones, double cents);
     static std::pair<int, double> convertToSemitonesAndCents(double alter);
 
@@ -192,6 +197,7 @@ class Converter
     const static EnumMap<core::FermataShape, api::MarkType> fermataMap;
     const static EnumMap<core::SoundID, api::SoundID> instrumentMap;
     const static EnumMap<core::KindValue, api::ChordKind> kindMap;
+    const static EnumMap<core::CancelLocation, api::CancelLocation> cancelLocationMap;
 
   private:
     template <typename CORE_TYPE, typename API_TYPE>

src/private/mx/impl/MeasureReader.cpp

@@ -484,6 +484,10 @@ std::optional<api::TransposeData> MeasureReader::parseAttributes(const core::Att
         if (traditionalKey.cancel().has_value())
         {
             keyData.cancel = traditionalKey.cancel()->value().value();
+            if (traditionalKey.cancel()->location().has_value())
+            {
+                keyData.cancelLocation = myConverter.convert(*traditionalKey.cancel()->location());
+            }
         }
 
         if (traditionalKey.mode().has_value())

src/private/mx/impl/PropertiesWriter.cpp

@@ -123,6 +123,11 @@ void PropertiesWriter::writeTraditionalKey(const api::KeyData &inKeyData, core::
     {
         core::Cancel cancel{};
         cancel.setValue(core::Fifths{inKeyData.cancel});
+        if (inKeyData.cancelLocation != api::CancelLocation::unspecified)
+        {
+            Converter converter;
+            cancel.setLocation(converter.convert(inKeyData.cancelLocation));
+        }
         tkg.setCancel(cancel);
     }
 

src/private/mxtest/api/KeyDataTest.cpp

@@ -10,6 +10,7 @@
 #include "cpul/cpulTestHarness.h"
 #include "mx/api/DocumentManager.h"
 #include "mx/core/generated/Attributes.h"
+#include "mx/core/generated/Cancel.h"
 #include "mx/core/generated/Document.h"
 #include "mx/core/generated/Key.h"
 #include "mx/core/generated/KeyChoice.h"
@@ -19,6 +20,7 @@
 #include "mx/core/generated/PartwisePart.h"
 #include "mx/core/generated/ScorePartwise.h"
 #include "mx/core/generated/TraditionalKeyGroup.h"
+#include "mxtest/api/TestHelpers.h"
 
 using namespace std;
 using namespace mx::api;
@@ -425,4 +427,157 @@ TEST(IsInitialized, KeyComponent)
     CHECK(k.accidental == Accidental{});
 }
 
+TEST(CancelLocationBeforeBarline, KeyData)
+{
+    // https://github.com/webern/mx/issues/272
+    KeyData key;
+    key.fifths = 0;
+    key.cancel = 2;
+    key.cancelLocation = CancelLocation::beforeBarline;
+
+    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);
+    const auto &coreKeyChoice = coreKey.choice();
+    CHECK(coreKeyChoice.isTraditionalKey());
+    const auto &coreTraditionalKey = coreKeyChoice.asTraditionalKey();
+
+    // check that the cancel and its location were written
+    REQUIRE(coreTraditionalKey.cancel().has_value());
+    CHECK_EQUAL(2, coreTraditionalKey.cancel()->value().value());
+    CHECK(coreTraditionalKey.cancel()->location().has_value())
+    if (coreTraditionalKey.cancel()->location().has_value())
+    {
+        CHECK(core::CancelLocation::Tag::beforeBarline == coreTraditionalKey.cancel()->location()->tag());
+    }
+
+    // serialize and deserialize
+    std::stringstream xml;
+    docMgr.writeToStream(originalId, xml);
+    docMgr.destroyDocument(originalId);
+    CHECK(xml.str().find("location=\"before-barline\"") != std::string::npos);
+    std::istringstream iss{xml.str()};
+    const auto deserializedIdResult = docMgr.createFromStream(iss);
+    REQUIRE(deserializedIdResult.ok());
+    const int deserializedId = deserializedIdResult.value();
+    const auto deserializedScoreResult = docMgr.getData(deserializedId);
+    docMgr.destroyDocument(deserializedId);
+    REQUIRE(deserializedScoreResult.ok());
+    const auto &deserializedScore = deserializedScoreResult.value();
+    const auto &deserializedKeys = deserializedScore.parts.at(0).measures.at(0).keys;
+    CHECK_EQUAL(1, deserializedKeys.size())
+    const auto deserializedKey = deserializedKeys.at(0);
+
+    CHECK_EQUAL(key.cancel, deserializedKey.cancel);
+    CHECK_EQUAL(CancelLocation::beforeBarline, deserializedKey.cancelLocation);
+}
+
+TEST(CancelLocationUnspecified, KeyData)
+{
+    // when cancelLocation is unspecified, no location attribute is written
+    KeyData key;
+    key.fifths = 0;
+    key.cancel = -3;
+
+    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);
+    const auto &coreKeyChoice = coreKey.choice();
+    CHECK(coreKeyChoice.isTraditionalKey());
+    const auto &coreTraditionalKey = coreKeyChoice.asTraditionalKey();
+
+    REQUIRE(coreTraditionalKey.cancel().has_value());
+    CHECK(!coreTraditionalKey.cancel()->location().has_value())
+
+    // serialize and deserialize
+    std::stringstream xml;
+    docMgr.writeToStream(originalId, xml);
+    docMgr.destroyDocument(originalId);
+    CHECK(xml.str().find("location=") == std::string::npos);
+    std::istringstream iss{xml.str()};
+    const auto deserializedIdResult = docMgr.createFromStream(iss);
+    REQUIRE(deserializedIdResult.ok());
+    const int deserializedId = deserializedIdResult.value();
+    const auto deserializedScoreResult = docMgr.getData(deserializedId);
+    docMgr.destroyDocument(deserializedId);
+    REQUIRE(deserializedScoreResult.ok());
+    const auto &deserializedScore = deserializedScoreResult.value();
+    const auto &deserializedKeys = deserializedScore.parts.at(0).measures.at(0).keys;
+    CHECK_EQUAL(1, deserializedKeys.size())
+    const auto deserializedKey = deserializedKeys.at(0);
+
+    CHECK_EQUAL(key.cancel, deserializedKey.cancel);
+    CHECK_EQUAL(CancelLocation::unspecified, deserializedKey.cancelLocation);
+}
+
+TEST(CancelLocationFromXml, KeyData)
+{
+    // mirrors data/synthetic/cancel.location.3.0.xml, but with location="right"
+    const std::string xml = R"(<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<score-partwise version="3.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>
+          <cancel location="right">2</cancel>
+          <fifths>0</fifths>
+          <mode>major</mode>
+        </key>
+      </attributes>
+      <note>
+        <pitch>
+          <step>C</step>
+          <octave>4</octave>
+        </pitch>
+        <duration>1</duration>
+        <type>quarter</type>
+      </note>
+    </measure>
+  </part>
+</score-partwise>
+)";
+    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());
+    CHECK_EQUAL(2, keys.at(0).cancel);
+    CHECK_EQUAL(CancelLocation::right, keys.at(0).cancelLocation);
+}
+
+TEST(KeyDataEquality_change_cancelLocation, KeyData)
+{
+    KeyData key1;
+    key1.fifths = 4;
+    key1.cancel = -1;
+    key1.cancelLocation = CancelLocation::left;
+    key1.mode = KeyMode::major;
+    key1.staffIndex = 0;
+    key1.tickTimePosition = 13;
+    auto key2 = key1;
+    CHECK(key1 == key2);
+    CHECK(!(key1 != key2));
+
+    // change one thing
+    key1.cancelLocation = CancelLocation::right;
+    CHECK(key1 != key2);
+    CHECK(!(key1 == key2));
+}
+
 #endif

src/private/mxtest/api/roundtrip-baseline.txt

@@ -191,3 +191,9 @@ lysuite/ly46c_Midmeasure_Clef.xml
 # repeat's times attribute survives the round-trip.
 lysuite/ly45a_SimpleRepeat.xml
 lysuite/ly45c_RepeatMultipleTimes.xml
+
+# Unblocked by #272 (key cancel location round-trip). The reader dropped the
+# cancel element's location attribute and the writer never emitted it. Added
+# KeyData::cancelLocation (CancelLocation::unspecified when absent) so the
+# attribute survives the round-trip.
+synthetic/cancel.location.3.0.xml