From c055a4f40103c1cbec10ba4fa66b597ff6585c52 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 29 Jun 2026 20:02:06 +0200 Subject: [PATCH 1/5] fix: preserve clef-octave-change zero value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add isOctaveChangeSpecified to ClefData, mirroring the isLineSpecified pattern. The writer guard was != 0, dropping valid 0 elements on round-trip. Fixes #257. --- src/include/mx/api/ClefData.h | 2 ++ src/private/mx/api/ClefData.cpp | 4 ++-- src/private/mx/impl/MeasureReader.cpp | 2 ++ src/private/mx/impl/PropertiesWriter.cpp | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/include/mx/api/ClefData.h b/src/include/mx/api/ClefData.h index 0ae189fd..0f4bd383 100644 --- a/src/include/mx/api/ClefData.h +++ b/src/include/mx/api/ClefData.h @@ -45,6 +45,7 @@ class ClefData // had no element so the round-trip does not inject an implied default (#228). bool isLineSpecified; int octaveChange; + bool isOctaveChangeSpecified; int tickTimePosition; ClefLocation location; // Visibility of the clef via the MusicXML print-object attribute. @@ -77,6 +78,7 @@ MXAPI_EQUALS_MEMBER(symbol) MXAPI_EQUALS_MEMBER(line) MXAPI_EQUALS_MEMBER(isLineSpecified) MXAPI_EQUALS_MEMBER(octaveChange) +MXAPI_EQUALS_MEMBER(isOctaveChangeSpecified) MXAPI_EQUALS_MEMBER(tickTimePosition) MXAPI_EQUALS_MEMBER(location) MXAPI_EQUALS_MEMBER(printObject) diff --git a/src/private/mx/api/ClefData.cpp b/src/private/mx/api/ClefData.cpp index 6507d710..5a445181 100644 --- a/src/private/mx/api/ClefData.cpp +++ b/src/private/mx/api/ClefData.cpp @@ -11,8 +11,8 @@ namespace api { ClefData::ClefData() : symbol{DEFAULT_CLEF_SYMBOL}, line{DEFAULT_CLEF_LINE}, isLineSpecified{true}, - octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, tickTimePosition{0}, location{ClefLocation::unspecified}, - printObject{Bool::unspecified} + octaveChange{DEFAULT_CLEF_OCTAVE_CHANGE}, isOctaveChangeSpecified{true}, tickTimePosition{0}, + location{ClefLocation::unspecified}, printObject{Bool::unspecified} { } diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index 5e1dd1ba..b2740ce9 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -809,10 +809,12 @@ void MeasureReader::importClef(const core::Clef &inClef) const if (inClef.clef().clefOctaveChange().has_value()) { clefData.octaveChange = *inClef.clef().clefOctaveChange(); + clefData.isOctaveChangeSpecified = true; } else { clefData.octaveChange = 0; + clefData.isOctaveChangeSpecified = false; } if (inClef.printObject().has_value()) diff --git a/src/private/mx/impl/PropertiesWriter.cpp b/src/private/mx/impl/PropertiesWriter.cpp index c85a581b..b8d5d763 100644 --- a/src/private/mx/impl/PropertiesWriter.cpp +++ b/src/private/mx/impl/PropertiesWriter.cpp @@ -216,7 +216,7 @@ void PropertiesWriter::writeClef(int staffIndex, const api::ClefData &inClefData cg.setLine(core::StaffLinePosition{inClefData.line}); } - if (inClefData.octaveChange != 0) + if (inClefData.isOctaveChangeSpecified) { cg.setClefOctaveChange(inClefData.octaveChange); } From fab4dc08d00539bf7d7e06eb580a104d0745839b Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 29 Jun 2026 20:19:55 +0200 Subject: [PATCH 2/5] build: copy classifier output to host path dump-api-roundtrip and classify-api-roundtrip wrote output into the Docker volume, invisible on the host. Add a post-run copy step to extract roundtrip-dump/ and classified.json to the host build/ directory. --- Makefile | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Makefile b/Makefile index 158286bc..c4dd7911 100644 --- a/Makefile +++ b/Makefile @@ -435,9 +435,15 @@ discover-api-roundtrip: $(DOCKER_STAMP) docker-volume dump-api-roundtrip: $(DOCKER_STAMP) docker-volume $(DOCKER_RUN) make dump-api-roundtrip BUILD_TYPE=$(BUILD_TYPE) + @mkdir -p $(BUILD_ROOT)/api/roundtrip-dump + $(DOCKER) run --rm -v $(DOCKER_VOLUME):/vol:ro -v $(CURDIR)/$(BUILD_ROOT)/api/roundtrip-dump:/out \ + alpine sh -c 'cp -a /vol/api/roundtrip-dump/. /out/' classify-api-roundtrip: $(DOCKER_STAMP) docker-volume $(DOCKER_RUN) make classify-api-roundtrip BUILD_TYPE=$(BUILD_TYPE) + @mkdir -p $(BUILD_ROOT)/api + $(DOCKER) run --rm -v $(DOCKER_VOLUME):/vol:ro -v $(CURDIR)/$(BUILD_ROOT)/api:/out \ + alpine cp /vol/api/classified.json /out/classified.json coverage-api: $(DOCKER_STAMP) docker-volume @rm -rf $(COV_DIR)/api From e2d6a88cb6c1f7ea5d8107eb873254f429451c3f Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 29 Jun 2026 20:39:30 +0200 Subject: [PATCH 3/5] fix test: diatonic always has a value --- data/synthetic/double.3.0.xml | 1 + data/synthetic/double.4.0.xml | 1 + src/private/mx/impl/Converter.cpp | 14 ++++---------- src/private/mxtest/api/TranspositionTest.cpp | 8 ++++---- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/data/synthetic/double.3.0.xml b/data/synthetic/double.3.0.xml index 2e7d7f44..47cbdffc 100644 --- a/data/synthetic/double.3.0.xml +++ b/data/synthetic/double.3.0.xml @@ -13,6 +13,7 @@ + 1 1 diff --git a/data/synthetic/double.4.0.xml b/data/synthetic/double.4.0.xml index 131f7258..0c7de2ab 100644 --- a/data/synthetic/double.4.0.xml +++ b/data/synthetic/double.4.0.xml @@ -13,6 +13,7 @@ + 1 1 diff --git a/src/private/mx/impl/Converter.cpp b/src/private/mx/impl/Converter.cpp index ecf1e656..4b57da42 100644 --- a/src/private/mx/impl/Converter.cpp +++ b/src/private/mx/impl/Converter.cpp @@ -1708,18 +1708,12 @@ mx::core::Transpose Converter::convertToTranspose(const mx::api::TransposeData & group.setChromatic(mx::core::Semitones{mx::core::Decimal{static_cast(chromatic)}}); } - if (inTransposeData.diatonic != 0) + auto harmonic = inTransposeData.diatonic; + if (octave != 0) { - auto harmonic = inTransposeData.diatonic; - if (octave != 0) - { - harmonic += octave * -7; - } - if (harmonic != 0) - { - group.setDiatonic(harmonic); - } + harmonic += octave * -7; } + group.setDiatonic(harmonic); transpose.setTranspose(std::move(group)); return transpose; diff --git a/src/private/mxtest/api/TranspositionTest.cpp b/src/private/mxtest/api/TranspositionTest.cpp index 03f707f0..fe50f1d0 100644 --- a/src/private/mxtest/api/TranspositionTest.cpp +++ b/src/private/mxtest/api/TranspositionTest.cpp @@ -489,7 +489,7 @@ TEST(Conversion01, Transposition) const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic}; const auto converted = mx::impl::Converter::convertToTranspose(original); CHECK_EQUAL(expectedChromatic, static_cast(converted.transpose().chromatic().value().value())); - CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0)); + CHECK(converted.transpose().diatonic().has_value()); CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0)); CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0)); CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0)); @@ -509,7 +509,7 @@ TEST(Conversion02, Transposition) const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic}; const auto converted = mx::impl::Converter::convertToTranspose(original); CHECK_EQUAL(expectedChromatic, static_cast(converted.transpose().chromatic().value().value())); - CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0)); + CHECK(converted.transpose().diatonic().has_value()); CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0)); CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0)); CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0)); @@ -529,7 +529,7 @@ TEST(Conversion03, Transposition) const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic}; const auto converted = mx::impl::Converter::convertToTranspose(original); CHECK_EQUAL(expectedChromatic, static_cast(converted.transpose().chromatic().value().value())); - CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0)); + CHECK(converted.transpose().diatonic().has_value()); CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0)); CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0)); CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0)); @@ -549,7 +549,7 @@ TEST(Conversion04, Transposition) const auto original = mx::api::TransposeData{transposeDataChromatic, transposeDataDiatonic}; const auto converted = mx::impl::Converter::convertToTranspose(original); CHECK_EQUAL(expectedChromatic, static_cast(converted.transpose().chromatic().value().value())); - CHECK(converted.transpose().diatonic().has_value() == (expectedDiatonic != 0)); + CHECK(converted.transpose().diatonic().has_value()); CHECK_EQUAL(expectedDiatonic, converted.transpose().diatonic().value_or(0)); CHECK(converted.transpose().octaveChange().has_value() == (expectedOctave != 0)); CHECK_EQUAL(expectedOctave, converted.transpose().octaveChange().value_or(0)); From 81114cc6e4c171a03ad705d0cf217c2d3d8d162a Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 29 Jun 2026 22:25:26 +0200 Subject: [PATCH 4/5] fix: preserve stem position attributes on round-trip --- src/include/mx/api/NoteData.h | 2 + src/private/mx/impl/NoteFunctions.cpp | 1 + src/private/mx/impl/NoteWriter.cpp | 1 + src/private/mxtest/api/roundtrip-baseline.txt | 6 + .../mxtest/impl/PositionFunctionsTest.cpp | 135 ++++++++++++++++++ 5 files changed, 145 insertions(+) diff --git a/src/include/mx/api/NoteData.h b/src/include/mx/api/NoteData.h index afabcdc7..99963d24 100644 --- a/src/include/mx/api/NoteData.h +++ b/src/include/mx/api/NoteData.h @@ -103,6 +103,7 @@ class NoteData PitchData pitchData; // step, alter, octave, accidental, etc int userRequestedVoiceNumber; Stem stem; + PositionData stemPositionData; // the time location of the note, within the measure, denominated in ticksPerQuarter which // is defined in ScoreData. in each measure, the note with tickTimePosition 0 is located at @@ -143,6 +144,7 @@ MXAPI_EQUALS_MEMBER(noteType) MXAPI_EQUALS_MEMBER(pitchData) MXAPI_EQUALS_MEMBER(userRequestedVoiceNumber) MXAPI_EQUALS_MEMBER(stem) +MXAPI_EQUALS_MEMBER(stemPositionData) MXAPI_EQUALS_MEMBER(tickTimePosition) MXAPI_EQUALS_MEMBER(durationData) MXAPI_EQUALS_MEMBER(beams) diff --git a/src/private/mx/impl/NoteFunctions.cpp b/src/private/mx/impl/NoteFunctions.cpp index 3bc2c5b4..6dd6ceab 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -135,6 +135,7 @@ api::NoteData NoteFunctions::parseNote() const if (reader.getIsStemSpecified()) { myOutNoteData.stem = converter.convert(reader.getStem()); + myOutNoteData.stemPositionData = impl::getPositionData(*myNote.stem()); } myOutNoteData.isTieStart = reader.getIsTieStart(); myOutNoteData.isTieStop = reader.getIsTieStop(); diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index a1ee7549..c997504c 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -422,6 +422,7 @@ void NoteWriter::setStemDirection() const core::Stem stem; stem.setValue(myConverter.convert(myNoteData.stem)); + impl::setAttributesFromPositionData(myNoteData.stemPositionData, stem); myOutNote.setStem(std::move(stem)); } diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index bfc9a3a0..36e1a194 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -149,3 +149,9 @@ musuite/testVoiceMapper3_ref.xml musuite/testVoicePiano1.xml musuite/testVolta1.xml musuite/testWords1.xml + +# Unblocked by #263 (clef-octave-change zero), #264 (diatonic zero), #265 (stem +# default-y zero): the zero-as-unset guard bugs in PropertiesWriter, Converter, +# and NoteWriter dropped valid zero values on round-trip. +custom/transposition.musicxml +ksuite/k002a_Fermatas.xml diff --git a/src/private/mxtest/impl/PositionFunctionsTest.cpp b/src/private/mxtest/impl/PositionFunctionsTest.cpp index 2b3b108d..0179bb13 100644 --- a/src/private/mxtest/impl/PositionFunctionsTest.cpp +++ b/src/private/mxtest/impl/PositionFunctionsTest.cpp @@ -6,10 +6,14 @@ #ifdef MX_COMPILE_IMPL_TESTS #include "cpul/cpulTestHarness.h" +#include "mx/api/DocumentManager.h" #include "mx/core/generated/Bracket.h" #include "mx/core/generated/Direction.h" +#include "mx/core/generated/Stem.h" #include "mx/impl/PositionFunctions.h" +#include + using namespace mx; using namespace mx::impl; @@ -76,4 +80,135 @@ TEST(setAttributesFromPositionDataDirectionAttributes, PositionFunctions) T_END +TEST(setAttributesFromPositionDataStem, PositionFunctions) +{ + core::Stem stem; + api::PositionData positionData; + + positionData.isDefaultYSpecified = true; + positionData.defaultY = 5.0; + positionData.isRelativeYSpecified = true; + positionData.relativeY = -3.0; + + impl::setAttributesFromPositionData(positionData, stem); + + CHECK(stem.defaultY().has_value()); + CHECK_DOUBLES_EQUAL(5.0, stem.defaultY()->value().value(), 0.0001); + CHECK(stem.relativeY().has_value()); + CHECK_DOUBLES_EQUAL(-3.0, stem.relativeY()->value().value(), 0.0001); + CHECK(!stem.defaultX().has_value()); + CHECK(!stem.relativeX().has_value()); +} + +T_END + +TEST(setAttributesFromPositionDataStemZero, PositionFunctions) +{ + core::Stem stem; + api::PositionData positionData; + + positionData.isDefaultYSpecified = true; + positionData.defaultY = 0.0; + + impl::setAttributesFromPositionData(positionData, stem); + + CHECK(stem.defaultY().has_value()); + CHECK_DOUBLES_EQUAL(0.0, stem.defaultY()->value().value(), 0.0001); +} + +T_END + +TEST(getPositionDataStem, PositionFunctions) +{ + core::Stem stem; + stem.setDefaultY(core::Tenths{core::Decimal{0.0}}); + stem.setRelativeY(core::Tenths{core::Decimal{-2.5}}); + + auto positionData = impl::getPositionData(stem); + + CHECK(positionData.isDefaultYSpecified); + CHECK_DOUBLES_EQUAL(0.0, positionData.defaultY, 0.0001); + CHECK(positionData.isRelativeYSpecified); + CHECK_DOUBLES_EQUAL(-2.5, positionData.relativeY, 0.0001); + CHECK(!positionData.isDefaultXSpecified); + CHECK(!positionData.isRelativeXSpecified); +} + +T_END + +TEST(stemPositionRoundTrip, PositionFunctions) +{ + core::Stem original; + original.setDefaultY(core::Tenths{core::Decimal{0.0}}); + + auto positionData = impl::getPositionData(original); + CHECK(positionData.isDefaultYSpecified); + + core::Stem roundTripped; + impl::setAttributesFromPositionData(positionData, roundTripped); + + CHECK(roundTripped.defaultY().has_value()); + CHECK_DOUBLES_EQUAL(0.0, roundTripped.defaultY()->value().value(), 0.0001); +} + +T_END + +TEST(stemDefaultYZeroApiRoundTrip, PositionFunctions) +{ + const char *xml = R"( + + + + + + + + + 1 + + G2 + + + C4 + 4 + whole + up + + + + +)"; + + auto &mgr = api::DocumentManager::getInstance(); + std::istringstream iss{xml}; + auto docIdResult = mgr.createFromStream(iss); + CHECK(docIdResult.ok()); + auto docId = docIdResult.value(); + + auto scoreResult = mgr.getData(docId); + CHECK(scoreResult.ok()); + auto scoreData = scoreResult.value(); + mgr.destroyDocument(docId); + + // Verify the reader populated stemPositionData + const auto ¬e = scoreData.parts.at(0).measures.at(0).staves.at(0).voices.at(0).notes.at(0); + CHECK(note.stem == api::Stem::up); + CHECK(note.stemPositionData.isDefaultYSpecified); + CHECK_DOUBLES_EQUAL(0.0, note.stemPositionData.defaultY, 0.0001); + + // Round-trip back to XML + auto docId2Result = mgr.createFromScore(scoreData); + CHECK(docId2Result.ok()); + auto docId2 = docId2Result.value(); + std::stringstream ss; + mgr.writeToStream(docId2, ss); + mgr.destroyDocument(docId2); + + const auto output = ss.str(); + CHECK(output.find("default-y") != std::string::npos); +} + +T_END + #endif From 6bc8e9d298e3f85de0a3de7348c8ff712b96cc55 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 29 Jun 2026 22:25:31 +0200 Subject: [PATCH 5/5] chore: gitignore build-* directories --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 33c1fac8..c1dc3291 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ build/ +build-*/ cmake-build-* src/private/mxtest/file/PathRoot.h *.DS_Store