feat: add score-part groups, measure-numbering multi-rest/system attrs, and swing to mx::api - #333
Merged
Merged
Conversation
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36642 |
| Functions | 74.3% | 6352 / 8553 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.2% | 6616 / 8152 |
| Functions | 68.1% | 2256 / 3313 |
| Branches | 49.8% | 5680 / 11401 |
Core HTML report | API HTML report
Commit eb2900a536754118e9ed6dd25fa4bbe3d5504c2e.
gen-quality
|
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36642 |
| Functions | 74.3% | 6352 / 8553 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.2% | 6616 / 8152 |
| Functions | 68.1% | 2256 / 3313 |
| Branches | 49.8% | 5680 / 11401 |
Core HTML report | API HTML report
Commit dbea134b5aeb7b06efa8140d4f903be76978980f.
gen-quality
|
webern
commented
Jul 11, 2026
webern
added a commit
that referenced
this pull request
Jul 11, 2026
…ness Addresses PR review feedback (#333): a no-space font-family list ("Maestro,engraved") failed the round-trip comparison against mx's normalized "Maestro, engraved" output. Editing the test fixture to add the space papered over the harness gap for one file; instead teach corert's normalizeForComparison pipeline to re-space any comma-separated-text attribute (currently just font-family, the schema's only user of that type) to "item, item" on both sides of the comparison, the way stripZerosFromDecimalFields already handles decimal spelling differences. The field list lives in the new CommaSeparatedFields.h, mirroring DecimalFields.h. Reverts data/rpatters1/timesigs_composite-ref.musicxml to byte-identical with main; the fixture no longer needs editing to pass.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36642 |
| Functions | 74.3% | 6352 / 8553 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.2% | 6616 / 8152 |
| Functions | 68.1% | 2256 / 3313 |
| Branches | 49.8% | 5680 / 11401 |
Core HTML report | API HTML report
Commit c86c448b9b853e24dd6190700f9728330e37f0c8.
gen-quality
|
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36642 |
| Functions | 74.3% | 6352 / 8553 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.2% | 6616 / 8152 |
| Functions | 68.1% | 2256 / 3313 |
| Branches | 49.8% | 5680 / 11401 |
Core HTML report | API HTML report
Commit a48fb6d5d431945db3fb108c9ea958c061bd0800.
gen-quality
|
webern
commented
Jul 12, 2026
…stem attrs, and swing
Adds the three mx::api gaps blocking rpatters1's original timesigs_composite-ref.musicxml
from the roundtrip baseline (the trimmed -modified variant was already pinned):
- PartData::groups models <score-part>/<group> (Finale-style MIDI/score groupings),
wired through PartReader/PartWriter.
- MeasureData gains measureNumberingMultipleRestAlways/Range (Bool) and
measureNumberingSystemRelation (new SystemRelation enum) for the <measure-numbering>
attributes beyond its text value, wired through MeasureReader/MeasureWriter and a new
Converter::convertSystemRelation bridge.
- SoundData::swing models <sound>/<swing> (straight vs. ratio, note type, style), wired
through SoundFunctions; SoundData.h's "intentionally not modeled" note is updated.
Also fixes data/rpatters1/timesigs_composite-ref.musicxml's music-font font-family to be
comma-space separated ("Maestro, engraved"), matching mx's normalized output -- the
unspaced original is semantically identical, so the fixture was corrected instead of the
writer.
timesigs_composite-ref.musicxml is now pinned in roundtrip-baseline.txt alongside the
existing -modified variant.
Quote the spec and explain that <score-part>/<group> is a Finale-style implementation detail (which version of a part: score/parts/sound/data), not the visual brace/bracket grouping (PartGroupData).
…ness Addresses PR review feedback (#333): a no-space font-family list ("Maestro,engraved") failed the round-trip comparison against mx's normalized "Maestro, engraved" output. Editing the test fixture to add the space papered over the harness gap for one file; instead teach corert's normalizeForComparison pipeline to re-space any comma-separated-text attribute (currently just font-family, the schema's only user of that type) to "item, item" on both sides of the comparison, the way stripZerosFromDecimalFields already handles decimal spelling differences. The field list lives in the new CommaSeparatedFields.h, mirroring DecimalFields.h. Reverts data/rpatters1/timesigs_composite-ref.musicxml to byte-identical with main; the fixture no longer needs editing to pass.
The field was never read from or written to MusicXML: <defaults> has
no measure-numbering child in the schema (it only appears inside a
per-measure <print>), so there was no wire representation to wire up.
The comment already said as much ("TODO - this appears not to be
used anywhere, please do not use"). The real, working feature is
MeasureData::measureNumbering, which is measure-scoped to match how
<measure-numbering> actually appears in MusicXML, and is unaffected
by this removal.
Breaking: removes a public DefaultsData field.
The <sound> child-element gaps (<midi-instrument>, <midi-device>, <play>, <offset>) aren't a deliberate scope decision, so don't document them as such.
webern
force-pushed
the
claude/timesigs-file-hillclimb-fltuj0
branch
from
July 12, 2026 11:51
3ae5be4 to
4f376ff
Compare
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.2% | 6640 / 8181 |
| Functions | 68.4% | 2270 / 3320 |
| Branches | 49.9% | 5707 / 11437 |
Core HTML report | API HTML report
Commit e5336bc0bd2b616a21feeff8d066c97d3fdb924d.
gen-quality
|
This was referenced Jul 12, 2026
webern
added a commit
that referenced
this pull request
Jul 12, 2026
## Human Summary Take @rpatters1's suggested design from #297 and attempts to implement it with a spanner identifying object and a sort of spanner resource pool. ## AI Summary MusicXML pairs a spanner's start with its stop using a `number` attribute, and two spanners of the same type that are open at the same point in the serialized stream must carry different numbers. Which spanners are open at the same point depends on the order the writer emits notes (voice interleaving via `backup`), so a caller cannot reliably choose the number. This PR moves number assignment into the writer. The design differs from the shape proposed in #297 (a second `spannerId` field beside `numberLevel`): two mutually exclusive fields side by side is the recurring defect that design principle #3 and #249 exist to eliminate. Instead the two are unified into one value type. - New `SpannerNumber` value type (`SpannerNumber.h`/`SpannerNumber.cpp`, following the `TimeChoice.h`/`.cpp` declaration/implementation split) with exactly three states: unspecified (default, no number emitted for a lone spanner), explicit level 1..16 (emitted verbatim; what the reader produces), and identity (an author-only, never-serialized label; start/continue/stop sharing an id are the same logical spanner and the writer assigns the number). A contradictory state is unrepresentable by construction. Constructed directly (`SpannerNumber{3}`, `SpannerNumber{"my-id"}`), not via factory functions. Follows the `TimeChoice` "choice" pattern: nothing throws. An out-of-range level or an empty id collapses to unspecified at construction, and calling `level()`/`identity()` for the wrong kind returns a harmless default (`NUMBER_LEVEL_UNSPECIFIED`, or an empty string) instead of an exception. - Breaking: `int numberLevel` is replaced by `SpannerNumber number` in `CurveStart`, `CurveContinue`, `CurveStop`, `WedgeStart`, `WedgeStop`, `SpannerStart`, and `SpannerStop`. Tuplets are untouched and keep the legacy `int` + `NUMBER_LEVEL_UNSPECIFIED`. - New `SpannerNumberResolver` runs once per part before writing. It walks the part in true serialization order (measures, staves, voices ascending, notes in vector order with curve stops/continues/starts per note, directions in vector order) and assigns each identity spanner the lowest free number from a 1..16 pool per staff and per spanner class (slur, tied, wedge, octave-shift, bracket, and dashes pools are independent). While an explicit spanner is open its level is reserved, so explicit and identity spanners sharing a pool never collide. Pool exhaustion (more than 16 concurrently open) throws rather than emitting an illegal number. - The wedge and octave-shift writers now emit numbers at all; previously they dropped them on write even when the reader had captured them, so explicit wedge/ottava numbers now survive a round trip. - Readers only ever produce unspecified or explicit, keeping read-to-write fidelity for files that already carry numbers. ## Testing - [x] `SpannerNumberTest`: three-state semantics, out-of-range/empty-input construction collapsing to unspecified, wrong-kind accessors returning harmless defaults (never throwing), equality across all kind combinations - [x] `SpannerIdentityTest`: the #297 scenario — slurs disjoint in musical time that overlap in the stream once voice 1 serializes ahead of voice 2 get distinct numbers (1, 1-reused, 2); explicit + identity pool sharing; slur/tied pool independence; wedge number round-trip - [x] `data/rpatters1/slurs_overbars.musicxml`: the file rpatters1 attached to #297 (a genuine Finale export, confirmed provenance in review), pinned in `roundtrip-baseline.txt` alongside his other contributed fixtures in `data/rpatters1/` - [x] `make test`, `make test-api-roundtrip` (166/166 pinned), `make test-core-dev` (835 corert files), `make check-core-dev`, `make validate-cpp`, `make test-cpp-unit`, `make probe-cpp` - [x] Unity build (`CMAKE_UNITY_BUILD=ON`, batch size 0) compiles - [x] Rebased over main (through #333) and CI re-run on the rebased branch ## References - Closes #297 - Related to #249 (one fact, one field: motivates unifying level and identity into a single type instead of adding a parallel field)
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
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
#268 introduced a test file that had a few more unsupported goodies in it. I want to support the file in full so this PR/commit adds:
Summary
data/rpatters1/timesigs_composite-ref.musicxml(the untrimmed original of the fixture thatmotivated the
TimeSignatureDataredesign in #268) carried three thingsmx::apididn't model:<score-part>/<group>, the<measure-numbering>multiple-rest-always/multiple-rest-range/systemattributes, and<sound>/<swing>. A trimmed-modifiedvariant was pinned instead sothe composite time-signature round trip could land in isolation. This PR adds the three missing
features so the original file passes too:
PartData::groups(std::vector<std::string>) models<score-part>'s repeatable<group>element, wired through
PartReader/PartWriter.MeasureData::measureNumberingMultipleRestAlways/measureNumberingMultipleRestRange(Bool)and
measureNumberingSystemRelation(newSystemRelationenum) model the<measure-numbering>attributes beyond its text value, wired through
MeasureReader/MeasureWriterand a newConverter::convertSystemRelationbridge.SoundData::swing(newSwingData/SwingNoteType) models<sound>/<swing>(straight vs.ratio, note type, style), wired through
SoundFunctions.cpp.SoundData.h's "intentionally notmodeled" comment is updated to drop
<swing>from the list.data/rpatters1/timesigs_composite-ref.musicxml'smusic-fontfont-familyis comma-only(
"Maestro,engraved"), which doesn't match mx's normalized comma-space output. Rather than editthe fixture, the corert
normalizeForComparisonpipeline now re-spaces any comma-separated-textattribute (currently just
font-family, the schema's only user of that type) to"item, item"onboth sides of the round-trip comparison (new
CommaSeparatedFields.h, mirroring the existingDecimalFields.h/stripZerosFromDecimalFieldspattern) — so the fixture stays byte-identical toits original form.
timesigs_composite-ref.musicxmlis now pinned inroundtrip-baseline.txtalongside the existing-modifiedvariant.Also removes
DefaultsData::measureNumbering(src/include/mx/api/DefaultsData.h), which wasdead code: MusicXML's
<defaults>element has nomeasure-numberingchild in the schema, so thefield could never be read from or written to a file. The real, working feature is
MeasureData::measureNumberingabove, which is measure-scoped to match how<measure-numbering>actually appears in MusicXML (inside a per-measure
<print>).Testing
ScorePartGroupApiTest.cpp(group round trip),SoundApiTest.cppswingcases,
MeasureDataTest.cppmeasureNumberingAttributesRoundTripmake test-api-roundtrip: 165 passed (of 165 pinned), including the newly-addedrpatters1/timesigs_composite-ref.musicxmlmake test: all tests passed (4831 assertions in 396 test cases)make check-core-dev: fmt-check and warning-free build both passmake test-core-dev: all corert tests passed (835 test cases)References