From 20b6867c227bb7987b328dff084d1a3159efe95f Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 6 Nov 2026 00:55:28 +0100 Subject: [PATCH 1/3] feat: an ornament owns the accidental marks written after it MusicXML allows an in two places: directly in , where it stands above or below the note on its own, and inside right after an ornament, where it belongs to that ornament (the sharp over a trill, the accidentals above and below a turn). Both used to read as flat MarkType::accidentalMark* marks, with the link to the ornament lost, and every one was written back at the level. MarkDataChoice gains a Kind::ornament alternative, OrnamentMarkData, holding a list of OrnamentAccidental entries (an api::Accidental and its position, measured from the ornament). A trill, any turn, a shake, a mordent, an inverted mordent, a schleifer or a haydn holds the accidentals that follow it, in order. An OrnamentMarkData with no accidentals collapses to none. Behaviour change: reading sharp used to give two marks, a trill and an accidentalMarkSharp. It now gives one trill mark whose choice.ornament().accidentals holds the sharp, and writing it puts the sharp back inside right after the trill. Code that looked in marks for a trill's accidental must look in the trill. Accidental marks you author, and ones read from , are unchanged and still write at the level. An accidental after a tremolo, an other-ornament or a wavy line has no ornament mark to hold it, so it is still read as a standalone accidental mark after the ornament marks, with a warning. On write, an entry left at Accidental::none, and ornament accidentals on a mark that is not written in , are dropped with an error diagnostic. An accidental-mark inside whose value the api has no member for (other) is dropped on read with an error diagnostic. Closes #443. --- data/api.features.xml | 2 +- src/include/mx/api/MarkData.h | 4 +- src/include/mx/api/MarkDataChoice.h | 54 +++++- src/private/mx/api/MarkDataChoice.cpp | 32 ++++ .../mx/impl/AccidentalMarkFunctions.cpp | 9 + src/private/mx/impl/AccidentalMarkFunctions.h | 4 + src/private/mx/impl/NotationsWriter.cpp | 41 ++++- src/private/mx/impl/NotationsWriter.h | 1 + src/private/mx/impl/OrnamentsFunctions.cpp | 99 +++++++++-- src/private/mx/impl/OrnamentsFunctions.h | 5 +- src/private/mxtest/api/MarkRoundTripTest.cpp | 157 +++++++++++++++++ src/private/mxtest/api/NoteDataTest.cpp | 161 +++++++++++++++--- src/private/mxtest/api/WavyLineApiTest.cpp | 40 +++++ 13 files changed, 563 insertions(+), 46 deletions(-) diff --git a/data/api.features.xml b/data/api.features.xml index 8d98ced92..ebe8feb3c 100644 --- a/data/api.features.xml +++ b/data/api.features.xml @@ -28,7 +28,7 @@ other - api::MarkType accidentalMark*; all members mapped except AccidentalValue::other (no api::MarkType::accidentalMarkOther member). + In <notations>: api::MarkType accidentalMark*. In <ornaments> after an ornament: api::OrnamentAccidental (api::Accidental) in the ornament mark's OrnamentMarkData; after tremolo, other-ornament or wavy-line it falls back to an accidentalMark* mark. All members mapped except AccidentalValue::other (no api member). Only value, position and placement are kept. Accidental as text (key/ornament context) (suspected). diff --git a/src/include/mx/api/MarkData.h b/src/include/mx/api/MarkData.h index 2fe21d5a1..cc4f33fc5 100644 --- a/src/include/mx/api/MarkData.h +++ b/src/include/mx/api/MarkData.h @@ -81,7 +81,9 @@ enum class MarkType otherOrnament, ///< MusicXML's 'other-ornament' value unknownOrnament, ///< Error state - // accidental marks + // accidental marks: an accidental shown above or below the note on its own. An accidental that + // belongs to an ornament, like the sharp over a trill, is held by that ornament instead + // (OrnamentMarkData in MarkDataChoice.h). accidentalMarkSharp, accidentalMarkNatural, accidentalMarkFlat, diff --git a/src/include/mx/api/MarkDataChoice.h b/src/include/mx/api/MarkDataChoice.h index 7b9b73b47..de86ab966 100644 --- a/src/include/mx/api/MarkDataChoice.h +++ b/src/include/mx/api/MarkDataChoice.h @@ -7,10 +7,13 @@ #include "mx/api/ApiCommon.h" #include "mx/api/DynamicsData.h" #include "mx/api/Id.h" +#include "mx/api/PitchData.h" +#include "mx/api/PositionData.h" #include #include #include +#include namespace mx { @@ -195,6 +198,41 @@ MXAPI_EQUALS_MEMBER(pitch) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(HarmonicMarkData); +// An accidental printed with an ornament: the sharp over a trill that raises its upper note, or the +// accidentals above and below a turn that alter its upper and lower notes. +struct OrnamentAccidental +{ + // The accidental to show. An entry left at none is not drawn. + Accidental accidental = Accidental::none; + + // Where the accidental sits, measured from the ornament rather than from the note. Placement + // above puts the accidental over the ornament, below puts it under. + PositionData positionData; +}; + +MXAPI_EQUALS_BEGIN(OrnamentAccidental) +MXAPI_EQUALS_MEMBER(accidental) +MXAPI_EQUALS_MEMBER(positionData) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(OrnamentAccidental); + +// Payload for an ornament that has accidentals: a trill, a turn of any kind, a shake, a mordent, an +// inverted mordent, a schleifer or a haydn. A trill whose upper note is sharpened is one +// MarkType::trillMark mark holding an OrnamentAccidental with Accidental::sharp. +// +// An accidental that stands above or below the note on its own, not tied to an ornament, is a +// MarkType::accidentalMark* mark of its own instead. +struct OrnamentMarkData +{ + // The ornament's accidentals, usually one. A turn may have one above it and one below it. + std::vector accidentals; +}; + +MXAPI_EQUALS_BEGIN(OrnamentMarkData) +MXAPI_EQUALS_MEMBER(accidentals) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(OrnamentMarkData); + // A variant class that carries data for MarkType values whose payload does not fit MarkData's // common fields. // @@ -222,7 +260,8 @@ class MarkDataChoice dynamic, compoundDynamics, otherNotation, - harmonic + harmonic, + ornament }; MarkDataChoice(); @@ -246,6 +285,10 @@ class MarkDataChoice MarkDataChoice(HarmonicMarkData value); + // Builds an ornament payload, unless it has no accidentals, in which case the result is none: + // an ornament without accidentals needs no payload. + MarkDataChoice(OrnamentMarkData value); + Kind kind() const; bool isNone() const; bool isTremolo() const; @@ -256,6 +299,7 @@ class MarkDataChoice bool isCompoundDynamics() const; bool isOtherNotation() const; bool isHarmonic() const; + bool isOrnament() const; // Returns a copy of the internally held TremoloMarkData. // @@ -293,11 +337,17 @@ class MarkDataChoice // HarmonicMarkData is returned. const HarmonicMarkData harmonic() const; + // Returns a copy of the internally held OrnamentMarkData. + // + // Check isOrnament() first. If this is not an ornament payload, a default constructed + // OrnamentMarkData, with no accidentals, is returned. + const OrnamentMarkData ornament() const; + bool operator==(const MarkDataChoice &other) const; private: std::variant + StandardDynamic, CompoundDynamicsData, OtherNotationMarkData, HarmonicMarkData, OrnamentMarkData> myValue; }; diff --git a/src/private/mx/api/MarkDataChoice.cpp b/src/private/mx/api/MarkDataChoice.cpp index cadb16412..dde5fd8d9 100644 --- a/src/private/mx/api/MarkDataChoice.cpp +++ b/src/private/mx/api/MarkDataChoice.cpp @@ -58,6 +58,20 @@ MarkDataChoice::MarkDataChoice(HarmonicMarkData value) : myValue{std::move(value { } +// An ornament with no accidentals is stored as none, so a plain trill compares equal however it +// was built. +MarkDataChoice::MarkDataChoice(OrnamentMarkData value) +{ + if (value.accidentals.empty()) + { + myValue = std::monostate{}; + } + else + { + myValue = std::move(value); + } +} + MarkDataChoice::Kind MarkDataChoice::kind() const { if (std::holds_alternative(myValue)) @@ -92,6 +106,10 @@ MarkDataChoice::Kind MarkDataChoice::kind() const { return Kind::harmonic; } + if (std::holds_alternative(myValue)) + { + return Kind::ornament; + } return Kind::none; } @@ -212,6 +230,20 @@ const HarmonicMarkData MarkDataChoice::harmonic() const return HarmonicMarkData{}; } +bool MarkDataChoice::isOrnament() const +{ + return std::holds_alternative(myValue); +} + +const OrnamentMarkData MarkDataChoice::ornament() const +{ + if (const auto *value = std::get_if(&myValue)) + { + return *value; + } + return OrnamentMarkData{}; +} + bool MarkDataChoice::operator==(const MarkDataChoice &other) const { return myValue == other.myValue; diff --git a/src/private/mx/impl/AccidentalMarkFunctions.cpp b/src/private/mx/impl/AccidentalMarkFunctions.cpp index 287aa7063..a72e6e01d 100644 --- a/src/private/mx/impl/AccidentalMarkFunctions.cpp +++ b/src/private/mx/impl/AccidentalMarkFunctions.cpp @@ -30,5 +30,14 @@ api::MarkData AccidentalMarkFunctions::parseAccidentalMark() const return markData; } + +api::OrnamentAccidental AccidentalMarkFunctions::parseOrnamentAccidental() const +{ + Converter converter; + api::OrnamentAccidental accidental; + accidental.accidental = converter.convert(myAccidentalMark.value()); + accidental.positionData = impl::getPositionData(myAccidentalMark); + return accidental; +} } // namespace impl } // namespace mx diff --git a/src/private/mx/impl/AccidentalMarkFunctions.h b/src/private/mx/impl/AccidentalMarkFunctions.h index 2c9ee74dd..b46153f4b 100644 --- a/src/private/mx/impl/AccidentalMarkFunctions.h +++ b/src/private/mx/impl/AccidentalMarkFunctions.h @@ -26,6 +26,10 @@ class AccidentalMarkFunctions AccidentalMarkFunctions(AccidentalMarkFunctions &&) = default; api::MarkData parseAccidentalMark() const; + // Reads an accidental-mark that follows an ornament inside as one of that + // ornament's accidentals. An accidental value the api has no member for comes back as none. + api::OrnamentAccidental parseOrnamentAccidental() const; + private: const core::AccidentalMark &myAccidentalMark; const impl::Cursor myCursor; diff --git a/src/private/mx/impl/NotationsWriter.cpp b/src/private/mx/impl/NotationsWriter.cpp index 55b2dbf67..313a5e546 100644 --- a/src/private/mx/impl/NotationsWriter.cpp +++ b/src/private/mx/impl/NotationsWriter.cpp @@ -383,6 +383,12 @@ core::Notations NotationsWriter::getNotations() const { this->addOrnament(mark, ornaments); } + else if (mark.choice.isOrnament()) + { + myScoreWriter.getDiagnostics().report( + api::Severity::error, api::DiagnosticCode::droppedData, cursorLocation(myCursor), + "ornament accidentals on a mark that is not an ornament are not written"); + } if (isMarkAccidentalMark(mark.markType)) { this->addAccidentalMark(mark, outNotations); @@ -1170,14 +1176,39 @@ void NotationsWriter::addOrnament(const api::MarkData &mark, core::Ornaments &ou return; } + addOrnamentAccidentals(mark, group); outOrnaments.addGroup(group); } -// An accidental mark is written at the notations level. MusicXML also allows one inside -// , 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 group is -// required to carry an ornament, so putting one there would mean inventing an ornament. Marks -// from either place are written here instead. +// An ornament's accidentals go in its own group, right after it, which is where MusicXML says an +// accidental belongs to the ornament before it. +void NotationsWriter::addOrnamentAccidentals(const api::MarkData &mark, core::OrnamentsGroup &outGroup) const +{ + if (!mark.choice.isOrnament()) + { + return; + } + + const auto payload = mark.choice.ornament(); + for (const auto &accidental : payload.accidentals) + { + if (accidental.accidental == api::Accidental::none) + { + myScoreWriter.getDiagnostics().report(api::Severity::error, api::DiagnosticCode::droppedData, + cursorLocation(myCursor), + "an ornament accidental of none is not written"); + continue; + } + + core::AccidentalMark accidentalMark; + accidentalMark.setValue(myConverter.convert(accidental.accidental)); + setAttributesFromPositionData(accidental.positionData, accidentalMark); + outGroup.addAccidentalMark(accidentalMark); + } +} + +// A standalone accidental mark is written directly in . An accidental that belongs to an +// ornament is not a mark of its own; addOrnamentAccidentals writes it inside . void NotationsWriter::addAccidentalMark(const api::MarkData &mark, core::Notations &outNotations) const { if (!isMarkAccidentalMark(mark.markType)) diff --git a/src/private/mx/impl/NotationsWriter.h b/src/private/mx/impl/NotationsWriter.h index ce4d9b0c8..4939c492d 100644 --- a/src/private/mx/impl/NotationsWriter.h +++ b/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 addOrnamentAccidentals(const api::MarkData &markData, core::OrnamentsGroup &outGroup) const; void addAccidentalMark(const api::MarkData &markData, core::Notations &outNotations) const; void addTechnical(const api::MarkData &markData, core::Technical &outTechnical) const; diff --git a/src/private/mx/impl/OrnamentsFunctions.cpp b/src/private/mx/impl/OrnamentsFunctions.cpp index b8ba69628..6737190d7 100644 --- a/src/private/mx/impl/OrnamentsFunctions.cpp +++ b/src/private/mx/impl/OrnamentsFunctions.cpp @@ -40,6 +40,46 @@ void ornamentsFunctionsParseMordentSpecificAttributes(const core::Mordent &m, ap } } +// The ornaments that hold the accidentals written after them: a trill, a turn of any kind, a +// shake, a mordent, an inverted mordent, a schleifer or a haydn. +bool ornamentsFunctionsCanOwnAccidentals(core::OrnamentsGroupChoice::Kind kind) +{ + switch (kind) + { + case core::OrnamentsGroupChoice::Kind::trillMark: + case core::OrnamentsGroupChoice::Kind::turn: + case core::OrnamentsGroupChoice::Kind::delayedTurn: + case core::OrnamentsGroupChoice::Kind::invertedTurn: + case core::OrnamentsGroupChoice::Kind::delayedInvertedTurn: + case core::OrnamentsGroupChoice::Kind::verticalTurn: + case core::OrnamentsGroupChoice::Kind::invertedVerticalTurn: + case core::OrnamentsGroupChoice::Kind::shake: + case core::OrnamentsGroupChoice::Kind::mordent: + case core::OrnamentsGroupChoice::Kind::invertedMordent: + case core::OrnamentsGroupChoice::Kind::schleifer: + case core::OrnamentsGroupChoice::Kind::haydn: + return true; + default: + return false; + } +} + +// Names an ornament that cannot hold accidentals, for diagnostics. +std::string ornamentsFunctionsHostName(core::OrnamentsGroupChoice::Kind kind) +{ + switch (kind) + { + case core::OrnamentsGroupChoice::Kind::tremolo: + return "a tremolo"; + case core::OrnamentsGroupChoice::Kind::otherOrnament: + return "an other-ornament"; + case core::OrnamentsGroupChoice::Kind::wavyLine: + return "a wavy-line"; + default: + return "an ornament"; + } +} + OrnamentsFunctions::OrnamentsFunctions(const core::Ornaments &inOrnaments, MeasureCursor inCursor, DiagnosticsContext diagnostics) : myOrnaments{inOrnaments}, myCursor{inCursor}, myDiagnostics{diagnostics} @@ -48,12 +88,10 @@ OrnamentsFunctions::OrnamentsFunctions(const core::Ornaments &inOrnaments, Measu void OrnamentsFunctions::parseOrnaments(api::NoteAttachmentData &outAttachments) const { - parseOrnamentsSet(outAttachments); - parseAccidentalMarkSet(outAttachments.marks); -} + // Accidentals that cannot belong to the ornament before them are added after every ornament + // of this element, as standalone accidental marks. + std::vector standaloneAccidentals; -void OrnamentsFunctions::parseOrnamentsSet(api::NoteAttachmentData &outAttachments) const -{ for (const auto &group : myOrnaments.group()) { const auto &choiceObj = group.choice(); @@ -63,6 +101,7 @@ void OrnamentsFunctions::parseOrnamentsSet(api::NoteAttachmentData &outAttachmen if (choiceObj.kind() == core::OrnamentsGroupChoice::Kind::wavyLine) { parseWavyLine(choiceObj.asWavyLine(), outAttachments); + parseStandaloneAccidentals(group, standaloneAccidentals); continue; } @@ -73,22 +112,62 @@ void OrnamentsFunctions::parseOrnamentsSet(api::NoteAttachmentData &outAttachmen parseOrnament(choiceObj, markData); markData.tickTimePosition = myCursor.tickTimePosition; + if (ornamentsFunctionsCanOwnAccidentals(choiceObj.kind())) + { + markData.choice = parseOwnedAccidentals(group); + } + else + { + parseStandaloneAccidentals(group, standaloneAccidentals); + } + if (markData.markType != api::MarkType::unknownOrnament) { outAttachments.marks.emplace_back(std::move(markData)); } } + + for (auto &accidental : standaloneAccidentals) + { + outAttachments.marks.emplace_back(std::move(accidental)); + } } -void OrnamentsFunctions::parseAccidentalMarkSet(std::vector &outMarks) const +api::OrnamentMarkData OrnamentsFunctions::parseOwnedAccidentals(const core::OrnamentsGroup &group) const { - for (const auto &group : myOrnaments.group()) + api::OrnamentMarkData payload; + for (const auto &am : group.accidentalMark()) { - for (const auto &am : group.accidentalMark()) + AccidentalMarkFunctions funcs{am, myCursor}; + auto accidental = funcs.parseOrnamentAccidental(); + if (accidental.accidental == api::Accidental::none) { - AccidentalMarkFunctions funcs{am, myCursor}; - outMarks.emplace_back(funcs.parseAccidentalMark()); + auto location = measureLocation(myCursor); + location.staffIndex = myCursor.staffIndex; + myDiagnostics.report(api::Severity::error, api::DiagnosticCode::droppedData, std::move(location), + "an ornament's accidental-mark \"" + std::string{am.value().toString()} + + "\" is not supported and is not read"); + continue; } + payload.accidentals.emplace_back(std::move(accidental)); + } + return payload; +} + +// MusicXML lets an accidental follow a tremolo, an other-ornament or a wavy line, but none of +// these hold accidentals in the api, so the accidental is read as a standalone accidental mark. +void OrnamentsFunctions::parseStandaloneAccidentals(const core::OrnamentsGroup &group, + std::vector &outMarks) const +{ + for (const auto &am : group.accidentalMark()) + { + AccidentalMarkFunctions funcs{am, myCursor}; + outMarks.emplace_back(funcs.parseAccidentalMark()); + auto location = measureLocation(myCursor); + location.staffIndex = myCursor.staffIndex; + myDiagnostics.report(api::Severity::warning, api::DiagnosticCode::valueAdjusted, std::move(location), + "an accidental-mark after " + ornamentsFunctionsHostName(group.choice().kind()) + + " is read as a standalone accidental mark"); } } diff --git a/src/private/mx/impl/OrnamentsFunctions.h b/src/private/mx/impl/OrnamentsFunctions.h index 1a377d316..a97a6ba9b 100644 --- a/src/private/mx/impl/OrnamentsFunctions.h +++ b/src/private/mx/impl/OrnamentsFunctions.h @@ -15,6 +15,7 @@ namespace mx namespace core { class Ornaments; +class OrnamentsGroup; class OrnamentsGroupChoice; } // namespace core @@ -35,9 +36,9 @@ class OrnamentsFunctions DiagnosticsContext myDiagnostics; private: - void parseOrnamentsSet(api::NoteAttachmentData &outAttachments) const; - void parseAccidentalMarkSet(std::vector &outMarks) const; void parseOrnament(const core::OrnamentsGroupChoice &choiceObj, api::MarkData &outMark) const; + api::OrnamentMarkData parseOwnedAccidentals(const core::OrnamentsGroup &group) const; + void parseStandaloneAccidentals(const core::OrnamentsGroup &group, std::vector &outMarks) const; }; } // namespace impl } // namespace mx diff --git a/src/private/mxtest/api/MarkRoundTripTest.cpp b/src/private/mxtest/api/MarkRoundTripTest.cpp index d3204a3b5..121c2a0df 100644 --- a/src/private/mxtest/api/MarkRoundTripTest.cpp +++ b/src/private/mxtest/api/MarkRoundTripTest.cpp @@ -373,4 +373,161 @@ TEST(CaesuraVariants, MarkRoundTrip) T_END; +// Writes a one-note score holding the given mark and returns the XML and the diagnostics the +// write reported. +// Returns the diagnostics carrying exactly the given message. +inline std::vector markRoundTripDiagnosticsWithMessage(const Diagnostics &diagnostics, + const std::string &message) +{ + std::vector out; + for (const auto &diagnostic : diagnostics.all()) + { + if (diagnostic.message == message) + { + out.push_back(diagnostic); + } + } + return out; +} + +inline std::string markRoundTripWriteWithDiagnostics(const MarkData &inMarkData, Diagnostics &outDiagnostics) +{ + ScoreData score; + score.parts.emplace_back(); + auto ¬e = score.parts.back().measures.emplace_back().staves.emplace_back().voices[0].notes.emplace_back(); + note.noteAttachmentData.marks.push_back(inMarkData); + auto document = fromScore(score, outDiagnostics); + if (!document.ok()) + return {}; + std::stringstream ss; + std::move(document).value().writeToStream(ss); + return ss.str(); +} + +TEST(OrnamentAccidentals, MarkRoundTrip) +{ + OrnamentAccidental sharp; + sharp.accidental = Accidental::sharp; + sharp.positionData.placement = Placement::above; + sharp.positionData.isDefaultYSpecified = true; + sharp.positionData.defaultY = 12.5; + + MarkData trill{Placement::unspecified, MarkType::trillMark}; + trill.choice = OrnamentMarkData{{sharp}}; + const auto trillMarks = roundTripMarkData(trill); + REQUIRE(trillMarks.size() == 1); + CHECK(trillMarks.front().markType == MarkType::trillMark); + REQUIRE(trillMarks.front().choice.isOrnament()); + CHECK(trill.choice == trillMarks.front().choice); + + OrnamentAccidental natural; + natural.accidental = Accidental::natural; + natural.positionData.placement = Placement::below; + OrnamentAccidental flat; + flat.accidental = Accidental::flat; + + MarkData mordent{Placement::above, MarkType::mordent}; + mordent.choice = OrnamentMarkData{{natural, flat}}; + const auto mordentMarks = roundTripMarkData(mordent); + REQUIRE(mordentMarks.size() == 1); + CHECK(mordentMarks.front().markType == MarkType::mordent); + CHECK(Placement::above == mordentMarks.front().positionData.placement); + CHECK(mordent.choice == mordentMarks.front().choice); +} + +T_END; + +// An accidental left at none is not drawn, so it is not written, and the write reports the loss. +// The ornament's other accidentals are still written. +TEST(OrnamentAccidentalOfNoneIsDropped, MarkRoundTrip) +{ + OrnamentAccidental none; + OrnamentAccidental sharp; + sharp.accidental = Accidental::sharp; + + MarkData trill{Placement::unspecified, MarkType::trillMark}; + trill.choice = OrnamentMarkData{{none, sharp}}; + + Diagnostics diagnostics; + const auto xml = markRoundTripWriteWithDiagnostics(trill, diagnostics); + CHECK(xml.find(">sharp") != std::string::npos); + const auto dropped = + markRoundTripDiagnosticsWithMessage(diagnostics, "an ornament accidental of none is not written"); + REQUIRE(dropped.size() == 1); + CHECK(Severity::error == dropped.front().severity); + CHECK(DiagnosticCode::droppedData == dropped.front().code); + + const auto marks = roundTripMarkData(trill); + REQUIRE(marks.size() == 1); + CHECK(MarkDataChoice{OrnamentMarkData{{sharp}}} == marks.front().choice); +} + +T_END; + +// Ornament accidentals on a mark that is not written in have nowhere to go. +TEST(OrnamentAccidentalsOnANonOrnamentAreDropped, MarkRoundTrip) +{ + OrnamentAccidental sharp; + sharp.accidental = Accidental::sharp; + MarkData accent{Placement::unspecified, MarkType::accent}; + accent.choice = OrnamentMarkData{{sharp}}; + + Diagnostics diagnostics; + const auto xml = markRoundTripWriteWithDiagnostics(accent, diagnostics); + CHECK(xml.find(" content. +inline std::string noteDataTestOneNoteXml(const std::string ¬ations) { - const std::string xml = R"( + return R"( MusicXML Part @@ -2119,41 +2117,154 @@ TEST(accidentalMarkOnAnOrnamentIsWrittenAtTheNotationsLevel, NoteData) 1 quarter - - - sharp - +)" + notations + + R"( )"; +} + +inline const std::vector ¬eDataTestFirstNoteMarks(const ScoreData &score) +{ + return score.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.marks; +} + +// issue #443: an accidental-mark inside belongs to the ornament before it, so the sharp +// over a trill is read as part of the trill and written back inside after it. +TEST(accidentalMarkOnAnOrnamentBelongsToTheOrnament, NoteData) +{ + const auto xml = noteDataTestOneNoteXml(R"( + + sharp + )"); const auto score = fromXml(xml); - const auto &marks = score.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.marks; - REQUIRE(2 == marks.size()); + const auto &marks = noteDataTestFirstNoteMarks(score); + REQUIRE(1 == marks.size()); CHECK(MarkType::trillMark == marks.at(0).markType); - CHECK(MarkType::accidentalMarkSharp == marks.at(1).markType); + REQUIRE(marks.at(0).choice.isOrnament()); + CHECK(MarkDataChoice::Kind::ornament == marks.at(0).choice.kind()); + const auto accidentals = marks.at(0).choice.ornament().accidentals; + REQUIRE(1 == accidentals.size()); + CHECK(Accidental::sharp == accidentals.at(0).accidental); + CHECK(Placement::above == accidentals.at(0).positionData.placement); const auto written = toXml(score); - CHECK(written.find(""); + const auto trillPos = written.find(""); + REQUIRE(ornamentsPos != std::string::npos); + REQUIRE(trillPos != std::string::npos); + REQUIRE(accidentalPos != std::string::npos); + REQUIRE(ornamentsEndPos != std::string::npos); + CHECK(ornamentsPos < trillPos); + CHECK(trillPos < accidentalPos); + CHECK(accidentalPos < ornamentsEndPos); - // both marks are still there after the source's ornament attachment is normalized away const auto reread = fromXml(written); - const auto &rereadMarks = - reread.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0).noteAttachmentData.marks; - REQUIRE(2 == rereadMarks.size()); - bool foundTrill = false; - bool foundSharp = false; - for (const auto &mark : rereadMarks) + CHECK(marks == noteDataTestFirstNoteMarks(reread)); +} + +T_END; + +// A turn may carry one accidental above it and another below it; both stay with the turn, in +// order. +TEST(turnKeepsItsAccidentalsInOrder, NoteData) +{ + const auto xml = noteDataTestOneNoteXml(R"( + + sharp + three-quarters-flat + )"); + + const auto score = fromXml(xml); + const auto &marks = noteDataTestFirstNoteMarks(score); + REQUIRE(1 == marks.size()); + CHECK(MarkType::turn == marks.at(0).markType); + const auto accidentals = marks.at(0).choice.ornament().accidentals; + REQUIRE(2 == accidentals.size()); + CHECK(Accidental::sharp == accidentals.at(0).accidental); + CHECK(Placement::above == accidentals.at(0).positionData.placement); + CHECK(Accidental::threeQuartersFlat == accidentals.at(1).accidental); + CHECK(Placement::below == accidentals.at(1).positionData.placement); + + const auto written = toXml(score); + const auto sharpPos = written.find(">sharp<"); + const auto flatPos = written.find(">three-quarters-flat<"); + REQUIRE(sharpPos != std::string::npos); + REQUIRE(flatPos != std::string::npos); + CHECK(written.find("")); + CHECK(marks == noteDataTestFirstNoteMarks(fromXml(written))); +} + +T_END; + +// Each ornament keeps its own accidentals when one element holds several ornaments. +TEST(eachOrnamentKeepsItsOwnAccidentals, NoteData) +{ + const auto xml = noteDataTestOneNoteXml(R"( + + flat + + natural + )"); + + const auto score = fromXml(xml); + const auto &marks = noteDataTestFirstNoteMarks(score); + REQUIRE(2 == marks.size()); + CHECK(MarkType::trillMark == marks.at(0).markType); + REQUIRE(1 == marks.at(0).choice.ornament().accidentals.size()); + CHECK(Accidental::flat == marks.at(0).choice.ornament().accidentals.at(0).accidental); + CHECK(MarkType::mordent == marks.at(1).markType); + REQUIRE(1 == marks.at(1).choice.ornament().accidentals.size()); + CHECK(Accidental::natural == marks.at(1).choice.ornament().accidentals.at(0).accidental); + CHECK(marks == noteDataTestFirstNoteMarks(fromXml(toXml(score)))); +} + +T_END; + +// A tremolo cannot hold accidentals, so an accidental after one is read as a standalone accidental +// mark, after the ornament marks, and the read says so. +TEST(accidentalMarkAfterTremoloIsStandalone, NoteData) +{ + const auto xml = noteDataTestOneNoteXml(R"( + 3 + flat + + )"); + + std::istringstream iss{xml}; + auto document = MusicXml::fromStream(iss); + REQUIRE(document.ok()); + Diagnostics diagnostics; + const auto scoreResult = getScore(document.value(), diagnostics); + REQUIRE(scoreResult.ok()); + const auto &marks = noteDataTestFirstNoteMarks(scoreResult.value()); + REQUIRE(3 == marks.size()); + CHECK(MarkType::tremoloSingleThree == marks.at(0).markType); + CHECK(MarkType::trillMark == marks.at(1).markType); + CHECK(marks.at(1).choice.isNone()); + CHECK(MarkType::accidentalMarkFlat == marks.at(2).markType); + CHECK(Placement::below == marks.at(2).positionData.placement); + + const std::string message = "an accidental-mark after a tremolo is read as a standalone accidental mark"; + std::vector fallbacks; + for (const auto &diagnostic : diagnostics.all()) { - foundTrill = foundTrill || (mark.markType == MarkType::trillMark); - foundSharp = foundSharp || (mark.markType == MarkType::accidentalMarkSharp); + if (diagnostic.message == message) + { + fallbacks.push_back(diagnostic); + } } - CHECK(foundTrill); - CHECK(foundSharp); + REQUIRE(1 == fallbacks.size()); + CHECK(Severity::warning == fallbacks.front().severity); + CHECK(DiagnosticCode::valueAdjusted == fallbacks.front().code); } T_END; diff --git a/src/private/mxtest/api/WavyLineApiTest.cpp b/src/private/mxtest/api/WavyLineApiTest.cpp index 28eac8ea5..3cdeab956 100644 --- a/src/private/mxtest/api/WavyLineApiTest.cpp +++ b/src/private/mxtest/api/WavyLineApiTest.cpp @@ -230,6 +230,46 @@ TEST(writesInsideOrnaments, WavyLine) T_END +// A trill's accidental stays with the trill: it is written right after the trill-mark, and the +// wavy line that starts on the same note follows. +TEST(trillAccidentalPrecedesWavyLineStart, WavyLine) +{ + using namespace wavyLineApiTest; + ScoreData score; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + auto &staff = measure.staves.back(); + auto &voice = staff.voices[0]; + + voice.notes.push_back(makeGraceNote(Step::c, 4)); + auto ¬e = voice.notes.back(); + MarkData trill{Placement::above, MarkType::trillMark}; + OrnamentAccidental sharp; + sharp.accidental = Accidental::sharp; + trill.choice = OrnamentMarkData{{sharp}}; + note.noteAttachmentData.marks.push_back(trill); + note.noteAttachmentData.wavyLineStarts.push_back(WavyLineStart{}); + + const auto xml = toXml(score); + mxpugi::xml_document xdoc; + REQUIRE(xdoc.load_string(xml.c_str())); + const auto ornaments = + xdoc.document_element().child("part").child("measure").child("note").child("notations").child("ornaments"); + REQUIRE(ornaments); + std::vector names; + for (const auto &child : ornaments.children()) + { + names.emplace_back(child.name()); + } + const std::vector expected{"trill-mark", "accidental-mark", "wavy-line"}; + CHECK(expected == names); +} + +T_END + // Requirement 9: the synthetic fixtures parse and surface pairing plus the shared attributes. TEST(readsSyntheticWavyLineFixture, WavyLine) { From 40fb54c7e7b65a87e6880ae6a8b7cc359e2061d8 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 6 Nov 2026 01:25:36 +0100 Subject: [PATCH 2/3] fix: do not write ornament accidentals after a tremolo or other-ornament A tremolo or other-ornament mark given an OrnamentMarkData choice wrote its accidentals inside after the ornament, with no diagnostic, but the reader reads an accidental there as a standalone accidental mark, so the score did not read back as it was written. The writer now asks the same question the reader does (which ornaments can own accidentals) and drops them on any other ornament with an error diagnostic, as it already did for a mark that is not an ornament. The OrnamentMarkData comment now says which marks can hold accidentals and what happens to an accidental written with a tremolo, an other-ornament or a wavy line. A test checks that every ornament that can own accidentals writes and reads them back. --- src/include/mx/api/MarkDataChoice.h | 15 +++--- src/private/mx/impl/NotationsWriter.cpp | 14 ++++- src/private/mx/impl/OrnamentsFunctions.cpp | 4 +- src/private/mx/impl/OrnamentsFunctions.h | 5 ++ src/private/mxtest/api/MarkRoundTripTest.cpp | 55 ++++++++++++++++++-- 5 files changed, 80 insertions(+), 13 deletions(-) diff --git a/src/include/mx/api/MarkDataChoice.h b/src/include/mx/api/MarkDataChoice.h index de86ab966..cffd1b818 100644 --- a/src/include/mx/api/MarkDataChoice.h +++ b/src/include/mx/api/MarkDataChoice.h @@ -202,11 +202,12 @@ MXAPI_NOT_EQUALS_AND_VECTORS(HarmonicMarkData); // accidentals above and below a turn that alter its upper and lower notes. struct OrnamentAccidental { - // The accidental to show. An entry left at none is not drawn. + // The accidental to show. An entry left at none is not written. Accidental accidental = Accidental::none; // Where the accidental sits, measured from the ornament rather than from the note. Placement - // above puts the accidental over the ornament, below puts it under. + // above puts the accidental over the ornament, below puts it under. The horizontal and vertical + // alignment fields are not used. PositionData positionData; }; @@ -216,12 +217,14 @@ MXAPI_EQUALS_MEMBER(positionData) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(OrnamentAccidental); -// Payload for an ornament that has accidentals: a trill, a turn of any kind, a shake, a mordent, an -// inverted mordent, a schleifer or a haydn. A trill whose upper note is sharpened is one +// Payload for an ornament that has accidentals. A trill whose upper note is sharpened is one // MarkType::trillMark mark holding an OrnamentAccidental with Accidental::sharp. // -// An accidental that stands above or below the note on its own, not tied to an ornament, is a -// MarkType::accidentalMark* mark of its own instead. +// These marks can hold accidentals: a trill, a turn of any kind, a shake, a mordent, an inverted +// mordent, a schleifer and a haydn. A tremolo or an other-ornament cannot, because its choice +// already carries TremoloMarkData or OtherMarkData; neither can a wavy line, which is not a mark. +// An accidental written with one of those, or one that stands above or below the note on its own, +// is a MarkType::accidentalMark* mark of its own. On any other mark this payload is not written. struct OrnamentMarkData { // The ornament's accidentals, usually one. A turn may have one above it and one below it. diff --git a/src/private/mx/impl/NotationsWriter.cpp b/src/private/mx/impl/NotationsWriter.cpp index 313a5e546..10483ac08 100644 --- a/src/private/mx/impl/NotationsWriter.cpp +++ b/src/private/mx/impl/NotationsWriter.cpp @@ -67,6 +67,7 @@ #include "mx/impl/GlissandoFunctions.h" #include "mx/impl/IdFunctions.h" #include "mx/impl/MarkDataFunctions.h" +#include "mx/impl/OrnamentsFunctions.h" #include "mx/impl/PositionFunctions.h" #include "mx/impl/ScoreWriter.h" #include "mx/impl/WavyLineFunctions.h" @@ -387,7 +388,7 @@ core::Notations NotationsWriter::getNotations() const { myScoreWriter.getDiagnostics().report( api::Severity::error, api::DiagnosticCode::droppedData, cursorLocation(myCursor), - "ornament accidentals on a mark that is not an ornament are not written"); + "ornament accidentals on a mark that cannot hold them are not written"); } if (isMarkAccidentalMark(mark.markType)) { @@ -1181,7 +1182,8 @@ void NotationsWriter::addOrnament(const api::MarkData &mark, core::Ornaments &ou } // An ornament's accidentals go in its own group, right after it, which is where MusicXML says an -// accidental belongs to the ornament before it. +// accidental belongs to the ornament before it. Only the ornaments the reader lets own accidentals +// may write them; anything else would come back as standalone accidental marks. void NotationsWriter::addOrnamentAccidentals(const api::MarkData &mark, core::OrnamentsGroup &outGroup) const { if (!mark.choice.isOrnament()) @@ -1189,6 +1191,14 @@ void NotationsWriter::addOrnamentAccidentals(const api::MarkData &mark, core::Or return; } + if (!ornamentsFunctionsCanOwnAccidentals(outGroup.choice().kind())) + { + myScoreWriter.getDiagnostics().report(api::Severity::error, api::DiagnosticCode::droppedData, + cursorLocation(myCursor), + "ornament accidentals on a mark that cannot hold them are not written"); + return; + } + const auto payload = mark.choice.ornament(); for (const auto &accidental : payload.accidentals) { diff --git a/src/private/mx/impl/OrnamentsFunctions.cpp b/src/private/mx/impl/OrnamentsFunctions.cpp index 6737190d7..8da487c85 100644 --- a/src/private/mx/impl/OrnamentsFunctions.cpp +++ b/src/private/mx/impl/OrnamentsFunctions.cpp @@ -40,8 +40,8 @@ void ornamentsFunctionsParseMordentSpecificAttributes(const core::Mordent &m, ap } } -// The ornaments that hold the accidentals written after them: a trill, a turn of any kind, a -// shake, a mordent, an inverted mordent, a schleifer or a haydn. +// A trill, a turn of any kind, a shake, a mordent, an inverted mordent, a schleifer or a haydn. A +// tremolo and an other-ornament already carry their own payload, and a wavy line is not a mark. bool ornamentsFunctionsCanOwnAccidentals(core::OrnamentsGroupChoice::Kind kind) { switch (kind) diff --git a/src/private/mx/impl/OrnamentsFunctions.h b/src/private/mx/impl/OrnamentsFunctions.h index a97a6ba9b..4e64c862c 100644 --- a/src/private/mx/impl/OrnamentsFunctions.h +++ b/src/private/mx/impl/OrnamentsFunctions.h @@ -21,6 +21,11 @@ class OrnamentsGroupChoice; namespace impl { +// Whether an ornament of this kind holds the accidental marks that follow it inside +// (api::OrnamentMarkData). The reader and the writer share this so that what one keeps on an +// ornament the other writes back there. +bool ornamentsFunctionsCanOwnAccidentals(core::OrnamentsGroupChoice::Kind kind); + class OrnamentsFunctions { public: diff --git a/src/private/mxtest/api/MarkRoundTripTest.cpp b/src/private/mxtest/api/MarkRoundTripTest.cpp index 121c2a0df..fe033edb5 100644 --- a/src/private/mxtest/api/MarkRoundTripTest.cpp +++ b/src/private/mxtest/api/MarkRoundTripTest.cpp @@ -373,8 +373,6 @@ TEST(CaesuraVariants, MarkRoundTrip) T_END; -// Writes a one-note score holding the given mark and returns the XML and the diagnostics the -// write reported. // Returns the diagnostics carrying exactly the given message. inline std::vector markRoundTripDiagnosticsWithMessage(const Diagnostics &diagnostics, const std::string &message) @@ -390,6 +388,8 @@ inline std::vector markRoundTripDiagnosticsWithMessage(const Diagnos return out; } +// Writes a one-note score holding the given mark and returns the XML, collecting the diagnostics +// the write reported. inline std::string markRoundTripWriteWithDiagnostics(const MarkData &inMarkData, Diagnostics &outDiagnostics) { ScoreData score; @@ -477,7 +477,7 @@ TEST(OrnamentAccidentalsOnANonOrnamentAreDropped, MarkRoundTrip) CHECK(xml.find("") != std::string::npos); + CHECK(xml.find(" Date: Fri, 6 Nov 2026 01:25:36 +0100 Subject: [PATCH 3/3] test: cover the remaining ornament-accidental read paths Covers an accidental after an other-ornament and after a wavy line (read as standalone marks, one warning each), an "other" accidental on a turn (dropped with an error, the turn's other accidental kept), and a note with both a standalone accidental and a trill that owns one. --- src/private/mxtest/api/NoteDataTest.cpp | 137 +++++++++++++++++++++--- 1 file changed, 122 insertions(+), 15 deletions(-) diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index 5b6702030..fa34459f7 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -2229,6 +2229,34 @@ TEST(eachOrnamentKeepsItsOwnAccidentals, NoteData) T_END; +// Reads the XML and returns the score, collecting the diagnostics the read reported. +inline ScoreData noteDataTestReadWithDiagnostics(const std::string &xml, Diagnostics &outDiagnostics) +{ + std::istringstream iss{xml}; + auto document = MusicXml::fromStream(iss); + if (!document.ok()) + return {}; + auto scoreResult = getScore(document.value(), outDiagnostics); + if (!scoreResult.ok()) + return {}; + return scoreResult.value(); +} + +// Returns the diagnostics carrying exactly the given message. +inline std::vector noteDataTestDiagnosticsWithMessage(const Diagnostics &diagnostics, + const std::string &message) +{ + std::vector out; + for (const auto &diagnostic : diagnostics.all()) + { + if (diagnostic.message == message) + { + out.push_back(diagnostic); + } + } + return out; +} + // A tremolo cannot hold accidentals, so an accidental after one is read as a standalone accidental // mark, after the ornament marks, and the read says so. TEST(accidentalMarkAfterTremoloIsStandalone, NoteData) @@ -2239,13 +2267,9 @@ TEST(accidentalMarkAfterTremoloIsStandalone, NoteData) )"); - std::istringstream iss{xml}; - auto document = MusicXml::fromStream(iss); - REQUIRE(document.ok()); Diagnostics diagnostics; - const auto scoreResult = getScore(document.value(), diagnostics); - REQUIRE(scoreResult.ok()); - const auto &marks = noteDataTestFirstNoteMarks(scoreResult.value()); + const auto score = noteDataTestReadWithDiagnostics(xml, diagnostics); + const auto &marks = noteDataTestFirstNoteMarks(score); REQUIRE(3 == marks.size()); CHECK(MarkType::tremoloSingleThree == marks.at(0).markType); CHECK(MarkType::trillMark == marks.at(1).markType); @@ -2253,15 +2277,8 @@ TEST(accidentalMarkAfterTremoloIsStandalone, NoteData) CHECK(MarkType::accidentalMarkFlat == marks.at(2).markType); CHECK(Placement::below == marks.at(2).positionData.placement); - const std::string message = "an accidental-mark after a tremolo is read as a standalone accidental mark"; - std::vector fallbacks; - for (const auto &diagnostic : diagnostics.all()) - { - if (diagnostic.message == message) - { - fallbacks.push_back(diagnostic); - } - } + const auto fallbacks = noteDataTestDiagnosticsWithMessage( + diagnostics, "an accidental-mark after a tremolo is read as a standalone accidental mark"); REQUIRE(1 == fallbacks.size()); CHECK(Severity::warning == fallbacks.front().severity); CHECK(DiagnosticCode::valueAdjusted == fallbacks.front().code); @@ -2269,4 +2286,94 @@ TEST(accidentalMarkAfterTremoloIsStandalone, NoteData) T_END; +// An other-ornament and a wavy line cannot hold accidentals either; theirs are read as standalone +// accidental marks too, one diagnostic each. +TEST(accidentalMarkAfterOtherOrnamentOrWavyLineIsStandalone, NoteData) +{ + const auto xml = noteDataTestOneNoteXml(R"( + x + sharp + + natural + )"); + + Diagnostics diagnostics; + const auto score = noteDataTestReadWithDiagnostics(xml, diagnostics); + const auto ¬e = score.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0); + const auto &marks = note.noteAttachmentData.marks; + REQUIRE(3 == marks.size()); + CHECK(MarkType::otherOrnament == marks.at(0).markType); + CHECK(MarkType::accidentalMarkSharp == marks.at(1).markType); + CHECK(MarkType::accidentalMarkNatural == marks.at(2).markType); + CHECK(1 == note.noteAttachmentData.wavyLineStarts.size()); + + CHECK(1 == noteDataTestDiagnosticsWithMessage( + diagnostics, "an accidental-mark after an other-ornament is read as a standalone accidental mark") + .size()); + CHECK(1 == noteDataTestDiagnosticsWithMessage( + diagnostics, "an accidental-mark after a wavy-line is read as a standalone accidental mark") + .size()); +} + +T_END; + +// The api has no accidental for MusicXML's "other" value, so an ornament's "other" accidental is +// left out, the read says so, and the ornament's other accidentals are kept. +TEST(otherAccidentalOnAnOrnamentIsDropped, NoteData) +{ + const auto xml = noteDataTestOneNoteXml(R"( + + other + flat + )"); + + Diagnostics diagnostics; + const auto score = noteDataTestReadWithDiagnostics(xml, diagnostics); + const auto &marks = noteDataTestFirstNoteMarks(score); + REQUIRE(1 == marks.size()); + CHECK(MarkType::turn == marks.at(0).markType); + const auto accidentals = marks.at(0).choice.ornament().accidentals; + REQUIRE(1 == accidentals.size()); + CHECK(Accidental::flat == accidentals.at(0).accidental); + + const auto dropped = noteDataTestDiagnosticsWithMessage( + diagnostics, "an ornament's accidental-mark \"other\" is not supported and is not read"); + REQUIRE(1 == dropped.size()); + CHECK(Severity::error == dropped.front().severity); + CHECK(DiagnosticCode::droppedData == dropped.front().code); +} + +T_END; + +// A note may have both an accidental of its own in and an ornament with an accidental; +// each keeps its place on write. +TEST(standaloneAndOrnamentAccidentalsOnOneNote, NoteData) +{ + const auto xml = noteDataTestOneNoteXml(R"( flat + + + sharp + )"); + + const auto score = fromXml(xml); + const auto &marks = noteDataTestFirstNoteMarks(score); + REQUIRE(2 == marks.size()); + CHECK(MarkType::accidentalMarkFlat == marks.at(0).markType); + CHECK(MarkType::trillMark == marks.at(1).markType); + REQUIRE(1 == marks.at(1).choice.ornament().accidentals.size()); + + const auto written = toXml(score); + const auto flatPos = written.find(">flat<"); + const auto ornamentsPos = written.find(""); + const auto sharpPos = written.find(">sharp<"); + REQUIRE(flatPos != std::string::npos); + REQUIRE(ornamentsPos != std::string::npos); + REQUIRE(sharpPos != std::string::npos); + CHECK(flatPos < ornamentsPos); + CHECK(ornamentsPos < sharpPos); + CHECK(marks == noteDataTestFirstNoteMarks(fromXml(written))); +} + +T_END; + #endif