From ea2dffcc0e8d357eb2e9e04c6923691ba5e0f5ed Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 29 Jun 2026 23:37:52 +0200 Subject: [PATCH 1/2] Clean dump dir before re-dumping roundtrips dump-api-roundtrip only writes pairs for failing files, so a file that flips to passing leaves a stale pair behind. The classifier then reads it as a current failure. rm -rf the dump dir, both in the build volume and the host copy, before each dump. --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index c4dd7911..c21b2e11 100644 --- a/Makefile +++ b/Makefile @@ -186,6 +186,7 @@ discover-api-roundtrip: dev # Output goes to build/api/roundtrip-dump/ (build dir, already gitignored). # Feeds the classifier: make dump-api-roundtrip && make classify-api-roundtrip dump-api-roundtrip: dev + rm -rf $(BUILD_ROOT)/api/roundtrip-dump mkdir -p $(BUILD_ROOT)/api/roundtrip-dump $(BUILD_ROOT)/api/mxtest-api-roundtrip discovery $(CURDIR)/data \ --dump $(CURDIR)/$(BUILD_ROOT)/api/roundtrip-dump @@ -435,6 +436,7 @@ discover-api-roundtrip: $(DOCKER_STAMP) docker-volume dump-api-roundtrip: $(DOCKER_STAMP) docker-volume $(DOCKER_RUN) make dump-api-roundtrip BUILD_TYPE=$(BUILD_TYPE) + @rm -rf $(BUILD_ROOT)/api/roundtrip-dump @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/' From e81764270f42cb008b9fced61690e4a9aaf6c5af Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 29 Jun 2026 23:37:52 +0200 Subject: [PATCH 2/2] Fix clef @number round-trip (#230) Stop emitting a spurious number="1" on single-staff parts and preserve the source's choice to include or omit the attribute. The writer derived the staff number from position but, for mid-measure clef changes in single-staff parts, passed the staff index straight through instead of suppressing it the way the measure-start path already did. Centralize the single/multi-staff rule in clefStaffIndex(), and add ClefData::writeStaffNumber - a ternary defaulting to an automatic rule (omit the implied 1 on a single-staff part, include otherwise) that round-trip uses to preserve a source that diverged from it. Grows the api round-trip baseline by 23 clef files. --- src/include/mx/api/ClefData.h | 9 +++- src/private/mx/api/ClefData.cpp | 2 +- src/private/mx/impl/MeasureReader.cpp | 16 ++++--- src/private/mx/impl/MeasureWriter.cpp | 11 ++--- src/private/mx/impl/MeasureWriter.h | 8 ++++ src/private/mx/impl/PropertiesWriter.cpp | 16 ++++++- src/private/mxtest/api/roundtrip-baseline.txt | 29 ++++++++++++ src/private/mxtest/impl/MeasureWriterTest.cpp | 44 ++++++++++++++++++- 8 files changed, 115 insertions(+), 20 deletions(-) diff --git a/src/include/mx/api/ClefData.h b/src/include/mx/api/ClefData.h index 0f4bd383..2f0fd89c 100644 --- a/src/include/mx/api/ClefData.h +++ b/src/include/mx/api/ClefData.h @@ -38,7 +38,12 @@ class ClefData public: ClefData(); - // int staffIndex; + // Most users can ignore this; leave it unspecified. It only controls whether the clef's + // optional number attribute is written. unspecified (the default) applies the right rule + // automatically: omit the number on a single-staff part (where 1 is implied) and include it + // otherwise. yes/no force the attribute on or off. It exists for round-trip fidelity - reading + // a file sets yes/no only when the source diverged from the automatic rule. + Bool writeStaffNumber; ClefSymbol symbol; int line; // When true (the default), the writer emits . Set to false when the source @@ -73,7 +78,7 @@ class ClefData }; MXAPI_EQUALS_BEGIN(ClefData) -// MXAPI_EQUALS_MEMBER( staffIndex ) +MXAPI_EQUALS_MEMBER(writeStaffNumber) MXAPI_EQUALS_MEMBER(symbol) MXAPI_EQUALS_MEMBER(line) MXAPI_EQUALS_MEMBER(isLineSpecified) diff --git a/src/private/mx/api/ClefData.cpp b/src/private/mx/api/ClefData.cpp index 5a445181..4219b7dc 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}, isLineSpecified{true}, + : writeStaffNumber{Bool::unspecified}, symbol{DEFAULT_CLEF_SYMBOL}, line{DEFAULT_CLEF_LINE}, isLineSpecified{true}, 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 b2740ce9..9d73ffdd 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -822,14 +822,20 @@ void MeasureReader::importClef(const core::Clef &inClef) const clefData.printObject = converter.convert(*inClef.printObject()); } - int celfStaffIndex = -1; - if (inClef.number().has_value()) + const bool sourceHasNumber = inClef.number().has_value(); + int celfStaffIndex = sourceHasNumber ? inClef.number()->value() - 1 : 0; + + // Auto rule (see ClefData::writeStaffNumber): include the number unless this is a single-staff + // part carrying the implied 1 (staff index 0). Record an explicit override only when the source + // diverges from that rule, so the common case stays unspecified. + const bool autoIncludes = !(myCurrentCursor.getNumStaves() == 1 && celfStaffIndex == 0); + if (sourceHasNumber && !autoIncludes) { - celfStaffIndex = inClef.number()->value() - 1; + clefData.writeStaffNumber = api::Bool::yes; } - else + else if (!sourceHasNumber && autoIncludes) { - celfStaffIndex = 0; + clefData.writeStaffNumber = api::Bool::no; } if (myCurrentCursor.tickTimePosition == 0) diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index ab8a2941..9064d516 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -144,12 +144,7 @@ void MeasureWriter::writeMeasureGlobals() auto clefEnd = staff.clefs.cend(); while (clefIter != clefEnd && clefIter->tickTimePosition == 0) { - int desiredStaffIndex = -1; - if (myHistory.getCursor().getNumStaves() > 1) - { - desiredStaffIndex = localStaffCounter; - } - myPropertiesWriter->writeClef(desiredStaffIndex, *clefIter); + myPropertiesWriter->writeClef(clefStaffIndex(localStaffCounter), *clefIter); ++clefIter; } ++localStaffCounter; @@ -443,7 +438,7 @@ void MeasureWriter::writeVoices(const api::StaffData &inStaff) while (clefIter != clefEnd && clefIter->tickTimePosition <= myHistory.getCursor().tickTimePosition) { - myPropertiesWriter->writeClef(myHistory.getCursor().staffIndex, *clefIter); + myPropertiesWriter->writeClef(clefStaffIndex(myHistory.getCursor().staffIndex), *clefIter); ++clefIter; } @@ -493,7 +488,7 @@ void MeasureWriter::writeVoices(const api::StaffData &inStaff) { MX_ASSERT(clefIter != inStaff.clefs.cend()); api::ClefData clef = *clefIter; - myPropertiesWriter->writeClef(myHistory.getCursor().staffIndex, clef); + myPropertiesWriter->writeClef(clefStaffIndex(myHistory.getCursor().staffIndex), clef); } if (myMeasureKeysIter != myMeasureKeysEnd) diff --git a/src/private/mx/impl/MeasureWriter.h b/src/private/mx/impl/MeasureWriter.h index b0b2af9c..0371e6b1 100644 --- a/src/private/mx/impl/MeasureWriter.h +++ b/src/private/mx/impl/MeasureWriter.h @@ -224,6 +224,14 @@ class MeasureWriter return myHistory.getCursor(); } + // A clef carries a staff number only in a multi-staff part; for a single-staff part the + // number is omitted (1 is assumed). Centralizes the rule so measure-start and mid-measure + // clef writes agree and single-staff parts do not emit a spurious number="1". + inline int clefStaffIndex(int staffIndex) const + { + return cursor().getNumStaves() > 1 ? staffIndex : -1; + } + template std::vector findItemsAtTimePosition(const std::vector &inItems, int inTickTimePosition) { std::vector outVector; diff --git a/src/private/mx/impl/PropertiesWriter.cpp b/src/private/mx/impl/PropertiesWriter.cpp index b8d5d763..55f24e66 100644 --- a/src/private/mx/impl/PropertiesWriter.cpp +++ b/src/private/mx/impl/PropertiesWriter.cpp @@ -202,9 +202,21 @@ void PropertiesWriter::writeClef(int staffIndex, const api::ClefData &inClefData { core::Clef mxClef{}; - if (staffIndex >= 0) + // staffIndex < 0 means a single-staff part (the caller's clefStaffIndex() collapses it), so the + // auto rule omits the implied 1; staffIndex >= 0 is a multi-staff part, so the auto rule emits + // staffIndex + 1. writeStaffNumber forces the decision either way (round-trip fidelity). + bool includeNumber = staffIndex >= 0; + if (inClefData.writeStaffNumber == api::Bool::yes) + { + includeNumber = true; + } + else if (inClefData.writeStaffNumber == api::Bool::no) + { + includeNumber = false; + } + if (includeNumber) { - mxClef.setNumber(core::StaffNumber{staffIndex + 1}); + mxClef.setNumber(core::StaffNumber{staffIndex >= 0 ? staffIndex + 1 : 1}); } Converter converter; diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index 36e1a194..a9670a1c 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -155,3 +155,32 @@ musuite/testWords1.xml # and NoteWriter dropped valid zero values on round-trip. custom/transposition.musicxml ksuite/k002a_Fermatas.xml + +# Unblocked by #230 (clef @number round-trip). The writer emitted a spurious +# number="1" on mid-measure clef changes in single-staff parts (the staffIndex +# was passed straight through instead of being suppressed for single-staff, as +# the measure-start path already did), and did not preserve a source-specified +# number. Source clefs here carry no number, so 1 is assumed and must be omitted. +ksuite/k006a_Clefs_sign_G_ln_2_oct_0_Treble.xml +ksuite/k006b_Clefs_sign_F_ln_4_oct_0_Bass.xml +ksuite/k006c_Clefs_sign_C_ln_3_oct_0_Viola.xml +ksuite/k006d_Clefs_sign_C_ln_4_oct_0_Tenor.xml +ksuite/k006e_Clefs_sign_C_ln_5_oct_0_Baritone.xml +ksuite/k006f_Clefs_sign_C_ln_1_oct_0_Soprano.xml +ksuite/k006g_Clefs_sign_C_ln_2_oct_0_Mezzo_Soprano.xml +ksuite/k006h_Clefs_sign_G_ln_1_oct_0_French.xml +ksuite/k006i_Clefs_sign_F_ln_3_oct_0_Varbaritone.xml +ksuite/k006j_Clefs_sign_F_ln_5_oct_0_Subbass.xml +ksuite/k006k_Clefs_sign_G_ln_2_oct_neg2_G_15MB.xml +ksuite/k006l_Clefs_sign_G_ln_2_oct_neg1_G_8VB.xml +ksuite/k006m_Clefs_sign_G_ln_2_oct_1_G_8VA.xml +ksuite/k006n_Clefs_sign_G_ln_2_oct_2_G_15MA.xml +ksuite/k006o_Clefs_sign_F_ln_4_oct_neg2_F_15MB.xml +ksuite/k006p_Clefs_sign_F_ln_4_oct_neg1_F_8VB.xml +ksuite/k006q_Clefs_sign_F_ln_4_oct_1_F_8VA.xml +ksuite/k006r_Clefs_sign_F_ln_4_oct_2_F_15MA.xml +ksuite/k006s_Clefs_sign_percussion_ln_3_oct_0_Percussion.xml +ksuite/k006t_Clefs_sign_TAB_ln_3_oct_0_Tab.xml +ksuite/k006u_Clefs_sign_jianpu_ln_3_oct_0_Jianpu.xml +ksuite/k006v_Clefs_sign_none_ln_3_oct_0_None.xml +lysuite/ly46c_Midmeasure_Clef.xml diff --git a/src/private/mxtest/impl/MeasureWriterTest.cpp b/src/private/mxtest/impl/MeasureWriterTest.cpp index c33c3fe1..1703485c 100644 --- a/src/private/mxtest/impl/MeasureWriterTest.cpp +++ b/src/private/mxtest/impl/MeasureWriterTest.cpp @@ -178,14 +178,54 @@ TEST(PropertiesButNoNotes, MeasureWriter) CHECK_DOUBLES_EQUAL(101.0, props.divisions()->value().value(), 0.01); CHECK_EQUAL(1, props.clef().size()); - CHECK(props.clef().back().number().has_value()); - CHECK(1 == props.clef().back().number()->value()); + // Single-staff part (numStaves == 1): the clef carries no number; 1 is assumed. The + // mid-measure write path previously emitted a spurious number="1" (#230). + CHECK(!props.clef().back().number().has_value()); CHECK(++mdcIter == mdcEnd); } T_END +TEST(MultiStaffClefKeepsNumber, MeasureWriter) +{ + // Counterpart to PropertiesButNoNotes: in a multi-staff part the clef number is required + // to disambiguate the staff, so it must be emitted (here staff 0 -> number 1). + mxtest::TestParameters params; + params.ticksPerQuarter = 101; + params.measureIndex = 0; + params.partIndex = 0; + params.numStaves = 2; + mxtest::TestItems t = mxtest::setupTestItems(params); + t.measureData->barlines.emplace_back(api::BarlineData{}); + auto &barline = t.measureData->barlines.front(); + barline.barlineType = api::BarlineType::heavyLight; + auto &staff = t.measureData->staves.at(0); + staff.clefs.emplace_back(api::ClefData{}); + auto &clef = staff.clefs.back(); + clef.symbol = api::ClefSymbol::percussion; + clef.line = 2; + clef.tickTimePosition = 101; + + const auto partwiseMeasure = t.measureWriter->getPartwiseMeasure(); + auto musicData = partwiseMeasure.musicData(); + auto mdcIter = musicData.begin(); + auto mdcEnd = musicData.end(); + + CHECK(mdcIter != mdcEnd); + CHECK(mdcIter->isBarline()); + + CHECK(++mdcIter != mdcEnd); + CHECK(mdcIter->isAttributes()); + const auto &props = mdcIter->asAttributes(); + + CHECK_EQUAL(1, props.clef().size()); + CHECK(props.clef().back().number().has_value()); + CHECK(1 == props.clef().back().number()->value()); +} + +T_END + TEST(staffDetailsWritesStaffLines, MeasureWriter) { mxtest::TestParameters params;