diff --git a/src/include/mx/api/NoteData.h b/src/include/mx/api/NoteData.h index 19fc1001..f5a8692a 100644 --- a/src/include/mx/api/NoteData.h +++ b/src/include/mx/api/NoteData.h @@ -135,6 +135,15 @@ class NoteData PitchData pitchData; // step, alter, octave, accidental, etc int userRequestedVoiceNumber; + // Most users can ignore this; leave it unspecified. It only controls whether the note's + // optional element is written. unspecified (the default) applies the right rule + // automatically: omit on a single-staff part (where 1 is implied) and include it + // otherwise. yes/no force the element on or off, except that no is ignored for a note that + // is not on the first staff (omitting there would move the note to staff 1). It + // exists for round-trip fidelity - reading a file sets yes/no only when the source diverged + // from the automatic rule. Same convention as ClefData::writeStaffNumber. + Bool writeStaffNumber; + // The zero-based index of the staff on which this note is displayed, for the rare case // that it differs from the staff that contains the note (cross-staff notation, e.g. a // beamed piano run or a chord that dips onto the other staff). The note stays in its @@ -189,6 +198,7 @@ MXAPI_EQUALS_MEMBER(isCue) MXAPI_EQUALS_MEMBER(graceSlash) MXAPI_EQUALS_MEMBER(pitchData) MXAPI_EQUALS_MEMBER(userRequestedVoiceNumber) +MXAPI_EQUALS_MEMBER(writeStaffNumber) MXAPI_EQUALS_MEMBER(crossStaffIndex) MXAPI_EQUALS_MEMBER(stem) MXAPI_EQUALS_MEMBER(stemPositionData) diff --git a/src/private/mx/api/NoteData.cpp b/src/private/mx/api/NoteData.cpp index 20d51b4a..619b2398 100644 --- a/src/private/mx/api/NoteData.cpp +++ b/src/private/mx/api/NoteData.cpp @@ -11,8 +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}, stem{Stem::unspecified}, - tickTimePosition{0}, durationData{}, beams{}, positionData{}, printData{}, noteAttachmentData{}, lyrics{} + notehead{Notehead::normal}, 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 d8a52bbe..d9392424 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -78,6 +78,19 @@ api::NoteData NoteFunctions::parseNote() const myOutNoteData.pitchData.octave = reader.getOctave(); myOutNoteData.userRequestedVoiceNumber = reader.getVoiceNumber(); + // Auto rule (see NoteData::writeStaffNumber): is included on a multi-staff part and + // omitted on a single-staff part. Record an override only when the source diverges from that + // rule, so the common case stays unspecified. + const bool staffAutoIncludes = myCursor.getNumStaves() > 1; + if (reader.getIsStaffSpecified() && !staffAutoIncludes) + { + myOutNoteData.writeStaffNumber = api::Bool::yes; + } + else if (!reader.getIsStaffSpecified() && staffAutoIncludes) + { + myOutNoteData.writeStaffNumber = api::Bool::no; + } + myOutNoteData.notehead = converter.convert(reader.getNoteheadValue()); if (reader.getIsDurationTypeSpecified()) diff --git a/src/private/mx/impl/NoteReader.cpp b/src/private/mx/impl/NoteReader.cpp index 4686e8c8..6516861a 100644 --- a/src/private/mx/impl/NoteReader.cpp +++ b/src/private/mx/impl/NoteReader.cpp @@ -144,8 +144,8 @@ NoteReader::NoteReader(const core::Note &mxNote) : myNote(mxNote), myNoteChoice(myNote.choice()), myFullNoteGroup(findFullNoteGroup(myNoteChoice)), 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), myVoiceNumber(0), - myNoteheadValue(core::NoteheadValue::normal()), myDurationType(core::NoteTypeValue::maxima()), + 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()), @@ -304,7 +304,8 @@ void NoteReader::setChord() void NoteReader::setStaffNumber() { - if (myNote.staff().has_value()) + myIsStaffSpecified = myNote.staff().has_value(); + if (myIsStaffSpecified) { myStaffNumber = *myNote.staff(); } diff --git a/src/private/mx/impl/NoteReader.h b/src/private/mx/impl/NoteReader.h index 8ecc3f1a..a5f51f50 100644 --- a/src/private/mx/impl/NoteReader.h +++ b/src/private/mx/impl/NoteReader.h @@ -117,6 +117,11 @@ class NoteReader return myStaffNumber; } + inline bool getIsStaffSpecified() const + { + return myIsStaffSpecified; + } + inline int getVoiceNumber() const { return myVoiceNumber; @@ -251,6 +256,7 @@ class NoteReader double myCents; int myOctave; int myStaffNumber; + bool myIsStaffSpecified; int myVoiceNumber; core::NoteheadValue myNoteheadValue; core::NoteTypeValue myDurationType; diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index 06b19d2f..4f8efd99 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -427,9 +427,25 @@ void NoteWriter::setStaffAndVoice() const // position, but the displayed staff comes from the override (NoteData.h) myOutNote.setStaff(*myNoteData.crossStaffIndex + 1); } - else if (myCursor.getNumStaves() > 1 && myCursor.staffIndex >= 0) + else if (myCursor.staffIndex >= 0) { - myOutNote.setStaff(myCursor.staffIndex + 1); + // Auto rule: is structurally required on a multi-staff part and implied (1) on a + // single-staff part. writeStaffNumber forces the decision either way (round-trip + // fidelity), except that no cannot suppress off the first staff - MusicXML would + // read the omission as staff 1. + bool includeStaff = myCursor.getNumStaves() > 1; + if (myNoteData.writeStaffNumber == api::Bool::yes) + { + includeStaff = true; + } + else if (myNoteData.writeStaffNumber == api::Bool::no && myCursor.staffIndex == 0) + { + includeStaff = false; + } + if (includeStaff) + { + myOutNote.setStaff(myCursor.staffIndex + 1); + } } const bool sourceHadVoice = myNoteData.userRequestedVoiceNumber != api::VALUE_UNSPECIFIED; diff --git a/src/private/mxtest/api/NoteDataTest.cpp b/src/private/mxtest/api/NoteDataTest.cpp index b63741fa..4bf2ce03 100644 --- a/src/private/mxtest/api/NoteDataTest.cpp +++ b/src/private/mxtest/api/NoteDataTest.cpp @@ -12,6 +12,7 @@ #include "mx/core/generated/Document.h" #include "mx/core/generated/MusicDataChoice.h" #include "mxtest/api/RoundTrip.h" +#include "mxtest/api/TestHelpers.h" #include "mxtest/file/Path.h" using namespace std; @@ -1557,4 +1558,137 @@ TEST(strongAccentDirection, NoteData) T_END; +TEST(explicitStaffOnSingleStaffPartRoundTrips, NoteData) +{ + const std::string xml = R"( + + + + x + + + + + + 1 + + + C4 + 1 + 1 + quarter + 1 + + + + +)"; + + const auto score = fromXml(xml); + const auto ¬es = score.parts.back().measures.back().staves.back().voices.at(0).notes; + REQUIRE(notes.size() == 1); + CHECK(notes.at(0).writeStaffNumber == Bool::yes); + + const auto outXml = toXml(score); + CHECK(outXml.find("1") != std::string::npos); +} + +T_END; + +TEST(omittedStaffOnMultiStaffPartRoundTrips, NoteData) +{ + const std::string xml = R"( + + + + x + + + + + + 1 + 2 + + + C5 + 1 + 1 + quarter + + 1 + + C3 + 1 + 1 + quarter + 2 + + + + +)"; + + const auto score = fromXml(xml); + const auto &staves = score.parts.back().measures.back().staves; + REQUIRE(staves.size() == 2); + const auto &firstStaffNotes = staves.at(0).voices.at(0).notes; + const auto &secondStaffNotes = staves.at(1).voices.at(0).notes; + REQUIRE(firstStaffNotes.size() == 1); + REQUIRE(secondStaffNotes.size() == 1); + CHECK(firstStaffNotes.at(0).writeStaffNumber == Bool::no); + CHECK(secondStaffNotes.at(0).writeStaffNumber == Bool::unspecified); + + const auto outXml = toXml(score); + CHECK(outXml.find("1") == std::string::npos); + CHECK(outXml.find("2") != std::string::npos); +} + +T_END; + +TEST(writeStaffNumberNoIsIgnoredOffFirstStaff, NoteData) +{ + ScoreData score; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + measure.staves.emplace_back(); + + NoteData note; + note.tickTimePosition = 0; + note.durationData.durationTimeTicks = score.ticksPerQuarter; + note.durationData.durationName = DurationName::quarter; + note.writeStaffNumber = Bool::no; + measure.staves.at(1).voices[0].notes.push_back(note); + + const auto xml = toXml(score); + CHECK(xml.find("2") != std::string::npos); +} + +T_END; + +TEST(implicitStaffOnSingleStaffPartOmitsElement, NoteData) +{ + 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 &voice = measure.staves.back().voices[0]; + + NoteData note; + note.tickTimePosition = 0; + note.durationData.durationTimeTicks = score.ticksPerQuarter; + note.durationData.durationName = DurationName::quarter; + voice.notes.push_back(note); + + const auto xml = toXml(score); + CHECK(xml.find("") == std::string::npos); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index ad7f81df..8bb4082a 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -318,6 +318,24 @@ lysuite/ly11h_TimeSignatures_SenzaMisura.xml # composite time-signature round-trip in isolation from those. rpatters1/timesigs_composite-ref-modified.musicxml +# Unblocked by NoteData::writeStaffNumber: NoteWriter only wrote +# for multi-staff parts, dropping a source's explicit (but structurally +# redundant) on a single-staff part. +lysuite/ly24c_GraceNote_MeasureEnd.xml + +# Also unblocked by NoteData::writeStaffNumber, in the other direction: a +# two-staff part whose last note legally omits (implied 1). The +# writer used to inject 1 there; the reader now records +# writeStaffNumber = no and the omission round-trips. +lysuite/ly02e_Rests_NoType.xml + +# Found already passing when discovery was re-run for the writeStaffNumber +# work; unlocked by earlier merged changes and never pinned. Pinned here to +# defend them. +custom/systems-and-pages.xml +ksuite/k008a_Beaming.xml +ksuite/k014a_Fermatas.xml + # The original of the file above, now unblocked by PartData::groups # (/), MeasureData's measureNumbering multiple-rest-*/ # system-relation attributes, and SoundData::swing (/). Its