fix: preserve staff-scoped staff-layout number="N" - #326
Merged
Conversation
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8551 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.7% | 6401 / 7933 |
| Functions | 67.7% | 2217 / 3275 |
| Branches | 49.5% | 5496 / 11100 |
Core HTML report | API HTML report
Commit 798455f0292544538e266fed9bfd514f07e9cb02.
gen-quality
|
This was referenced Jul 7, 2026
webern
force-pushed
the
fix-staff-layout-number
branch
from
July 7, 2026 09:23
c48404d to
beefd31
Compare
staff-layout number="N"
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8551 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.8% | 6434 / 7963 |
| Functions | 67.8% | 2221 / 3276 |
| Branches | 49.6% | 5524 / 11148 |
Core HTML report | API HTML report
Commit 1b8a0ab9ac2c26582882e07d4be01984f5f5b484.
gen-quality
|
webern
commented
Jul 8, 2026
Both the per-measure <print> and score-level <defaults> readers took only the first <staff-layout>'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 <staff-layout> 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
webern
force-pushed
the
fix-staff-layout-number
branch
from
July 11, 2026 14:07
beefd31 to
2e7307a
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.0% | 6567 / 8111 |
| Functions | 68.0% | 2249 / 3306 |
| Branches | 49.7% | 5646 / 11369 |
Core HTML report | API HTML report
Commit 4d5d770eb8f65c58d321191a2c1d52c5ccc557ea.
gen-quality
|
2 tasks
webern
added a commit
that referenced
this pull request
Jul 12, 2026
## Human Summary
I asked Fable, while I still have access to it, to improve Agent
performance with better instructions putting them in places more likely
to be read and incorporating some of my latest pet-peeves.
## Summary
Agents were not reading docs/ai/design/api-design-principles.md, and
several recurring review
themes were written down nowhere. This PR moves the doctrine to where
agents actually look and
captures the missing rules, without growing AGENTS.md (it shrinks by 8
lines).
- New mx-api-doctrine skill whose description says it MUST be read
before designing or changing
anything in the api layer. It contains the seven design principles
(moved verbatim from the
deleted doc) plus the previously unwritten doctrine:
- Audience: the api is for people creating music and building,
modifying, and interpreting
MusicXML files. Round-tripping is how we develop the api, not what it is
for. Header comments
explain notation semantics and how MusicXML encodes them;
"source"/"preserved" framing is
allowed only on true fidelity knobs. No documenting what is not modeled,
no banner comments
(from review feedback on #326, #333, #321).
- Failure model: misuse is made unrepresentable or degrades to a
documented fallback. UB must be
unreachable from the public interface, nothing throws across the api
boundary (never for a
failed precondition), and Result stays quarantined at the
DocumentManager I/O boundary.
- Choice types: the TimeChoice / MarkDataChoice pattern for either/or
payloads (copy-returning
accessors, default-constructed wrong-kind fallback, auto-collapse,
common case kept prominent
in its own header), from the #321 redesign and the #330 review.
- AGENTS.md digest replaced with a pointer to the skill; the add-feature
skill and the Copilot
api-headers review instructions repointed and extended with the same
rules.
- Deleted the mx-architecture skill: generic architect boilerplate that
wasn't helping. Its few
mx-specific facts live elsewhere (gen design in gen/DESIGN.md; the
error-handling stance is now
concrete doctrine in the new skill).
Net -33 lines repo-wide.
## Testing
- [x] No dangling references: grep for api-design-principles and
mx-architecture is clean
- [x] Docs/skills only; no C++ or build changes, so no test suites apply
## References
- Related: #292 (created the principles doc and the AGENTS.md digest
this replaces)
- Captures review feedback from #321, #326, #330, #333
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.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36644 |
| Functions | 74.3% | 6352 / 8554 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.5% | 6890 / 8457 |
| Functions | 68.6% | 2296 / 3346 |
| Branches | 50.1% | 5959 / 11905 |
Core HTML report | API HTML report
Commit a1bf5284132e7e65362c910890fe3f28c4e62096.
gen-quality
|
Replace std::vector<StaffDistanceData> with std::map<int, double>, 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.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36644 |
| Functions | 74.3% | 6352 / 8554 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 82.0% | 6950 / 8472 |
| Functions | 68.9% | 2307 / 3347 |
| Branches | 50.7% | 6041 / 11923 |
Core HTML report | API HTML report
Commit b518bd703adb9918b936c405e13c5307609a5ec9.
gen-quality
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
TODO: human writes here
Summary
Both the per-measure
<print>and score-level<defaults>readers took only the first<staff-layout>'s distance and dropped itsnumberattribute entirely, folding it into the singleunscoped
SystemLayoutData::staffDistance. For a multi-staff part (e.g. a 2-staff piano/organ)whose only
staff-layoutis scoped to staff 2, this silently misattributed the distance to staff 1(the schema default for a missing
number) instead of just dropping the attribute -- a real,if quiet, correctness bug, not only a fidelity loss.
Added
SystemLayoutData::staffDistances, astd::map<int, double>keyed by zero-based staffindex, holding spacing for staves that need their own value; the existing singular
staffDistancekeeps its meaning for the common unscoped case, and for a given staff a map entrywins. The map shape follows the
MeasureData::staffTimeSignaturesprecedent for staff-scopednumber="N"overrides -- key by the owning staff rather than stamping a label field on the items-- which also makes two conflicting distances for the same staff unrepresentable. Both readers
(
ScoreReader::scanForSystemInfo,LayoutFunctions::addStaffLayout) and both writers(
MeasureWriter::writeSystemInfo,LayoutFunctions::addSystemMargins) handle it symmetrically,since
<defaults>and per-measure<print>already shareSystemLayoutData. Header comments arewritten for the api user (what the fields mean and when to set them) per the design doctrine,
rather than describing reader/writer behavior.
Testing
staffScopedStaffDistancetest (PrintLayoutRoundTripTest.cpp)make test: all pass (4808 assertions in 391 test cases, plus the three examples)make test-api-roundtrip: 164 passed, 0 failed (of 164 pinned; no regressions)make fmt/make check: cleanReferences