diff --git a/src/private/mx/impl/DirectionWriter.cpp b/src/private/mx/impl/DirectionWriter.cpp index d4b04b2f..3b86c1a3 100644 --- a/src/private/mx/impl/DirectionWriter.cpp +++ b/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 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. quarter + // = half. 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 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 + // 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)); diff --git a/src/private/mx/impl/MetronomeReader.cpp b/src/private/mx/impl/MetronomeReader.cpp index 6349481a..d63389b7 100644 --- a/src/private/mx/impl/MetronomeReader.cpp +++ b/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 -- ... ... + // -- 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: .. = ... + // 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(leftBeatUnit.beatUnitDot().size()); + + const auto &rightBeatUnit = beatUnitPer.choice().asGroup().beatUnit(); + myOutTempoData.metricModulation.rightDurationName = converter.convert(rightBeatUnit.beatUnit()); + myOutTempoData.metricModulation.rightDots = static_cast(rightBeatUnit.beatUnitDot().size()); } } // namespace impl } // namespace mx diff --git a/src/private/mxtest/api/MetronomeApiTest.cpp b/src/private/mxtest/api/MetronomeApiTest.cpp index 06292da8..5f5c90a3 100644 --- a/src/private/mxtest/api/MetronomeApiTest.cpp +++ b/src/private/mxtest/api/MetronomeApiTest.cpp @@ -9,6 +9,9 @@ #include "mx/api/DocumentManager.h" #include "mxtest/api/RoundTrip.h" +#include +#include + 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 in one +// . `metronomeBody` is the inner markup of the element. +std::string makeMetronomeDoc(const std::string &metronomeBody) +{ + return R"( + + + + x + + + + + + + )" + + metronomeBody + R"( + + + + + +)"; +} +} // 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"( + + quarter + + equals + + eighth + + )"); + + 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 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"( + quarter + fast + )"); + + 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(out.parts.size())); + if (out.parts.empty()) + { + return; + } + const auto &measures = out.parts.back().measures; + CHECK_EQUAL(1, static_cast(measures.size())); + if (measures.empty() || measures.back().staves.empty()) + { + return; + } + const auto &directions = measures.back().staves.back().directions; + CHECK_EQUAL(1, static_cast(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