feat: an ornament owns the accidental marks written after it

data/api.features.xml

@@ -28,7 +28,7 @@
           <missing>other</missing>
         </enum>
       </enums>
-      <notes>api::MarkType accidentalMark*; all members mapped except AccidentalValue::other (no api::MarkType::accidentalMarkOther member).</notes>
+      <notes>In &lt;notations&gt;: api::MarkType accidentalMark*. In &lt;ornaments&gt; after an ornament: api::OrnamentAccidental (api::Accidental) in the ornament mark's OrnamentMarkData; after tremolo, other-ornament or wavy-line it falls back to an accidentalMark* mark. All members mapped except AccidentalValue::other (no api member). Only value, position and placement are kept.</notes>
     </feature>
     <feature name="accidental-text" support="partial">
       <notes>Accidental as text (key/ornament context) (suspected).</notes>

src/include/mx/api/MarkData.h

@@ -81,7 +81,9 @@ enum class MarkType
     otherOrnament,      ///< MusicXML's 'other-ornament' value
     unknownOrnament,    ///< Error state
 
-    // accidental marks
+    // accidental marks: an accidental shown above or below the note on its own. An accidental that
+    // belongs to an ornament, like the sharp over a trill, is held by that ornament instead
+    // (OrnamentMarkData in MarkDataChoice.h).
     accidentalMarkSharp,
     accidentalMarkNatural,
     accidentalMarkFlat,

src/include/mx/api/MarkDataChoice.h

@@ -7,10 +7,13 @@
 #include "mx/api/ApiCommon.h"
 #include "mx/api/DynamicsData.h"
 #include "mx/api/Id.h"
+#include "mx/api/PitchData.h"
+#include "mx/api/PositionData.h"
 
 #include <optional>
 #include <string>
 #include <variant>
+#include <vector>
 
 namespace mx
 {
@@ -195,6 +198,44 @@ MXAPI_EQUALS_MEMBER(pitch)
 MXAPI_EQUALS_END;
 MXAPI_NOT_EQUALS_AND_VECTORS(HarmonicMarkData);
 
+// An accidental printed with an ornament: the sharp over a trill that raises its upper note, or the
+// accidentals above and below a turn that alter its upper and lower notes.
+struct OrnamentAccidental
+{
+    // The accidental to show. An entry left at none is not written.
+    Accidental accidental = Accidental::none;
+
+    // Where the accidental sits, measured from the ornament rather than from the note. Placement
+    // above puts the accidental over the ornament, below puts it under. The horizontal and vertical
+    // alignment fields are not used.
+    PositionData positionData;
+};
+
+MXAPI_EQUALS_BEGIN(OrnamentAccidental)
+MXAPI_EQUALS_MEMBER(accidental)
+MXAPI_EQUALS_MEMBER(positionData)
+MXAPI_EQUALS_END;
+MXAPI_NOT_EQUALS_AND_VECTORS(OrnamentAccidental);
+
+// Payload for an ornament that has accidentals. A trill whose upper note is sharpened is one
+// MarkType::trillMark mark holding an OrnamentAccidental with Accidental::sharp.
+//
+// These marks can hold accidentals: a trill, a turn of any kind, a shake, a mordent, an inverted
+// mordent, a schleifer and a haydn. A tremolo or an other-ornament cannot, because its choice
+// already carries TremoloMarkData or OtherMarkData; neither can a wavy line, which is not a mark.
+// An accidental written with one of those, or one that stands above or below the note on its own,
+// is a MarkType::accidentalMark* mark of its own. On any other mark this payload is not written.
+struct OrnamentMarkData
+{
+    // The ornament's accidentals, usually one. A turn may have one above it and one below it.
+    std::vector<OrnamentAccidental> accidentals;
+};
+
+MXAPI_EQUALS_BEGIN(OrnamentMarkData)
+MXAPI_EQUALS_MEMBER(accidentals)
+MXAPI_EQUALS_END;
+MXAPI_NOT_EQUALS_AND_VECTORS(OrnamentMarkData);
+
 // A variant class that carries data for MarkType values whose payload does not fit MarkData's
 // common fields.
 //
@@ -222,7 +263,8 @@ class MarkDataChoice
         dynamic,
         compoundDynamics,
         otherNotation,
-        harmonic
+        harmonic,
+        ornament
     };
 
     MarkDataChoice();
@@ -246,6 +288,10 @@ class MarkDataChoice
 
     MarkDataChoice(HarmonicMarkData value);
 
+    // Builds an ornament payload, unless it has no accidentals, in which case the result is none:
+    // an ornament without accidentals needs no payload.
+    MarkDataChoice(OrnamentMarkData value);
+
     Kind kind() const;
     bool isNone() const;
     bool isTremolo() const;
@@ -256,6 +302,7 @@ class MarkDataChoice
     bool isCompoundDynamics() const;
     bool isOtherNotation() const;
     bool isHarmonic() const;
+    bool isOrnament() const;
 
     // Returns a copy of the internally held TremoloMarkData.
     //
@@ -293,11 +340,17 @@ class MarkDataChoice
     // HarmonicMarkData is returned.
     const HarmonicMarkData harmonic() const;
 
+    // Returns a copy of the internally held OrnamentMarkData.
+    //
+    // Check isOrnament() first. If this is not an ornament payload, a default constructed
+    // OrnamentMarkData, with no accidentals, is returned.
+    const OrnamentMarkData ornament() const;
+
     bool operator==(const MarkDataChoice &other) const;
 
   private:
     std::variant<std::monostate, TremoloMarkData, ArpeggiateMarkData, NonArpeggiateMarkData, OtherMarkData,
-                 StandardDynamic, CompoundDynamicsData, OtherNotationMarkData, HarmonicMarkData>
+                 StandardDynamic, CompoundDynamicsData, OtherNotationMarkData, HarmonicMarkData, OrnamentMarkData>
         myValue;
 };
 

src/private/mx/api/MarkDataChoice.cpp

@@ -58,6 +58,20 @@ MarkDataChoice::MarkDataChoice(HarmonicMarkData value) : myValue{std::move(value
 {
 }
 
+// An ornament with no accidentals is stored as none, so a plain trill compares equal however it
+// was built.
+MarkDataChoice::MarkDataChoice(OrnamentMarkData value)
+{
+    if (value.accidentals.empty())
+    {
+        myValue = std::monostate{};
+    }
+    else
+    {
+        myValue = std::move(value);
+    }
+}
+
 MarkDataChoice::Kind MarkDataChoice::kind() const
 {
     if (std::holds_alternative<TremoloMarkData>(myValue))
@@ -92,6 +106,10 @@ MarkDataChoice::Kind MarkDataChoice::kind() const
     {
         return Kind::harmonic;
     }
+    if (std::holds_alternative<OrnamentMarkData>(myValue))
+    {
+        return Kind::ornament;
+    }
     return Kind::none;
 }
 
@@ -212,6 +230,20 @@ const HarmonicMarkData MarkDataChoice::harmonic() const
     return HarmonicMarkData{};
 }
 
+bool MarkDataChoice::isOrnament() const
+{
+    return std::holds_alternative<OrnamentMarkData>(myValue);
+}
+
+const OrnamentMarkData MarkDataChoice::ornament() const
+{
+    if (const auto *value = std::get_if<OrnamentMarkData>(&myValue))
+    {
+        return *value;
+    }
+    return OrnamentMarkData{};
+}
+
 bool MarkDataChoice::operator==(const MarkDataChoice &other) const
 {
     return myValue == other.myValue;

src/private/mx/impl/AccidentalMarkFunctions.cpp

@@ -30,5 +30,14 @@ api::MarkData AccidentalMarkFunctions::parseAccidentalMark() const
 
     return markData;
 }
+
+api::OrnamentAccidental AccidentalMarkFunctions::parseOrnamentAccidental() const
+{
+    Converter converter;
+    api::OrnamentAccidental accidental;
+    accidental.accidental = converter.convert(myAccidentalMark.value());
+    accidental.positionData = impl::getPositionData(myAccidentalMark);
+    return accidental;
+}
 } // namespace impl
 } // namespace mx

src/private/mx/impl/AccidentalMarkFunctions.h

@@ -26,6 +26,10 @@ class AccidentalMarkFunctions
     AccidentalMarkFunctions(AccidentalMarkFunctions &&) = default;
     api::MarkData parseAccidentalMark() const;
 
+    // Reads an accidental-mark that follows an ornament inside <ornaments> as one of that
+    // ornament's accidentals. An accidental value the api has no member for comes back as none.
+    api::OrnamentAccidental parseOrnamentAccidental() const;
+
   private:
     const core::AccidentalMark &myAccidentalMark;
     const impl::Cursor myCursor;

src/private/mx/impl/NotationsWriter.cpp

@@ -67,6 +67,7 @@
 #include "mx/impl/GlissandoFunctions.h"
 #include "mx/impl/IdFunctions.h"
 #include "mx/impl/MarkDataFunctions.h"
+#include "mx/impl/OrnamentsFunctions.h"
 #include "mx/impl/PositionFunctions.h"
 #include "mx/impl/ScoreWriter.h"
 #include "mx/impl/WavyLineFunctions.h"
@@ -383,6 +384,12 @@ core::Notations NotationsWriter::getNotations() const
         {
             this->addOrnament(mark, ornaments);
         }
+        else if (mark.choice.isOrnament())
+        {
+            myScoreWriter.getDiagnostics().report(
+                api::Severity::error, api::DiagnosticCode::droppedData, cursorLocation(myCursor),
+                "ornament accidentals on a mark that cannot hold them are not written");
+        }
         if (isMarkAccidentalMark(mark.markType))
         {
             this->addAccidentalMark(mark, outNotations);
@@ -1170,14 +1177,48 @@ void NotationsWriter::addOrnament(const api::MarkData &mark, core::Ornaments &ou
         return;
     }
 
+    addOrnamentAccidentals(mark, group);
     outOrnaments.addGroup(group);
 }
 
-// An accidental mark is written at the notations level. MusicXML also allows one inside
-// <ornaments>, next to the ornament it decorates, but the api does not record which of the two
-// places a mark came from -- both arrive here as the same MarkType -- and an <ornaments> group is
-// required to carry an ornament, so putting one there would mean inventing an ornament. Marks
-// from either place are written here instead.
+// An ornament's accidentals go in its own group, right after it, which is where MusicXML says an
+// accidental belongs to the ornament before it. Only the ornaments the reader lets own accidentals
+// may write them; anything else would come back as standalone accidental marks.
+void NotationsWriter::addOrnamentAccidentals(const api::MarkData &mark, core::OrnamentsGroup &outGroup) const
+{
+    if (!mark.choice.isOrnament())
+    {
+        return;
+    }
+
+    if (!ornamentsFunctionsCanOwnAccidentals(outGroup.choice().kind()))
+    {
+        myScoreWriter.getDiagnostics().report(api::Severity::error, api::DiagnosticCode::droppedData,
+                                              cursorLocation(myCursor),
+                                              "ornament accidentals on a mark that cannot hold them are not written");
+        return;
+    }
+
+    const auto payload = mark.choice.ornament();
+    for (const auto &accidental : payload.accidentals)
+    {
+        if (accidental.accidental == api::Accidental::none)
+        {
+            myScoreWriter.getDiagnostics().report(api::Severity::error, api::DiagnosticCode::droppedData,
+                                                  cursorLocation(myCursor),
+                                                  "an ornament accidental of none is not written");
+            continue;
+        }
+
+        core::AccidentalMark accidentalMark;
+        accidentalMark.setValue(myConverter.convert(accidental.accidental));
+        setAttributesFromPositionData(accidental.positionData, accidentalMark);
+        outGroup.addAccidentalMark(accidentalMark);
+    }
+}
+
+// A standalone accidental mark is written directly in <notations>. An accidental that belongs to an
+// ornament is not a mark of its own; addOrnamentAccidentals writes it inside <ornaments>.
 void NotationsWriter::addAccidentalMark(const api::MarkData &mark, core::Notations &outNotations) const
 {
     if (!isMarkAccidentalMark(mark.markType))

src/private/mx/impl/NotationsWriter.h

@@ -36,6 +36,7 @@ class NotationsWriter
     core::NotationsChoice makeTechnicalNotationsChoice() const;
     void addArticulation(const api::MarkData &markData, core::Articulations &outArticulations) const;
     void addOrnament(const api::MarkData &markData, core::Ornaments &outOrnaments) const;
+    void addOrnamentAccidentals(const api::MarkData &markData, core::OrnamentsGroup &outGroup) const;
     void addAccidentalMark(const api::MarkData &markData, core::Notations &outNotations) const;
     void addTechnical(const api::MarkData &markData, core::Technical &outTechnical) const;