From cdac93545266caf58cda85bef0804c9ef29e9d04 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 15:29:49 +0000 Subject: [PATCH 1/3] fix: do not invent for scores with no time resolution A source that declares no anywhere reads as ticksPerQuarter = 0 (LCM of an empty set). The writer then emitted divisions for the first measure of every part, and core::PositiveDivisions clamped the 0 to a bogus 1e-06. The writer now omits when the score has no time resolution; an otherwise-empty is no longer flushed. Pins the 41 corpus files this flips to PASS (212 total). --- src/include/mx/api/ScoreData.h | 6 +++ src/private/mx/impl/MeasureWriter.cpp | 8 ++- src/private/mxtest/api/roundtrip-baseline.txt | 48 ++++++++++++++++++ src/private/mxtest/impl/MeasureWriterTest.cpp | 49 +++++++++++++++++++ 4 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/include/mx/api/ScoreData.h b/src/include/mx/api/ScoreData.h index 63fde1fd1..81852c679 100644 --- a/src/include/mx/api/ScoreData.h +++ b/src/include/mx/api/ScoreData.h @@ -79,6 +79,12 @@ class ScoreData DefaultsData defaults; std::vector parts; std::vector partGroups; + + /// The score's time resolution: how many `tickTimePosition`/duration ticks make up one + /// quarter note. Corresponds to the `` MusicXML element. Choose a value with + /// enough factors for the durations you need (the default accommodates tuplets through + /// septuplets). Zero means the score has no time resolution at all — no durations exist + /// anywhere — and no `` is written. int ticksPerQuarter; /// Specifies page breaks, system breaks, and changes to system and page layout. Global/default page and diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index 481bff7ff..d050e6133 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -117,7 +117,13 @@ void MeasureWriter::writeMeasureGlobals() if (myHistory.getCursor().isFirstMeasureInPart) { - myPropertiesWriter->writeDivisions(myHistory.getCursor().getGlobalTicksPerQuarter()); + // A score with no time resolution (ticksPerQuarter <= 0: the source declared no + // and contains no durations) must not have one invented for it; writing + // 0 here would be clamped to a bogus positive value by core::PositiveDivisions. + if (myHistory.getCursor().getGlobalTicksPerQuarter() > 0) + { + myPropertiesWriter->writeDivisions(myHistory.getCursor().getGlobalTicksPerQuarter()); + } if (myMeasureData.staves.size() > 1) { diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 8bb4082ac..64395d60e 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -350,3 +350,51 @@ rpatters1/timesigs_composite-ref.musicxml # the serialized stream once voice 1 is written ahead of voice 2, plus a wedge. # Pins explicit slur/wedge numbers surviving read -> write -> read verbatim. rpatters1/slurs_overbars.musicxml + +# Unblocked by the spurious-divisions fix: a score whose source declares no +# anywhere (and has no durations) reads as ticksPerQuarter = 0, +# and the writer used to invent 0.000001 +# for the first measure of every part (LCM of an empty set = 0, clamped to a +# bogus positive by core::PositiveDivisions). The writer now omits +# when the score has no time resolution. +lysuite/ly51b_Header_Quotes.xml +mjbsuite/HasMusicXmlVersionFalse.xml +mjbsuite/HasMusicXmlVersionTrue.xml +synthetic/beat-type.3.0.xml +synthetic/beats.3.0.xml +synthetic/bracket.3.0.xml +synthetic/dashes.3.0.xml +synthetic/encoding-description.3.0.xml +synthetic/first.4.0.xml +synthetic/group.3.0.xml +synthetic/instrument-abbreviation.3.0.xml +synthetic/instrument-name.3.0.xml +synthetic/instrument-sound-enum.3.0.xml +synthetic/interchangeable.3.0.xml +synthetic/lyric-font.3.0.xml +synthetic/miscellaneous-field.3.0.xml +synthetic/movement-number.3.0.xml +synthetic/movement-title.3.0.xml +synthetic/music-font.3.0.xml +synthetic/n.3.1.xml +synthetic/numeral-fifths.4.0.xml +synthetic/numeral-mode.4.0.xml +synthetic/numeral.4.0.xml +synthetic/other-dynamics.3.0.xml +synthetic/pf.3.1.xml +synthetic/second.4.0.xml +synthetic/senza-misura.3.0.xml +synthetic/sfzp.3.1.xml +synthetic/software.3.0.xml +synthetic/staff-size.4.0.xml +synthetic/straight.4.0.xml +synthetic/swing-style.4.0.xml +synthetic/swing-type.4.0.xml +synthetic/swing.4.0.xml +synthetic/time-relation.3.0.xml +synthetic/virtual-library.3.0.xml +synthetic/virtual-name.3.0.xml +synthetic/voice.3.0.xml +synthetic/word-font.3.0.xml +synthetic/work-number.3.0.xml +synthetic/work-title.3.0.xml diff --git a/src/private/mxtest/impl/MeasureWriterTest.cpp b/src/private/mxtest/impl/MeasureWriterTest.cpp index ee8883458..4f2bfe947 100644 --- a/src/private/mxtest/impl/MeasureWriterTest.cpp +++ b/src/private/mxtest/impl/MeasureWriterTest.cpp @@ -224,6 +224,55 @@ TEST(PropertiesButNoNotes, MeasureWriter) T_END +TEST(ZeroTicksPerQuarterWritesNoDivisions, MeasureWriter) +{ + // A score with no time resolution (ticksPerQuarter == 0, e.g. a source that declared no + // and has no durations) must not have a bogus invented for it. + mxtest::TestParameters params; + params.ticksPerQuarter = 0; + params.measureIndex = 0; + params.partIndex = 0; + params.numStaves = 1; + mxtest::TestItems t = mxtest::setupTestItems(params); + + const auto partwiseMeasure = t.measureWriter->getPartwiseMeasure(); + CHECK(partwiseMeasure.musicData().empty()); +} + +T_END + +TEST(ZeroTicksPerQuarterKeepsOtherProperties, MeasureWriter) +{ + // With ticksPerQuarter == 0 the element is still written when it has real + // content (here a clef); only the child is suppressed. + mxtest::TestParameters params; + params.ticksPerQuarter = 0; + params.measureIndex = 0; + params.partIndex = 0; + params.numStaves = 1; + mxtest::TestItems t = mxtest::setupTestItems(params); + auto &staff = t.measureData->staves.at(0); + staff.clefs.emplace_back(api::ClefData{}); + auto &clef = staff.clefs.back(); + clef.symbol = api::ClefSymbol::g; + clef.line = 2; + + const auto partwiseMeasure = t.measureWriter->getPartwiseMeasure(); + auto musicData = partwiseMeasure.musicData(); + auto mdcIter = musicData.begin(); + auto mdcEnd = musicData.end(); + + CHECK(mdcIter != mdcEnd); + CHECK(mdcIter->isAttributes()); + const auto &props = mdcIter->asAttributes(); + CHECK(!props.divisions().has_value()); + CHECK_EQUAL(1, props.clef().size()); + + CHECK(++mdcIter == mdcEnd); +} + +T_END + TEST(MultiStaffClefKeepsNumber, MeasureWriter) { // Counterpart to PropertiesButNoNotes: in a multi-staff part the clef number is required From 4e6932e5a57f2b103ed5a448eafa3f174d96142a Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 15:59:57 +0000 Subject: [PATCH 2/3] feat: round-trip arpeggiate and non-arpeggiate attributes Adds ArpeggiateMarkData (number, unbroken, id) and NonArpeggiateMarkData (top/bottom placement, number, id) as MarkDataChoice alternatives and wires them through the reader and writer. The non-arpeggiate bracket end previously always wrote as type="top". Pins the 5 corpus files this flips to PASS (217 total). --- src/include/mx/api/MarkDataChoice.h | 79 ++++++++++++++++++- src/private/mx/api/MarkDataChoice.cpp | 50 +++++++++++- src/private/mx/impl/ArpeggiateFunctions.cpp | 17 ++++ .../mx/impl/NonArpeggiateFunctions.cpp | 15 ++++ src/private/mx/impl/NotationsWriter.cpp | 33 ++++++++ src/private/mxtest/api/MarkRoundTripTest.cpp | 45 ++++++++++- src/private/mxtest/api/roundtrip-baseline.txt | 9 +++ 7 files changed, 243 insertions(+), 5 deletions(-) diff --git a/src/include/mx/api/MarkDataChoice.h b/src/include/mx/api/MarkDataChoice.h index 44a8e0de6..f801bb526 100644 --- a/src/include/mx/api/MarkDataChoice.h +++ b/src/include/mx/api/MarkDataChoice.h @@ -7,6 +7,7 @@ #include "mx/api/ApiCommon.h" #include +#include #include namespace mx @@ -28,6 +29,60 @@ MXAPI_EQUALS_MEMBER(tremoloMarks) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(TremoloMarkData); +// Payload for the arpeggiate mark types (MarkType::arpeggiate / arpeggiateUp / arpeggiateDown). +// The up/down direction of the arpeggio's wavy line is encoded in the MarkType itself. +struct ArpeggiateMarkData +{ + // Distinguishes overlapping arpeggios: notes sharing a number belong to one arpeggio (e.g. + // an arpeggio spanning both staves of a keyboard part) while different numbers arpeggiate + // independently. Corresponds to the `number` attribute. + std::optional number; + + // yes: the arpeggio is drawn as one unbroken line across staves instead of one line per + // staff. Corresponds to the `unbroken` attribute (MusicXML 4.0). + Bool unbroken = Bool::unspecified; + + // The element's `id` attribute (MusicXML 3.1). + std::optional id; +}; + +MXAPI_EQUALS_BEGIN(ArpeggiateMarkData) +MXAPI_EQUALS_MEMBER(number) +MXAPI_EQUALS_MEMBER(unbroken) +MXAPI_EQUALS_MEMBER(id) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(ArpeggiateMarkData); + +// Which end of a non-arpeggiate bracket a note carries: the bracket's top or bottom. +enum class NonArpeggiatePlacement +{ + top, + bottom, +}; + +// Payload for MarkType::nonArpeggiate: a bracket indicating that a chord is NOT to be +// arpeggiated. MusicXML marks only the top and bottom notes of the chord, each carrying a +// element that states which end of the bracket it is. +struct NonArpeggiateMarkData +{ + // Which end of the bracket this note carries. Corresponds to the required `type` attribute. + NonArpeggiatePlacement placement = NonArpeggiatePlacement::top; + + // Distinguishes overlapping brackets, like ArpeggiateMarkData::number. Corresponds to the + // `number` attribute. + std::optional number; + + // The element's `id` attribute (MusicXML 3.1). + std::optional id; +}; + +MXAPI_EQUALS_BEGIN(NonArpeggiateMarkData) +MXAPI_EQUALS_MEMBER(placement) +MXAPI_EQUALS_MEMBER(number) +MXAPI_EQUALS_MEMBER(id) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(NonArpeggiateMarkData); + // A variant class that carries data for MarkType values whose payload does not fit MarkData's // common fields. // @@ -48,16 +103,24 @@ class MarkDataChoice enum class Kind { none, - tremolo + tremolo, + arpeggiate, + nonArpeggiate }; MarkDataChoice(); MarkDataChoice(TremoloMarkData value); + MarkDataChoice(ArpeggiateMarkData value); + + MarkDataChoice(NonArpeggiateMarkData value); + Kind kind() const; bool isNone() const; bool isTremolo() const; + bool isArpeggiate() const; + bool isNonArpeggiate() const; // Returns a copy of the internally held TremoloMarkData. // @@ -65,10 +128,22 @@ class MarkDataChoice // TremoloMarkData is returned. const TremoloMarkData tremolo() const; + // Returns a copy of the internally held ArpeggiateMarkData. + // + // Check isArpeggiate() first. If this is not an arpeggiate payload, a default constructed + // ArpeggiateMarkData is returned. + const ArpeggiateMarkData arpeggiate() const; + + // Returns a copy of the internally held NonArpeggiateMarkData. + // + // Check isNonArpeggiate() first. If this is not a non-arpeggiate payload, a default + // constructed NonArpeggiateMarkData is returned. + const NonArpeggiateMarkData nonArpeggiate() const; + bool operator==(const MarkDataChoice &other) const; private: - std::variant myValue; + std::variant myValue; }; MXAPI_NOT_EQUALS_AND_VECTORS(MarkDataChoice); diff --git a/src/private/mx/api/MarkDataChoice.cpp b/src/private/mx/api/MarkDataChoice.cpp index 2e429628b..ad8e3d7b8 100644 --- a/src/private/mx/api/MarkDataChoice.cpp +++ b/src/private/mx/api/MarkDataChoice.cpp @@ -19,9 +19,29 @@ MarkDataChoice::MarkDataChoice(TremoloMarkData value) : myValue{std::move(value) { } +MarkDataChoice::MarkDataChoice(ArpeggiateMarkData value) : myValue{std::move(value)} +{ +} + +MarkDataChoice::MarkDataChoice(NonArpeggiateMarkData value) : myValue{std::move(value)} +{ +} + MarkDataChoice::Kind MarkDataChoice::kind() const { - return std::holds_alternative(myValue) ? Kind::tremolo : Kind::none; + if (std::holds_alternative(myValue)) + { + return Kind::tremolo; + } + if (std::holds_alternative(myValue)) + { + return Kind::arpeggiate; + } + if (std::holds_alternative(myValue)) + { + return Kind::nonArpeggiate; + } + return Kind::none; } bool MarkDataChoice::isNone() const @@ -43,6 +63,34 @@ const TremoloMarkData MarkDataChoice::tremolo() const return TremoloMarkData{}; } +bool MarkDataChoice::isArpeggiate() const +{ + return std::holds_alternative(myValue); +} + +const ArpeggiateMarkData MarkDataChoice::arpeggiate() const +{ + if (const auto *value = std::get_if(&myValue)) + { + return *value; + } + return ArpeggiateMarkData{}; +} + +bool MarkDataChoice::isNonArpeggiate() const +{ + return std::holds_alternative(myValue); +} + +const NonArpeggiateMarkData MarkDataChoice::nonArpeggiate() const +{ + if (const auto *value = std::get_if(&myValue)) + { + return *value; + } + return NonArpeggiateMarkData{}; +} + bool MarkDataChoice::operator==(const MarkDataChoice &other) const { return myValue == other.myValue; diff --git a/src/private/mx/impl/ArpeggiateFunctions.cpp b/src/private/mx/impl/ArpeggiateFunctions.cpp index 0d1216906..79984af4c 100644 --- a/src/private/mx/impl/ArpeggiateFunctions.cpp +++ b/src/private/mx/impl/ArpeggiateFunctions.cpp @@ -4,6 +4,7 @@ #include "mx/impl/ArpeggiateFunctions.h" #include "mx/core/generated/Arpeggiate.h" +#include "mx/impl/Converter.h" #include "mx/impl/MarkDataFunctions.h" namespace mx @@ -36,6 +37,22 @@ api::MarkData ArpeggiateFunctions::parseArpeggiate() const impl::parseMarkDataAttributes(myArpeggiate, markData); markData.tickTimePosition = myCursor.tickTimePosition; + api::ArpeggiateMarkData arpeggiateData{}; + if (myArpeggiate.number().has_value()) + { + arpeggiateData.number = myArpeggiate.number()->value(); + } + if (myArpeggiate.unbroken().has_value()) + { + Converter converter; + arpeggiateData.unbroken = converter.convert(*myArpeggiate.unbroken()); + } + if (myArpeggiate.id().has_value()) + { + arpeggiateData.id = myArpeggiate.id()->value(); + } + markData.choice = arpeggiateData; + return markData; } } // namespace impl diff --git a/src/private/mx/impl/NonArpeggiateFunctions.cpp b/src/private/mx/impl/NonArpeggiateFunctions.cpp index 70eb137be..c78e957ac 100644 --- a/src/private/mx/impl/NonArpeggiateFunctions.cpp +++ b/src/private/mx/impl/NonArpeggiateFunctions.cpp @@ -21,6 +21,21 @@ api::MarkData NonArpeggiateFunctions::parseNonArpeggiate() const api::MarkData markData{api::MarkType::nonArpeggiate}; impl::parseMarkDataAttributes(myNonArpeggiate, markData); markData.tickTimePosition = myCursor.tickTimePosition; + + api::NonArpeggiateMarkData nonArpeggiateData{}; + nonArpeggiateData.placement = myNonArpeggiate.type().tag() == core::TopBottom::Tag::bottom + ? api::NonArpeggiatePlacement::bottom + : api::NonArpeggiatePlacement::top; + if (myNonArpeggiate.number().has_value()) + { + nonArpeggiateData.number = myNonArpeggiate.number()->value(); + } + if (myNonArpeggiate.id().has_value()) + { + nonArpeggiateData.id = myNonArpeggiate.id()->value(); + } + markData.choice = nonArpeggiateData; + return markData; } } // namespace impl diff --git a/src/private/mx/impl/NotationsWriter.cpp b/src/private/mx/impl/NotationsWriter.cpp index aec8f86a8..cf7312ec0 100644 --- a/src/private/mx/impl/NotationsWriter.cpp +++ b/src/private/mx/impl/NotationsWriter.cpp @@ -3,6 +3,7 @@ // Distributed under the MIT License #include "mx/impl/NotationsWriter.h" +#include "mx/core/Token.h" #include "mx/core/generated/Arpeggiate.h" #include "mx/core/generated/ArrowChoice.h" #include "mx/core/generated/ArrowChoiceGroup.h" @@ -30,6 +31,7 @@ #include "mx/core/generated/Mordent.h" #include "mx/core/generated/NonArpeggiate.h" #include "mx/core/generated/NotationsChoice.h" +#include "mx/core/generated/NumberLevel.h" #include "mx/core/generated/OrnamentsGroup.h" #include "mx/core/generated/OrnamentsGroupChoice.h" #include "mx/core/generated/OtherPlacementText.h" @@ -41,6 +43,7 @@ #include "mx/core/generated/StrongAccent.h" #include "mx/core/generated/TechnicalChoice.h" #include "mx/core/generated/Tied.h" +#include "mx/core/generated/TopBottom.h" #include "mx/core/generated/Tremolo.h" #include "mx/core/generated/TremoloMarks.h" #include "mx/core/generated/TremoloType.h" @@ -52,6 +55,7 @@ #include "mx/core/generated/UpDown.h" #include "mx/core/generated/UprightInverted.h" #include "mx/core/generated/WavyLine.h" +#include "mx/impl/Converter.h" #include "mx/impl/CurveFunctions.h" #include "mx/impl/DynamicsWriter.h" #include "mx/impl/MarkDataFunctions.h" @@ -358,6 +362,20 @@ core::Notations NotationsWriter::getNotations() const { core::NonArpeggiate nonArpeggiate; impl::setAttributesFromMarkData(mark, nonArpeggiate); + + const auto nonArpeggiateData = mark.choice.nonArpeggiate(); + nonArpeggiate.setType(nonArpeggiateData.placement == api::NonArpeggiatePlacement::bottom + ? core::TopBottom::bottom() + : core::TopBottom::top()); + if (nonArpeggiateData.number.has_value()) + { + nonArpeggiate.setNumber(core::NumberLevel{*nonArpeggiateData.number}); + } + if (nonArpeggiateData.id.has_value()) + { + nonArpeggiate.setID(core::Token{*nonArpeggiateData.id}); + } + outNotations.addChoice(core::NotationsChoice::nonArpeggiate(nonArpeggiate)); } else if (isMarkArpeggiate(mark.markType)) @@ -378,6 +396,21 @@ core::Notations NotationsWriter::getNotations() const arpeggiate.setDirection(core::UpDown::down()); } + const auto arpeggiateData = mark.choice.arpeggiate(); + if (arpeggiateData.number.has_value()) + { + arpeggiate.setNumber(core::NumberLevel{*arpeggiateData.number}); + } + if (arpeggiateData.unbroken != api::Bool::unspecified) + { + Converter converter; + arpeggiate.setUnbroken(converter.convert(arpeggiateData.unbroken)); + } + if (arpeggiateData.id.has_value()) + { + arpeggiate.setID(core::Token{*arpeggiateData.id}); + } + outNotations.addChoice(core::NotationsChoice::arpeggiate(arpeggiate)); } } diff --git a/src/private/mxtest/api/MarkRoundTripTest.cpp b/src/private/mxtest/api/MarkRoundTripTest.cpp index 1a5198689..de76fcc1f 100644 --- a/src/private/mxtest/api/MarkRoundTripTest.cpp +++ b/src/private/mxtest/api/MarkRoundTripTest.cpp @@ -19,7 +19,7 @@ namespace // Round-trips a single note-attached mark of the given MarkType through the // full serialize -> deserialize path and returns the marks read back from the // first note. Used by the enum data-loss regression tests below. -std::vector roundTripMark(MarkType inMarkType) +std::vector roundTripMarkData(const MarkData &inMarkData) { ScoreData score; score.parts.emplace_back(); @@ -31,7 +31,7 @@ std::vector roundTripMark(MarkType inMarkType) auto &voice = staff.voices[0]; voice.notes.emplace_back(); auto ¬e = voice.notes.back(); - note.noteAttachmentData.marks.emplace_back(Placement::unspecified, inMarkType); + note.noteAttachmentData.marks.push_back(inMarkData); auto &mgr = DocumentManager::getInstance(); const auto r1 = mgr.createFromScore(score); @@ -58,6 +58,11 @@ std::vector roundTripMark(MarkType inMarkType) return onote.noteAttachmentData.marks; } +std::vector roundTripMark(MarkType inMarkType) +{ + return roundTripMarkData(MarkData{Placement::unspecified, inMarkType}); +} + bool hasMark(const std::vector &marks, MarkType inMarkType) { for (const auto &m : marks) @@ -227,4 +232,40 @@ TEST(FermataCurlew, MarkRoundTrip) T_END; +TEST(ArpeggiateAttributes, MarkRoundTrip) +{ + MarkData mark{Placement::unspecified, MarkType::arpeggiateUp}; + ArpeggiateMarkData payload{}; + payload.number = 2; + payload.unbroken = Bool::yes; + payload.id = "arp-1"; + mark.choice = payload; + + const auto marks = roundTripMarkData(mark); + REQUIRE(marks.size() == 1); + CHECK(marks.front().markType == MarkType::arpeggiateUp); + REQUIRE(marks.front().choice.isArpeggiate()); + CHECK(payload == marks.front().choice.arpeggiate()); +} + +T_END; + +TEST(NonArpeggiateAttributes, MarkRoundTrip) +{ + MarkData mark{Placement::unspecified, MarkType::nonArpeggiate}; + NonArpeggiateMarkData payload{}; + payload.placement = NonArpeggiatePlacement::bottom; + payload.number = 1; + payload.id = "nonarp-1"; + mark.choice = payload; + + const auto marks = roundTripMarkData(mark); + REQUIRE(marks.size() == 1); + CHECK(marks.front().markType == MarkType::nonArpeggiate); + REQUIRE(marks.front().choice.isNonArpeggiate()); + CHECK(payload == marks.front().choice.nonArpeggiate()); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 64395d60e..c21fbe8cb 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -398,3 +398,12 @@ synthetic/voice.3.0.xml synthetic/word-font.3.0.xml synthetic/work-number.3.0.xml synthetic/work-title.3.0.xml + +# Unblocked by the arpeggiate/non-arpeggiate attribute payloads +# (ArpeggiateMarkData / NonArpeggiateMarkData in MarkDataChoice): number, +# unbroken, id, and the non-arpeggiate top/bottom type now round-trip. +synthetic/arpeggiate.3.0.xml +synthetic/arpeggiate.3.1.xml +synthetic/arpeggiate.4.0.xml +synthetic/non-arpeggiate.3.0.xml +synthetic/non-arpeggiate.3.1.xml From ba1ba1155c474283617e24d0f7f5d9e2158d0fea Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 16:10:45 +0000 Subject: [PATCH 3/3] fix: round-trip the caesura value MarkType::caesura now round-trips as the common empty form instead of acquiring a spurious 'normal' text value, and new caesuraNormal/Thick/Short/Curved/Single mark types carry the explicit MusicXML 3.1 values that were previously flattened to 'normal'. Pins the 3 corpus files this flips to PASS (220 total). --- src/include/mx/api/MarkData.h | 7 +++++- src/private/mx/api/MarkData.cpp | 7 ++++-- .../mx/impl/ArticulationsFunctions.cpp | 22 ++++++++++++++++ src/private/mx/impl/Converter.cpp | 5 ++++ src/private/mx/impl/NotationsWriter.cpp | 25 +++++++++++++++++++ src/private/mxtest/api/MarkRoundTripTest.cpp | 20 +++++++++++++++ src/private/mxtest/api/roundtrip-baseline.txt | 7 ++++++ 7 files changed, 90 insertions(+), 3 deletions(-) diff --git a/src/include/mx/api/MarkData.h b/src/include/mx/api/MarkData.h index 4fb831b07..a5489d072 100644 --- a/src/include/mx/api/MarkData.h +++ b/src/include/mx/api/MarkData.h @@ -34,7 +34,12 @@ enum class MarkType doit, falloff, breathMark, - caesura, + caesura, // an empty , the common form; renders the same as caesuraNormal + caesuraNormal, + caesuraThick, + caesuraShort, + caesuraCurved, + caesuraSingle, stress, unstress, softAccent, diff --git a/src/private/mx/api/MarkData.cpp b/src/private/mx/api/MarkData.cpp index f5e6bf8d6..ca546eb77 100644 --- a/src/private/mx/api/MarkData.cpp +++ b/src/private/mx/api/MarkData.cpp @@ -105,8 +105,11 @@ bool isMarkArticulation(MarkType markType) (markType == MarkType::detachedLegato) || (markType == MarkType::staccatissimo) || (markType == MarkType::spiccato) || (markType == MarkType::scoop) || (markType == MarkType::plop) || (markType == MarkType::doit) || (markType == MarkType::falloff) || (markType == MarkType::breathMark) || - (markType == MarkType::caesura) || (markType == MarkType::stress) || (markType == MarkType::unstress) || - (markType == MarkType::softAccent) || (markType == MarkType::otherArticulation); + (markType == MarkType::caesura) || (markType == MarkType::caesuraNormal) || + (markType == MarkType::caesuraThick) || (markType == MarkType::caesuraShort) || + (markType == MarkType::caesuraCurved) || (markType == MarkType::caesuraSingle) || + (markType == MarkType::stress) || (markType == MarkType::unstress) || (markType == MarkType::softAccent) || + (markType == MarkType::otherArticulation); } bool isMarkOrnament(MarkType markType) diff --git a/src/private/mx/impl/ArticulationsFunctions.cpp b/src/private/mx/impl/ArticulationsFunctions.cpp index 4bcdbe168..2e5cdc1ca 100644 --- a/src/private/mx/impl/ArticulationsFunctions.cpp +++ b/src/private/mx/impl/ArticulationsFunctions.cpp @@ -4,6 +4,7 @@ #include "mx/impl/ArticulationsFunctions.h" #include "mx/core/generated/ArticulationsChoice.h" +#include "mx/core/generated/CaesuraValue.h" #include "mx/impl/Converter.h" #include "mx/impl/MarkDataFunctions.h" #include "mx/impl/PositionFunctions.h" @@ -108,6 +109,27 @@ void ArticulationsFunctions::parseArticulation(const core::ArticulationsChoice & case core::ArticulationsChoice::Kind::caesura: { parseMarkDataAttributes(inArticulation.asCaesura(), outMark); outMark.name = "caesura"; + switch (inArticulation.asCaesura().value().tag()) + { + case core::CaesuraValue::Tag::empty: + outMark.markType = api::MarkType::caesura; + break; + case core::CaesuraValue::Tag::normal: + outMark.markType = api::MarkType::caesuraNormal; + break; + case core::CaesuraValue::Tag::thick: + outMark.markType = api::MarkType::caesuraThick; + break; + case core::CaesuraValue::Tag::short_: + outMark.markType = api::MarkType::caesuraShort; + break; + case core::CaesuraValue::Tag::curved: + outMark.markType = api::MarkType::caesuraCurved; + break; + case core::CaesuraValue::Tag::single: + outMark.markType = api::MarkType::caesuraSingle; + break; + } break; } case core::ArticulationsChoice::Kind::stress: { diff --git a/src/private/mx/impl/Converter.cpp b/src/private/mx/impl/Converter.cpp index 386be73ec..299c14220 100644 --- a/src/private/mx/impl/Converter.cpp +++ b/src/private/mx/impl/Converter.cpp @@ -174,6 +174,11 @@ const Converter::EnumMap Convert {core::ArticulationsChoice::Kind::falloff, api::MarkType::falloff}, {core::ArticulationsChoice::Kind::breathMark, api::MarkType::breathMark}, {core::ArticulationsChoice::Kind::caesura, api::MarkType::caesura}, + {core::ArticulationsChoice::Kind::caesura, api::MarkType::caesuraNormal}, + {core::ArticulationsChoice::Kind::caesura, api::MarkType::caesuraThick}, + {core::ArticulationsChoice::Kind::caesura, api::MarkType::caesuraShort}, + {core::ArticulationsChoice::Kind::caesura, api::MarkType::caesuraCurved}, + {core::ArticulationsChoice::Kind::caesura, api::MarkType::caesuraSingle}, {core::ArticulationsChoice::Kind::stress, api::MarkType::stress}, {core::ArticulationsChoice::Kind::unstress, api::MarkType::unstress}, {core::ArticulationsChoice::Kind::softAccent, api::MarkType::softAccent}, diff --git a/src/private/mx/impl/NotationsWriter.cpp b/src/private/mx/impl/NotationsWriter.cpp index cf7312ec0..49f1613ca 100644 --- a/src/private/mx/impl/NotationsWriter.cpp +++ b/src/private/mx/impl/NotationsWriter.cpp @@ -11,6 +11,7 @@ #include "mx/core/generated/ArticulationsChoice.h" #include "mx/core/generated/BreathMark.h" #include "mx/core/generated/Caesura.h" +#include "mx/core/generated/CaesuraValue.h" #include "mx/core/generated/EmptyLine.h" #include "mx/core/generated/EmptyPlacement.h" #include "mx/core/generated/EmptyPlacementSmufl.h" @@ -542,6 +543,30 @@ void NotationsWriter::addArticulation(const api::MarkData &mark, core::Articulat case core::ArticulationsChoice::Kind::caesura: { core::Caesura c; setAttributesFromPositionData(mark.positionData, c); + // MarkType::caesura is the common empty form ; the variants carry an + // explicit text value. + auto caesuraValue = core::CaesuraValue::empty(); + if (mark.markType == api::MarkType::caesuraNormal) + { + caesuraValue = core::CaesuraValue::normal(); + } + else if (mark.markType == api::MarkType::caesuraThick) + { + caesuraValue = core::CaesuraValue::thick(); + } + else if (mark.markType == api::MarkType::caesuraShort) + { + caesuraValue = core::CaesuraValue::short_(); + } + else if (mark.markType == api::MarkType::caesuraCurved) + { + caesuraValue = core::CaesuraValue::curved(); + } + else if (mark.markType == api::MarkType::caesuraSingle) + { + caesuraValue = core::CaesuraValue::single(); + } + c.setValue(caesuraValue); outArticulations.addChoice(core::ArticulationsChoice::caesura(c)); break; } diff --git a/src/private/mxtest/api/MarkRoundTripTest.cpp b/src/private/mxtest/api/MarkRoundTripTest.cpp index de76fcc1f..191a90e79 100644 --- a/src/private/mxtest/api/MarkRoundTripTest.cpp +++ b/src/private/mxtest/api/MarkRoundTripTest.cpp @@ -268,4 +268,24 @@ TEST(NonArpeggiateAttributes, MarkRoundTrip) T_END; +TEST(CaesuraEmpty, MarkRoundTrip) +{ + // The common empty form must not acquire a "normal" text value on write. + const auto marks = roundTripMark(MarkType::caesura); + CHECK(hasMark(marks, MarkType::caesura)); +} + +T_END; + +TEST(CaesuraVariants, MarkRoundTrip) +{ + CHECK(hasMark(roundTripMark(MarkType::caesuraNormal), MarkType::caesuraNormal)); + CHECK(hasMark(roundTripMark(MarkType::caesuraThick), MarkType::caesuraThick)); + CHECK(hasMark(roundTripMark(MarkType::caesuraShort), MarkType::caesuraShort)); + CHECK(hasMark(roundTripMark(MarkType::caesuraCurved), MarkType::caesuraCurved)); + CHECK(hasMark(roundTripMark(MarkType::caesuraSingle), MarkType::caesuraSingle)); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index c21fbe8cb..70314f431 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -407,3 +407,10 @@ synthetic/arpeggiate.3.1.xml synthetic/arpeggiate.4.0.xml synthetic/non-arpeggiate.3.0.xml synthetic/non-arpeggiate.3.1.xml + +# Unblocked by the caesura value fix: MarkType::caesura now round-trips the +# common empty element form, and the caesuraNormal/Thick/Short/Curved/Single +# variants carry an explicit text value. +ksuite/k001a_Articulations.xml +musuite/testNoteAttributes2.xml +musuite/testNoteAttributes2_ref.xml