feat: round-trip arpeggiate and non-arpeggiate attributes

src/include/mx/api/MarkDataChoice.h

@@ -7,6 +7,7 @@
 #include "mx/api/ApiCommon.h"
 
 #include <optional>
+#include <string>
 #include <variant>
 
 namespace mx
@@ -28,6 +29,60 @@ MXAPI_EQUALS_MEMBER(tremoloMarks)
 MXAPI_EQUALS_END;
 MXAPI_NOT_EQUALS_AND_VECTORS(TremoloMarkData);
 
+// Payload for the arpeggiate mark types (MarkType::arpeggiate / arpeggiateUp / arpeggiateDown).
+// The up/down direction of the arpeggio's wavy line is encoded in the MarkType itself.
+struct ArpeggiateMarkData
+{
+    // Distinguishes overlapping arpeggios: notes sharing a number belong to one arpeggio (e.g.
+    // an arpeggio spanning both staves of a keyboard part) while different numbers arpeggiate
+    // independently. Corresponds to the `number` attribute.
+    std::optional<int> number;
+
+    // yes: the arpeggio is drawn as one unbroken line across staves instead of one line per
+    // staff. Corresponds to the `unbroken` attribute (MusicXML 4.0).
+    Bool unbroken = Bool::unspecified;
+
+    // The element's `id` attribute (MusicXML 3.1).
+    std::optional<std::string> id;
+};
+
+MXAPI_EQUALS_BEGIN(ArpeggiateMarkData)
+MXAPI_EQUALS_MEMBER(number)
+MXAPI_EQUALS_MEMBER(unbroken)
+MXAPI_EQUALS_MEMBER(id)
+MXAPI_EQUALS_END;
+MXAPI_NOT_EQUALS_AND_VECTORS(ArpeggiateMarkData);
+
+// Which end of a non-arpeggiate bracket a note carries: the bracket's top or bottom.
+enum class NonArpeggiatePlacement
+{
+    top,
+    bottom,
+};
+
+// Payload for MarkType::nonArpeggiate: a bracket indicating that a chord is NOT to be
+// arpeggiated. MusicXML marks only the top and bottom notes of the chord, each carrying a
+// <non-arpeggiate> element that states which end of the bracket it is.
+struct NonArpeggiateMarkData
+{
+    // Which end of the bracket this note carries. Corresponds to the required `type` attribute.
+    NonArpeggiatePlacement placement = NonArpeggiatePlacement::top;
+
+    // Distinguishes overlapping brackets, like ArpeggiateMarkData::number. Corresponds to the
+    // `number` attribute.
+    std::optional<int> number;
+
+    // The element's `id` attribute (MusicXML 3.1).
+    std::optional<std::string> id;
+};
+
+MXAPI_EQUALS_BEGIN(NonArpeggiateMarkData)
+MXAPI_EQUALS_MEMBER(placement)
+MXAPI_EQUALS_MEMBER(number)
+MXAPI_EQUALS_MEMBER(id)
+MXAPI_EQUALS_END;
+MXAPI_NOT_EQUALS_AND_VECTORS(NonArpeggiateMarkData);
+
 // A variant class that carries data for MarkType values whose payload does not fit MarkData's
 // common fields.
 //
@@ -48,27 +103,47 @@ class MarkDataChoice
     enum class Kind
     {
         none,
-        tremolo
+        tremolo,
+        arpeggiate,
+        nonArpeggiate
     };
 
     MarkDataChoice();
 
     MarkDataChoice(TremoloMarkData value);
 
+    MarkDataChoice(ArpeggiateMarkData value);
+
+    MarkDataChoice(NonArpeggiateMarkData value);
+
     Kind kind() const;
     bool isNone() const;
     bool isTremolo() const;
+    bool isArpeggiate() const;
+    bool isNonArpeggiate() const;
 
     // Returns a copy of the internally held TremoloMarkData.
     //
     // Check isTremolo() first. If this is not a tremolo payload, a default constructed
     // TremoloMarkData is returned.
     const TremoloMarkData tremolo() const;
 
+    // Returns a copy of the internally held ArpeggiateMarkData.
+    //
+    // Check isArpeggiate() first. If this is not an arpeggiate payload, a default constructed
+    // ArpeggiateMarkData is returned.
+    const ArpeggiateMarkData arpeggiate() const;
+
+    // Returns a copy of the internally held NonArpeggiateMarkData.
+    //
+    // Check isNonArpeggiate() first. If this is not a non-arpeggiate payload, a default
+    // constructed NonArpeggiateMarkData is returned.
+    const NonArpeggiateMarkData nonArpeggiate() const;
+
     bool operator==(const MarkDataChoice &other) const;
 
   private:
-    std::variant<std::monostate, TremoloMarkData> myValue;
+    std::variant<std::monostate, TremoloMarkData, ArpeggiateMarkData, NonArpeggiateMarkData> myValue;
 };
 
 MXAPI_NOT_EQUALS_AND_VECTORS(MarkDataChoice);

src/private/mx/api/MarkDataChoice.cpp

@@ -19,9 +19,29 @@ MarkDataChoice::MarkDataChoice(TremoloMarkData value) : myValue{std::move(value)
 {
 }
 
+MarkDataChoice::MarkDataChoice(ArpeggiateMarkData value) : myValue{std::move(value)}
+{
+}
+
+MarkDataChoice::MarkDataChoice(NonArpeggiateMarkData value) : myValue{std::move(value)}
+{
+}
+
 MarkDataChoice::Kind MarkDataChoice::kind() const
 {
-    return std::holds_alternative<TremoloMarkData>(myValue) ? Kind::tremolo : Kind::none;
+    if (std::holds_alternative<TremoloMarkData>(myValue))
+    {
+        return Kind::tremolo;
+    }
+    if (std::holds_alternative<ArpeggiateMarkData>(myValue))
+    {
+        return Kind::arpeggiate;
+    }
+    if (std::holds_alternative<NonArpeggiateMarkData>(myValue))
+    {
+        return Kind::nonArpeggiate;
+    }
+    return Kind::none;
 }
 
 bool MarkDataChoice::isNone() const
@@ -43,6 +63,34 @@ const TremoloMarkData MarkDataChoice::tremolo() const
     return TremoloMarkData{};
 }
 
+bool MarkDataChoice::isArpeggiate() const
+{
+    return std::holds_alternative<ArpeggiateMarkData>(myValue);
+}
+
+const ArpeggiateMarkData MarkDataChoice::arpeggiate() const
+{
+    if (const auto *value = std::get_if<ArpeggiateMarkData>(&myValue))
+    {
+        return *value;
+    }
+    return ArpeggiateMarkData{};
+}
+
+bool MarkDataChoice::isNonArpeggiate() const
+{
+    return std::holds_alternative<NonArpeggiateMarkData>(myValue);
+}
+
+const NonArpeggiateMarkData MarkDataChoice::nonArpeggiate() const
+{
+    if (const auto *value = std::get_if<NonArpeggiateMarkData>(&myValue))
+    {
+        return *value;
+    }
+    return NonArpeggiateMarkData{};
+}
+
 bool MarkDataChoice::operator==(const MarkDataChoice &other) const
 {
     return myValue == other.myValue;

src/private/mx/impl/ArpeggiateFunctions.cpp

@@ -4,6 +4,7 @@
 
 #include "mx/impl/ArpeggiateFunctions.h"
 #include "mx/core/generated/Arpeggiate.h"
+#include "mx/impl/Converter.h"
 #include "mx/impl/MarkDataFunctions.h"
 
 namespace mx
@@ -36,6 +37,22 @@ api::MarkData ArpeggiateFunctions::parseArpeggiate() const
     impl::parseMarkDataAttributes(myArpeggiate, markData);
     markData.tickTimePosition = myCursor.tickTimePosition;
 
+    api::ArpeggiateMarkData arpeggiateData{};
+    if (myArpeggiate.number().has_value())
+    {
+        arpeggiateData.number = myArpeggiate.number()->value();
+    }
+    if (myArpeggiate.unbroken().has_value())
+    {
+        Converter converter;
+        arpeggiateData.unbroken = converter.convert(*myArpeggiate.unbroken());
+    }
+    if (myArpeggiate.id().has_value())
+    {
+        arpeggiateData.id = myArpeggiate.id()->value();
+    }
+    markData.choice = arpeggiateData;
+
     return markData;
 }
 } // namespace impl

src/private/mx/impl/NonArpeggiateFunctions.cpp

@@ -21,6 +21,21 @@ api::MarkData NonArpeggiateFunctions::parseNonArpeggiate() const
     api::MarkData markData{api::MarkType::nonArpeggiate};
     impl::parseMarkDataAttributes(myNonArpeggiate, markData);
     markData.tickTimePosition = myCursor.tickTimePosition;
+
+    api::NonArpeggiateMarkData nonArpeggiateData{};
+    nonArpeggiateData.placement = myNonArpeggiate.type().tag() == core::TopBottom::Tag::bottom
+                                      ? api::NonArpeggiatePlacement::bottom
+                                      : api::NonArpeggiatePlacement::top;
+    if (myNonArpeggiate.number().has_value())
+    {
+        nonArpeggiateData.number = myNonArpeggiate.number()->value();
+    }
+    if (myNonArpeggiate.id().has_value())
+    {
+        nonArpeggiateData.id = myNonArpeggiate.id()->value();
+    }
+    markData.choice = nonArpeggiateData;
+
     return markData;
 }
 } // namespace impl

src/private/mx/impl/NotationsWriter.cpp

@@ -3,6 +3,7 @@
 // Distributed under the MIT License
 
 #include "mx/impl/NotationsWriter.h"
+#include "mx/core/Token.h"
 #include "mx/core/generated/Arpeggiate.h"
 #include "mx/core/generated/ArrowChoice.h"
 #include "mx/core/generated/ArrowChoiceGroup.h"
@@ -30,6 +31,7 @@
 #include "mx/core/generated/Mordent.h"
 #include "mx/core/generated/NonArpeggiate.h"
 #include "mx/core/generated/NotationsChoice.h"
+#include "mx/core/generated/NumberLevel.h"
 #include "mx/core/generated/OrnamentsGroup.h"
 #include "mx/core/generated/OrnamentsGroupChoice.h"
 #include "mx/core/generated/OtherPlacementText.h"
@@ -41,6 +43,7 @@
 #include "mx/core/generated/StrongAccent.h"
 #include "mx/core/generated/TechnicalChoice.h"
 #include "mx/core/generated/Tied.h"
+#include "mx/core/generated/TopBottom.h"
 #include "mx/core/generated/Tremolo.h"
 #include "mx/core/generated/TremoloMarks.h"
 #include "mx/core/generated/TremoloType.h"
@@ -52,6 +55,7 @@
 #include "mx/core/generated/UpDown.h"
 #include "mx/core/generated/UprightInverted.h"
 #include "mx/core/generated/WavyLine.h"
+#include "mx/impl/Converter.h"
 #include "mx/impl/CurveFunctions.h"
 #include "mx/impl/DynamicsWriter.h"
 #include "mx/impl/MarkDataFunctions.h"
@@ -358,6 +362,20 @@ core::Notations NotationsWriter::getNotations() const
         {
             core::NonArpeggiate nonArpeggiate;
             impl::setAttributesFromMarkData(mark, nonArpeggiate);
+
+            const auto nonArpeggiateData = mark.choice.nonArpeggiate();
+            nonArpeggiate.setType(nonArpeggiateData.placement == api::NonArpeggiatePlacement::bottom
+                                      ? core::TopBottom::bottom()
+                                      : core::TopBottom::top());
+            if (nonArpeggiateData.number.has_value())
+            {
+                nonArpeggiate.setNumber(core::NumberLevel{*nonArpeggiateData.number});
+            }
+            if (nonArpeggiateData.id.has_value())
+            {
+                nonArpeggiate.setID(core::Token{*nonArpeggiateData.id});
+            }
+
             outNotations.addChoice(core::NotationsChoice::nonArpeggiate(nonArpeggiate));
         }
         else if (isMarkArpeggiate(mark.markType))
@@ -378,6 +396,21 @@ core::Notations NotationsWriter::getNotations() const
                 arpeggiate.setDirection(core::UpDown::down());
             }
 
+            const auto arpeggiateData = mark.choice.arpeggiate();
+            if (arpeggiateData.number.has_value())
+            {
+                arpeggiate.setNumber(core::NumberLevel{*arpeggiateData.number});
+            }
+            if (arpeggiateData.unbroken != api::Bool::unspecified)
+            {
+                Converter converter;
+                arpeggiate.setUnbroken(converter.convert(arpeggiateData.unbroken));
+            }
+            if (arpeggiateData.id.has_value())
+            {
+                arpeggiate.setID(core::Token{*arpeggiateData.id});
+            }
+
             outNotations.addChoice(core::NotationsChoice::arpeggiate(arpeggiate));
         }
     }

src/private/mxtest/api/MarkRoundTripTest.cpp

@@ -19,7 +19,7 @@ namespace
 // Round-trips a single note-attached mark of the given MarkType through the
 // full serialize -> deserialize path and returns the marks read back from the
 // first note. Used by the enum data-loss regression tests below.
-std::vector<MarkData> roundTripMark(MarkType inMarkType)
+std::vector<MarkData> roundTripMarkData(const MarkData &inMarkData)
 {
     ScoreData score;
     score.parts.emplace_back();
@@ -31,7 +31,7 @@ std::vector<MarkData> roundTripMark(MarkType inMarkType)
     auto &voice = staff.voices[0];
     voice.notes.emplace_back();
     auto &note = voice.notes.back();
-    note.noteAttachmentData.marks.emplace_back(Placement::unspecified, inMarkType);
+    note.noteAttachmentData.marks.push_back(inMarkData);
 
     auto &mgr = DocumentManager::getInstance();
     const auto r1 = mgr.createFromScore(score);
@@ -58,6 +58,11 @@ std::vector<MarkData> roundTripMark(MarkType inMarkType)
     return onote.noteAttachmentData.marks;
 }
 
+std::vector<MarkData> roundTripMark(MarkType inMarkType)
+{
+    return roundTripMarkData(MarkData{Placement::unspecified, inMarkType});
+}
+
 bool hasMark(const std::vector<MarkData> &marks, MarkType inMarkType)
 {
     for (const auto &m : marks)
@@ -227,4 +232,40 @@ TEST(FermataCurlew, MarkRoundTrip)
 
 T_END;
 
+TEST(ArpeggiateAttributes, MarkRoundTrip)
+{
+    MarkData mark{Placement::unspecified, MarkType::arpeggiateUp};
+    ArpeggiateMarkData payload{};
+    payload.number = 2;
+    payload.unbroken = Bool::yes;
+    payload.id = "arp-1";
+    mark.choice = payload;
+
+    const auto marks = roundTripMarkData(mark);
+    REQUIRE(marks.size() == 1);
+    CHECK(marks.front().markType == MarkType::arpeggiateUp);
+    REQUIRE(marks.front().choice.isArpeggiate());
+    CHECK(payload == marks.front().choice.arpeggiate());
+}
+
+T_END;
+
+TEST(NonArpeggiateAttributes, MarkRoundTrip)
+{
+    MarkData mark{Placement::unspecified, MarkType::nonArpeggiate};
+    NonArpeggiateMarkData payload{};
+    payload.placement = NonArpeggiatePlacement::bottom;
+    payload.number = 1;
+    payload.id = "nonarp-1";
+    mark.choice = payload;
+
+    const auto marks = roundTripMarkData(mark);
+    REQUIRE(marks.size() == 1);
+    CHECK(marks.front().markType == MarkType::nonArpeggiate);
+    REQUIRE(marks.front().choice.isNonArpeggiate());
+    CHECK(payload == marks.front().choice.nonArpeggiate());
+}
+
+T_END;
+
 #endif

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

@@ -398,3 +398,12 @@ synthetic/voice.3.0.xml
 synthetic/word-font.3.0.xml
 synthetic/work-number.3.0.xml
 synthetic/work-title.3.0.xml
+
+# Unblocked by the arpeggiate/non-arpeggiate attribute payloads
+# (ArpeggiateMarkData / NonArpeggiateMarkData in MarkDataChoice): number,
+# unbroken, id, and the non-arpeggiate top/bottom type now round-trip.
+synthetic/arpeggiate.3.0.xml
+synthetic/arpeggiate.3.1.xml
+synthetic/arpeggiate.4.0.xml
+synthetic/non-arpeggiate.3.0.xml
+synthetic/non-arpeggiate.3.1.xml