From 7a2c4ce085243a96bf6a5cab2bc5a54c1b24f34a Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 23 Aug 2026 17:48:08 +0000 Subject: [PATCH] feat: preserve source voice names in mx::api mx::api keys the voices of a staff by index and the writer named every voice from that index, so a voice whose element said something else lost its name. MusicXML does not require a voice to be named with a number, and a part whose voices are numbered straight through its staves rather than restarting on each one does not survive renumbering either. VoiceData gains a label. Empty, the default, numbers the voice from its index as before. The reader fills it in from the element and then drops it again wherever it is just the number the writer would have used anyway, so the common case stays empty. Which notes get a element does not change, so a source that leaves it off a chord member still does. The one addition is a voice where no note carried a number: there the label is the only thing that can ask for the element, so it does. Pins two more files in the api round-trip baseline. --- src/include/mx/api/VoiceData.h | 9 +- src/private/mx/impl/MeasureReader.cpp | 32 +++++- src/private/mx/impl/MeasureReader.h | 3 +- src/private/mx/impl/MeasureWriter.cpp | 3 +- src/private/mx/impl/NoteReader.cpp | 10 +- src/private/mx/impl/NoteReader.h | 8 ++ src/private/mx/impl/NoteWriter.cpp | 21 +++- src/private/mx/impl/NoteWriter.h | 4 +- src/private/mxtest/api/VoiceLabelApiTest.cpp | 108 ++++++++++++++++++ src/private/mxtest/api/roundtrip-baseline.txt | 7 ++ 10 files changed, 190 insertions(+), 15 deletions(-) create mode 100644 src/private/mxtest/api/VoiceLabelApiTest.cpp diff --git a/src/include/mx/api/VoiceData.h b/src/include/mx/api/VoiceData.h index b919edfe8..384a2e37e 100644 --- a/src/include/mx/api/VoiceData.h +++ b/src/include/mx/api/VoiceData.h @@ -16,12 +16,19 @@ namespace api class VoiceData { public: + // What MusicXML calls this voice, the text of the element on each of its notes. + // Voices are normally numbered and the api keys them by index, so leave this empty and each + // voice is written with its own number. Set it when the name is not that number: a part + // whose voices are numbered straight through its staves instead of restarting on each one, + // or a program that writes words such as "FirstVoice" rather than numbers. + std::string label; + std::vector notes; }; inline bool operator==(const VoiceData &lhs, const VoiceData &rhs) { - return areVectorsEqual(lhs.notes, rhs.notes); + return lhs.label == rhs.label && areVectorsEqual(lhs.notes, rhs.notes); } MXAPI_NOT_EQUALS_AND_VECTORS(VoiceData); diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index 8adc66065..27d0bfa79 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -431,7 +431,8 @@ void MeasureReader::parseNote(const core::Note &inMxNote, const core::Note *next MX_ASSERT(bucketStaffIndex >= 0); MX_ASSERT(static_cast(bucketStaffIndex) < myOutMeasureData.staves.size()); myPreviousNoteBucketStaffIndex = bucketStaffIndex; - insertNoteData(std::move(noteData), myCurrentCursor.staffIndex, myCurrentCursor.voiceIndex); + insertNoteData(std::move(noteData), myCurrentCursor.staffIndex, myCurrentCursor.voiceIndex, + noteReader.getVoiceLabel()); } void MeasureReader::scanForCrossStaffBeamGroups() const @@ -1158,7 +1159,7 @@ void MeasureReader::importClef(const core::Clef &inClef) const insertClef(std::move(clefData), celfStaffIndex); } -void MeasureReader::insertNoteData(api::NoteData &¬eData, int staff, int voice) const +void MeasureReader::insertNoteData(api::NoteData &¬eData, int staff, int voice, const std::string &voiceLabel) const { MX_ASSERT(staff >= 0); MX_ASSERT(static_cast(staff) < myOutMeasureData.staves.size()); @@ -1169,6 +1170,14 @@ void MeasureReader::insertNoteData(api::NoteData &¬eData, int staff, int voic staffRef.voices[voice] = api::VoiceData{}; } auto &voiceRef = staffRef.voices[voice]; + + // The first note that names the voice names it for all of them. Notes without a get + // the name of the voice they land in. + if (voiceRef.label.empty()) + { + voiceRef.label = voiceLabel; + } + voiceRef.notes.emplace_back(std::move(noteData)); } @@ -1193,6 +1202,25 @@ void MeasureReader::consolidateVoicesForAllStaves() const collapseVoicesAutomatically(staff); } } + + dropRedundantVoiceLabels(); +} + +// A voice whose name is the number the writer would give it anyway does not need to carry one. +// Clearing those keeps VoiceData::label empty in the common case, where it stands for a voice +// name the index cannot express. +void MeasureReader::dropRedundantVoiceLabels() const +{ + for (auto &staff : myOutMeasureData.staves) + { + for (auto &voicePair : staff.voices) + { + if (voicePair.second.label == std::to_string(voicePair.first + 1)) + { + voicePair.second.label.clear(); + } + } + } } void MeasureReader::takeUserRequestedVoiceNumbers(api::StaffData &staff) const diff --git a/src/private/mx/impl/MeasureReader.h b/src/private/mx/impl/MeasureReader.h index d1f756ee9..5edbc05d7 100644 --- a/src/private/mx/impl/MeasureReader.h +++ b/src/private/mx/impl/MeasureReader.h @@ -113,9 +113,10 @@ class MeasureReader void importStaffDetails(const core::Attributes &inMxAttributes) const; void importClefs(std::span inClefs) const; void importClef(const core::Clef &inClef) const; - void insertNoteData(api::NoteData &¬eData, int staff, int voice) const; + void insertNoteData(api::NoteData &¬eData, int staff, int voice, const std::string &voiceLabel) const; void insertClef(api::ClefData &&clefData, int staff) const; void consolidateVoicesForAllStaves() const; + void dropRedundantVoiceLabels() const; void takeUserRequestedVoiceNumbers(api::StaffData &staff) const; void collapseVoicesAutomatically(api::StaffData &staff) const; bool isUserRequestedVoiceNumberConsistent(const api::VoiceData &voiceData) const; diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index 8fb7d7e5e..e03f071de 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -602,7 +602,8 @@ void MeasureWriter::writeVoices(const api::StaffData &inStaff) myPreviousCursor.isChordActive, voice.second.notes, noteIndex, - numVoices}; + numVoices, + voice.second.label}; myOutMeasure.addMusicData(core::MusicDataChoice::note(writer.getNote(isStartOfChord))); myHistory.log("addNote cursorTime " + std::to_string(myHistory.getCursor().tickTimePosition) + ", noteTime " + std::to_string(apiNote.tickTimePosition)); diff --git a/src/private/mx/impl/NoteReader.cpp b/src/private/mx/impl/NoteReader.cpp index 47c567195..191d0b83b 100644 --- a/src/private/mx/impl/NoteReader.cpp +++ b/src/private/mx/impl/NoteReader.cpp @@ -154,9 +154,9 @@ 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()), myNoteheadFilled{}, myNoteheadSmufl{}, - myDurationType(core::NoteTypeValue::maxima()), myIsDurationTypeSpecified(false), myNumDots(0), myBeams(), - myTimeModificationActualNotes(-1), myTimeModificationNormalNotes(-1), + myVoiceNumber(0), myVoiceLabel{}, 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}, @@ -328,13 +328,15 @@ void NoteReader::setStaffNumber() void NoteReader::setVoiceNumber() { myVoiceNumber = api::VALUE_UNSPECIFIED; + myVoiceLabel.clear(); if (!myNote.editorialVoice().voice().has_value()) { return; } - utility::stringToInt(myNote.editorialVoice().voice()->c_str(), myVoiceNumber); + myVoiceLabel = *myNote.editorialVoice().voice(); + utility::stringToInt(myVoiceLabel.c_str(), myVoiceNumber); } void NoteReader::setNoteheadItems() diff --git a/src/private/mx/impl/NoteReader.h b/src/private/mx/impl/NoteReader.h index a4591b06f..44e560f3a 100644 --- a/src/private/mx/impl/NoteReader.h +++ b/src/private/mx/impl/NoteReader.h @@ -129,6 +129,13 @@ class NoteReader return myVoiceNumber; } + // The text of the note's element, empty when it had none. A voice name does not have + // to be a number, so getVoiceNumber above cannot stand in for it. + inline const std::string &getVoiceLabel() const + { + return myVoiceLabel; + } + inline core::NoteheadValue getNoteheadValue() const { return myNoteheadValue; @@ -270,6 +277,7 @@ class NoteReader int myStaffNumber; bool myIsStaffSpecified; int myVoiceNumber; + std::string myVoiceLabel; core::NoteheadValue myNoteheadValue; std::optional myNoteheadFilled; std::optional myNoteheadSmufl; diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index 24deff2ea..f82ab9ed9 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -69,11 +69,11 @@ core::Syllabic convertLyricSyllabicForNoteWriter(api::LyricSyllabic value) NoteWriter::NoteWriter(const api::NoteData &inNoteData, const MeasureCursor &inCursor, const ScoreWriter &inScoreWriter, bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, - int inNoteIndex, int inNumVoices) + int inNoteIndex, int inNumVoices, const std::string &inVoiceLabel) : myNoteData{inNoteData}, myCursor{inCursor}, myScoreWriter{inScoreWriter}, myConverter{}, myIsPreviousNoteAChordMember{isPreviousNoteAChordMember}, mySiblingNotes{inSiblingNotes}, - myNoteIndex{inNoteIndex}, myNumVoices{inNumVoices}, myOutNote{}, myOutFullNoteGroup{}, myOutTies{}, - myOutTieNotationsChoices{} + myNoteIndex{inNoteIndex}, myNumVoices{inNumVoices}, myVoiceLabel{inVoiceLabel}, myOutNote{}, myOutFullNoteGroup{}, + myOutTies{}, myOutTieNotationsChoices{} { } @@ -492,13 +492,24 @@ void NoteWriter::setStaffAndVoice() const } } + // VoiceData::label decides what goes in the element, replacing the voice's number + // when the number cannot express its name. Which notes get the element is otherwise + // unchanged, so a source that left off a chord member still does. The exception is a + // voice where no note carried a number: there the label is the only thing asking for the + // element, so it is what puts one there. + const bool hasVoiceLabel = !myVoiceLabel.empty(); const bool sourceHadVoice = myNoteData.userRequestedVoiceNumber != api::VALUE_UNSPECIFIED; const bool isNonDefaultVoice = myCursor.voiceIndex > 0; const bool isMultiVoiceStaff = myNumVoices > 1; - if (myCursor.voiceIndex >= 0 && (sourceHadVoice || isNonDefaultVoice || isMultiVoiceStaff)) + const bool isVoiceNumbered = std::any_of(mySiblingNotes.cbegin(), mySiblingNotes.cend(), [](const auto ¬e) { + return note.userRequestedVoiceNumber != api::VALUE_UNSPECIFIED; + }); + const bool labelAsksForVoice = hasVoiceLabel && !isVoiceNumbered; + + if (myCursor.voiceIndex >= 0 && (sourceHadVoice || isNonDefaultVoice || isMultiVoiceStaff || labelAsksForVoice)) { auto editorialVoice = myOutNote.editorialVoice(); - editorialVoice.setVoice(std::to_string(myCursor.voiceIndex + 1)); + editorialVoice.setVoice(hasVoiceLabel ? myVoiceLabel : std::to_string(myCursor.voiceIndex + 1)); myOutNote.setEditorialVoice(std::move(editorialVoice)); } } diff --git a/src/private/mx/impl/NoteWriter.h b/src/private/mx/impl/NoteWriter.h index 1073e83bf..6b961ecde 100644 --- a/src/private/mx/impl/NoteWriter.h +++ b/src/private/mx/impl/NoteWriter.h @@ -12,6 +12,7 @@ #include "mx/impl/Converter.h" #include "mx/impl/MeasureCursor.h" +#include #include namespace mx @@ -25,7 +26,7 @@ class NoteWriter public: NoteWriter(const api::NoteData &inNoteData, const MeasureCursor &inCursor, const ScoreWriter &inScoreWriter, bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, int inNoteIndex, - int inNumVoices); + int inNumVoices, const std::string &inVoiceLabel); core::Note getNote(bool isStartOfChord) const; @@ -38,6 +39,7 @@ class NoteWriter const std::vector &mySiblingNotes; const int myNoteIndex; const int myNumVoices; + const std::string &myVoiceLabel; mutable core::Note myOutNote; mutable core::FullNoteGroup myOutFullNoteGroup; mutable std::vector myOutTies; diff --git a/src/private/mxtest/api/VoiceLabelApiTest.cpp b/src/private/mxtest/api/VoiceLabelApiTest.cpp new file mode 100644 index 000000000..a60e5b213 --- /dev/null +++ b/src/private/mxtest/api/VoiceLabelApiTest.cpp @@ -0,0 +1,108 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mxtest/control/CompileControl.h" +#ifdef MX_COMPILE_API_TESTS + +#include "cpul/cpulTestHarness.h" +#include "mx/api/DocumentManager.h" +#include "mxtest/api/RoundTrip.h" +#include "mxtest/api/TestHelpers.h" + +using namespace std; +using namespace mx::api; +using namespace mxtest; + +// A one-measure, one-staff score with the requested number of voices, each holding a whole note +// so the voices overlap and all of them are written. +ScoreData voiceLabelMakeScore(int numVoices) +{ + ScoreData score; + score.ticksPerQuarter = 4; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.uniqueId = "P1"; + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + + for (int i = 0; i < numVoices; ++i) + { + auto &voice = measure.staves.back().voices[i]; + voice.notes.emplace_back(); + voice.notes.back().durationData.durationName = DurationName::whole; + voice.notes.back().durationData.durationTimeTicks = 16; + } + + return score; +} + +TEST(anUnlabeledVoiceIsWrittenWithItsNumber, VoiceLabel) +{ + const auto score = voiceLabelMakeScore(2); + const auto xml = toXml(score); + CHECK(xml.find("1") != std::string::npos); + CHECK(xml.find("2") != std::string::npos); + + const auto out = roundTrip(score); + const auto &voices = out.parts.at(0).measures.at(0).staves.at(0).voices; + REQUIRE(voices.size() == 2); + CHECK_EQUAL("", voices.at(0).label); + CHECK_EQUAL("", voices.at(1).label); +} + +// MusicXML does not require a voice to be named with a number, and some programs write words. +// The number the api would otherwise give the voice cannot express that. +TEST(aNamedVoiceKeepsItsName, VoiceLabel) +{ + auto score = voiceLabelMakeScore(2); + score.parts.at(0).measures.at(0).staves.at(0).voices.at(0).label = "FirstVoice"; + score.parts.at(0).measures.at(0).staves.at(0).voices.at(1).label = "SecondVoice"; + + const auto xml = toXml(score); + CHECK(xml.find("FirstVoice") != std::string::npos); + CHECK(xml.find("SecondVoice") != std::string::npos); + + const auto out = roundTrip(score); + const auto &voices = out.parts.at(0).measures.at(0).staves.at(0).voices; + REQUIRE(voices.size() == 2); + CHECK_EQUAL("FirstVoice", voices.at(0).label); + CHECK_EQUAL("SecondVoice", voices.at(1).label); +} + +// A lone voice named 5 is written as voice 5 rather than renumbered to 1. Reading it back turns +// the number into the voice's index, so the name is no longer needed and the label comes back +// empty. Writing that again still gives voice 5. +TEST(aVoiceNamedPastTheStaffVoiceCountKeepsItsNumber, VoiceLabel) +{ + auto score = voiceLabelMakeScore(1); + score.parts.at(0).measures.at(0).staves.at(0).voices.at(0).label = "5"; + + const auto xml = toXml(score); + CHECK(xml.find("5") != std::string::npos); + + const auto out = roundTrip(score); + const auto &voices = out.parts.at(0).measures.at(0).staves.at(0).voices; + REQUIRE(voices.size() == 1); + REQUIRE(voices.count(4) == 1); + CHECK_EQUAL("", voices.at(4).label); + CHECK(toXml(out).find("5") != std::string::npos); +} + +// Reading drops a name that is just the number the writer would have used anyway, so the common +// case comes back with an empty label. +TEST(aNameThatMatchesTheVoiceNumberIsDropped, VoiceLabel) +{ + auto score = voiceLabelMakeScore(2); + score.parts.at(0).measures.at(0).staves.at(0).voices.at(0).label = "1"; + score.parts.at(0).measures.at(0).staves.at(0).voices.at(1).label = "2"; + + const auto out = roundTrip(score); + const auto &voices = out.parts.at(0).measures.at(0).staves.at(0).voices; + REQUIRE(voices.size() == 2); + CHECK_EQUAL("", voices.at(0).label); + CHECK_EQUAL("", voices.at(1).label); +} + +#endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 112214d07..61bf8738b 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -726,3 +726,10 @@ lysuite/ly33b_Spanners_Tie.xml # the ordering is fixed, but the writer restates / # on a start whose source omits them.) rpatters1/gliss_to_rest-ref.xml + +# Unblocked by VoiceData::label: mx renumbered every voice from the position of +# its container, so a voice whose element said something else -- a word +# rather than a number, or a number the api's own index does not reproduce -- +# was written under the wrong name. +musuite/testChordNoVoice.xml +musuite/testStringVoiceName.xml