fix: stop api round-trip crashes on metronome/tempo marks

src/private/mx/impl/DirectionWriter.cpp

@@ -39,6 +39,7 @@
 #include "mx/core/generated/MetronomeChoice.h"
 #include "mx/core/generated/MetronomeChoiceGroup.h"
 #include "mx/core/generated/MetronomeChoiceGroupChoice.h"
+#include "mx/core/generated/MetronomeChoiceGroupChoiceGroup.h"
 #include "mx/core/generated/MusicDataChoice.h"
 #include "mx/core/generated/Numeral.h"
 #include "mx/core/generated/NumeralKey.h"
@@ -71,7 +72,6 @@
 #include "mx/impl/PrintFunctions.h"
 #include "mx/impl/SoundFunctions.h"
 #include "mx/impl/SpannerFunctions.h"
-#include "mx/utility/Throw.h"
 #include "mx/utility/Unused.h"
 
 namespace mx
@@ -331,35 +331,55 @@ std::vector<core::MusicDataChoice> DirectionWriter::getDirectionLikeThings()
         addDirectionType(std::move(dt), direction);
     }
 
-    for (const auto &tempo : myDirectionData.tempos)
-    {
-        if (tempo.tempoType != api::TempoType::beatsPerMinute)
-        {
-            MX_THROW("Only api::TempoType::beatsPerMinute is supported, others are not implemented");
-        }
-
+    const auto makeBeatUnitGroup = [&](api::DurationName durationName, int dots) {
         core::BeatUnitGroup beatUnitGroup{};
-        Converter converter;
-        beatUnitGroup.setBeatUnit(converter.convert(tempo.beatsPerMinute.durationName));
-
-        for (int d = 0; d < tempo.beatsPerMinute.dots; ++d)
+        beatUnitGroup.setBeatUnit(myConverter.convert(durationName));
+        for (int d = 0; d < dots; ++d)
         {
             beatUnitGroup.addBeatUnitDot(core::Empty{});
         }
+        return beatUnitGroup;
+    };
 
-        core::PerMinute pm{};
-        pm.setValue(std::to_string(tempo.beatsPerMinute.beatsPerMinute));
-
-        core::MetronomeChoiceGroupChoice mgChoice = core::MetronomeChoiceGroupChoice::perMinute(pm);
+    for (const auto &tempo : myDirectionData.tempos)
+    {
+        core::Metronome metronome{};
 
-        core::MetronomeChoiceGroup mcg{};
-        mcg.setBeatUnit(beatUnitGroup);
-        mcg.setChoice(mgChoice);
+        if (tempo.tempoType == api::TempoType::beatsPerMinute)
+        {
+            // beat-unit (+dots) followed by a numeric per-minute.
+            core::PerMinute pm{};
+            pm.setValue(std::to_string(tempo.beatsPerMinute.beatsPerMinute));
 
-        core::MetronomeChoice metChoice = core::MetronomeChoice::group(mcg);
+            core::MetronomeChoiceGroup mcg{};
+            mcg.setBeatUnit(makeBeatUnitGroup(tempo.beatsPerMinute.durationName, tempo.beatsPerMinute.dots));
+            mcg.setChoice(core::MetronomeChoiceGroupChoice::perMinute(pm));
+            metronome.setChoice(core::MetronomeChoice::group(mcg));
+        }
+        else if (tempo.tempoType == api::TempoType::metricModulation)
+        {
+            // Metric modulation: two beat-units, e.g. <beat-unit>quarter</beat-unit>
+            // = <beat-unit>half</beat-unit>. The second beat-unit is the 'group'
+            // alternative of the choice.
+            const auto &mm = tempo.metricModulation;
+            core::MetronomeChoiceGroupChoiceGroup rightBeatUnitHolder{};
+            rightBeatUnitHolder.setBeatUnit(makeBeatUnitGroup(mm.rightDurationName, mm.rightDots));
 
-        core::Metronome metronome{};
-        metronome.setChoice(metChoice);
+            core::MetronomeChoiceGroup mcg{};
+            mcg.setBeatUnit(makeBeatUnitGroup(mm.leftDurationName, mm.leftDots));
+            mcg.setChoice(core::MetronomeChoiceGroupChoice::group(rightBeatUnitHolder));
+            metronome.setChoice(core::MetronomeChoice::group(mcg));
+        }
+        else
+        {
+            // tempoText (a non-numeric <per-minute> whose beat-unit was not
+            // preserved by the api) and 'unspecified' (e.g. the metronome-note
+            // form, which the api does not model) have no faithful <metronome>
+            // representation. Skip rather than crash or fabricate wrong structure.
+            // Previously the writer threw here, producing no output (CREATEFAIL).
+            // See issue #218.
+            continue;
+        }
 
         core::DirectionType dt{};
         dt.setChoice(core::DirectionTypeChoice::metronome(metronome));

src/private/mx/impl/MetronomeReader.cpp

@@ -8,10 +8,10 @@
 #include "mx/core/generated/MetronomeChoice.h"
 #include "mx/core/generated/MetronomeChoiceGroup.h"
 #include "mx/core/generated/MetronomeChoiceGroupChoice.h"
+#include "mx/core/generated/MetronomeChoiceGroupChoiceGroup.h"
 #include "mx/core/generated/PerMinute.h"
 #include "mx/impl/Converter.h"
 #include "mx/utility/StringToInt.h"
-#include "mx/utility/Throw.h"
 
 namespace mx
 {
@@ -74,7 +74,11 @@ void MetronomeReader::parseBeatUnitPer() const
 
 void MetronomeReader::parseNoteRelationNote() const
 {
-    MX_THROW("wtf is this");
+    // The metronome-note form -- <metronome-note> ... <metronome-relation> ...
+    // <metronome-note> -- has no representation in api::TempoData yet. Leave the
+    // tempo 'unspecified' rather than crashing the whole api pipeline; the writer
+    // skips unspecified tempos. Previously this threw and produced no output at
+    // all (GETDATAFAIL). See issue #218.
 }
 
 void MetronomeReader::parseBeatsPerMinute() const
@@ -101,6 +105,22 @@ void MetronomeReader::parseBeatsPerMinute() const
 
 void MetronomeReader::parseMetronomeModulation() const
 {
+    // Metric modulation: <beat-unit>..</beat-unit> = <beat-unit>..</beat-unit>.
+    // The left beat-unit lives directly on the group; the right beat-unit is the
+    // 'group' alternative of the metronome choice. Previously this was an empty
+    // stub that left the tempo 'unspecified', which then crashed the writer
+    // (CREATEFAIL). See issue #218.
+    myOutTempoData.tempoType = api::TempoType::metricModulation;
+    const auto &beatUnitPer = myBeatUnitPerOrNoteRelationNoteChoice.asGroup();
+    Converter converter;
+
+    const auto &leftBeatUnit = beatUnitPer.beatUnit();
+    myOutTempoData.metricModulation.leftDurationName = converter.convert(leftBeatUnit.beatUnit());
+    myOutTempoData.metricModulation.leftDots = static_cast<int>(leftBeatUnit.beatUnitDot().size());
+
+    const auto &rightBeatUnit = beatUnitPer.choice().asGroup().beatUnit();
+    myOutTempoData.metricModulation.rightDurationName = converter.convert(rightBeatUnit.beatUnit());
+    myOutTempoData.metricModulation.rightDots = static_cast<int>(rightBeatUnit.beatUnitDot().size());
 }
 } // namespace impl
 } // namespace mx

src/private/mxtest/api/MetronomeApiTest.cpp

@@ -9,6 +9,9 @@
 #include "mx/api/DocumentManager.h"
 #include "mxtest/api/RoundTrip.h"
 
+#include <sstream>
+#include <string>
+
 using namespace std;
 using namespace mx::api;
 using namespace mxtest;
@@ -55,6 +58,147 @@ TEST(roundTripBpm, MetronomeApi)
     CHECK_EQUAL(expectedTickTimePosition, actualDirection.tickTimePosition);
 }
 
+// --- issue #218: metronome/tempo marks must not crash the api pipeline -------
+
+namespace
+{
+// A minimal partwise document carrying exactly one <metronome> in one
+// <direction>. `metronomeBody` is the inner markup of the <metronome> element.
+std::string makeMetronomeDoc(const std::string &metronomeBody)
+{
+    return R"(<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<score-partwise version="3.0">
+  <part-list>
+    <score-part id="P1">
+      <part-name>x</part-name>
+    </score-part>
+  </part-list>
+  <part id="P1">
+    <measure number="1">
+      <direction>
+        <direction-type>
+          <metronome>)" +
+           metronomeBody + R"(</metronome>
+        </direction-type>
+      </direction>
+    </measure>
+  </part>
+</score-partwise>
+)";
+}
+} // namespace
+
+// The metronome-note form has no api::TempoData representation. It used to throw
+// "wtf is this" in the reader, which DocumentManager turned into a getData()
+// failure -- the whole file produced no output (GETDATAFAIL). After the fix the
+// tempo is dropped but reading succeeds.
+TEST(metronomeNoteFormReadDoesNotFail, MetronomeApi)
+{
+    const std::string xml = makeMetronomeDoc(R"(
+            <metronome-note>
+              <metronome-type>quarter</metronome-type>
+            </metronome-note>
+            <metronome-relation>equals</metronome-relation>
+            <metronome-note>
+              <metronome-type>eighth</metronome-type>
+            </metronome-note>
+          )");
+
+    auto &mgr = DocumentManager::getInstance();
+    std::istringstream iss{xml};
+    const auto idResult = mgr.createFromStream(iss);
+    CHECK(idResult.ok());
+    if (!idResult.ok())
+    {
+        return;
+    }
+    const auto dataResult = mgr.getData(idResult.value());
+    mgr.destroyDocument(idResult.value());
+    // Before the fix this was an error (GETDATAFAIL); now reading must succeed.
+    CHECK(dataResult.ok());
+}
+
+// A non-numeric <per-minute> is legal -- per-minute is an xs:string. The reader
+// represents it as tempoText, and the writer used to throw on any non-bpm tempo,
+// failing createFromScore (CREATEFAIL). After the fix the tempo is skipped and
+// createFromScore succeeds.
+TEST(nonNumericPerMinuteWriteDoesNotFail, MetronomeApi)
+{
+    const std::string xml = makeMetronomeDoc(R"(
+            <beat-unit>quarter</beat-unit>
+            <per-minute>fast</per-minute>
+          )");
+
+    auto &mgr = DocumentManager::getInstance();
+    std::istringstream iss{xml};
+    const auto idResult = mgr.createFromStream(iss);
+    CHECK(idResult.ok());
+    if (!idResult.ok())
+    {
+        return;
+    }
+    const auto dataResult = mgr.getData(idResult.value());
+    mgr.destroyDocument(idResult.value());
+    CHECK(dataResult.ok());
+    if (!dataResult.ok())
+    {
+        return;
+    }
+    const auto id2Result = mgr.createFromScore(dataResult.value());
+    // Before the fix createFromScore threw (CREATEFAIL); now it must succeed.
+    CHECK(id2Result.ok());
+    if (id2Result.ok())
+    {
+        mgr.destroyDocument(id2Result.value());
+    }
+}
+
+// Metric modulation (two beat-units) used to be an empty read stub plus a writer
+// throw. It now round-trips through the api.
+TEST(roundTripMetricModulation, MetronomeApi)
+{
+    ScoreData score;
+    score.ticksPerQuarter = 100;
+    score.parts.emplace_back();
+    score.parts.back().measures.emplace_back();
+    score.parts.back().measures.back().staves.emplace_back();
+    score.parts.back().measures.back().staves.back().directions.emplace_back();
+    auto &direction = score.parts.back().measures.back().staves.back().directions.back();
+    direction.tempos.emplace_back();
+    auto &tempo = direction.tempos.back();
+    tempo.tempoType = TempoType::metricModulation;
+    tempo.metricModulation.leftDurationName = DurationName::quarter;
+    tempo.metricModulation.leftDots = 1;
+    tempo.metricModulation.rightDurationName = DurationName::half;
+    tempo.metricModulation.rightDots = 0;
+
+    const auto out = roundTrip(score);
+
+    CHECK_EQUAL(1, static_cast<int>(out.parts.size()));
+    if (out.parts.empty())
+    {
+        return;
+    }
+    const auto &measures = out.parts.back().measures;
+    CHECK_EQUAL(1, static_cast<int>(measures.size()));
+    if (measures.empty() || measures.back().staves.empty())
+    {
+        return;
+    }
+    const auto &directions = measures.back().staves.back().directions;
+    CHECK_EQUAL(1, static_cast<int>(directions.size()));
+    if (directions.empty() || directions.back().tempos.empty())
+    {
+        return;
+    }
+    const auto &outTempo = directions.back().tempos.back();
+    CHECK(TempoType::metricModulation == outTempo.tempoType);
+    CHECK(DurationName::quarter == outTempo.metricModulation.leftDurationName);
+    CHECK_EQUAL(1, outTempo.metricModulation.leftDots);
+    CHECK(DurationName::half == outTempo.metricModulation.rightDurationName);
+    CHECK_EQUAL(0, outTempo.metricModulation.rightDots);
+}
+
 T_END
 
 #endif