From 0418f9e0e5f0f4f863c9f0a7e60650ecc95e33fd Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 6 Nov 2026 01:53:14 +0100 Subject: [PATCH 1/3] feat: accidental marks keep how they are drawn An accidental-mark kept only its value and position. Its parentheses, bracket, size, SMuFL glyph, id, font and color were dropped on read and never written. A standalone accidental mark now carries these in a new AccidentalMarkData alternative of MarkDataChoice, with its font and color in MarkData::printData. A payload with nothing set is none, so a mark built from its MarkType alone still compares equal after a write and a read. An accidental that belongs to an ornament (OrnamentAccidental) gains the same fields directly, plus PrintData. The new SymbolSize enum in ApiCommon.h carries MusicXML's size attribute. The reader and the writer each fill both shapes through one shared helper. A smufl name that is not a SMuFL accidental glyph is not written and is reported as dropped data. Pins synthetic/accidental-mark.3.0.xml and accidental-mark.3.1.xml in the round-trip baseline. --- data/api.features.xml | 2 +- src/include/mx/api/ApiCommon.h | 19 ++ src/include/mx/api/MarkDataChoice.h | 77 ++++++- src/private/mx/api/MarkDataChoice.cpp | 34 +++ .../mx/impl/AccidentalMarkFunctions.cpp | 36 +++- src/private/mx/impl/Converter.cpp | 17 ++ src/private/mx/impl/Converter.h | 4 + src/private/mx/impl/NotationsWriter.cpp | 50 ++++- src/private/mx/impl/NotationsWriter.h | 3 + src/private/mxtest/api/MarkRoundTripTest.cpp | 193 ++++++++++++++++++ src/private/mxtest/api/NoteDataTest.cpp | 55 +++++ src/private/mxtest/api/roundtrip-baseline.txt | 7 + 12 files changed, 489 insertions(+), 8 deletions(-) diff --git a/data/api.features.xml b/data/api.features.xml index ebe8feb3c..e6759afd3 100644 --- a/data/api.features.xml +++ b/data/api.features.xml @@ -28,7 +28,7 @@ other - 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. + 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). Every attribute is kept: parentheses, bracket, size (api::SymbolSize), smufl and id in api::AccidentalMarkData (MarkDataChoice) or directly on api::OrnamentAccidental; font and color in PrintData; position and placement in PositionData. Accidental as text (key/ornament context) (suspected). diff --git a/src/include/mx/api/ApiCommon.h b/src/include/mx/api/ApiCommon.h index 7c4cbc8b8..4dd497cb6 100644 --- a/src/include/mx/api/ApiCommon.h +++ b/src/include/mx/api/ApiCommon.h @@ -110,6 +110,25 @@ enum class Enclosure decagon, none }; + +// How large a symbol is drawn compared with ordinary notation, MusicXML's size attribute. +enum class SymbolSize +{ + // Leave the size unstated, so the application draws the symbol at its usual size. + unspecified, + + // The normal size, stated explicitly. + full, + + // The smaller size used for cue notes. + cue, + + // The smaller size used for grace notes in a cue passage. + graceCue, + + // Larger than normal. + large +}; } // namespace api } // namespace mx diff --git a/src/include/mx/api/MarkDataChoice.h b/src/include/mx/api/MarkDataChoice.h index cffd1b818..947862155 100644 --- a/src/include/mx/api/MarkDataChoice.h +++ b/src/include/mx/api/MarkDataChoice.h @@ -9,6 +9,7 @@ #include "mx/api/Id.h" #include "mx/api/PitchData.h" #include "mx/api/PositionData.h" +#include "mx/api/PrintData.h" #include #include @@ -198,6 +199,41 @@ MXAPI_EQUALS_MEMBER(pitch) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(HarmonicMarkData); +// Payload for the accidental mark types (MarkType::accidentalMarkSharp and the rest): how the +// accidental is drawn. Its font and color are in MarkData::printData. Leaving every field at its +// default is the same as having no payload. +struct AccidentalMarkData +{ + // yes: the accidental is drawn in parentheses. Parentheses and square brackets are common ways + // to show an editorial accidental. Leave it unspecified to let the application decide. + Bool parentheses = Bool::unspecified; + + // yes: the accidental is drawn in square brackets. Leave it unspecified to let the application + // decide. + Bool bracket = Bool::unspecified; + + // How large the accidental is drawn. + SymbolSize size = SymbolSize::unspecified; + + // The SMuFL glyph to draw when a specific one is wanted, for example "accidentalSharp". It must + // be the name of a SMuFL accidental, one that starts with acc, medRenFla, medRenNatura, + // medRenShar or kievanAccidental; another name is not written. Leave it absent to draw the + // accidental the MarkType names. + std::optional smufl; + + // An identifier for this accidental, unique in the document (see Id.h). + std::optional id; +}; + +MXAPI_EQUALS_BEGIN(AccidentalMarkData) +MXAPI_EQUALS_MEMBER(parentheses) +MXAPI_EQUALS_MEMBER(bracket) +MXAPI_EQUALS_MEMBER(size) +MXAPI_EQUALS_MEMBER(smufl) +MXAPI_EQUALS_MEMBER(id) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(AccidentalMarkData); + // 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 @@ -209,11 +245,35 @@ struct OrnamentAccidental // above puts the accidental over the ornament, below puts it under. The horizontal and vertical // alignment fields are not used. PositionData positionData; + + // The accidental's font and color. printObject is not used. + PrintData printData; + + // As AccidentalMarkData::parentheses. + Bool parentheses = Bool::unspecified; + + // As AccidentalMarkData::bracket. + Bool bracket = Bool::unspecified; + + // As AccidentalMarkData::size. + SymbolSize size = SymbolSize::unspecified; + + // As AccidentalMarkData::smufl. Leave it absent to draw the accidental named by accidental. + std::optional smufl; + + // As AccidentalMarkData::id. + std::optional id; }; MXAPI_EQUALS_BEGIN(OrnamentAccidental) MXAPI_EQUALS_MEMBER(accidental) MXAPI_EQUALS_MEMBER(positionData) +MXAPI_EQUALS_MEMBER(printData) +MXAPI_EQUALS_MEMBER(parentheses) +MXAPI_EQUALS_MEMBER(bracket) +MXAPI_EQUALS_MEMBER(size) +MXAPI_EQUALS_MEMBER(smufl) +MXAPI_EQUALS_MEMBER(id) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(OrnamentAccidental); @@ -264,7 +324,8 @@ class MarkDataChoice compoundDynamics, otherNotation, harmonic, - ornament + ornament, + accidentalMark }; MarkDataChoice(); @@ -292,6 +353,10 @@ class MarkDataChoice // an ornament without accidentals needs no payload. MarkDataChoice(OrnamentMarkData value); + // Builds an accidental mark payload, unless every field is left at its default, in which case + // the result is none. + MarkDataChoice(AccidentalMarkData value); + Kind kind() const; bool isNone() const; bool isTremolo() const; @@ -303,6 +368,7 @@ class MarkDataChoice bool isOtherNotation() const; bool isHarmonic() const; bool isOrnament() const; + bool isAccidentalMark() const; // Returns a copy of the internally held TremoloMarkData. // @@ -346,11 +412,18 @@ class MarkDataChoice // OrnamentMarkData, with no accidentals, is returned. const OrnamentMarkData ornament() const; + // Returns a copy of the internally held AccidentalMarkData. + // + // Check isAccidentalMark() first. If this is not an accidental mark payload, a default + // constructed AccidentalMarkData is returned. + const AccidentalMarkData accidentalMark() const; + bool operator==(const MarkDataChoice &other) const; private: std::variant + StandardDynamic, CompoundDynamicsData, OtherNotationMarkData, HarmonicMarkData, OrnamentMarkData, + AccidentalMarkData> myValue; }; diff --git a/src/private/mx/api/MarkDataChoice.cpp b/src/private/mx/api/MarkDataChoice.cpp index dde5fd8d9..62ac33549 100644 --- a/src/private/mx/api/MarkDataChoice.cpp +++ b/src/private/mx/api/MarkDataChoice.cpp @@ -72,6 +72,22 @@ MarkDataChoice::MarkDataChoice(OrnamentMarkData value) } } +// An accidental mark with nothing set is stored as none, so a plain accidental mark compares equal +// however it was built. +MarkDataChoice::MarkDataChoice(AccidentalMarkData value) +{ + const bool isEmpty = value.parentheses == Bool::unspecified && value.bracket == Bool::unspecified && + value.size == SymbolSize::unspecified && !value.smufl.has_value() && !value.id.has_value(); + if (isEmpty) + { + myValue = std::monostate{}; + } + else + { + myValue = std::move(value); + } +} + MarkDataChoice::Kind MarkDataChoice::kind() const { if (std::holds_alternative(myValue)) @@ -110,6 +126,10 @@ MarkDataChoice::Kind MarkDataChoice::kind() const { return Kind::ornament; } + if (std::holds_alternative(myValue)) + { + return Kind::accidentalMark; + } return Kind::none; } @@ -244,6 +264,20 @@ const OrnamentMarkData MarkDataChoice::ornament() const return OrnamentMarkData{}; } +bool MarkDataChoice::isAccidentalMark() const +{ + return std::holds_alternative(myValue); +} + +const AccidentalMarkData MarkDataChoice::accidentalMark() const +{ + if (const auto *value = std::get_if(&myValue)) + { + return *value; + } + return AccidentalMarkData{}; +} + 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 a72e6e01d..5cc12a8cd 100644 --- a/src/private/mx/impl/AccidentalMarkFunctions.cpp +++ b/src/private/mx/impl/AccidentalMarkFunctions.cpp @@ -4,6 +4,7 @@ #include "mx/impl/AccidentalMarkFunctions.h" #include "mx/core/generated/AccidentalMark.h" +#include "mx/impl/IdFunctions.h" #include "mx/impl/MarkDataFunctions.h" namespace mx @@ -11,6 +12,35 @@ namespace mx namespace impl { +// Reads how an accidental-mark is drawn into either api shape that holds it: a standalone mark's +// AccidentalMarkData, whose position and print data live on the MarkData, or an OrnamentAccidental, +// which has the same fields itself. +template +void accidentalMarkFunctionsParseAttributes(const core::AccidentalMark &inAccidentalMark, ACCIDENTAL_FIELDS &outFields, + api::PositionData &outPositionData, api::PrintData &outPrintData) +{ + Converter converter; + outPositionData = impl::getPositionData(inAccidentalMark); + outPrintData = impl::getPrintData(inAccidentalMark); + if (inAccidentalMark.parentheses().has_value()) + { + outFields.parentheses = converter.convert(*inAccidentalMark.parentheses()); + } + if (inAccidentalMark.bracket().has_value()) + { + outFields.bracket = converter.convert(*inAccidentalMark.bracket()); + } + if (inAccidentalMark.size().has_value()) + { + outFields.size = converter.convert(*inAccidentalMark.size()); + } + if (inAccidentalMark.smufl().has_value()) + { + outFields.smufl = inAccidentalMark.smufl()->toString(); + } + outFields.id = impl::getId(inAccidentalMark); +} + AccidentalMarkFunctions::AccidentalMarkFunctions(const core::AccidentalMark &inAccidentalMark, impl::Cursor inCursor) : myAccidentalMark{inAccidentalMark}, myCursor{inCursor} { @@ -26,7 +56,9 @@ api::MarkData AccidentalMarkFunctions::parseAccidentalMark() const markData.tickTimePosition = myCursor.tickTimePosition; markData.name = std::string{accidentalValue.toString()}; - markData.positionData = impl::getPositionData(myAccidentalMark); + api::AccidentalMarkData payload; + accidentalMarkFunctionsParseAttributes(myAccidentalMark, payload, markData.positionData, markData.printData); + markData.choice = payload; return markData; } @@ -36,7 +68,7 @@ api::OrnamentAccidental AccidentalMarkFunctions::parseOrnamentAccidental() const Converter converter; api::OrnamentAccidental accidental; accidental.accidental = converter.convert(myAccidentalMark.value()); - accidental.positionData = impl::getPositionData(myAccidentalMark); + accidentalMarkFunctionsParseAttributes(myAccidentalMark, accidental, accidental.positionData, accidental.printData); return accidental; } } // namespace impl diff --git a/src/private/mx/impl/Converter.cpp b/src/private/mx/impl/Converter.cpp index 52647238c..3a9bfe310 100644 --- a/src/private/mx/impl/Converter.cpp +++ b/src/private/mx/impl/Converter.cpp @@ -1629,6 +1629,13 @@ const Converter::EnumMap Converter::enclos {core::EnclosureShape::none(), api::Enclosure::none}, }; +const Converter::EnumMap Converter::symbolSizeMap = { + {core::SymbolSize::full(), api::SymbolSize::full}, + {core::SymbolSize::cue(), api::SymbolSize::cue}, + {core::SymbolSize::graceCue(), api::SymbolSize::graceCue}, + {core::SymbolSize::large(), api::SymbolSize::large}, +}; + api::Step Converter::convert(core::Step inStep) const { return findApiItem(stepMap, api::Step::c, inStep); @@ -2198,6 +2205,16 @@ core::EnclosureShape Converter::convert(api::Enclosure value) const return findCoreItem(enclosureMap, core::EnclosureShape::none(), value); } +api::SymbolSize Converter::convert(core::SymbolSize value) const +{ + return findApiItem(symbolSizeMap, api::SymbolSize::unspecified, value); +} + +core::SymbolSize Converter::convert(api::SymbolSize value) const +{ + return findCoreItem(symbolSizeMap, core::SymbolSize::full(), value); +} + double Converter::convertToAlter(int semitones, double cents) { double alter = 0.0; diff --git a/src/private/mx/impl/Converter.h b/src/private/mx/impl/Converter.h index 280533806..6afd084a1 100644 --- a/src/private/mx/impl/Converter.h +++ b/src/private/mx/impl/Converter.h @@ -56,6 +56,7 @@ #include "mx/core/generated/StickLocation.h" #include "mx/core/generated/StickMaterial.h" #include "mx/core/generated/StickType.h" +#include "mx/core/generated/SymbolSize.h" #include "mx/core/generated/SystemRelation.h" #include "mx/core/generated/SystemRelationNumber.h" #include "mx/core/generated/TechnicalChoice.h" @@ -259,6 +260,8 @@ class Converter core::TipDirection convert(api::TipDirection value) const; api::Enclosure convert(core::EnclosureShape value) const; core::EnclosureShape convert(api::Enclosure value) const; + api::SymbolSize convert(core::SymbolSize value) const; + core::SymbolSize convert(api::SymbolSize value) const; static double convertToAlter(int semitones, double cents); static std::pair convertToSemitonesAndCents(double alter); @@ -327,6 +330,7 @@ class Converter const static EnumMap stickLocationMap; const static EnumMap tipDirectionMap; const static EnumMap enclosureMap; + const static EnumMap symbolSizeMap; private: template diff --git a/src/private/mx/impl/NotationsWriter.cpp b/src/private/mx/impl/NotationsWriter.cpp index 10483ac08..58a30064d 100644 --- a/src/private/mx/impl/NotationsWriter.cpp +++ b/src/private/mx/impl/NotationsWriter.cpp @@ -1212,13 +1212,57 @@ void NotationsWriter::addOrnamentAccidentals(const api::MarkData &mark, core::Or core::AccidentalMark accidentalMark; accidentalMark.setValue(myConverter.convert(accidental.accidental)); - setAttributesFromPositionData(accidental.positionData, accidentalMark); + setAccidentalMarkAttributes(accidental, accidental.positionData, accidental.printData, accidentalMark); outGroup.addAccidentalMark(accidentalMark); } } +// Writes how an accidental-mark is drawn from either api shape that holds it: a standalone mark's +// AccidentalMarkData, whose position and print data live on the MarkData, or an OrnamentAccidental, +// which has the same fields itself. accidental-mark has no print-object, so printObject is dropped +// by setAttributesFromPrintData. +template +void NotationsWriter::setAccidentalMarkAttributes(const ACCIDENTAL_FIELDS &inFields, + const api::PositionData &inPositionData, + const api::PrintData &inPrintData, + core::AccidentalMark &outAccidentalMark) const +{ + setAttributesFromPositionData(inPositionData, outAccidentalMark); + setAttributesFromPrintData(inPrintData, outAccidentalMark); + if (inFields.parentheses != api::Bool::unspecified) + { + outAccidentalMark.setParentheses(myConverter.convert(inFields.parentheses)); + } + if (inFields.bracket != api::Bool::unspecified) + { + outAccidentalMark.setBracket(myConverter.convert(inFields.bracket)); + } + if (inFields.size != api::SymbolSize::unspecified) + { + outAccidentalMark.setSize(myConverter.convert(inFields.size)); + } + if (inFields.smufl.has_value()) + { + // The strict parse: the lenient one would repair a wrong name into some other glyph. + core::SmuflAccidentalGlyphName glyph; + if (core::SmuflAccidentalGlyphName::tryParse(*inFields.smufl, glyph)) + { + outAccidentalMark.setSmufl(glyph); + } + else + { + myScoreWriter.getDiagnostics().report(api::Severity::error, api::DiagnosticCode::droppedData, + cursorLocation(myCursor), + "accidental-mark smufl \"" + *inFields.smufl + + "\" is not a SMuFL accidental glyph name and is not written"); + } + } + setId(inFields.id, outAccidentalMark, myScoreWriter.getDiagnostics(), cursorLocation(myCursor)); +} + // 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 . +// ornament is not a mark of its own; addOrnamentAccidentals writes it inside . A choice +// that holds anything but AccidentalMarkData is ignored. void NotationsWriter::addAccidentalMark(const api::MarkData &mark, core::Notations &outNotations) const { if (!isMarkAccidentalMark(mark.markType)) @@ -1228,7 +1272,7 @@ void NotationsWriter::addAccidentalMark(const api::MarkData &mark, core::Notatio core::AccidentalMark accidentalMark; accidentalMark.setValue(myConverter.convertAccidentalMark(mark.markType)); - setAttributesFromPositionData(mark.positionData, accidentalMark); + setAccidentalMarkAttributes(mark.choice.accidentalMark(), mark.positionData, mark.printData, accidentalMark); outNotations.addChoice(core::NotationsChoice::accidentalMark(accidentalMark)); } diff --git a/src/private/mx/impl/NotationsWriter.h b/src/private/mx/impl/NotationsWriter.h index 4939c492d..d469ff113 100644 --- a/src/private/mx/impl/NotationsWriter.h +++ b/src/private/mx/impl/NotationsWriter.h @@ -38,6 +38,9 @@ class NotationsWriter 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; + template + void setAccidentalMarkAttributes(const ACCIDENTAL_FIELDS &inFields, const api::PositionData &inPositionData, + const api::PrintData &inPrintData, core::AccidentalMark &outAccidentalMark) const; void addTechnical(const api::MarkData &markData, core::Technical &outTechnical) const; // Kept as their own (non-inlined) methods rather than loops inlined in getNotations(), the diff --git a/src/private/mxtest/api/MarkRoundTripTest.cpp b/src/private/mxtest/api/MarkRoundTripTest.cpp index fe033edb5..92d7003c7 100644 --- a/src/private/mxtest/api/MarkRoundTripTest.cpp +++ b/src/private/mxtest/api/MarkRoundTripTest.cpp @@ -353,6 +353,23 @@ TEST(HarmonicDefaultsToNoPayload, MarkRoundTrip) T_END; +// A plain accidental mark has no payload, so one authored with only its MarkType reads back equal. +TEST(AccidentalMarkDefaultsToNoPayload, MarkRoundTrip) +{ + const MarkData mark{MarkType::accidentalMarkSharp}; + const auto marks = roundTripMarkData(mark); + REQUIRE(marks.size() == 1); + CHECK(marks.front().choice.isNone()); + + // The reader spells the accidental out in name, which the writer ignores. + auto readBack = marks.front(); + CHECK("sharp" == readBack.name); + readBack.name = mark.name; + CHECK(mark == readBack); +} + +T_END; + TEST(CaesuraEmpty, MarkRoundTrip) { // The common empty form must not acquire a "normal" text value on write. @@ -579,4 +596,180 @@ TEST(OrnamentChoiceAccessorsAndEquality, MarkRoundTrip) T_END; +// A standalone accidental mark keeps how it is drawn: its payload, its font and color, and its +// position. +TEST(AccidentalMarkAttributes, MarkRoundTrip) +{ + MarkData mark{Placement::above, MarkType::accidentalMarkFlat}; + mark.positionData.isDefaultXSpecified = true; + mark.positionData.defaultX = 3.5; + mark.printData.fontData.fontFamily = {"Bravura"}; + mark.printData.fontData.style = FontStyle::italic; + mark.printData.fontData.weight = FontWeight::bold; + mark.printData.fontData.sizeType = FontSizeType::point; + mark.printData.fontData.sizePoint = 12.0; + mark.printData.isColorSpecified = true; + mark.printData.color.red = 255; + mark.printData.color.green = 0; + mark.printData.color.blue = 0; + + AccidentalMarkData payload; + payload.parentheses = Bool::yes; + payload.bracket = Bool::no; + payload.size = SymbolSize::cue; + payload.smufl = "accidentalFlatSmall"; + payload.id = "acc-1"; + mark.choice = payload; + + const auto marks = roundTripMarkData(mark); + REQUIRE(marks.size() == 1); + CHECK(MarkType::accidentalMarkFlat == marks.front().markType); + REQUIRE(marks.front().choice.isAccidentalMark()); + CHECK(payload == marks.front().choice.accidentalMark()); + CHECK(mark.printData == marks.front().printData); + CHECK(mark.positionData == marks.front().positionData); +} + +T_END; + +// Every size reads back as written. +TEST(AccidentalMarkSizes, MarkRoundTrip) +{ + for (const auto size : {SymbolSize::full, SymbolSize::cue, SymbolSize::graceCue, SymbolSize::large}) + { + MarkData mark{MarkType::accidentalMarkNatural}; + AccidentalMarkData payload; + payload.size = size; + mark.choice = payload; + + const auto marks = roundTripMarkData(mark); + REQUIRE(marks.size() == 1); + CHECK(size == marks.front().choice.accidentalMark().size); + } +} + +T_END; + +// An ornament's accidental keeps how it is drawn, just as a standalone one does. +TEST(OrnamentAccidentalAttributes, MarkRoundTrip) +{ + OrnamentAccidental sharp; + sharp.accidental = Accidental::sharp; + sharp.positionData.placement = Placement::above; + sharp.printData.fontData.fontFamily = {"Bravura"}; + sharp.printData.fontData.style = FontStyle::normal; + sharp.printData.isColorSpecified = true; + sharp.printData.color.red = 0; + sharp.printData.color.green = 0; + sharp.printData.color.blue = 255; + sharp.parentheses = Bool::no; + sharp.bracket = Bool::yes; + sharp.size = SymbolSize::large; + sharp.smufl = "accidentalSharp"; + sharp.id = "trill-sharp"; + + MarkData trill{Placement::unspecified, MarkType::trillMark}; + trill.choice = OrnamentMarkData{{sharp}}; + const auto marks = roundTripMarkData(trill); + REQUIRE(marks.size() == 1); + CHECK(MarkType::trillMark == marks.front().markType); + REQUIRE(marks.front().choice.isOrnament()); + const auto accidentals = marks.front().choice.ornament().accidentals; + REQUIRE(accidentals.size() == 1); + CHECK(sharp == accidentals.front()); +} + +T_END; + +// A smufl name that is not a SMuFL accidental is not written, and the write reports it. The +// accidental itself is still written. +TEST(AccidentalMarkSmuflNotAnAccidental, MarkRoundTrip) +{ + const std::string message = "accidental-mark smufl \"noteheadBlack\" is not a SMuFL accidental glyph name and is " + "not written"; + + MarkData mark{MarkType::accidentalMarkSharp}; + AccidentalMarkData payload; + payload.smufl = "noteheadBlack"; + mark.choice = payload; + + OrnamentAccidental sharp; + sharp.accidental = Accidental::sharp; + sharp.smufl = "noteheadBlack"; + MarkData trill{Placement::unspecified, MarkType::trillMark}; + trill.choice = OrnamentMarkData{{sharp}}; + + for (const auto &input : {mark, trill}) + { + Diagnostics diagnostics; + const auto xml = markRoundTripWriteWithDiagnostics(input, diagnostics); + CHECK(xml.find(">sharp") != std::string::npos); + CHECK(xml.find("smufl=") == std::string::npos); + const auto dropped = markRoundTripDiagnosticsWithMessage(diagnostics, message); + REQUIRE(dropped.size() == 1); + CHECK(Severity::error == dropped.front().severity); + CHECK(DiagnosticCode::droppedData == dropped.front().code); + } +} + +T_END; + +// An accidental mark payload with nothing set is none, asking for the wrong kind returns a default +// value, and every field of both accidental shapes takes part in equality. +TEST(AccidentalMarkChoiceAccessorsAndEquality, MarkRoundTrip) +{ + const MarkDataChoice empty{AccidentalMarkData{}}; + CHECK(empty.isNone()); + CHECK(!empty.isAccidentalMark()); + CHECK(empty == MarkDataChoice{}); + + AccidentalMarkData parenthesized; + parenthesized.parentheses = Bool::yes; + const MarkDataChoice choice{parenthesized}; + CHECK(MarkDataChoice::Kind::accidentalMark == choice.kind()); + CHECK(choice.isAccidentalMark()); + CHECK(choice.ornament().accidentals.empty()); + CHECK(MarkDataChoice{}.accidentalMark() == AccidentalMarkData{}); + CHECK(MarkDataChoice{HarmonicMarkData{}}.accidentalMark() == AccidentalMarkData{}); + + const AccidentalMarkData base; + auto changed = base; + changed.parentheses = Bool::yes; + CHECK(base != changed); + changed = base; + changed.bracket = Bool::yes; + CHECK(base != changed); + changed = base; + changed.size = SymbolSize::cue; + CHECK(base != changed); + changed = base; + changed.smufl = "accidentalSharp"; + CHECK(base != changed); + changed = base; + changed.id = "acc-1"; + CHECK(base != changed); + + const OrnamentAccidental owned; + auto ownedChanged = owned; + ownedChanged.printData.isColorSpecified = true; + CHECK(owned != ownedChanged); + ownedChanged = owned; + ownedChanged.parentheses = Bool::yes; + CHECK(owned != ownedChanged); + ownedChanged = owned; + ownedChanged.bracket = Bool::yes; + CHECK(owned != ownedChanged); + ownedChanged = owned; + ownedChanged.size = SymbolSize::cue; + CHECK(owned != ownedChanged); + ownedChanged = owned; + ownedChanged.smufl = "accidentalSharp"; + CHECK(owned != ownedChanged); + ownedChanged = owned; + ownedChanged.id = "acc-1"; + CHECK(owned != ownedChanged); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index fa34459f7..7f13fce3b 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -2097,6 +2097,61 @@ TEST(accidentalMarkSurvivesWriteAndRead, NoteData) T_END; +// Parse the synthetic accidental-mark file, a trill with a sharp that sets every attribute +// accidental-mark has, and confirm the trill's accidental carries all of them. +TEST(readsSyntheticAccidentalMarkFixture, NoteData) +{ + const std::string path = mxtest::getResourcesDirectoryPath() + "synthetic/accidental-mark.3.1.xml"; + auto docResult = MusicXml::fromFile(path); + REQUIRE(docResult.ok()); + const auto scoreResult = getScore(std::move(docResult).value()); + REQUIRE(scoreResult.ok()); + const auto &score = scoreResult.value(); + + const auto &marks = + score.parts.back().measures.back().staves.back().voices.at(0).notes.back().noteAttachmentData.marks; + REQUIRE(1 == marks.size()); + CHECK(MarkType::trillMark == marks.at(0).markType); + REQUIRE(marks.at(0).choice.isOrnament()); + const auto accidentals = marks.at(0).choice.ornament().accidentals; + REQUIRE(1 == accidentals.size()); + const auto &sharp = accidentals.at(0); + + CHECK(Accidental::sharp == sharp.accidental); + CHECK(Bool::yes == sharp.parentheses); + CHECK(Bool::yes == sharp.bracket); + CHECK(SymbolSize::full == sharp.size); + REQUIRE(sharp.smufl.has_value()); + CHECK("accidentalSharp" == *sharp.smufl); + REQUIRE(sharp.id.has_value()); + CHECK("id3" == sharp.id->value()); + + CHECK(sharp.positionData.isDefaultXSpecified); + CHECK_DOUBLES_EQUAL(1.0, sharp.positionData.defaultX, 0.00001); + CHECK(sharp.positionData.isDefaultYSpecified); + CHECK_DOUBLES_EQUAL(1.0, sharp.positionData.defaultY, 0.00001); + CHECK(sharp.positionData.isRelativeXSpecified); + CHECK_DOUBLES_EQUAL(1.0, sharp.positionData.relativeX, 0.00001); + CHECK(sharp.positionData.isRelativeYSpecified); + CHECK_DOUBLES_EQUAL(1.0, sharp.positionData.relativeY, 0.00001); + CHECK(Placement::above == sharp.positionData.placement); + + REQUIRE(1 == sharp.printData.fontData.fontFamily.size()); + CHECK("x" == sharp.printData.fontData.fontFamily.at(0)); + CHECK(FontStyle::normal == sharp.printData.fontData.style); + CHECK(FontWeight::normal == sharp.printData.fontData.weight); + CHECK(FontSizeType::point == sharp.printData.fontData.sizeType); + CHECK_DOUBLES_EQUAL(1.0, sharp.printData.fontData.sizePoint, 0.00001); + CHECK(sharp.printData.isColorSpecified); + CHECK(sharp.printData.color.isAlphaSpecified); + CHECK(255 == sharp.printData.color.alpha); + CHECK(0 == sharp.printData.color.red); + CHECK(0 == sharp.printData.color.green); + CHECK(0 == sharp.printData.color.blue); +} + +T_END; + // Builds a one-note score whose note carries the given content. inline std::string noteDataTestOneNoteXml(const std::string ¬ations) { diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index f46b0615c..02e55e549 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -401,6 +401,13 @@ synthetic/word-font.3.0.xml synthetic/work-number.3.0.xml synthetic/work-title.3.0.xml +# Unblocked by AccidentalMarkData and the matching OrnamentAccidental fields. An +# accidental-mark kept only its value and position; its parentheses, bracket, size, +# smufl glyph, id, font and color are now carried too, both standalone and when it +# belongs to an ornament, as the trill's sharp in these fixtures does. +synthetic/accidental-mark.3.0.xml +synthetic/accidental-mark.3.1.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. From e023849b12bf5bb179de1f9176ead0c92c70a70d Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 6 Nov 2026 02:11:45 +0100 Subject: [PATCH 2/3] fix: say what symbol sizes and accidental-mark print data mean SymbolSize::unspecified leaves the size to the application rather than promising the usual size, and cue and graceCue now follow MusicXML's note-type defaults: cue is the size of a cue note or a grace note, graceCue the size of a note that is both. AccidentalMarkData::size says that a different size is, like parentheses and brackets, a common way to show an editorial accidental, and the payload's comment says that MarkData::printData's printObject is not used for an accidental mark. --- src/include/mx/api/ApiCommon.h | 6 +++--- src/include/mx/api/MarkDataChoice.h | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/include/mx/api/ApiCommon.h b/src/include/mx/api/ApiCommon.h index 4dd497cb6..e823e1005 100644 --- a/src/include/mx/api/ApiCommon.h +++ b/src/include/mx/api/ApiCommon.h @@ -114,16 +114,16 @@ enum class Enclosure // How large a symbol is drawn compared with ordinary notation, MusicXML's size attribute. enum class SymbolSize { - // Leave the size unstated, so the application draws the symbol at its usual size. + // Leave the size unstated and let the application choose. unspecified, // The normal size, stated explicitly. full, - // The smaller size used for cue notes. + // The size of a cue note or a grace note. cue, - // The smaller size used for grace notes in a cue passage. + // The size of a grace note that is also a cue note. graceCue, // Larger than normal. diff --git a/src/include/mx/api/MarkDataChoice.h b/src/include/mx/api/MarkDataChoice.h index 947862155..8418e25bc 100644 --- a/src/include/mx/api/MarkDataChoice.h +++ b/src/include/mx/api/MarkDataChoice.h @@ -200,8 +200,8 @@ MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(HarmonicMarkData); // Payload for the accidental mark types (MarkType::accidentalMarkSharp and the rest): how the -// accidental is drawn. Its font and color are in MarkData::printData. Leaving every field at its -// default is the same as having no payload. +// accidental is drawn. Its font and color are in MarkData::printData, whose printObject is not used. +// Leaving every field at its default is the same as having no payload. struct AccidentalMarkData { // yes: the accidental is drawn in parentheses. Parentheses and square brackets are common ways @@ -212,7 +212,8 @@ struct AccidentalMarkData // decide. Bool bracket = Bool::unspecified; - // How large the accidental is drawn. + // How large the accidental is drawn. Like parentheses and brackets, a different size is a common + // way to show an editorial accidental. SymbolSize size = SymbolSize::unspecified; // The SMuFL glyph to draw when a specific one is wanted, for example "accidentalSharp". It must From 68f8fc21195c8233e824c25b2d2cca6a738697a9 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 6 Nov 2026 02:11:46 +0100 Subject: [PATCH 3/3] test: read and write a standalone accidental-mark's attributes as xml Reads a standalone accidental-mark that sets parentheses, bracket, size, font, color, smufl and id, checks each field, and checks the written xml carries each attribute. A bare accidental-mark beside it reads with no payload and default print data. Corrects the comment on AccidentalMarkDefaultsToNoPayload: the mark reads back equal in everything but name, which the reader fills in and the MarkData constructor leaves empty for accidental marks. --- src/private/mxtest/api/MarkRoundTripTest.cpp | 6 ++- src/private/mxtest/api/NoteDataTest.cpp | 49 ++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/private/mxtest/api/MarkRoundTripTest.cpp b/src/private/mxtest/api/MarkRoundTripTest.cpp index 92d7003c7..78b50d195 100644 --- a/src/private/mxtest/api/MarkRoundTripTest.cpp +++ b/src/private/mxtest/api/MarkRoundTripTest.cpp @@ -353,7 +353,8 @@ TEST(HarmonicDefaultsToNoPayload, MarkRoundTrip) T_END; -// A plain accidental mark has no payload, so one authored with only its MarkType reads back equal. +// A plain accidental mark has no payload and no print data, so one authored with only its MarkType +// reads back equal in everything but name. TEST(AccidentalMarkDefaultsToNoPayload, MarkRoundTrip) { const MarkData mark{MarkType::accidentalMarkSharp}; @@ -361,7 +362,8 @@ TEST(AccidentalMarkDefaultsToNoPayload, MarkRoundTrip) REQUIRE(marks.size() == 1); CHECK(marks.front().choice.isNone()); - // The reader spells the accidental out in name, which the writer ignores. + // The reader spells the accidental out in name, which the writer ignores. The MarkData + // constructor leaves name empty for accidental marks, so name is the one field that differs. auto readBack = marks.front(); CHECK("sharp" == readBack.name); readBack.name = mark.name; diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index 7f13fce3b..e08f364b6 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -2431,4 +2431,53 @@ TEST(standaloneAndOrnamentAccidentalsOnOneNote, NoteData) T_END; +// A standalone accidental-mark reads how it is drawn into AccidentalMarkData and MarkData::printData +// and writes each attribute back. One without these attributes has no payload and no print data. +TEST(standaloneAccidentalMarkKeepsHowItIsDrawn, NoteData) +{ + const auto xml = noteDataTestOneNoteXml( + R"( natural + flat)"); + + const auto score = fromXml(xml); + const auto &marks = noteDataTestFirstNoteMarks(score); + REQUIRE(2 == marks.size()); + + const auto &natural = marks.at(0); + CHECK(MarkType::accidentalMarkNatural == natural.markType); + CHECK(Placement::below == natural.positionData.placement); + CHECK(FontSizeType::point == natural.printData.fontData.sizeType); + CHECK_DOUBLES_EQUAL(10.0, natural.printData.fontData.sizePoint, 0.00001); + CHECK(natural.printData.isColorSpecified); + CHECK(0 == natural.printData.color.red); + CHECK(255 == natural.printData.color.green); + CHECK(0 == natural.printData.color.blue); + REQUIRE(natural.choice.isAccidentalMark()); + const auto payload = natural.choice.accidentalMark(); + CHECK(Bool::no == payload.parentheses); + CHECK(Bool::yes == payload.bracket); + CHECK(SymbolSize::graceCue == payload.size); + REQUIRE(payload.smufl.has_value()); + CHECK("accidentalNaturalSmall" == *payload.smufl); + REQUIRE(payload.id.has_value()); + CHECK("nat1" == payload.id->value()); + + const auto &flat = marks.at(1); + CHECK(MarkType::accidentalMarkFlat == flat.markType); + CHECK(flat.choice.isNone()); + CHECK(PrintData{} == flat.printData); + + const auto written = toXml(score); + for (const std::string attribute : + {"parentheses=\"no\"", "bracket=\"yes\"", "size=\"grace-cue\"", "font-size=\"10\"", "color=\"#00FF00\"", + "smufl=\"accidentalNaturalSmall\"", "id=\"nat1\""}) + { + CHECK(written.find(attribute) != std::string::npos); + } + CHECK(written.find("flat") != std::string::npos); + CHECK(marks == noteDataTestFirstNoteMarks(fromXml(written))); +} + +T_END; + #endif