From fc0f4fca4365a8d55a6a2b35b5799796a6ab8afb Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Thu, 25 Jun 2026 06:17:01 +0000 Subject: [PATCH 1/4] fix: suppress spurious injection for notes that had none (#228) Add DurationData::isDurationNameSpecified (default true). The reader sets it false when the source note omits ; the writer emits only when the flag is set. The existing derive-on-read (for consumer convenience) is preserved -- callers still get a durationName even when the source had none -- but it is no longer re-emitted on the write path. Pin 4 newly passing corpus files. --- src/include/mx/api/DurationData.h | 10 ++++++++-- src/private/mx/api/DurationData.cpp | 4 ++-- src/private/mx/impl/NoteFunctions.cpp | 2 ++ src/private/mx/impl/NoteWriter.cpp | 3 ++- src/private/mxtest/api/roundtrip-baseline.txt | 8 ++++++++ 5 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/include/mx/api/DurationData.h b/src/include/mx/api/DurationData.h index 1e6ea7ca3..824d6ecfd 100644 --- a/src/include/mx/api/DurationData.h +++ b/src/include/mx/api/DurationData.h @@ -66,8 +66,13 @@ struct DurationData DurationData(); DurationName durationName; // i.e. quarter, eighth etc - int durationDots; // dots - int durationTimeTicks; // length of the note denominated in ticksPerQuarter + // When true (the default), the writer emits . Set to false when the source + // had no element so the round-trip does not inject an implied default (#228). + // When authoring, leave true and set durationName to control the display type; + // durationName and durationTimeTicks are independent (type is graphical, duration is sound). + bool isDurationNameSpecified; + int durationDots; // dots + int durationTimeTicks; // length of the note denominated in ticksPerQuarter bool isTied; // affects sound only. is the note combined sound-wise with the following note of the same pitch int timeModificationActualNotes; // i.e. for a triplet this would be 3 int timeModificationNormalNotes; // i.e. for a triplet this would be 2 @@ -78,6 +83,7 @@ struct DurationData MXAPI_EQUALS_BEGIN(DurationData) MXAPI_EQUALS_MEMBER(durationName) +MXAPI_EQUALS_MEMBER(isDurationNameSpecified) MXAPI_EQUALS_MEMBER(durationDots) MXAPI_EQUALS_MEMBER(durationTimeTicks) MXAPI_EQUALS_MEMBER(isTied) diff --git a/src/private/mx/api/DurationData.cpp b/src/private/mx/api/DurationData.cpp index 23de31a79..cbb1a2eaf 100644 --- a/src/private/mx/api/DurationData.cpp +++ b/src/private/mx/api/DurationData.cpp @@ -9,8 +9,8 @@ namespace mx namespace api { DurationData::DurationData() - : durationName{DurationName::unspecified}, durationDots{0}, durationTimeTicks{1}, isTied{false}, - timeModificationActualNotes{1}, timeModificationNormalNotes{1}, + : durationName{DurationName::unspecified}, isDurationNameSpecified{true}, durationDots{0}, durationTimeTicks{1}, + isTied{false}, timeModificationActualNotes{1}, timeModificationNormalNotes{1}, timeModificationNormalType{api::DurationName::unspecified}, timeModificationNormalTypeDots{0} { } diff --git a/src/private/mx/impl/NoteFunctions.cpp b/src/private/mx/impl/NoteFunctions.cpp index 131736117..3bc2c5b41 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -92,10 +92,12 @@ api::NoteData NoteFunctions::parseNote() const if (reader.getIsDurationTypeSpecified()) { myOutNoteData.durationData.durationName = converter.convert(reader.getDurationType()); + myOutNoteData.durationData.isDurationNameSpecified = true; } else { myOutNoteData.durationData.durationName = deriveNoteTypeFromDurationValue(reader); + myOutNoteData.durationData.isDurationNameSpecified = false; } myOutNoteData.durationData.durationDots = reader.getNumDots(); diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index dd34322ea..593bd48b2 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -386,7 +386,8 @@ void NoteWriter::setStaffAndVoice() const void NoteWriter::setDurationNameAndDots() const { - if (!myNoteData.isRest || !myNoteData.isMeasureRest) + const bool isMeasureRest = myNoteData.isRest && myNoteData.isMeasureRest; + if (myNoteData.durationData.isDurationNameSpecified && !isMeasureRest) { core::NoteType noteType; noteType.setValue(myConverter.convert(myNoteData.durationData.durationName)); diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 87c9511fa..c7a1c6c79 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -73,3 +73,11 @@ synthetic/segno.3.0.xml synthetic/segno.3.1.xml synthetic/coda.3.0.xml synthetic/coda.3.1.xml + +# Unblocked by #228: the writer no longer injects a spurious element for +# notes whose source omitted it. DurationData::isDurationNameSpecified tracks +# presence; the writer emits only when the flag is set. +lysuite/ly41d_StaffGroups_Nested.xml +lysuite/ly41f_StaffGroups_Overlapping.xml +lysuite/ly45d_Repeats_Nested_Alternatives.xml +lysuite/ly45e_Repeats_Nested_Alternatives.xml From 3bd3fb3ba175b24b9bebdb1251f75d31a939f3e7 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Thu, 25 Jun 2026 21:03:33 +0000 Subject: [PATCH 2/4] fix: suppress spurious injection in when source omitted it (#228) --- src/include/mx/api/ClefData.h | 4 ++++ src/private/mx/api/ClefData.cpp | 2 +- src/private/mx/impl/MeasureReader.cpp | 2 ++ src/private/mx/impl/PropertiesWriter.cpp | 2 +- src/private/mxtest/api/roundtrip-baseline.txt | 5 +++++ 5 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/include/mx/api/ClefData.h b/src/include/mx/api/ClefData.h index aa7280383..2a15ebbdb 100644 --- a/src/include/mx/api/ClefData.h +++ b/src/include/mx/api/ClefData.h @@ -41,6 +41,9 @@ class ClefData // int staffIndex; ClefSymbol symbol; int line; + // When true (the default), the writer emits . Set to false when the source + // had no element so the round-trip does not inject an implied default (#228). + bool isLineSpecified; int octaveChange; int tickTimePosition; ClefLocation location; @@ -69,6 +72,7 @@ MXAPI_EQUALS_BEGIN(ClefData) // MXAPI_EQUALS_MEMBER( staffIndex ) MXAPI_EQUALS_MEMBER(symbol) MXAPI_EQUALS_MEMBER(line) +MXAPI_EQUALS_MEMBER(isLineSpecified) MXAPI_EQUALS_MEMBER(octaveChange) MXAPI_EQUALS_MEMBER(tickTimePosition) MXAPI_EQUALS_MEMBER(location) diff --git a/src/private/mx/api/ClefData.cpp b/src/private/mx/api/ClefData.cpp index ca0f202ad..1fddde9cc 100644 --- a/src/private/mx/api/ClefData.cpp +++ b/src/private/mx/api/ClefData.cpp @@ -10,7 +10,7 @@ namespace mx namespace api { ClefData::ClefData() - : symbol{DEFAULT_CLEF_SYMBOL}, line{DEFAULT_CLEF_LINE}, octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, + : symbol{DEFAULT_CLEF_SYMBOL}, line{DEFAULT_CLEF_LINE}, isLineSpecified{true}, octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, tickTimePosition{0}, location{ClefLocation::unspecified} { } diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index b6e237bea..564876d64 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -759,9 +759,11 @@ void MeasureReader::importClef(const core::Clef &inClef) const if (inClef.clef().line().has_value()) { clefData.line = inClef.clef().line()->value(); + clefData.isLineSpecified = true; } else { + clefData.isLineSpecified = false; switch (clefData.symbol) { case api::ClefSymbol::g: diff --git a/src/private/mx/impl/PropertiesWriter.cpp b/src/private/mx/impl/PropertiesWriter.cpp index 10f6099b2..7673ca3db 100644 --- a/src/private/mx/impl/PropertiesWriter.cpp +++ b/src/private/mx/impl/PropertiesWriter.cpp @@ -210,7 +210,7 @@ void PropertiesWriter::writeClef(int staffIndex, const api::ClefData &inClefData core::ClefGroup cg{}; cg.setSign(converter.convert(inClefData.symbol)); - if (inClefData.line >= 0) + if (inClefData.isLineSpecified) { cg.setLine(core::StaffLinePosition{inClefData.line}); } diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index c7a1c6c79..b6568588d 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -81,3 +81,8 @@ lysuite/ly41d_StaffGroups_Nested.xml lysuite/ly41f_StaffGroups_Overlapping.xml lysuite/ly45d_Repeats_Nested_Alternatives.xml lysuite/ly45e_Repeats_Nested_Alternatives.xml + +# Unblocked by #228 (add:line): the writer no longer injects a spurious +# element in when the source omitted it. ClefData::isLineSpecified tracks +# presence; PropertiesWriter emits only when the flag is set. +lysuite/ly12a_Clefs.xml From df0950597c8420dda5294c2402dbae94417d3bb0 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Thu, 25 Jun 2026 21:11:35 +0000 Subject: [PATCH 3/4] fix: suppress spurious injection for notes that had none (#228) --- src/private/mx/impl/MeasureWriter.cpp | 3 ++- src/private/mx/impl/NoteWriter.cpp | 11 +++++++---- src/private/mx/impl/NoteWriter.h | 4 +++- src/private/mxtest/api/roundtrip-baseline.txt | 7 +++++++ 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index 5cfb36400..16e494c00 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -377,6 +377,7 @@ void MeasureWriter::writeStaves() void MeasureWriter::writeVoices(const api::StaffData &inStaff) { + const int numVoices = static_cast(inStaff.voices.size()); auto clefIter = inStaff.clefs.cbegin(); const auto clefEnd = inStaff.clefs.cend(); while (clefIter != clefEnd && clefIter->tickTimePosition == 0) @@ -453,7 +454,7 @@ void MeasureWriter::writeVoices(const api::StaffData &inStaff) NoteWriter writer{ apiNote, myHistory.getCursor(), myScoreWriter, myPreviousCursor.isChordActive, voice.second.notes, - noteIndex}; + noteIndex, numVoices}; 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/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index 593bd48b2..a1ee75492 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -33,10 +33,11 @@ namespace impl { NoteWriter::NoteWriter(const api::NoteData &inNoteData, const MeasureCursor &inCursor, const ScoreWriter &inScoreWriter, bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, - int inNoteIndex) + int inNoteIndex, int inNumVoices) : myNoteData{inNoteData}, myCursor{inCursor}, myScoreWriter{inScoreWriter}, myConverter{}, myIsPreviousNoteAChordMember{isPreviousNoteAChordMember}, mySiblingNotes{inSiblingNotes}, - myNoteIndex{inNoteIndex}, myOutNote{}, myOutFullNoteGroup{}, myOutTies{}, myOutTieNotationsChoices{} + myNoteIndex{inNoteIndex}, myNumVoices{inNumVoices}, myOutNote{}, myOutFullNoteGroup{}, myOutTies{}, + myOutTieNotationsChoices{} { } @@ -375,13 +376,15 @@ void NoteWriter::setStaffAndVoice() const myOutNote.setStaff(myCursor.staffIndex + 1); } - if (myCursor.voiceIndex >= 0) + const bool sourceHadVoice = myNoteData.userRequestedVoiceNumber != -1; + const bool isNonDefaultVoice = myCursor.voiceIndex > 0; + const bool isMultiVoiceStaff = myNumVoices > 1; + if (myCursor.voiceIndex >= 0 && (sourceHadVoice || isNonDefaultVoice || isMultiVoiceStaff)) { auto editorialVoice = myOutNote.editorialVoice(); editorialVoice.setVoice(std::to_string(myCursor.voiceIndex + 1)); myOutNote.setEditorialVoice(std::move(editorialVoice)); } - // TODO - only show voice number if it is needed i.e. only show if != 0 or voiceCount > 1 } void NoteWriter::setDurationNameAndDots() const diff --git a/src/private/mx/impl/NoteWriter.h b/src/private/mx/impl/NoteWriter.h index a9fed82e4..718615b9b 100644 --- a/src/private/mx/impl/NoteWriter.h +++ b/src/private/mx/impl/NoteWriter.h @@ -24,7 +24,8 @@ class NoteWriter { public: NoteWriter(const api::NoteData &inNoteData, const MeasureCursor &inCursor, const ScoreWriter &inScoreWriter, - bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, int inNoteIndex); + bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, int inNoteIndex, + int inNumVoices); core::Note getNote(bool isStartOfChord) const; @@ -36,6 +37,7 @@ class NoteWriter const bool myIsPreviousNoteAChordMember; const std::vector &mySiblingNotes; const int myNoteIndex; + const int myNumVoices; mutable core::Note myOutNote; mutable core::FullNoteGroup myOutFullNoteGroup; mutable std::vector myOutTies; diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index b6568588d..e873f5649 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -86,3 +86,10 @@ lysuite/ly45e_Repeats_Nested_Alternatives.xml # element in when the source omitted it. ClefData::isLineSpecified tracks # presence; PropertiesWriter emits only when the flag is set. lysuite/ly12a_Clefs.xml + +# Unblocked by #228 (add:voice): the writer no longer injects a spurious +# element for notes whose source omitted it. NoteData::userRequestedVoiceNumber +# (already -1 when absent) gates NoteWriter::setStaffAndVoice() voice emission. +mjbsuite/ChordDirectionPlacement.xml +mjbsuite/hello_timewise.xml +synthetic/key-accidental.3.0.xml From 671ac4dd9f3236477b83a0d2123398892c42e0ec Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Thu, 25 Jun 2026 21:33:27 +0000 Subject: [PATCH 4/4] chore: fmt --- src/private/mx/api/ClefData.cpp | 4 ++-- src/private/mx/impl/MeasureWriter.cpp | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/private/mx/api/ClefData.cpp b/src/private/mx/api/ClefData.cpp index 1fddde9cc..984eb1425 100644 --- a/src/private/mx/api/ClefData.cpp +++ b/src/private/mx/api/ClefData.cpp @@ -10,8 +10,8 @@ namespace mx namespace api { ClefData::ClefData() - : symbol{DEFAULT_CLEF_SYMBOL}, line{DEFAULT_CLEF_LINE}, isLineSpecified{true}, octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, - tickTimePosition{0}, location{ClefLocation::unspecified} + : symbol{DEFAULT_CLEF_SYMBOL}, line{DEFAULT_CLEF_LINE}, isLineSpecified{true}, + octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, tickTimePosition{0}, location{ClefLocation::unspecified} { } diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index 16e494c00..e5ee6a9d7 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -452,9 +452,13 @@ void MeasureWriter::writeVoices(const api::StaffData &inStaff) myPropertiesWriter->flushBuffer(); writeDirections(directionIter, directionEnd, noteIter, std::cbegin(voice.second.notes), noteEnd); - NoteWriter writer{ - apiNote, myHistory.getCursor(), myScoreWriter, myPreviousCursor.isChordActive, voice.second.notes, - noteIndex, numVoices}; + NoteWriter writer{apiNote, + myHistory.getCursor(), + myScoreWriter, + myPreviousCursor.isChordActive, + voice.second.notes, + noteIndex, + numVoices}; myOutMeasure.addMusicData(core::MusicDataChoice::note(writer.getNote(isStartOfChord))); myHistory.log("addNote cursorTime " + std::to_string(myHistory.getCursor().tickTimePosition) + ", noteTime " + std::to_string(apiNote.tickTimePosition));