From f52c88ec843e79c28eeecb73e65d8560cd7aa95c Mon Sep 17 00:00:00 2001 From: Robert Patterson Date: Wed, 5 Aug 2026 19:46:27 -0500 Subject: [PATCH] feat: ending numbers and display text in mx::api ## Summary MusicXML `` carries a list-valued `number` attribute -- the passes the ending is played on -- and an element text body holding the printed label, which the spec uses when that label differs from the numbers. An ending played on passes 1, 2, and 3 is commonly labeled `1-3`. `mx::core::Ending` models both, as an `EndingNumber` wrapping a `std::vector` and a string value, but `mx::api` dropped all of it: `BarlineData` carried `EndingType endingType` and a single `int endingNumber`, so `MeasureReader::parseBarline` kept `values().front()` and discarded the rest, and neither direction ever touched `Ending::value()`. `BarlineData::endingType` and `BarlineData::endingNumber` are replaced by one `std::optional ending`: - `EndingData::numbers` is the full list of passes. Empty means a blank ending, MusicXML's way of saying a bracket is present but which passes it covers is unknown -- a state the old `int` could not express, since 0 meant unspecified. - `EndingData::text` is the printed label. Left empty, the writer emits no text node and the ending displays its numbers, so authoring without a label produces the same XML as before. `EndingType::none` is removed. Presence is now the optional, so the enum's three values match `Converter::endingMap` one for one and an ending can no longer exist with a type that says there is no ending. **Breaking:** both fields are public `ScoreData`. The writer always sets `number`, which the schema requires; an empty list serializes as `number=""`. `Converter` gains the reverse `core::StartStopDiscontinue` to `api::EndingType` conversion, which replaces a hand-written switch in the reader with the existing bridge table. `lysuite/ly45f_Repeats_InvalidEndings.xml` is now pinned (363 -> 364). It failed the strict compare on exactly this gap, `expected 'number=1, 2, 3', actual 'number=1'`, and is the only file whose status changed across the 838-file corpus. `data/synthetic/ending.3.0.xml` and `ending.4.0.xml` still fail on the `` attributes mx::api does not model -- `print-object`, `end-length`, `text-x`, `text-y`, font, color, and `system` -- so neither is pinned. No new corpus files, so no pinned-count or audit regeneration. ## Testing - [x] Seven new cases in RepeatApiTest: multi-number round trip with a label, exact serialization of `number="1, 2, 3"` and the text node, the read path, an ending with no label, a blank ending, absence, and a read of the `data/synthetic/ending.3.0.xml` shape whose text body differs from its number - [x] Full api suite passes (5921 assertions in 528 test cases) - [x] `make api-roundtrip` passes (364 of 364 pinned) - [x] `make core-roundtrip-test` passes (839 test cases) - [x] `make core-unit` passes (212 assertions in 41 test cases) - [x] `make fmt-check` clean on touched files ## References - Closes #387 --- data/api.features.xml | 3 +- src/include/mx/api/BarlineData.h | 53 +++++- src/private/mx/impl/Converter.cpp | 5 + src/private/mx/impl/Converter.h | 1 + src/private/mx/impl/MeasureReader.cpp | 31 +--- src/private/mx/impl/MeasureWriter.cpp | 17 +- src/private/mxtest/api/RepeatApiTest.cpp | 174 ++++++++++++++++++ src/private/mxtest/api/roundtrip-baseline.txt | 10 +- 8 files changed, 251 insertions(+), 43 deletions(-) diff --git a/data/api.features.xml b/data/api.features.xml index fe802bd73..09105a946 100644 --- a/data/api.features.xml +++ b/data/api.features.xml @@ -297,7 +297,8 @@ EncodingData.encodingDescription; EncodingFunctions.cpp. - BarlineData ending (EndingType via endingMap). + BarlineData.ending -> EndingData: type (EndingType via endingMap), the full number + list, and the display text. InstrumentData.soloOrEnsemble=ensemble. diff --git a/src/include/mx/api/BarlineData.h b/src/include/mx/api/BarlineData.h index 314028346..967a5112b 100644 --- a/src/include/mx/api/BarlineData.h +++ b/src/include/mx/api/BarlineData.h @@ -7,6 +7,10 @@ #include "mx/api/ApiCommon.h" #include "mx/api/PositionData.h" +#include +#include +#include + namespace mx { namespace api @@ -29,14 +33,45 @@ enum class BarlineType short_ }; +// Where a barline sits in an ending (volta) bracket. start opens the bracket on the left barline +// of the ending's first measure; stop closes it with a downward jog, as a first ending usually +// does; discontinue closes it without a jog, as a final ending usually does. enum class EndingType { - none, start, stop, discontinue }; +// An ending (volta) bracket: the numbered bracket drawn over the measures played on a particular +// pass through a repeat. +class EndingData +{ + public: + EndingType type; + + // The passes on which this ending is played, in the order they are written: an ending marked + // "1, 2, 3" is played on passes 1, 2, and 3. Numbering starts at 1; a value below 1 is written + // as 1. Leave this empty for a blank ending -- the way MusicXML says that a bracket is here + // but which passes it covers is unknown. + std::vector numbers; + + // The label drawn under the bracket. Leave it empty to have the numbers themselves displayed; + // set it only when the label differs from them, as when passes 1, 2, and 3 are labeled "1-3". + std::string text; + + EndingData() : type{EndingType::start}, numbers{}, text{} + { + } +}; + +MXAPI_EQUALS_BEGIN(EndingData) +MXAPI_EQUALS_MEMBER(type) +MXAPI_EQUALS_MEMBER(numbers) +MXAPI_EQUALS_MEMBER(text) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(EndingData); + // Whether a repeat mark faces forward (the start of a repeated section, drawn |:) or backward // (the end, drawn :|). enum class RepeatDirection @@ -63,11 +98,11 @@ class BarlineData public: int tickTimePosition; BarlineType barlineType; - EndingType endingType; - int endingNumber; + // The ending (volta) bracket this barline starts or ends, if any. + std::optional ending; bool repeat; // Number of times a backward repeat is played (the repeat's `times` attribute). 0 = not - // specified, mirroring endingNumber above. + // specified. int repeatTimes; // Whether the repeat faces forward (start of a repeated section) or backward (end). Leave // unspecified to let mx infer it from the barline's position -- a repeat on a left or @@ -82,10 +117,9 @@ class BarlineData HorizontalAlignment location; BarlineData() - : tickTimePosition{0}, barlineType{BarlineType::normal}, endingType{EndingType::none}, endingNumber{0}, - repeat{false}, repeatTimes{0}, repeatDirection{RepeatDirection::unspecified}, - repeatAfterJump{Bool::unspecified}, repeatWinged{RepeatWinged::unspecified}, - location{HorizontalAlignment::unspecified} + : tickTimePosition{0}, barlineType{BarlineType::normal}, ending{}, repeat{false}, repeatTimes{0}, + repeatDirection{RepeatDirection::unspecified}, repeatAfterJump{Bool::unspecified}, + repeatWinged{RepeatWinged::unspecified}, location{HorizontalAlignment::unspecified} { } }; @@ -93,8 +127,7 @@ class BarlineData MXAPI_EQUALS_BEGIN(BarlineData) MXAPI_EQUALS_MEMBER(tickTimePosition) MXAPI_EQUALS_MEMBER(barlineType) -MXAPI_EQUALS_MEMBER(endingType) -MXAPI_EQUALS_MEMBER(endingNumber) +MXAPI_EQUALS_MEMBER(ending) MXAPI_EQUALS_MEMBER(repeat) MXAPI_EQUALS_MEMBER(repeatTimes) MXAPI_EQUALS_MEMBER(repeatDirection) diff --git a/src/private/mx/impl/Converter.cpp b/src/private/mx/impl/Converter.cpp index 59f5346df..8dc0b8709 100644 --- a/src/private/mx/impl/Converter.cpp +++ b/src/private/mx/impl/Converter.cpp @@ -1893,6 +1893,11 @@ core::StartStopDiscontinue Converter::convert(api::EndingType value) const return findCoreItem(endingMap, core::StartStopDiscontinue::start(), value); } +api::EndingType Converter::convert(core::StartStopDiscontinue value) const +{ + return findApiItem(endingMap, api::EndingType::start, value); +} + core::BackwardForward Converter::convert(api::RepeatDirection value) const { return findCoreItem(repeatDirectionMap, core::BackwardForward::backward(), value); diff --git a/src/private/mx/impl/Converter.h b/src/private/mx/impl/Converter.h index a558edabb..a83359724 100644 --- a/src/private/mx/impl/Converter.h +++ b/src/private/mx/impl/Converter.h @@ -169,6 +169,7 @@ class Converter api::BarlineType convert(core::BarStyle value) const; core::StartStopDiscontinue convert(api::EndingType value) const; + api::EndingType convert(core::StartStopDiscontinue value) const; core::BackwardForward convert(api::RepeatDirection value) const; api::RepeatDirection convert(core::BackwardForward value) const; diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index 03b1ab293..e4babb5a8 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -914,8 +914,6 @@ void MeasureReader::parseBarline(const core::Barline &inMxBarline) const auto barline = api::BarlineData{}; auto loc = api::HorizontalAlignment::unspecified; auto style = api::BarlineType::unspecified; - auto endingType = api::EndingType::none; - auto endingNumber = 0; auto repeat = false; auto repeatTimes = 0; auto repeatDirection = api::RepeatDirection::unspecified; @@ -944,28 +942,15 @@ void MeasureReader::parseBarline(const core::Barline &inMxBarline) const if (inMxBarline.ending().has_value()) { - const auto &ending = *inMxBarline.ending(); + const auto &mxEnding = *inMxBarline.ending(); + auto endingData = api::EndingData{}; + endingData.type = myConverter.convert(mxEnding.type()); - switch (ending.type().tag()) - { - case core::StartStopDiscontinue::Tag::start: - endingType = api::EndingType::start; - break; - - case core::StartStopDiscontinue::Tag::stop: - endingType = api::EndingType::stop; - break; + const auto numValues = mxEnding.number().values(); + endingData.numbers.assign(numValues.begin(), numValues.end()); + endingData.text = mxEnding.value(); - case core::StartStopDiscontinue::Tag::discontinue: - endingType = api::EndingType::discontinue; - break; - }; - - const auto &numValues = ending.number().values(); - if (!numValues.empty()) - { - endingNumber = numValues.front(); - } + barline.ending = std::move(endingData); } if (inMxBarline.repeat().has_value()) @@ -988,8 +973,6 @@ void MeasureReader::parseBarline(const core::Barline &inMxBarline) const barline.barlineType = style; barline.location = loc; - barline.endingType = endingType; - barline.endingNumber = endingNumber; barline.repeat = repeat; barline.repeatTimes = repeatTimes; barline.repeatDirection = repeatDirection; diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index 0f651348f..4ff2bf2ad 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -847,16 +847,21 @@ void MeasureWriter::writeBarlines(int tickTimePosition) barlineElement.setBarStyle(bsc); } - if (myBarlinesIter->endingType != api::EndingType::none) + if (myBarlinesIter->ending.has_value()) { + const auto &endingData = *myBarlinesIter->ending; core::Ending ending{}; - ending.setType(myConverter.convert(myBarlinesIter->endingType)); + ending.setType(myConverter.convert(endingData.type)); - if (myBarlinesIter->endingNumber > 0) + // number is a required attribute; an empty list serializes as number="", which is + // MusicXML's blank ending. + ending.setNumber(core::EndingNumber{endingData.numbers}); + + // The text is written only when the author supplied one. Left empty, the ending + // displays its numbers. + if (!endingData.text.empty()) { - core::EndingNumber en{}; - en.addValue(myBarlinesIter->endingNumber); - ending.setNumber(en); + ending.setValue(endingData.text); } barlineElement.setEnding(ending); diff --git a/src/private/mxtest/api/RepeatApiTest.cpp b/src/private/mxtest/api/RepeatApiTest.cpp index b39a1612f..10ad1d7c4 100644 --- a/src/private/mxtest/api/RepeatApiTest.cpp +++ b/src/private/mxtest/api/RepeatApiTest.cpp @@ -144,4 +144,178 @@ TEST(repeatOmittedAttributesAreUnspecified, Repeat) T_END; +// Wrap an element in the smallest score that can carry one. +static std::string endingXmlForRepeat(const std::string &inEndingElement) +{ + return R"( + + + + x + + + + + + )" + + inEndingElement + + R"( + + + +)"; +} + +// An ending played on several passes keeps every pass number, and its display label survives +// alongside them. This is the "1, 2, 3" ending labeled "1-3" case. +TEST(endingMultipleNumbersAndTextRoundTrip, Repeat) +{ + auto score = makeScoreWithBarlineForRepeat(); + auto &barline = score.parts.back().measures.back().barlines.back(); + barline.location = HorizontalAlignment::left; + EndingData ending; + ending.type = EndingType::start; + ending.numbers = {1, 2, 3}; + ending.text = "1-3"; + barline.ending = ending; + + const auto out = roundTrip(score); + + const auto &obarlines = out.parts.back().measures.back().barlines; + REQUIRE(obarlines.size() == 1); + const auto &ob = obarlines.front(); + REQUIRE(ob.ending.has_value()); + CHECK(EndingType::start == ob.ending->type); + REQUIRE(ob.ending->numbers.size() == 3); + CHECK_EQUAL(1, ob.ending->numbers.at(0)); + CHECK_EQUAL(2, ob.ending->numbers.at(1)); + CHECK_EQUAL(3, ob.ending->numbers.at(2)); + CHECK_EQUAL(std::string{"1-3"}, ob.ending->text); +} + +T_END; + +// The serialized MusicXML must spell the number list the way the spec does and carry the label as +// the element's text. +TEST(endingSerializesNumberListAndText, Repeat) +{ + auto score = makeScoreWithBarlineForRepeat(); + auto &barline = score.parts.back().measures.back().barlines.back(); + barline.location = HorizontalAlignment::left; + EndingData ending; + ending.type = EndingType::start; + ending.numbers = {1, 2, 3}; + ending.text = "1-3"; + barline.ending = ending; + + const auto xml = toXml(score); + + CHECK(xml.find(R"(number="1, 2, 3")") != std::string::npos); + CHECK(xml.find(R"(type="start")") != std::string::npos); + CHECK(xml.find(">1-3") != std::string::npos); +} + +T_END; + +// The reader must surface the whole number list, not just the first pass, along with the label. +TEST(endingMultipleNumbersAndTextReadFromXml, Repeat) +{ + const auto score = fromXml(endingXmlForRepeat(R"(1-3)")); + + REQUIRE(score.parts.size() == 1); + const auto &barlines = score.parts.back().measures.back().barlines; + REQUIRE(barlines.size() == 1); + const auto &ob = barlines.front(); + REQUIRE(ob.ending.has_value()); + CHECK(EndingType::start == ob.ending->type); + REQUIRE(ob.ending->numbers.size() == 3); + CHECK_EQUAL(1, ob.ending->numbers.at(0)); + CHECK_EQUAL(2, ob.ending->numbers.at(1)); + CHECK_EQUAL(3, ob.ending->numbers.at(2)); + CHECK_EQUAL(std::string{"1-3"}, ob.ending->text); +} + +T_END; + +// An ending with no label reads back with empty text, and writing one emits no text node -- the +// numbers are what gets displayed. +TEST(endingWithoutTextOmitsTextNode, Repeat) +{ + const auto score = fromXml(endingXmlForRepeat(R"()")); + const auto &ob = score.parts.back().measures.back().barlines.front(); + REQUIRE(ob.ending.has_value()); + CHECK(EndingType::stop == ob.ending->type); + REQUIRE(ob.ending->numbers.size() == 1); + CHECK_EQUAL(1, ob.ending->numbers.at(0)); + CHECK(ob.ending->text.empty()); + + const auto xml = toXml(score); + CHECK(xml.find("") == std::string::npos); + CHECK(xml.find(R"(number="1")") != std::string::npos); +} + +T_END; + +// A blank ending -- a bracket whose passes are unknown -- is an ending with no numbers, and writes +// as number="". +TEST(endingBlankNumberRoundTrip, Repeat) +{ + auto score = makeScoreWithBarlineForRepeat(); + auto &barline = score.parts.back().measures.back().barlines.back(); + barline.location = HorizontalAlignment::left; + EndingData ending; + ending.type = EndingType::start; + barline.ending = ending; + + const auto xml = toXml(score); + CHECK(xml.find(R"(number="")") != std::string::npos); + + const auto out = roundTrip(score); + const auto &ob = out.parts.back().measures.back().barlines.front(); + REQUIRE(ob.ending.has_value()); + CHECK(EndingType::start == ob.ending->type); + CHECK(ob.ending->numbers.empty()); + CHECK(ob.ending->text.empty()); +} + +T_END; + +// A barline that carries no ending must report none, and must not emit an element. +TEST(barlineWithoutEndingIsAbsent, Repeat) +{ + auto score = makeScoreWithBarlineForRepeat(); + auto &barline = score.parts.back().measures.back().barlines.back(); + barline.barlineType = BarlineType::lightHeavy; + barline.location = HorizontalAlignment::right; + + const auto xml = toXml(score); + CHECK(xml.find(" api read path against the shape of data/synthetic/ending.3.0.xml, whose ending +// carries a text body ("x") that differs from its number attribute ("1"). The attributes mx::api +// does not model are ignored; the number and the label must both arrive. +TEST(endingTextDiffersFromNumberReadFromXml, Repeat) +{ + const auto score = fromXml( + endingXmlForRepeat(R"(x)")); + + const auto &ob = score.parts.back().measures.back().barlines.front(); + REQUIRE(ob.ending.has_value()); + CHECK(EndingType::start == ob.ending->type); + REQUIRE(ob.ending->numbers.size() == 1); + CHECK_EQUAL(1, ob.ending->numbers.at(0)); + CHECK_EQUAL(std::string{"x"}, ob.ending->text); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 7f811545b..72a311cac 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -188,8 +188,8 @@ lysuite/ly46c_Midmeasure_Clef.xml # Unblocked by #271 (repeat @times round-trip). The reader recorded only a bool # for a repeat and dropped the times count; the writer never emitted it. Added -# BarlineData::repeatTimes (0 = unspecified, mirroring endingNumber) so a backward -# repeat's times attribute survives the round-trip. +# BarlineData::repeatTimes (0 = unspecified) so a backward repeat's times +# attribute survives the round-trip. lysuite/ly45a_SimpleRepeat.xml lysuite/ly45c_RepeatMultipleTimes.xml @@ -633,3 +633,9 @@ synthetic/tied.cue.4.0.xml # children, which both directions previously dropped. The writer also now emits the # harmonic's print-object attribute. synthetic/harmonic.3.0.xml + +# Unblocked by #387 (ending numbers and display text). BarlineData carried a single +# int endingNumber, so the reader kept only the first value of the ending's number +# attribute: this file's number="1, 2, 3" came back as number="1". BarlineData::ending +# is now an optional EndingData holding the whole number list plus the display text. +lysuite/ly45f_Repeats_InvalidEndings.xml