fix(api): clef number round-trip

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/'

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 <line>. 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)

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}
 {

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)

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)

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 <typename T> std::vector<T> findItemsAtTimePosition(const std::vector<T> &inItems, int inTickTimePosition)
     {
         std::vector<T> outVector;

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;

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

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;