From 2e7307a562e3a212163593ffdae20c2d9566d7e0 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Tue, 7 Jul 2026 06:41:31 +0000 Subject: [PATCH 1/3] fix: preserve staff-scoped Both the per-measure and score-level readers took only the first 's distance and dropped its number attribute entirely, folding it into the single unscoped SystemLayoutData::staffDistance. For a multi-staff part (e.g. a 2-staff piano/organ) whose only staff-layout is scoped to staff 2, this silently misattributed the distance to staff 1 (the schema default for a missing number) instead of dropping the number. Added SystemLayoutData::staffDistances (vector of new StaffDistanceData: staffIndex + distance), populated only when the source's had an explicit number attribute; the existing singular staffDistance keeps its meaning for the common unscoped case. Both readers (ScoreReader:: scanForSystemInfo, LayoutFunctions::addStaffLayout) and both writers (MeasureWriter::writeSystemInfo, LayoutFunctions::addSystemMargins) handle it symmetrically since defaults and per-measure print share SystemLayoutData. ## Testing - [x] New `staffScopedStaffDistance` test (PrintLayoutRoundTripTest.cpp) - [x] make test: all pass (4729 assertions in 379 test cases, plus examples) - [x] make test-api-roundtrip: 158 passed, 0 failed (of 158 pinned; no regressions -- this fix corrects a wrong-staff misattribution rather than unlocking a new fully-passing file) - [x] make fmt / make check: clean Closes #281 --- src/include/mx/api/SystemLayoutData.h | 36 +++++++++++++++++-- src/private/mx/impl/LayoutFunctions.cpp | 34 +++++++++++++----- src/private/mx/impl/MeasureWriter.cpp | 10 ++++++ src/private/mx/impl/ScoreReader.cpp | 20 +++++++---- .../mxtest/api/PrintLayoutRoundTripTest.cpp | 27 ++++++++++++++ 5 files changed, 110 insertions(+), 17 deletions(-) diff --git a/src/include/mx/api/SystemLayoutData.h b/src/include/mx/api/SystemLayoutData.h index 8d4801b99..167bf9603 100644 --- a/src/include/mx/api/SystemLayoutData.h +++ b/src/include/mx/api/SystemLayoutData.h @@ -16,6 +16,29 @@ namespace mx { namespace api { +/// A scoped to one staff of a multi-staff part (MusicXML's number attribute): +/// the space between the bottom line of the previous staff and the top line of this one. +/// staffIndex is zero-based; a source with an explicit number attribute always populates +/// SystemLayoutData::staffDistances with one of these rather than the unscoped +/// SystemLayoutData::staffDistance, so the number round-trips faithfully. +class StaffDistanceData +{ + public: + int staffIndex; + double staffDistance; + + explicit inline StaffDistanceData(int inStaffIndex = INDEX_UNSPECIFIED, double inStaffDistance = DOUBLE_UNSPECIFIED) + : staffIndex(inStaffIndex), staffDistance(inStaffDistance) + { + } +}; + +MXAPI_EQUALS_BEGIN(StaffDistanceData) +MXAPI_EQUALS_MEMBER(staffIndex) +MXAPI_DOUBLES_EQUALS_MEMBER(staffDistance) +MXAPI_EQUALS_END; +MXAPI_NOT_EQUALS_AND_VECTORS(StaffDistanceData); + /// Margins and spacing for staff systems. class SystemLayoutData { @@ -29,13 +52,19 @@ class SystemLayoutData /// Distance from the top margin of the page to the top line of the first staff on the page, in tenths. OptionalDouble topSystemDistance; - /// the space between staves within the same system, in tenths. + /// the space between staves within the same system, in tenths. Only populated when the source's + /// had no number attribute (the common single-staff-distance case); a source with + /// an explicit number populates staffDistances instead. See StaffDistanceData. OptionalDouble staffDistance; + /// Per-staff overrides of staffDistance for parts with more than one staff-scoped + /// (e.g. an organ's third staff needing its own spacing). Normally empty. + std::vector staffDistances; + /// Returns true if any of the members have values. inline bool isUsed() const { - return margins || systemDistance || topSystemDistance || staffDistance; + return margins || systemDistance || topSystemDistance || staffDistance || !staffDistances.empty(); } explicit inline SystemLayoutData(std::optional inMargins = std::nullopt, @@ -43,7 +72,7 @@ class SystemLayoutData OptionalDouble inTopSystemDistance = std::nullopt, OptionalDouble inStaffDistance = std::nullopt) : margins(inMargins), systemDistance(inSystemDistance), topSystemDistance{inTopSystemDistance}, - staffDistance(inStaffDistance) + staffDistance(inStaffDistance), staffDistances{} { } }; @@ -53,6 +82,7 @@ MXAPI_EQUALS_MEMBER(margins) MXAPI_EQUALS_MEMBER(systemDistance) MXAPI_EQUALS_MEMBER(topSystemDistance) MXAPI_EQUALS_MEMBER(staffDistance) +MXAPI_EQUALS_MEMBER(staffDistances) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(SystemLayoutData); } // namespace api diff --git a/src/private/mx/impl/LayoutFunctions.cpp b/src/private/mx/impl/LayoutFunctions.cpp index a497e1ae3..0072d0169 100644 --- a/src/private/mx/impl/LayoutFunctions.cpp +++ b/src/private/mx/impl/LayoutFunctions.cpp @@ -236,6 +236,15 @@ void addSystemMargins(const api::DefaultsData &inDefaults, core::ScoreHeaderGrou needsDefaults = true; } + for (const auto &staffDistance : inDefaults.systemLayout.staffDistances) + { + core::StaffLayout staffLayout; + staffLayout.setNumber(core::StaffNumber{staffDistance.staffIndex + 1}); + staffLayout.setStaffDistance(toTenths(staffDistance.staffDistance)); + layout.addStaffLayout(staffLayout); + needsDefaults = true; + } + if (needsDefaults) { layout.setSystemLayout(systemLayout); @@ -427,15 +436,24 @@ void addStaffLayout(const core::ScoreHeaderGroup &inScoreHeaderGroup, api::Defau return; } - const auto &staffLayouts = inScoreHeaderGroup.defaults()->layout().staffLayout(); - if (staffLayouts.empty()) - { - return; - } - - if (staffLayouts.front().staffDistance().has_value()) + // A source with an explicit number attribute is staff-scoped (api::StaffDistanceData); one + // with no number applies to staff 1 by schema default and is captured as the plain + // unscoped value. Mirrors ScoreReader::scanForSystemInfo's per-measure handling. + for (const auto &staffLayout : inScoreHeaderGroup.defaults()->layout().staffLayout()) { - outDefaults.systemLayout.staffDistance = staffLayouts.front().staffDistance()->value().value(); + if (!staffLayout.staffDistance().has_value()) + { + continue; + } + const double distance = staffLayout.staffDistance()->value().value(); + if (staffLayout.number().has_value()) + { + outDefaults.systemLayout.staffDistances.emplace_back(staffLayout.number()->value() - 1, distance); + } + else + { + outDefaults.systemLayout.staffDistance = distance; + } } } diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index bbc5c885f..5480d1d2f 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -26,6 +26,7 @@ #include "mx/core/generated/Repeat.h" #include "mx/core/generated/RightLeftMiddle.h" #include "mx/core/generated/StaffLayout.h" +#include "mx/core/generated/StaffNumber.h" #include "mx/core/generated/SystemLayout.h" #include "mx/core/generated/SystemMargins.h" #include "mx/core/generated/Tenths.h" @@ -263,6 +264,15 @@ void MeasureWriter::writeSystemInfo() outLayoutGroup.addStaffLayout(outStaffLayout); outPrint.setLayout(outLayoutGroup); } + + for (const auto &staffDistance : inSystemLayout.staffDistances) + { + core::StaffLayout outStaffLayout{}; + outStaffLayout.setNumber(core::StaffNumber{staffDistance.staffIndex + 1}); + outStaffLayout.setStaffDistance(core::Tenths{core::Decimal{staffDistance.staffDistance}}); + outLayoutGroup.addStaffLayout(outStaffLayout); + outPrint.setLayout(outLayoutGroup); + } } myOutMeasure.addMusicData(core::MusicDataChoice::print(outPrint)); diff --git a/src/private/mx/impl/ScoreReader.cpp b/src/private/mx/impl/ScoreReader.cpp index 6873d05c7..b772f1b84 100644 --- a/src/private/mx/impl/ScoreReader.cpp +++ b/src/private/mx/impl/ScoreReader.cpp @@ -466,15 +466,23 @@ void ScoreReader::scanForSystemInfo() const } } - // Per-measure staff-distance. (We model a single - // staff-distance, matching how the defaults staff-layout is - // mapped; the first staff-layout's distance is used.) + // Per-measure staff-distance. A source with an explicit number + // attribute is staff-scoped (api::StaffDistanceData); one with no number applies + // to staff 1 by schema default and is captured as the plain unscoped value. for (const auto &staffLayout : layoutGroup.staffLayout()) { - if (staffLayout.staffDistance().has_value()) + if (!staffLayout.staffDistance().has_value()) { - systemData.layout.staffDistance = staffLayout.staffDistance()->value().value(); - break; + continue; + } + const double distance = staffLayout.staffDistance()->value().value(); + if (staffLayout.number().has_value()) + { + systemData.layout.staffDistances.emplace_back(staffLayout.number()->value() - 1, distance); + } + else + { + systemData.layout.staffDistance = distance; } } diff --git a/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp b/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp index dee526883..691b5f8ec 100644 --- a/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp +++ b/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp @@ -71,4 +71,31 @@ TEST(printLayoutRoundTrip, perMeasureStaffDistance) CHECK_EQUAL(120.0, sysOut.layout.systemDistance.value()); } +// A is scoped to one staff of a multi-staff part; the writer +// only ever emitted an unscoped (matching SystemLayoutData::staffDistance), +// and the reader dropped the number attribute on the one it read back, silently +// misattributing the distance to staff 1. Verify a staff-scoped distance survives via +// SystemLayoutData::staffDistances. +TEST(printLayoutRoundTrip, staffScopedStaffDistance) +{ + auto in = makeScoreWithMeasures(1); + auto &staff0 = in.parts.front().measures.front().staves.front(); + StaffData staff1 = staff0; + in.parts.front().measures.front().staves.push_back(staff1); + + SystemData sys{}; + sys.layout.staffDistances.emplace_back(1, 65.0); + in.layout.emplace(0, LayoutData{sys, PageData{}}); + + const auto out = mxtest::roundTrip(in); + + const auto it = out.layout.find(0); + REQUIRE(it != out.layout.end()); + const auto &sysOut = it->second.system; + CHECK(!static_cast(sysOut.layout.staffDistance)); + REQUIRE(1 == sysOut.layout.staffDistances.size()); + CHECK_EQUAL(1, sysOut.layout.staffDistances.front().staffIndex); + CHECK_EQUAL(65.0, sysOut.layout.staffDistances.front().staffDistance); +} + #endif From af3c3ac7494d34a66f81bc5b1425d90fc1d975eb Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 13:44:33 +0000 Subject: [PATCH 2/3] docs: rewrite SystemLayoutData comments for the api audience Replace the round-trip-harness viewpoint ('a source with an explicit number attribute always populates...') with musician-facing comments that say what the fields mean and when an author should use each one. --- src/include/mx/api/SystemLayoutData.h | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/include/mx/api/SystemLayoutData.h b/src/include/mx/api/SystemLayoutData.h index 167bf9603..33f13481f 100644 --- a/src/include/mx/api/SystemLayoutData.h +++ b/src/include/mx/api/SystemLayoutData.h @@ -16,15 +16,17 @@ namespace mx { namespace api { -/// A scoped to one staff of a multi-staff part (MusicXML's number attribute): -/// the space between the bottom line of the previous staff and the top line of this one. -/// staffIndex is zero-based; a source with an explicit number attribute always populates -/// SystemLayoutData::staffDistances with one of these rather than the unscoped -/// SystemLayoutData::staffDistance, so the number round-trips faithfully. +/// The vertical space above one specific staff of a multi-staff part (MusicXML's +/// ). Use this only when a staff needs its own spacing, e.g. extra room +/// above an organ's pedal staff; the ordinary single spacing value is +/// SystemLayoutData::staffDistance. class StaffDistanceData { public: + /// Which staff of the part this spacing applies to, zero-based from the top staff. int staffIndex; + + /// The space between the bottom line of the previous staff and the top line of this staff, in tenths. double staffDistance; explicit inline StaffDistanceData(int inStaffIndex = INDEX_UNSPECIFIED, double inStaffDistance = DOUBLE_UNSPECIFIED) @@ -52,13 +54,13 @@ class SystemLayoutData /// Distance from the top margin of the page to the top line of the first staff on the page, in tenths. OptionalDouble topSystemDistance; - /// the space between staves within the same system, in tenths. Only populated when the source's - /// had no number attribute (the common single-staff-distance case); a source with - /// an explicit number populates staffDistances instead. See StaffDistanceData. + /// The space between staves within the same system, in tenths. Applies to every staff of a part + /// that does not have its own entry in staffDistances. OptionalDouble staffDistance; - /// Per-staff overrides of staffDistance for parts with more than one staff-scoped - /// (e.g. an organ's third staff needing its own spacing). Normally empty. + /// Per-staff spacing for multi-staff parts. Normally empty; only add an entry when one staff + /// needs spacing different from staffDistance. For a given staff, an entry here wins; otherwise + /// staffDistance applies. std::vector staffDistances; /// Returns true if any of the members have values. From 41ad785e8a0ac8ee062ab855870bfdf4f25f4fac Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 14:14:27 +0000 Subject: [PATCH 3/3] refactor: staffDistances as a map keyed by staff index Replace std::vector with std::map, following the MeasureData::staffTimeSignatures precedent for staff-scoped number="N" overrides: key the map by the owner instead of stamping a label field on the item. Two conflicting distances for the same staff are now unrepresentable, and the StaffDistanceData class (with its sentinel-value constructor defaults) is deleted. --- src/include/mx/api/SystemLayoutData.h | 34 ++++--------------- src/private/mx/impl/LayoutFunctions.cpp | 12 +++---- src/private/mx/impl/MeasureWriter.cpp | 6 ++-- src/private/mx/impl/ScoreReader.cpp | 4 +-- .../mxtest/api/PrintLayoutRoundTripTest.cpp | 9 ++--- 5 files changed, 22 insertions(+), 43 deletions(-) diff --git a/src/include/mx/api/SystemLayoutData.h b/src/include/mx/api/SystemLayoutData.h index 33f13481f..3594967c0 100644 --- a/src/include/mx/api/SystemLayoutData.h +++ b/src/include/mx/api/SystemLayoutData.h @@ -9,6 +9,7 @@ #include "mx/api/LeftRight.h" #include "mx/api/PageLayoutData.h" +#include #include #include @@ -16,31 +17,6 @@ namespace mx { namespace api { -/// The vertical space above one specific staff of a multi-staff part (MusicXML's -/// ). Use this only when a staff needs its own spacing, e.g. extra room -/// above an organ's pedal staff; the ordinary single spacing value is -/// SystemLayoutData::staffDistance. -class StaffDistanceData -{ - public: - /// Which staff of the part this spacing applies to, zero-based from the top staff. - int staffIndex; - - /// The space between the bottom line of the previous staff and the top line of this staff, in tenths. - double staffDistance; - - explicit inline StaffDistanceData(int inStaffIndex = INDEX_UNSPECIFIED, double inStaffDistance = DOUBLE_UNSPECIFIED) - : staffIndex(inStaffIndex), staffDistance(inStaffDistance) - { - } -}; - -MXAPI_EQUALS_BEGIN(StaffDistanceData) -MXAPI_EQUALS_MEMBER(staffIndex) -MXAPI_DOUBLES_EQUALS_MEMBER(staffDistance) -MXAPI_EQUALS_END; -MXAPI_NOT_EQUALS_AND_VECTORS(StaffDistanceData); - /// Margins and spacing for staff systems. class SystemLayoutData { @@ -58,10 +34,12 @@ class SystemLayoutData /// that does not have its own entry in staffDistances. OptionalDouble staffDistance; - /// Per-staff spacing for multi-staff parts. Normally empty; only add an entry when one staff - /// needs spacing different from staffDistance. For a given staff, an entry here wins; otherwise + /// Per-staff spacing for multi-staff parts, keyed by zero-based staff index: the space between + /// the bottom line of the previous staff and the top line of the keyed staff, in tenths. + /// Normally empty; only add an entry when one staff needs its own spacing, e.g. extra room + /// above an organ's pedal staff. For a given staff, an entry here wins; otherwise /// staffDistance applies. - std::vector staffDistances; + std::map staffDistances; /// Returns true if any of the members have values. inline bool isUsed() const diff --git a/src/private/mx/impl/LayoutFunctions.cpp b/src/private/mx/impl/LayoutFunctions.cpp index 0072d0169..9f8200bd4 100644 --- a/src/private/mx/impl/LayoutFunctions.cpp +++ b/src/private/mx/impl/LayoutFunctions.cpp @@ -236,11 +236,11 @@ void addSystemMargins(const api::DefaultsData &inDefaults, core::ScoreHeaderGrou needsDefaults = true; } - for (const auto &staffDistance : inDefaults.systemLayout.staffDistances) + for (const auto &[staffIndex, staffDistance] : inDefaults.systemLayout.staffDistances) { core::StaffLayout staffLayout; - staffLayout.setNumber(core::StaffNumber{staffDistance.staffIndex + 1}); - staffLayout.setStaffDistance(toTenths(staffDistance.staffDistance)); + staffLayout.setNumber(core::StaffNumber{staffIndex + 1}); + staffLayout.setStaffDistance(toTenths(staffDistance)); layout.addStaffLayout(staffLayout); needsDefaults = true; } @@ -436,8 +436,8 @@ void addStaffLayout(const core::ScoreHeaderGroup &inScoreHeaderGroup, api::Defau return; } - // A source with an explicit number attribute is staff-scoped (api::StaffDistanceData); one - // with no number applies to staff 1 by schema default and is captured as the plain + // A source with an explicit number attribute is staff-scoped (a staffDistances map entry); + // one with no number applies to staff 1 by schema default and is captured as the plain // unscoped value. Mirrors ScoreReader::scanForSystemInfo's per-measure handling. for (const auto &staffLayout : inScoreHeaderGroup.defaults()->layout().staffLayout()) { @@ -448,7 +448,7 @@ void addStaffLayout(const core::ScoreHeaderGroup &inScoreHeaderGroup, api::Defau const double distance = staffLayout.staffDistance()->value().value(); if (staffLayout.number().has_value()) { - outDefaults.systemLayout.staffDistances.emplace_back(staffLayout.number()->value() - 1, distance); + outDefaults.systemLayout.staffDistances[staffLayout.number()->value() - 1] = distance; } else { diff --git a/src/private/mx/impl/MeasureWriter.cpp b/src/private/mx/impl/MeasureWriter.cpp index 5480d1d2f..dd2164133 100644 --- a/src/private/mx/impl/MeasureWriter.cpp +++ b/src/private/mx/impl/MeasureWriter.cpp @@ -265,11 +265,11 @@ void MeasureWriter::writeSystemInfo() outPrint.setLayout(outLayoutGroup); } - for (const auto &staffDistance : inSystemLayout.staffDistances) + for (const auto &[staffIndex, staffDistance] : inSystemLayout.staffDistances) { core::StaffLayout outStaffLayout{}; - outStaffLayout.setNumber(core::StaffNumber{staffDistance.staffIndex + 1}); - outStaffLayout.setStaffDistance(core::Tenths{core::Decimal{staffDistance.staffDistance}}); + outStaffLayout.setNumber(core::StaffNumber{staffIndex + 1}); + outStaffLayout.setStaffDistance(core::Tenths{core::Decimal{staffDistance}}); outLayoutGroup.addStaffLayout(outStaffLayout); outPrint.setLayout(outLayoutGroup); } diff --git a/src/private/mx/impl/ScoreReader.cpp b/src/private/mx/impl/ScoreReader.cpp index b772f1b84..4dad7d8df 100644 --- a/src/private/mx/impl/ScoreReader.cpp +++ b/src/private/mx/impl/ScoreReader.cpp @@ -467,7 +467,7 @@ void ScoreReader::scanForSystemInfo() const } // Per-measure staff-distance. A source with an explicit number - // attribute is staff-scoped (api::StaffDistanceData); one with no number applies + // attribute is staff-scoped (a staffDistances map entry); one with no number applies // to staff 1 by schema default and is captured as the plain unscoped value. for (const auto &staffLayout : layoutGroup.staffLayout()) { @@ -478,7 +478,7 @@ void ScoreReader::scanForSystemInfo() const const double distance = staffLayout.staffDistance()->value().value(); if (staffLayout.number().has_value()) { - systemData.layout.staffDistances.emplace_back(staffLayout.number()->value() - 1, distance); + systemData.layout.staffDistances[staffLayout.number()->value() - 1] = distance; } else { diff --git a/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp b/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp index 691b5f8ec..0ff7f4b9b 100644 --- a/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp +++ b/src/private/mxtest/api/PrintLayoutRoundTripTest.cpp @@ -84,7 +84,7 @@ TEST(printLayoutRoundTrip, staffScopedStaffDistance) in.parts.front().measures.front().staves.push_back(staff1); SystemData sys{}; - sys.layout.staffDistances.emplace_back(1, 65.0); + sys.layout.staffDistances[1] = 65.0; in.layout.emplace(0, LayoutData{sys, PageData{}}); const auto out = mxtest::roundTrip(in); @@ -93,9 +93,10 @@ TEST(printLayoutRoundTrip, staffScopedStaffDistance) REQUIRE(it != out.layout.end()); const auto &sysOut = it->second.system; CHECK(!static_cast(sysOut.layout.staffDistance)); - REQUIRE(1 == sysOut.layout.staffDistances.size()); - CHECK_EQUAL(1, sysOut.layout.staffDistances.front().staffIndex); - CHECK_EQUAL(65.0, sysOut.layout.staffDistances.front().staffDistance); + CHECK_EQUAL(1, static_cast(sysOut.layout.staffDistances.size())); + const auto sdIt = sysOut.layout.staffDistances.find(1); + REQUIRE(sdIt != sysOut.layout.staffDistances.end()); + CHECK_EQUAL(65.0, sdIt->second); } #endif