fix: write back tuplet position data, midi-unpitched and accidental marks

src/include/mx/api/MarkData.h

@@ -222,6 +222,7 @@ bool isMarkFermata(MarkType);
 bool isMarkArpeggiate(MarkType);
 bool isMarkNonArpeggiate(MarkType);
 bool isMarkOtherNotation(MarkType);
+bool isMarkAccidentalMark(MarkType);
 
 bool isMarkCustom(MarkType);
 std::string getCustomMarkName(MarkType);

src/private/mx/api/MarkData.cpp

@@ -75,6 +75,35 @@ bool isMarkOtherNotation(MarkType markType)
     return markType == MarkType::otherNotation;
 }
 
+// Every accidental-mark value. accidentalUnknown is not one of them: it is the error state, and
+// the converter would turn it into a natural, inventing a glyph the caller did not ask for.
+bool isMarkAccidentalMark(MarkType markType)
+{
+    return (markType == MarkType::accidentalMarkSharp) || (markType == MarkType::accidentalMarkNatural) ||
+           (markType == MarkType::accidentalMarkFlat) || (markType == MarkType::accidentalMarkDoubleSharp) ||
+           (markType == MarkType::accidentalMarkSharpSharp) || (markType == MarkType::accidentalMarkFlatFlat) ||
+           (markType == MarkType::accidentalMarkNaturalSharp) || (markType == MarkType::accidentalMarkNaturalFlat) ||
+           (markType == MarkType::accidentalMarkQuarterFlat) || (markType == MarkType::accidentalMarkQuarterSharp) ||
+           (markType == MarkType::accidentalMarkThreeQuartersFlat) ||
+           (markType == MarkType::accidentalMarkThreeQuartersSharp) ||
+           (markType == MarkType::accidentalMarkSharpDown) || (markType == MarkType::accidentalMarkSharpUp) ||
+           (markType == MarkType::accidentalMarkNaturalDown) || (markType == MarkType::accidentalMarkNaturalUp) ||
+           (markType == MarkType::accidentalMarkFlatDown) || (markType == MarkType::accidentalMarkFlatUp) ||
+           (markType == MarkType::accidentalMarkDoubleSharpDown) ||
+           (markType == MarkType::accidentalMarkDoubleSharpUp) || (markType == MarkType::accidentalMarkFlatFlatDown) ||
+           (markType == MarkType::accidentalMarkFlatFlatUp) || (markType == MarkType::accidentalMarkArrowDown) ||
+           (markType == MarkType::accidentalMarkArrowUp) || (markType == MarkType::accidentalMarkTripleSharp) ||
+           (markType == MarkType::accidentalMarkTripleFlat) ||
+           (markType == MarkType::accidentalMarkSlashQuarterSharp) ||
+           (markType == MarkType::accidentalMarkSlashSharp) || (markType == MarkType::accidentalMarkSlashFlat) ||
+           (markType == MarkType::accidentalMarkDoubleSlashFlat) || (markType == MarkType::accidentalMarkSharp1) ||
+           (markType == MarkType::accidentalMarkSharp2) || (markType == MarkType::accidentalMarkSharp3) ||
+           (markType == MarkType::accidentalMarkSharp5) || (markType == MarkType::accidentalMarkFlat1) ||
+           (markType == MarkType::accidentalMarkFlat2) || (markType == MarkType::accidentalMarkFlat3) ||
+           (markType == MarkType::accidentalMarkFlat4) || (markType == MarkType::accidentalMarkSori) ||
+           (markType == MarkType::accidentalMarkKoron);
+}
+
 bool isMarkPedal(MarkType markType)
 {
     return (markType == MarkType::pedal) || (markType == MarkType::damp);

src/private/mx/impl/NotationsWriter.cpp

@@ -4,6 +4,7 @@
 
 #include "mx/impl/NotationsWriter.h"
 #include "mx/core/Token.h"
+#include "mx/core/generated/AccidentalMark.h"
 #include "mx/core/generated/Arpeggiate.h"
 #include "mx/core/generated/ArrowChoice.h"
 #include "mx/core/generated/ArrowChoiceGroup.h"
@@ -103,6 +104,7 @@ core::NotationsChoice notationsWriterMakeTupletStop(const api::TupletStop &inTup
 {
     core::Tuplet tuplet;
     tuplet.setType(core::StartStop::stop());
+    setAttributesFromPositionData(inTupletStop.positionData, tuplet);
     setId(inTupletStop.id, tuplet, diagnostics, location);
 
     if (inTupletStop.numberLevel > 0)
@@ -236,6 +238,7 @@ core::Notations NotationsWriter::getNotations() const
     {
         core::Tuplet tuplet;
         tuplet.setType(core::StartStop::start());
+        setAttributesFromPositionData(tupletStart.positionData, tuplet);
         setId(tupletStart.id, tuplet, myScoreWriter.getDiagnostics(), cursorLocation(myCursor));
 
         core::TupletPortion actual;
@@ -336,6 +339,10 @@ core::Notations NotationsWriter::getNotations() const
         {
             this->addOrnament(mark, ornaments);
         }
+        if (isMarkAccidentalMark(mark.markType))
+        {
+            this->addAccidentalMark(mark, outNotations);
+        }
         if (isMarkTechnical(mark.markType))
         {
             this->addTechnical(mark, technicals);
@@ -982,6 +989,24 @@ void NotationsWriter::addOrnament(const api::MarkData &mark, core::Ornaments &ou
     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.
+void NotationsWriter::addAccidentalMark(const api::MarkData &mark, core::Notations &outNotations) const
+{
+    if (!isMarkAccidentalMark(mark.markType))
+    {
+        return;
+    }
+
+    core::AccidentalMark accidentalMark;
+    accidentalMark.setValue(myConverter.convertAccidentalMark(mark.markType));
+    setAttributesFromPositionData(mark.positionData, accidentalMark);
+    outNotations.addChoice(core::NotationsChoice::accidentalMark(accidentalMark));
+}
+
 void NotationsWriter::addTechnical(const api::MarkData &mark, core::Technical &outTechnical) const
 {
     if (!isMarkTechnical(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 addAccidentalMark(const api::MarkData &markData, core::Notations &outNotations) const;
     void addTechnical(const api::MarkData &markData, core::Technical &outTechnical) const;
 
     // Kept as their own (non-inlined) methods rather than loops inlined in getNotations(), the

src/private/mx/impl/PartWriter.cpp

@@ -270,6 +270,15 @@ core::ScorePart PartWriter::getScorePart() const
         midiInstrument.setMIDIProgram(program);
     }
 
+    if (myPartData.instrumentData.midiData.unpitched != api::VALUE_UNSPECIFIED)
+    {
+        addMidiElement = true;
+        const core::MIDI128 unpitched{myPartData.instrumentData.midiData.unpitched};
+        reportAdjusted(diagnostics(), partLocation(myPartIndex), "midi-unpitched",
+                       myPartData.instrumentData.midiData.unpitched, unpitched.value());
+        midiInstrument.setMIDIUnpitched(unpitched);
+    }
+
     if (myPartData.instrumentData.midiData.isElevationSpecified)
     {
         addMidiElement = true;

src/private/mxtest/api/MidiNameRoundTripTest.cpp

@@ -13,9 +13,7 @@
 using namespace std;
 using namespace mx::api;
 
-namespace
-{
-ScoreData makeScoreWithMidiName(const std::string &midiName)
+static ScoreData midiRoundTripMakeScore(const std::string &midiName)
 {
     VoiceData voiceData;
     NoteData n;
@@ -45,15 +43,15 @@ ScoreData makeScoreWithMidiName(const std::string &midiName)
 }
 
 // The same score, but with the instrument id left to the library.
-ScoreData makeScoreWithMidiNameAndNoInstrumentId(const std::string &midiName)
+static ScoreData midiRoundTripMakeScoreWithNoId(const std::string &midiName)
 {
-    auto scoreData = makeScoreWithMidiName(midiName);
+    auto scoreData = midiRoundTripMakeScore(midiName);
     scoreData.parts.at(0).instrumentData.uniqueId.clear();
     return scoreData;
 }
 
 // The value of the first id attribute after the given tag name.
-std::string idAfterTag(const std::string &xml, const std::string &tag)
+static std::string midiRoundTripIdAfterTag(const std::string &xml, const std::string &tag)
 {
     const auto tagPosition = xml.find(tag);
     if (tagPosition == std::string::npos)
@@ -73,16 +71,15 @@ std::string idAfterTag(const std::string &xml, const std::string &tag)
     }
     return xml.substr(valueStart, valueEnd - valueStart);
 }
-} // namespace
 
 TEST(midiInstrumentId, isTheSameOnEveryWrite)
 {
-    const auto scoreData = makeScoreWithMidiNameAndNoInstrumentId("Flute Player One");
+    const auto scoreData = midiRoundTripMakeScoreWithNoId("Flute Player One");
     const auto first = mxtest::toXml(scoreData);
 
     // Writing an unrelated score in between used to shift the id the library gives the
     // instrument, because the counter behind it outlived the call.
-    static_cast<void>(mxtest::toXml(makeScoreWithMidiName("Other Player")));
+    static_cast<void>(mxtest::toXml(midiRoundTripMakeScore("Other Player")));
 
     const auto second = mxtest::toXml(scoreData);
     CHECK(!first.empty());
@@ -91,17 +88,17 @@ TEST(midiInstrumentId, isTheSameOnEveryWrite)
 
 TEST(midiInstrumentId, isReferencedByTheMidiInstrument)
 {
-    const auto xml = mxtest::toXml(makeScoreWithMidiNameAndNoInstrumentId("Flute Player One"));
-    const auto scoreInstrumentId = idAfterTag(xml, "<score-instrument");
-    const auto midiInstrumentId = idAfterTag(xml, "<midi-instrument");
+    const auto xml = mxtest::toXml(midiRoundTripMakeScoreWithNoId("Flute Player One"));
+    const auto scoreInstrumentId = midiRoundTripIdAfterTag(xml, "<score-instrument");
+    const auto midiInstrumentId = midiRoundTripIdAfterTag(xml, "<midi-instrument");
     CHECK(!scoreInstrumentId.empty());
     CHECK_EQUAL(scoreInstrumentId, midiInstrumentId);
 }
 
 TEST(midiInstrumentId, survivesWriteAndRead)
 {
-    const auto scoreData = makeScoreWithMidiNameAndNoInstrumentId("Flute Player One");
-    const auto writtenId = idAfterTag(mxtest::toXml(scoreData), "<score-instrument");
+    const auto scoreData = midiRoundTripMakeScoreWithNoId("Flute Player One");
+    const auto writtenId = midiRoundTripIdAfterTag(mxtest::toXml(scoreData), "<score-instrument");
 
     const auto out = mxtest::roundTrip(scoreData);
     REQUIRE(out.parts.size() == 1);
@@ -111,7 +108,7 @@ TEST(midiInstrumentId, survivesWriteAndRead)
 TEST(midiNameRoundTrip, survivesWriteAndRead)
 {
     const std::string expected = "Flute Player One";
-    const auto in = makeScoreWithMidiName(expected);
+    const auto in = midiRoundTripMakeScore(expected);
     const auto out = mxtest::roundTrip(in);
     REQUIRE(out.parts.size() == 1);
     CHECK_EQUAL(expected, out.parts.at(0).instrumentData.midiData.name);
@@ -122,7 +119,7 @@ TEST(midiDeviceRoundTrip, portSurvivesWriteAndRead)
     // A midi-device carrying only attributes (empty text) must still round-trip, e.g.
     // <midi-device port="1"></midi-device>. writeDeviceId defaults to unspecified, so no id is
     // emitted here; only the port is exercised.
-    auto in = makeScoreWithMidiName("Flute Player One");
+    auto in = midiRoundTripMakeScore("Flute Player One");
     in.parts.at(0).instrumentData.midiData.devicePort = 1;
 
     const auto out = mxtest::roundTrip(in);
@@ -137,7 +134,7 @@ TEST(midiDeviceRoundTrip, writeDeviceIdSurvivesWriteAndRead)
     // When a source spells out the device-to-instrument link, mx re-emits it as
     // <midi-device id="P1-I1" ...>, taking the id from the part's instrument (uniqueId "P1-I1"
     // here), so the flag survives the round-trip.
-    auto in = makeScoreWithMidiName("Flute Player One");
+    auto in = midiRoundTripMakeScore("Flute Player One");
     in.parts.at(0).instrumentData.midiData.devicePort = 1;
     in.parts.at(0).instrumentData.midiData.writeDeviceId = Bool::yes;
 
@@ -146,4 +143,28 @@ TEST(midiDeviceRoundTrip, writeDeviceIdSurvivesWriteAndRead)
     CHECK(out.parts.at(0).instrumentData.midiData.writeDeviceId == Bool::yes);
 }
 
+// issue #443: midi-unpitched names the MIDI note a percussion instrument sounds. The reader read
+// it and the writer never wrote it.
+TEST(midiUnpitchedRoundTrip, survivesWriteAndRead)
+{
+    auto in = midiRoundTripMakeScore("Flute Player One");
+    in.parts.at(0).instrumentData.midiData.unpitched = 43;
+
+    const auto written = mxtest::toXml(in);
+    CHECK(written.find("<midi-unpitched>43</midi-unpitched>") != std::string::npos);
+
+    const auto out = mxtest::roundTrip(in);
+    REQUIRE(out.parts.size() == 1);
+    CHECK_EQUAL(43, out.parts.at(0).instrumentData.midiData.unpitched);
+}
+
+// VALUE_UNSPECIFIED means the part has no unpitched note, and an ordinary pitched instrument
+// always does: nothing is written for it.
+TEST(midiUnpitchedAbsentByDefault, writesNoElement)
+{
+    const auto in = midiRoundTripMakeScore("Flute Player One");
+    const auto written = mxtest::toXml(in);
+    CHECK(written.find("midi-unpitched") == std::string::npos);
+}
+
 #endif