From c0913283515fd6c9c9d3c0d9972dc9a28b6801ea Mon Sep 17 00:00:00 2001 From: Robert Patterson Date: Wed, 29 Jul 2026 13:18:55 -0500 Subject: [PATCH 1/2] Add notehead filled and smufl to NoteData Expose the element's filled and smufl attributes, which mx::core already modeled but mx::api dropped in both directions. NoteWriter now emits when either is set, even for a normal notehead value. Also adds the missing MXAPI_EQUALS_MEMBER(notehead), which had never been in the equality block. Six tests in NoteDataTest; no roundtrip-baseline.txt additions. --- src/include/mx/api/NoteData.h | 20 ++++ src/private/mx/api/NoteData.cpp | 6 +- src/private/mx/impl/NoteFunctions.cpp | 7 ++ src/private/mx/impl/NoteReader.cpp | 32 ++++-- src/private/mx/impl/NoteReader.h | 16 ++- src/private/mx/impl/NoteWriter.cpp | 28 ++++- src/private/mxtest/api/NoteDataTest.cpp | 143 ++++++++++++++++++++++++ 7 files changed, 233 insertions(+), 19 deletions(-) diff --git a/src/include/mx/api/NoteData.h b/src/include/mx/api/NoteData.h index f5a8692a1..488105000 100644 --- a/src/include/mx/api/NoteData.h +++ b/src/include/mx/api/NoteData.h @@ -13,6 +13,7 @@ #include "mx/api/PrintData.h" #include +#include #include namespace mx @@ -132,6 +133,22 @@ class NoteData bool isCue; Notehead notehead; + + // The element's filled attribute: whether the notehead shape is drawn solid or + // hollow. Leave it unspecified to get MusicXML's default, which follows the note's duration + // (hollow for a half note and longer, solid for shorter). Set yes or no to override that, + // for example a solid whole note in a rhythm-notation part. + Bool noteheadFilled; + + // The element's smufl attribute: the canonical SMuFL glyph name to draw for this + // notehead, for example "noteheadSlashHorizontalEnds". It names a glyph directly, so a + // notation program can draw a notehead that MusicXML has no value for -- pair it with + // Notehead::other -- or narrow one that MusicXML names only broadly, such as + // Notehead::cluster. Note-name noteheads (the SMuFL ranges U+E150-U+E1AF and U+EEE0-U+EEFF) + // are not written this way; MusicXML spells those out in . An empty string + // names no glyph and is written the same as leaving this empty. + std::optional noteheadSmufl; + PitchData pitchData; // step, alter, octave, accidental, etc int userRequestedVoiceNumber; @@ -196,6 +213,9 @@ MXAPI_EQUALS_MEMBER(tieLetRing) MXAPI_EQUALS_MEMBER(isGrace) MXAPI_EQUALS_MEMBER(isCue) MXAPI_EQUALS_MEMBER(graceSlash) +MXAPI_EQUALS_MEMBER(notehead) +MXAPI_EQUALS_MEMBER(noteheadFilled) +MXAPI_EQUALS_MEMBER(noteheadSmufl) MXAPI_EQUALS_MEMBER(pitchData) MXAPI_EQUALS_MEMBER(userRequestedVoiceNumber) MXAPI_EQUALS_MEMBER(writeStaffNumber) diff --git a/src/private/mx/api/NoteData.cpp b/src/private/mx/api/NoteData.cpp index 619b23984..31fbc42a2 100644 --- a/src/private/mx/api/NoteData.cpp +++ b/src/private/mx/api/NoteData.cpp @@ -11,9 +11,9 @@ namespace api NoteData::NoteData() : isRest{false}, isMeasureRest{false}, isUnpitched{false}, isDisplayStepOctaveSpecified{false}, isChord{false}, isTieStart{false}, isTieStop{false}, tieLetRing{}, isGrace{false}, graceSlash{Bool::unspecified}, isCue{false}, - notehead{Notehead::normal}, pitchData{}, userRequestedVoiceNumber{VALUE_UNSPECIFIED}, - writeStaffNumber{Bool::unspecified}, stem{Stem::unspecified}, tickTimePosition{0}, durationData{}, beams{}, - positionData{}, printData{}, noteAttachmentData{}, lyrics{} + notehead{Notehead::normal}, noteheadFilled{Bool::unspecified}, noteheadSmufl{}, pitchData{}, + userRequestedVoiceNumber{VALUE_UNSPECIFIED}, writeStaffNumber{Bool::unspecified}, stem{Stem::unspecified}, + tickTimePosition{0}, durationData{}, beams{}, positionData{}, printData{}, noteAttachmentData{}, lyrics{} { } } // namespace api diff --git a/src/private/mx/impl/NoteFunctions.cpp b/src/private/mx/impl/NoteFunctions.cpp index d93924242..1a85f87ba 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -93,6 +93,13 @@ api::NoteData NoteFunctions::parseNote() const myOutNoteData.notehead = converter.convert(reader.getNoteheadValue()); + if (reader.getNoteheadFilled().has_value()) + { + myOutNoteData.noteheadFilled = converter.convert(*reader.getNoteheadFilled()); + } + + myOutNoteData.noteheadSmufl = reader.getNoteheadSmufl(); + if (reader.getIsDurationTypeSpecified()) { myOutNoteData.durationData.durationName = converter.convert(reader.getDurationType()); diff --git a/src/private/mx/impl/NoteReader.cpp b/src/private/mx/impl/NoteReader.cpp index 1aa8006c6..25b3796f7 100644 --- a/src/private/mx/impl/NoteReader.cpp +++ b/src/private/mx/impl/NoteReader.cpp @@ -145,20 +145,21 @@ NoteReader::NoteReader(const core::Note &mxNote) myIsNormal(false), myIsGrace(false), myIsCue(false), myIsRest(false), myIsChord(false), myIsMeasureRest(false), myIsUnpitched(false), myIsPitch(false), myIsDisplayStepOctaveSpecified(false), myDurationValue(0.0), myStep(core::Step::c()), myAlter(0), myCents(0.0), myOctave(4), myStaffNumber(0), myIsStaffSpecified(false), - myVoiceNumber(0), myNoteheadValue(core::NoteheadValue::normal()), myDurationType(core::NoteTypeValue::maxima()), - myIsDurationTypeSpecified(false), myNumDots(0), myBeams(), myTimeModificationActualNotes(-1), - myTimeModificationNormalNotes(-1), myTimeModificationNormalType(core::NoteTypeValue::maxima()), - myTimeModificationNormalTypeDots(0), myHasAccidental(false), myAccidental(core::AccidentalValue::natural()), - myIsAccidentalParenthetical(false), myIsAccidentalCautionary{false}, myIsAccidentalEditorial{false}, - myIsAccidentalBracketed{false}, myIsStemSpecified{false}, myStem{}, myIsGraceSlashSpecified{false}, - myGraceSlash{}, myIsTieStart{false}, myIsTieStop{false}, myHasLyric{false} + myVoiceNumber(0), myNoteheadValue(core::NoteheadValue::normal()), myNoteheadFilled{}, myNoteheadSmufl{}, + myDurationType(core::NoteTypeValue::maxima()), myIsDurationTypeSpecified(false), myNumDots(0), myBeams(), + myTimeModificationActualNotes(-1), myTimeModificationNormalNotes(-1), + myTimeModificationNormalType(core::NoteTypeValue::maxima()), myTimeModificationNormalTypeDots(0), + myHasAccidental(false), myAccidental(core::AccidentalValue::natural()), myIsAccidentalParenthetical(false), + myIsAccidentalCautionary{false}, myIsAccidentalEditorial{false}, myIsAccidentalBracketed{false}, + myIsStemSpecified{false}, myStem{}, myIsGraceSlashSpecified{false}, myGraceSlash{}, myIsTieStart{false}, + myIsTieStop{false}, myHasLyric{false} { setNormalGraceCueItems(); setRestPitchUnpitchedItems(); setChord(); setStaffNumber(); setVoiceNumber(); - setNoteheadValue(); + setNoteheadItems(); setDurationType(); setNumDots(); setBeams(); @@ -323,11 +324,20 @@ void NoteReader::setVoiceNumber() utility::stringToInt(myNote.editorialVoice().voice()->c_str(), myVoiceNumber); } -void NoteReader::setNoteheadValue() +void NoteReader::setNoteheadItems() { - if (myNote.notehead().has_value()) + if (!myNote.notehead().has_value()) { - myNoteheadValue = myNote.notehead()->value(); + return; + } + + const auto ¬ehead = *myNote.notehead(); + myNoteheadValue = notehead.value(); + myNoteheadFilled = notehead.filled(); + + if (notehead.smufl().has_value()) + { + myNoteheadSmufl = notehead.smufl()->toString(); } } diff --git a/src/private/mx/impl/NoteReader.h b/src/private/mx/impl/NoteReader.h index a5f51f501..413dd7abc 100644 --- a/src/private/mx/impl/NoteReader.h +++ b/src/private/mx/impl/NoteReader.h @@ -7,7 +7,9 @@ #include "mx/api/LyricData.h" #include "mx/core/generated/Note.h" +#include #include +#include #include namespace mx @@ -132,6 +134,16 @@ class NoteReader return myNoteheadValue; } + inline const std::optional &getNoteheadFilled() const + { + return myNoteheadFilled; + } + + inline const std::optional &getNoteheadSmufl() const + { + return myNoteheadSmufl; + } + inline core::NoteTypeValue getDurationType() const { return myDurationType; @@ -259,6 +271,8 @@ class NoteReader bool myIsStaffSpecified; int myVoiceNumber; core::NoteheadValue myNoteheadValue; + std::optional myNoteheadFilled; + std::optional myNoteheadSmufl; core::NoteTypeValue myDurationType; bool myIsDurationTypeSpecified; int myNumDots; @@ -289,7 +303,7 @@ class NoteReader void setChord(); void setStaffNumber(); void setVoiceNumber(); - void setNoteheadValue(); + void setNoteheadItems(); void setDurationType(); void setNumDots(); void setBeams(); diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index 2645462fe..fcef3c10b 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -20,6 +20,7 @@ #include "mx/core/generated/NormalNoteGroup.h" #include "mx/core/generated/Pitch.h" #include "mx/core/generated/Rest.h" +#include "mx/core/generated/SmuflGlyphName.h" #include "mx/core/generated/Syllabic.h" #include "mx/core/generated/TextElementData.h" #include "mx/core/generated/Tied.h" @@ -477,12 +478,31 @@ void NoteWriter::setDurationNameAndDots() const void NoteWriter::setNotehead() const { - if (myNoteData.notehead != mx::api::Notehead::normal) + const bool isFilledSpecified = myNoteData.noteheadFilled != api::Bool::unspecified; + // An empty glyph name names no glyph, so it is treated the same as no smufl at all. + const bool isSmuflSpecified = myNoteData.noteheadSmufl.has_value() && !myNoteData.noteheadSmufl->empty(); + + // stays out of the file for a plain notehead, but filled or smufl still needs + // the element even when the value itself is 'normal'. + if (myNoteData.notehead == api::Notehead::normal && !isFilledSpecified && !isSmuflSpecified) + { + return; + } + + core::Notehead notehead; + notehead.setValue(myConverter.convert(myNoteData.notehead)); + + if (isFilledSpecified) { - core::Notehead notehead; - notehead.setValue(myConverter.convert(myNoteData.notehead)); - myOutNote.setNotehead(std::move(notehead)); + notehead.setFilled(myConverter.convert(myNoteData.noteheadFilled)); } + + if (isSmuflSpecified) + { + notehead.setSmufl(core::SmuflGlyphName{*myNoteData.noteheadSmufl}); + } + + myOutNote.setNotehead(std::move(notehead)); } void NoteWriter::setStemDirection() const diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index b5f718087..6039f9651 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -1471,6 +1471,149 @@ TEST(noteheadOtherRoundtrip, NoteData) CHECK(outNote.notehead == Notehead::other); } +T_END; + +// A one-note score whose single quarter note is ready for notehead fields to be set on it. +ScoreData makeNoteheadScore() +{ + ScoreData score; + score.ticksPerQuarter = 96; + 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]; + + NoteData note; + note.durationData.durationName = DurationName::quarter; + note.durationData.durationTimeTicks = 96; + voice.notes.push_back(note); + + return score; +} + +const NoteData &firstNoteheadNote(const ScoreData &score) +{ + return score.parts.front().measures.front().staves.front().voices.begin()->second.notes.front(); +} + +// filled="no" on an otherwise normal notehead: the element has to be written even +// though the notehead value itself is the default. +TEST(noteheadFilledRoundtrip, NoteData) +{ + auto score = makeNoteheadScore(); + score.parts.back().measures.back().staves.back().voices.at(0).notes.back().noteheadFilled = Bool::no; + + const std::string xml = mxtest::toXml(score); + CHECK(xml.find(R"(normal)") != std::string::npos); + + const auto outScore = mxtest::fromXml(xml); + CHECK(Bool::no == firstNoteheadNote(outScore).noteheadFilled); + CHECK(Notehead::normal == firstNoteheadNote(outScore).notehead); +} + +T_END; + +// filled="yes" alongside a non-default notehead value. +TEST(noteheadFilledYesWithShapeRoundtrip, NoteData) +{ + auto score = makeNoteheadScore(); + auto ¬e = score.parts.back().measures.back().staves.back().voices.at(0).notes.back(); + note.notehead = Notehead::diamond; + note.noteheadFilled = Bool::yes; + + const std::string xml = mxtest::toXml(score); + CHECK(xml.find(R"(diamond)") != std::string::npos); + + const auto outScore = mxtest::fromXml(xml); + CHECK(Bool::yes == firstNoteheadNote(outScore).noteheadFilled); + CHECK(Notehead::diamond == firstNoteheadNote(outScore).notehead); +} + +T_END; + +// The smufl attribute names a glyph that MusicXML has no notehead value for; it pairs with the +// 'other' notehead value. +TEST(noteheadSmuflRoundtrip, NoteData) +{ + auto score = makeNoteheadScore(); + auto ¬e = score.parts.back().measures.back().staves.back().voices.at(0).notes.back(); + note.notehead = Notehead::other; + note.noteheadSmufl = "noteheadSlashHorizontalEnds"; + + const std::string xml = mxtest::toXml(score); + CHECK(xml.find(R"(smufl="noteheadSlashHorizontalEnds")") != std::string::npos); + + const auto outScore = mxtest::fromXml(xml); + const auto &outNote = firstNoteheadNote(outScore); + CHECK(Notehead::other == outNote.notehead); + REQUIRE(outNote.noteheadSmufl.has_value()); + CHECK_EQUAL("noteheadSlashHorizontalEnds", *outNote.noteheadSmufl); +} + +T_END; + +// Both attributes at once, refining a notehead value that MusicXML names only broadly. +TEST(noteheadFilledAndSmuflRoundtrip, NoteData) +{ + auto score = makeNoteheadScore(); + auto ¬e = score.parts.back().measures.back().staves.back().voices.at(0).notes.back(); + note.notehead = Notehead::cluster; + note.noteheadFilled = Bool::yes; + note.noteheadSmufl = "noteheadClusterSquareBlack"; + + const auto outScore = mxtest::fromXml(mxtest::toXml(score)); + const auto &outNote = firstNoteheadNote(outScore); + CHECK(Notehead::cluster == outNote.notehead); + CHECK(Bool::yes == outNote.noteheadFilled); + REQUIRE(outNote.noteheadSmufl.has_value()); + CHECK_EQUAL("noteheadClusterSquareBlack", *outNote.noteheadSmufl); +} + +T_END; + +// A note that sets neither field writes no element at all. +TEST(noteheadAttributesDefaultToAbsent, NoteData) +{ + const auto score = makeNoteheadScore(); + + const std::string xml = mxtest::toXml(score); + CHECK(xml.find(" api read path independently of what the writer emits. +TEST(noteheadSyntheticFileRead, NoteData) +{ + const std::string path = mxtest::getResourcesDirectoryPath() + "synthetic/notehead.3.1.xml"; + auto &docMgr = DocumentManager::getInstance(); + const auto docIdResult = docMgr.createFromFile(path); + REQUIRE(docIdResult.ok()); + const int docId = docIdResult.value(); + const auto scoreResult = docMgr.getData(docId); + docMgr.destroyDocument(docId); + REQUIRE(scoreResult.ok()); + const auto &score = scoreResult.value(); + REQUIRE(score.parts.size() == 1); + REQUIRE(score.parts.front().measures.size() == 1); + + const auto &outNote = firstNoteheadNote(score); + CHECK(Notehead::slash == outNote.notehead); + CHECK(Bool::yes == outNote.noteheadFilled); + REQUIRE(outNote.noteheadSmufl.has_value()); + CHECK_EQUAL("noteheadBlack", *outNote.noteheadSmufl); +} + +T_END; + TEST(printObjectNo, NoteData) { ScoreData score; From 509c9e0743f5c2391346a772b739c9aa67fda36e Mon Sep 17 00:00:00 2001 From: Robert Patterson Date: Wed, 29 Jul 2026 13:57:48 -0500 Subject: [PATCH 2/2] Fix dangling reference in the notehead default-absent test The test bound a reference to firstNoteheadNote(mxtest::fromXml(xml)). Lifetime extension does not apply when a reference binds to one returned by a function, so the temporary ScoreData was destroyed at the end of the statement and outNote dangled. Hold the score in a local first, as the neighbouring notehead tests already do. MSVC's debug CRT caught this by filling the freed block with 0xDD, which read back as -572662307; libc++ happened not to reuse the block, so the Linux and macOS jobs passed. --- src/private/mxtest/api/NoteDataTest.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index 6039f9651..1749a7f4e 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -1582,7 +1582,8 @@ TEST(noteheadAttributesDefaultToAbsent, NoteData) const std::string xml = mxtest::toXml(score); CHECK(xml.find("