diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index e03f071de..1c2908b30 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -526,14 +526,13 @@ void MeasureWriter::writeVoices(const api::StaffData &inStaff) myHistory.setVoiceIndex(voice.first); auto noteIter = voice.second.notes.cbegin(); auto noteEnd = voice.second.notes.cend(); - int noteIndex = 0; if (directionIter != directionEnd) { writeDirections(directionIter, directionEnd, noteIter, std::cbegin(voice.second.notes), noteEnd); } - for (; noteIter != noteEnd; ++noteIter, ++noteIndex) + for (; noteIter != noteEnd; ++noteIter) { bool isStartOfChord = false; @@ -596,14 +595,9 @@ 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, - voice.second.label}; + NoteWriter writer{ + apiNote, myHistory.getCursor(), myScoreWriter, myPreviousCursor.isChordActive, voice.second.notes, + 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/NoteFunctions.cpp b/src/private/mx/impl/NoteFunctions.cpp index 0e735899d..6f1a61c3e 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -134,22 +134,14 @@ api::NoteData NoteFunctions::parseNote() const myOutNoteData.durationData.timeModificationActualNotes = reader.getTimeModificationActualNotes(); myOutNoteData.durationData.timeModificationNormalNotes = reader.getTimeModificationNormalNotes(); - myOutNoteData.durationData.timeModificationNormalType = converter.convert(reader.getTimeModificationNormalType()); - myOutNoteData.durationData.timeModificationNormalTypeDots = reader.getTimeModificationNormalTypeDots(); - - const core::NoteTypeValue timeModType = reader.getTimeModificationNormalType(); - const int timeModTypeDots = reader.getTimeModificationNormalTypeDots(); - // TODO: should this be ||? Either conjunct alone proves was present (the - // reader defaults normalType to the note's own type with 0 dots when absent), so && drops - // an explicit undotted , and a dotted one equal to the note's own type, as - // "unspecified". Also, the two assignments above are dead stores -- both branches below - // overwrite them. Issue candidate. - bool isTimeModTypeSpecified = (timeModTypeDots > 0) && (timeModType != reader.getDurationType()); - - if (isTimeModTypeSpecified) + + // An absent maps to unspecified rather than to the reader's derived default, + // so the writer can tell the difference and does not invent the element on output (#428). + if (reader.getIsTimeModificationNormalTypeSpecified()) { - myOutNoteData.durationData.timeModificationNormalType = converter.convert(timeModType); - myOutNoteData.durationData.timeModificationNormalTypeDots = timeModTypeDots; + myOutNoteData.durationData.timeModificationNormalType = + converter.convert(reader.getTimeModificationNormalType()); + myOutNoteData.durationData.timeModificationNormalTypeDots = reader.getTimeModificationNormalTypeDots(); } else { diff --git a/src/private/mx/impl/NoteReader.cpp b/src/private/mx/impl/NoteReader.cpp index 191d0b83b..4078c857b 100644 --- a/src/private/mx/impl/NoteReader.cpp +++ b/src/private/mx/impl/NoteReader.cpp @@ -158,7 +158,8 @@ NoteReader::NoteReader(const core::Note &mxNote) 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), + myIsTimeModificationNormalTypeSpecified(false), 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} @@ -425,6 +426,7 @@ void NoteReader::setTimeModification() const auto &grp = *mxTimeMod.group(); myTimeModificationNormalType = grp.normalType(); myTimeModificationNormalTypeDots = static_cast(grp.normalDot().size()); + myIsTimeModificationNormalTypeSpecified = true; } else { diff --git a/src/private/mx/impl/NoteReader.h b/src/private/mx/impl/NoteReader.h index 44e560f3a..693e00b91 100644 --- a/src/private/mx/impl/NoteReader.h +++ b/src/private/mx/impl/NoteReader.h @@ -191,6 +191,13 @@ class NoteReader return myTimeModificationNormalTypeDots; } + // True when the carried a element. When false, the + // normal type getters return a default derived from the note's own type. + inline bool getIsTimeModificationNormalTypeSpecified() const + { + return myIsTimeModificationNormalTypeSpecified; + } + inline bool getHasAccidental() const { return myHasAccidental; @@ -289,6 +296,7 @@ class NoteReader int myTimeModificationNormalNotes; core::NoteTypeValue myTimeModificationNormalType; int myTimeModificationNormalTypeDots; + bool myIsTimeModificationNormalTypeSpecified; bool myHasAccidental; core::AccidentalValue myAccidental; bool myIsAccidentalParenthetical; diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index f82ab9ed9..f8debbb7b 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -39,10 +39,8 @@ #include "mx/impl/PrintFunctions.h" #include "mx/impl/ScoreWriter.h" #include "mx/impl/WriteRefusal.h" -#include "mx/utility/Throw.h" #include -#include namespace mx { @@ -69,11 +67,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, const std::string &inVoiceLabel) + int inNumVoices, const std::string &inVoiceLabel) : myNoteData{inNoteData}, myCursor{inCursor}, myScoreWriter{inScoreWriter}, myConverter{}, myIsPreviousNoteAChordMember{isPreviousNoteAChordMember}, mySiblingNotes{inSiblingNotes}, - myNoteIndex{inNoteIndex}, myNumVoices{inNumVoices}, myVoiceLabel{inVoiceLabel}, myOutNote{}, myOutFullNoteGroup{}, - myOutTies{}, myOutTieNotationsChoices{} + myNumVoices{inNumVoices}, myVoiceLabel{inVoiceLabel}, myOutNote{}, myOutFullNoteGroup{}, myOutTies{}, + myOutTieNotationsChoices{} { } @@ -179,109 +177,24 @@ core::Note NoteWriter::getNote(bool isStartOfChord) const timeMod.setActualNotes(myNoteData.durationData.timeModificationActualNotes); timeMod.setNormalNotes(myNoteData.durationData.timeModificationNormalNotes); - // find the tuplet start note and TupletStart object - bool isTupletStartFound = false; - int tupletStartIndex = myNoteIndex; - mx::api::NoteData tupletStartNote{}; - mx::api::TupletStart tupletStart{}; - - for (; tupletStartIndex >= 0 && mySiblingNotes.size() > 0; --tupletStartIndex) - { - const auto &siblingNote = mySiblingNotes.at(static_cast(tupletStartIndex)); - if (siblingNote.noteAttachmentData.tupletStarts.size() == 1) - { - isTupletStartFound = true; - tupletStartNote = siblingNote; - tupletStart = siblingNote.noteAttachmentData.tupletStarts.at(0); - break; - } - } - - // TODO: is recomputed from sibling TupletStart/TupletStop geometry; - // DurationData.timeModificationNormalType is never consulted as a fallback. Tuplets - // expressed only via (no notations -- legal MusicXML) - // hit this no-op throw in release builds and silently lose . Issue candidate. - if (!isTupletStartFound) - { - MX_DEBUG_THROW("tupletStart was not found"); - } - - // find the tuplet stop note and TupletStop object - bool isTupletStopFound = false; - int tupletStopIndex = tupletStartIndex; - mx::api::NoteData tupletStopNote{}; - mx::api::TupletStop tupletStop{}; - - if (isTupletStartFound) - { - for (; tupletStopIndex < static_cast(mySiblingNotes.size()); ++tupletStopIndex) - { - const auto &siblingNote = mySiblingNotes.at(static_cast(tupletStopIndex)); - if (siblingNote.noteAttachmentData.tupletStops.size() == 1) - { - isTupletStopFound = true; - tupletStopNote = siblingNote; - tupletStop = siblingNote.noteAttachmentData.tupletStops.at(0); - break; - } - } - } - - if (!isTupletStopFound) + // is the author's statement, taken straight from DurationData. When it + // is unspecified it is omitted, and MusicXML reads the absence as the note's own + // (#428). + if (myNoteData.durationData.timeModificationNormalType != api::DurationName::unspecified) { - MX_DEBUG_THROW("tupletStop was not found"); - } + core::TimeModificationGroup group; + group.setNormalType(myConverter.convert(myNoteData.durationData.timeModificationNormalType)); - // calculate the distance between the two - if (isTupletStartFound && isTupletStopFound) - { - const auto tickTimeDistance = - (tupletStopNote.tickTimePosition + tupletStopNote.durationData.durationTimeTicks) - - tupletStartNote.tickTimePosition; - - if (tickTimeDistance > 0 && tupletStart.normalNumber != 0 && tupletStart.actualNumber != 0) - { - // calculate the tuplet normal type and dots based on the distance between start and stop and the ratio - const double normalLength = - static_cast(tickTimeDistance) / static_cast(tupletStart.normalNumber); - - mx::api::DurationName normalName = mx::api::DurationName::unspecified; - int normalDots = 0; - const bool isNormalNameAndDotsFound = this->findNormalNameAndDots(normalName, normalDots, normalLength); - if (isNormalNameAndDotsFound) - { - core::TimeModificationGroup group; - group.setNormalType(myConverter.convert(normalName)); - - std::vector normalDotsVec; - for (int i = 0; i < normalDots; ++i) - { - normalDotsVec.emplace_back(); - } - group.setNormalDot(std::move(normalDotsVec)); - timeMod.setGroup(std::move(group)); - } - else - { - MX_DEBUG_THROW("this->findNormalNameAndDots could not find what it was looking for. This probably " - "means that the file has a badly specified tuplet."); - } - } - else + std::vector normalDots; + for (int i = 0; i < myNoteData.durationData.timeModificationNormalTypeDots; ++i) { - MX_DEBUG_THROW("one of these things was not true ( tickTimeDistance > 0 && tupletStart.normalNumber != " - "0 && tupletStart.actualNumber != 0 )"); + normalDots.emplace_back(); } - } - else - { - MX_DEBUG_THROW("one of these things was not true ( isTupletStartFound && isTupletStopFound )"); + group.setNormalDot(std::move(normalDots)); + timeMod.setGroup(std::move(group)); } myOutNote.setTimeModification(std::move(timeMod)); - - // TODO - decide what happens if the user entered specific tuplet type in the - // duration data, possibly remove those fields from duration data. } setLyrics(); @@ -730,85 +643,5 @@ void NoteWriter::setMiscData() const editorialVoice.setFootnote(std::move(footnote)); myOutNote.setEditorialVoice(std::move(editorialVoice)); } - -bool NoteWriter::findNormalNameAndDots(mx::api::DurationName &ioName, int &ioDots, double inTickLength) const -{ - const auto equals = [&](double a, double b) { return std::abs(a - b) < 0.0001; }; - - const auto isMatch = [&](double durQuarters, int numDots, mx::api::DurationName name) { - if (equals(mx::api::applyDots(durQuarters * static_cast(myCursor.ticksPerQuarter), numDots), - inTickLength)) - { - ioName = name; - ioDots = numDots; - return true; - } - - return false; - }; - - for (int dots = 0; dots < 4; ++dots) - { - - if (isMatch(mx::api::DUR_QUARTERS_VALUE_QUARTER, dots, mx::api::DurationName::quarter)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_EIGHTH, dots, mx::api::DurationName::eighth)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_16TH, dots, mx::api::DurationName::dur16th)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_MAXIMA, dots, mx::api::DurationName::maxima)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_LONGA, dots, mx::api::DurationName::longa)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_BREVE, dots, mx::api::DurationName::breve)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_WHOLE, dots, mx::api::DurationName::whole)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_HALF, dots, mx::api::DurationName::half)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_32ND, dots, mx::api::DurationName::dur32nd)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_64TH, dots, mx::api::DurationName::dur64th)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_128TH, dots, mx::api::DurationName::dur128th)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_256TH, dots, mx::api::DurationName::dur256th)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_512TH, dots, mx::api::DurationName::dur512th)) - { - return true; - } - else if (isMatch(mx::api::DUR_QUARTERS_VALUE_1024TH, dots, mx::api::DurationName::dur1024th)) - { - return true; - } - } - - return false; -} } // namespace impl } // namespace mx diff --git a/src/private/mx/impl/NoteWriter.h b/src/private/mx/impl/NoteWriter.h index 6b961ecde..8ea602f0d 100644 --- a/src/private/mx/impl/NoteWriter.h +++ b/src/private/mx/impl/NoteWriter.h @@ -25,8 +25,8 @@ class NoteWriter { public: NoteWriter(const api::NoteData &inNoteData, const MeasureCursor &inCursor, const ScoreWriter &inScoreWriter, - bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, int inNoteIndex, - int inNumVoices, const std::string &inVoiceLabel); + bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, int inNumVoices, + const std::string &inVoiceLabel); core::Note getNote(bool isStartOfChord) const; @@ -37,7 +37,6 @@ class NoteWriter const Converter myConverter; const bool myIsPreviousNoteAChordMember; const std::vector &mySiblingNotes; - const int myNoteIndex; const int myNumVoices; const std::string &myVoiceLabel; mutable core::Note myOutNote; @@ -57,7 +56,6 @@ class NoteWriter void setStemDirection() const; void setLyrics() const; void setMiscData() const; - bool findNormalNameAndDots(mx::api::DurationName &ioName, int &ioDots, double inTickLength) const; }; } // namespace impl } // namespace mx diff --git a/src/private/mxtest/api/FreezingRoundTrip.cpp b/src/private/mxtest/api/FreezingRoundTrip.cpp index ad3ac27e4..1a4b578a1 100644 --- a/src/private/mxtest/api/FreezingRoundTrip.cpp +++ b/src/private/mxtest/api/FreezingRoundTrip.cpp @@ -531,12 +531,23 @@ TEST(checkMissingNormalTypeSimple, Freezing) { const auto testData = getTestData(tupletType); const auto musicData = testData.getMusicData(0, 0); + + // The first note's eighth differs from its own and + // must survive the round trip. const auto &mdc1 = musicData.second[1]; REQUIRE(mdc1.isNote()); const auto ¬e1 = mdc1.asNote(); REQUIRE(note1.timeModification().has_value()); REQUIRE(note1.timeModification()->group().has_value()); CHECK(mx::core::NoteTypeValue::Tag::eighth == note1.timeModification()->group()->normalType().tag()); + + // The second note has no in the source, and the writer must not invent + // one (#428). + const auto &mdc2 = musicData.second[2]; + REQUIRE(mdc2.isNote()); + const auto ¬e2 = mdc2.asNote(); + REQUIRE(note2.timeModification().has_value()); + CHECK(!note2.timeModification()->group().has_value()); } TEST(checkMissingNormalType, Freezing) @@ -585,21 +596,33 @@ TEST(checkMissingNormalType, Freezing) { const auto &savedNote = savedNotes[i]->asNote(); REQUIRE(savedNote.timeModification().has_value()); - CHECK(savedNote.timeModification()->group().has_value()); + // presence must match the original: present where the source + // wrote one, absent where it did not (#428). const auto &origNote = originalNotes[i]->asNote(); - if (origNote.timeModification()->group().has_value()) + const bool origHasGroup = origNote.timeModification()->group().has_value(); + const bool savedHasGroup = savedNote.timeModification()->group().has_value(); + + { + std::stringstream message2; + message2 << "( origHasGroup == savedHasGroup ), "; + message2 << "( " << origHasGroup << " == " << savedHasGroup << " ), "; + message2 << " partIndex = " << partIndex << ", measureIndex = " << measureIndex; + CHECK_WITH_MESSAGE(origHasGroup == savedHasGroup, message2.str()); + } + + if (origHasGroup && savedHasGroup) { const auto originalType = origNote.timeModification()->group()->normalType(); const auto savedType = savedNote.timeModification()->group()->normalType(); if (originalType != savedType) { - std::stringstream message2; - message2 << "( originalType == savedType ), "; - message2 << "( " << originalType.toString() << " == " << savedType.toString() << " ), "; - message2 << " partIndex = " << partIndex << ", measureIndex = " << measureIndex; - CHECK_WITH_MESSAGE(originalType == savedType, message2.str()); + std::stringstream message3; + message3 << "( originalType == savedType ), "; + message3 << "( " << originalType.toString() << " == " << savedType.toString() << " ), "; + message3 << " partIndex = " << partIndex << ", measureIndex = " << measureIndex; + CHECK_WITH_MESSAGE(originalType == savedType, message3.str()); } } }