api: preserve an explicit or omitted staff on notes - #323
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.8% | 6396 / 7918 |
| Functions | 67.7% | 2218 / 3276 |
| Branches | 49.6% | 5478 / 11054 |
Core HTML report | API HTML report
Commit dfe9185c493f76563deb828adeb87f8dbac6a396.
gen-quality
|
This was referenced Jul 7, 2026
staff on single-staff parts
webern
force-pushed
the
fix-note-staff-drop
branch
from
July 7, 2026 09:23
d08a608 to
fa268e0
Compare
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.9% | 6429 / 7948 |
| Functions | 67.8% | 2222 / 3277 |
| Branches | 49.6% | 5506 / 11102 |
Core HTML report | API HTML report
Commit 0a6fe0968dd76663538ff05cb0550d46a92a66c8.
gen-quality
|
NoteWriter::setStaffAndVoice only wrote <staff> when the part had more than one staff, since staff number is otherwise redundant with containment (measure -> staves -> voices -> notes). But <staff> is legal MusicXML on a single-staff part too, and many real-world exporters write it unconditionally -- so a source's explicit <staff> was silently dropped on round-trip on every single-staff part, the single largest drop signature in the api round-trip corpus. Added NoteData::isStaffValueSpecified (mirrors the existing DirectionData::isStaffValueSpecified), set by NoteReader whenever the source note carried an explicit <staff>. NoteWriter now emits <staff> whenever the part is multi-staff (unchanged, structurally required) or the source had it explicitly, matching the same source-had-it-explicitly convention already used for <voice> (NoteData::userRequestedVoiceNumber). Closes #275
webern
force-pushed
the
fix-note-staff-drop
branch
from
July 11, 2026 14:07
fa268e0 to
855325a
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.1% | 6562 / 8096 |
| Functions | 68.0% | 2250 / 3307 |
| Branches | 49.7% | 5628 / 11323 |
Core HTML report | API HTML report
Commit 79d7520a7d7b145c95cc1b1073c5bef77ce625f3.
gen-quality
|
Replace the two-state bool isStaffValueSpecified with Bool writeStaffNumber, the same principle-7 fidelity-knob shape as ClefData::writeStaffNumber: unspecified (the default) applies the automatic rule (omit <staff> on a single-staff part, include it otherwise), yes/no force the element on or off, and the reader records an override only when the source diverges from the automatic rule. The bool could not express omission: a multi-staff note whose source legally omitted <staff> (implied 1) had <staff>1</staff> injected on round-trip. It also made api -> xml -> api non-idempotent on multi-staff parts (authored false read back as true on every note), and exporters that write <staff> unconditionally stamped the flag true corpus-wide, recording noise instead of divergence. The writer ignores no for a note off the first staff, where omission would move the note to staff 1.
lysuite/ly02e_Rests_NoType.xml is unlocked by writeStaffNumber's no direction: a two-staff part whose last note legally omits <staff> no longer has <staff>1</staff> injected on round-trip. custom/systems-and-pages.xml, ksuite/k008a_Beaming.xml, and ksuite/k014a_Fermatas.xml were found already passing when discovery was re-run; they were unlocked by earlier merged changes and never pinned.
staff on single-staff partsstaff on notes (ternary writeStaffNumber)
staff on notes (ternary writeStaffNumber)staff on notes
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.1% | 6940 / 8452 |
| Functions | 68.9% | 2307 / 3347 |
| Branches | 50.7% | 6021 / 11875 |
Core HTML report | API HTML report
Commit bce576fb2cae560fc7134d318cfa438201a0c3cc.
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
This is a round-trip fidelity knob.
mx::apiwas automatically deciding when to emit a staff number and when not to, but this did not match some files. So we added a ternaryBoolso that an author (or round trip) could turn on a forceable write of the staff number when it is not technically needed.AI Summary
NoteWriter::setStaffAndVoiceonly wrote<staff>when the part had more than one staff, sincestaff number is otherwise redundant with containment (
measure -> staves -> voices -> notes). But<staff>is legal MusicXML on a single-staff part too, and many real-world exporters(Finale/Sibelius/MuseScore) write it unconditionally -- so a source's explicit
<staff>wassilently dropped on every single-staff-part note on round-trip. This was the single largest drop
signature in the api round-trip corpus (
drop:staff, ~285 of 675 currently-failing files).Added
NoteData::writeStaffNumber, a ternaryBoolfidelity knob with the same shape asClefData::writeStaffNumber(doctrine principle 7):unspecified(the default): the automatic rule -- omit<staff>on a single-staff part(where 1 is implied), include it otherwise. Authors never need to touch it.
yes/no: force the element on or off. The reader sets these only when the sourcediverges from the automatic rule, so the common case stays
unspecified.nofor a note that is not on the first staff, where omitting<staff>would silently move the note to staff 1.
Why not a two-state bool
The first cut of this PR used
bool isStaffValueSpecified(defaultfalse), mirroring thelegacy
DirectionDatafield. That shape had three defects the ternary fixes:authoring path -- and it is exactly the
bool is...Specifiedpattern issue api: replace value +isSpecifiedbool pairs with an explicit optional value type #249 exists toretire (new instances are rejected by
.github/instructions/api-headers.instructions.md).<staff>(implied 1) had
<staff>1</staff>injected on round-trip -- the same fidelity-bug class inthe reverse direction. The ternary's
nocloses it (and unlockslysuite/ly02e_Rests_NoType.xml, which pins exactly that shape).falseread backas
trueon every note, so twoScoreDatas for identical music compared unequal. Withdivergence-only recording, sources that follow the sensible rule read back
unspecified,matching authored scores.
Testing
explicitStaffOnSingleStaffPartRoundTrips(reader setsyes, writer emits),implicitStaffOnSingleStaffPartOmitsElement(authored default omits),omittedStaffOnMultiStaffPartRoundTrips(reader setsno/ leavesunspecified, writeromits/emits per note),
writeStaffNumberNoIsIgnoredOffFirstStaff(the off-first-staff guard)in
NoteDataTest.cppmake test: all pass (4925 assertions in 413 test cases, plus the three examples)make test-api-roundtrip: 171 passed, 0 failed (of 171 pinned)make discover-api-roundtrip: no regressions; newly pinnedly24c_GraceNote_MeasureEndand
ly02e_Rests_NoType(this PR's two directions), pluscustom/systems-and-pages.xml,ksuite/k008a_Beaming.xml, andksuite/k014a_Fermatas.xml, found already passing fromearlier merged work and pinned to defend them
make fmt/make check: cleanReferences