feat(api): support the full time-signature model - #321
Merged
Conversation
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.9% | 6448 / 7974 |
| Functions | 67.7% | 2224 / 3284 |
| Branches | 49.8% | 5542 / 11132 |
Core HTML report | API HTML report
Commit 302722164546a53f08e0194c3f6204a040bb3c5b.
gen-quality
|
webern
force-pushed
the
claude/eloquent-turing-us0rtv
branch
from
July 7, 2026 09:24
96c10f3 to
16274e6
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.0% | 6481 / 8004 |
| Functions | 67.8% | 2228 / 3285 |
| Branches | 49.8% | 5570 / 11180 |
Core HTML report | API HTML report
Commit 156117869514f0a8463e5a473ccf4c0c7291398b.
gen-quality
|
webern
force-pushed
the
claude/eloquent-turing-us0rtv
branch
from
July 8, 2026 21:16
16274e6 to
1cd06bf
Compare
webern
commented
Jul 8, 2026
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.9% | 6481 / 8009 |
| Functions | 67.8% | 2228 / 3285 |
| Branches | 49.8% | 5568 / 11186 |
Core HTML report | API HTML report
Commit d5a425752ce17d92fa004a46cd7bdbccc17179b4.
gen-quality
|
Redesign the time-signature api around a TimeChoice that switches
between the simple case (the ordinary N/D you want 99% of the time) and
a complex case that quarantines everything unusual. The simple type and
the complex machinery live in separate headers, so ordinary 4/4 work
never has to look at the complex surface.
Public shape (src/include/mx/api/):
- TimeSignatureData.h: TimeFraction (shared beats/beat-type leaf) and
TimeSignatureData { TimeSignatureSymbol symbol; TimeFraction fraction; }
where the symbol is the narrow {unspecified, common, cut}.
- ComplexTimeSignature.h: the full ComplexTimeSymbol vocabulary,
TimeSeparator, TimeRelation, InterchangeableTimeSignature,
MeteredTimeSignature, and ComplexTimeSignature -- itself a
{ metered, senzaMisura } choice mirroring the spec's one real fork.
- TimeChoice.h: the { simple, complex } switch, carrying the
whole-<time> attributes (isImplicit, display).
Guarantees:
- TimeChoice::complex auto-collapses a simple-equivalent meter (one
fraction, no interchangeable, no separator, a symbol the simple case
can express) back to simple, so a plain N/D can never hide in the
complex arena and there is one canonical representation.
- The two symbol enums do not overlap on the unusual values:
single-number/note/dotted-note exist only in the complex vocabulary,
so they cannot appear on the simple type.
Per-staff signatures move to std::map<int, TimeChoice>
staffTimeSignatures on MeasureData (keyed by staff index; no staff
index baked into the signature type).
Choice-type logic (factories, accessors, equality, auto-collapse) lives
in TUs under src/private/mx/api/, keeping the headers declaration-only.
impl: TimeReader translates each core::Time to a TimeChoice;
PropertiesWriter::writeTime regenerates all shapes and takes the staff
index separately; MeasureReader/MeasureWriter route by staff scope and
carry per-staff overrides forward; Converter gains narrow/full symbol,
separator, and relation tables.
Four corpus files newly pass the api round-trip and are pinned
(ly11d/e composite, ly11f symbols, ly11h senza-misura).
webern
force-pushed
the
claude/eloquent-turing-us0rtv
branch
from
July 8, 2026 22:19
1cd06bf to
0a3855c
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.1% | 6565 / 8093 |
| Functions | 68.1% | 2250 / 3306 |
| Branches | 49.8% | 5637 / 11317 |
Core HTML report | API HTML report
Commit 52649b0a7f0c21fdcba3401fb3ba170b6c92396b.
gen-quality
|
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8552 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 81.0% | 6557 / 8091 |
| Functions | 68.0% | 2249 / 3306 |
| Branches | 49.7% | 5626 / 11321 |
Core HTML report | API HTML report
Commit 70f0b898698bbc65d694318ef2c71f3f4ffe9001.
gen-quality
|
The simple()/complex()/metered()/senzaMisura() accessors return a default-constructed value on the wrong kind rather than throwing, so 'Precondition' was the wrong word (it implies undefined behavior when violated). Reword to point the reader at the isX() check instead.
…nd-trip allow list timesigs_composite-ref.musicxml is the repro rpatters1 attached to #268 before this PR's TimeSignatureData redesign began; it now round-trips losslessly alongside the other fixtures this PR unblocks.
The api always normalizes a font-family list's comma separator to ", " on write (core::FontFamily), so the fixture's single-space-free "Maestro,engraved" byte-diverged from the round-tripped output despite matching in substance. Match the writer's normalized spacing.
<score-part>/<group> (Finale's ensemble-grouping metadata) has no mx::api surface -- PartData carries no field for it and PartReader silently drops it, which is an unrelated pre-existing gap this fixture isn't meant to exercise. Trimmed so the fixture stays scoped to the composite time-signature round-trip it was added to pin.
Contributor
|
I quite like the approach you have taken. |
… instead Restores timesigs_composite-ref.musicxml byte-exact to the original rpatters1 attached to #268 -- it stays out of the round-trip allow list as-is, since it also exercises unrelated pre-existing mx::api gaps (score-part/group, music-font comma spacing, measure-numbering multiple-rest-* attributes). timesigs_composite-ref-modified.musicxml is a new sibling with just those three unrelated spots trimmed; it's the one pinned in roundtrip-baseline.txt, keeping the fixture scoped to what it's meant to exercise: the composite/additive time-signature round-trip. Bumped the corert pinned file count (833 -> 834) for the added file.
<sound>/<swing> is documented (SoundData.h) as intentionally out of mx::api's scope alongside midi-instrument/midi-device/play/offset. Another unrelated pre-existing gap this fixture wasn't meant to exercise; trimmed from the modified sibling like the other three.
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% | 6557 / 8091 |
| Functions | 68.0% | 2249 / 3306 |
| Branches | 49.7% | 5626 / 11321 |
Core HTML report | API HTML report
Commit f98af336227c89db9cb098652a2162777ee5e78f.
gen-quality
|
This was referenced Jul 11, 2026
webern
added a commit
that referenced
this pull request
Jul 12, 2026
## Human Summary I do not love this but I am going to merge it. `MarkData` and `MarkType` interact in a terrible way. If your `MarkType` is something, like mordent, then `MarkData` might be used to carry some additional information relevant only to mordents. Rather than continue this practice I introduced a MarkDataChoice type for future additions to this pattern, the first of which is multi-note tremolos which, I guess, you can specify the number of tremolo marks for. This is not a clear API and ultimately it stems from a lack of Rust enums, which are so so wonderful compared to C++. I guess the `std::variant` is C++'s answer to Rust enums, but it's ugly has exceptions and undefined behavior, so I'm hiding it behind these "choice" classes (this started with #321). Ultimately, I hope this is clear enough for coding agents to understand and use correctly even if it is just, not a great interface. ## Summary `mx::api` could represent single-note (glyph) tremolos but silently dropped the measured, two-note form written as `<tremolo type="start">`/`<tremolo type="stop">` on adjacent notes. This adds it. - `MarkType` gains `tremoloStart` and `tremoloStop`, marking the first and second note of a measured tremolo. The existing `tremoloSingleOne..Five` glyph marks are unchanged. - `MarkData` gains a `choice` field (`MarkDataChoice`), a variant class (mirroring the `TimeChoice`/`ComplexTimeSignature` pattern) that carries payloads for mark types whose data doesn't fit the common fields. For `tremoloStart`/`tremoloStop` it holds `TremoloMarkData` with `tremoloMarks` (`std::optional<int>`), the slash count (0-8). The `tremoloSingle*` marks keep encoding their count in the enumerator itself, as before. - MusicXML's `<tremolo>` has no `number` attribute (unlike slur/tie), so there is nothing to key a cross-note match on. Pairing the start on one note with the stop on a later note is left to the api consumer via document order, the same way `NoteData::isTieStart`/`isTieStop` already work. - `OrnamentsFunctions.cpp` (read) and `NotationsWriter.cpp` (write) are updated symmetrically; `isMarkTremolo`/`isMarkOrnament` include the two new mark types. Unmeasured tremolos are still not representable and continue to be dropped as before. - Per review, `MarkData.h` now documents which of its fields are common to all marks vs. specific to one mark type, and directs future mark-specific additions to a new `MarkDataChoice` alternative (`MarkDataChoice.h`) rather than a new ad hoc direct field. ## Testing - [x] New tests in `NoteDataTest.cpp`: an in-memory round trip of two adjacent notes carrying `tremoloStart`/`tremoloStop` marks, and a read test against `data/synthetic/tremolo.3.0.xml` - [x] `make test`: all pass (4821 assertions in 392 test cases, plus the three examples) - [x] `make test-api-roundtrip`: 164 passed, 0 failed (of 164 pinned; no regressions) - [x] `make discover-api-roundtrip`: no newly-attributable passes; the two corpus files with real start/stop tremolo content (`musuite/testTremolo.xml`, `lysuite/ly23e_Tuplets_Tremolo.xml`) still fail on unrelated pre-existing subset gaps (an `identification`/`rights` mismatch and a `time-modification` gap, respectively), so no `roundtrip-baseline.txt` entry was added - [x] `make fmt` / `make check` clean ## References - Closes #329
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
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
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
When @rpatters1 opened #268, my immediate thought was, oh no, we have to support all of the time signature model. My reasoning is that just adding the one case that is needed now will only last so long before a different semantic situation is encountered.
I wanted to preserve some sort of "simple" path, so I created a "choice" class where
TimeSignatureDataused to be. It can be eithersimpleorcomplexwhere simple basically meansnumerator/denominatorwith or without theCorCutsymbol.Anything beyond that is shuttled into
ComplexTimeSignaturewhere, I believe, we have modeled all of what MusicXML supports.!!Breaking!!
We use "breaking" somewhat loosely around here, since just about any
mx::apichange introduces some amount of change to the client code, but this one is bigger than the usual breakage. It requires edits anywhere time signatures are involved in client code. As such, I spent some extra time on it and I hope I have a stable design here.Summary
Redesigns the time-signature model in
mx::apito cover the full MusicXML<time>element instead of a single beats/beat-type pair with a narrow symbol set.TimeSignatureDatais now built around a sharedTimeFractionleaf (beats/beatType); itsTimeSignatureSymbolis narrowed tounspecified/common/cut.ComplexTimeSignature(ComplexTimeSignature.h) quarantines everything unusual: composite/additive multi-fraction meters, the full symbol vocabulary (singleNumber,note,dottedNote), separators, interchangeable (dual) meters, and senza-misura.TimeChoice(TimeChoice.h) is a{simple, complex}switch carrying the whole-<time>attributes (isImplicit,display). Building aTimeChoicefrom aComplexTimeSignaturethat turns out to be simple-equivalent auto-collapses it back tosimple, so there's always one canonical representation.MeasureData::timeSignatureis now aTimeChoice; a newMeasureData::staffTimeSignatures(std::map<int, TimeChoice>) holds per-staff<time number="N">overrides.TimeReadernow translates every<time>in a measure (not just the first) into aTimeChoice+ staff index;PropertiesWriter::writeTimeregenerates all shapes and writes per-staffnumber=;MeasureReader/MeasureWriter/Cursorcarry per-staff overrides forward measure-to-measure;Convertergains symbol/separator/relation enum tables.roundtrip-baseline.txt(ly11d/ly11ecomposite meters,ly11fsymbols,ly11hsenza-misura).This is a breaking change to
mx::api:beats/beatTypemove intofraction,isImplicit/displaymove fromTimeSignatureDataontoTimeChoice, andTimeSignatureSymbol::singleNumbermoves to the complex-onlyComplexTimeSymbol.Testing
TimeSignatureApiTest.cpp: new cases for default/auto-collapse/stays-complex/equality behavior, plus round-trip coverage for composite meters, senza-misura (with and without glyph), interchangeable pairs, widened symbol+separator, and per-staff overrides with carry-forwardroundtrip-baseline.txt:ly11d,ly11e,ly11f,ly11hunpinned (now pass the api round-trip)TimeChoiceconstruction (ApiK007a/c,ApiK009bSlur,ApiK014aFermatas,ApiK015aLayout,ApiLy43e,ApiMuAccidentals1,ChordApiTest)timesigs_composite-ref.musicxmlwhich was submitted by @rpatters1 to cover his use caseReferences