feat: support multi-note tremolos in mx::api - #330
Merged
Conversation
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36642 |
| 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% | 6437 / 7956 |
| Functions | 68.1% | 2230 / 3276 |
| Branches | 49.7% | 5525 / 11122 |
Core HTML report | API HTML report
Commit 0f5c38ddfffa33ff61e573490bf363e32ab9f2a6.
gen-quality
|
Add MarkType::tremoloStart/tremoloStop plus MarkData::tremoloMarks (the slash count, 0-8) so a <tremolo type="start"/"stop"> pair spanning two adjacent notes can be authored and round-tripped. The existing tremoloSingleOne..Five glyph marks are unchanged. Pairing across notes is left to the api consumer via document order, matching the isTieStart/ isTieStop precedent -- MusicXML's <tremolo> has no `number` attribute to key off of. Closes #329.
webern
force-pushed
the
claude/mx-api-interface-jdo2yy
branch
from
July 11, 2026 14:08
33aead4 to
64dd3de
Compare
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.1% | 6570 / 8104 |
| Functions | 68.3% | 2258 / 3306 |
| Branches | 49.8% | 5647 / 11343 |
Core HTML report | API HTML report
Commit b46e1c636f08c0674a1ce0bca256965aed706ec5.
gen-quality
|
webern
commented
Jul 12, 2026
Comment on lines
+258
to
+262
| // The tremolo mark count (MusicXML <tremolo> text value, 0-8 slashes). Only meaningful | ||
| // when markType is tremoloStart or tremoloStop; absent means "not specified" (the writer | ||
| // falls back to a default). The tremoloSingle* mark types encode their slash count in the | ||
| // enumerator itself and leave this absent. | ||
| std::optional<int> tremoloMarks; |
Owner
Author
There was a problem hiding this comment.
The MarkData class is really quite a mess. It has fields that are only relevant when the mark is of the associated type that they are relevant to.
Here's what I think we should do. We should create a MarkDataChoice case similar to how we recently handled time signatures (follow that pattern of variant use without throwing or UB)
Any new items that need to go on MarkData but only apply to certain marks should go in the choice field. The choice field SHOULD align with the selected mark type but it is not enforced.
We document the fields of MarkData that are (more or less) common to all marks, and separate them from existing fields (with documentation) from fields that only apply to certain marks, and we document that new such cases should be added to the markdatachoice class instead of being added as direct fields.
Per review: MarkData was accumulating direct fields that only apply to one mark type. Introduce MarkDataChoice (a variant class following the TimeChoice/ComplexTimeSignature pattern) to hold such mark-specific payloads, starting with TremoloMarkData for tremoloStart/tremoloStop's slash count. MarkData::tremoloMarks is replaced by MarkData::choice, and MarkData.h now documents which fields are common vs. mark-specific, directing future mark-specific additions to MarkDataChoice instead of new direct fields.
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.0% | 6581 / 8120 |
| Functions | 68.3% | 2263 / 3313 |
| Branches | 49.8% | 5653 / 11357 |
Core HTML report | API HTML report
Commit 12e54fd374eb479b85d1211eaee6a2f32f21e9e6.
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
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
I do not love this but I am going to merge it.
MarkDataandMarkTypeinteract in a terrible way. If yourMarkTypeis something, like mordent, thenMarkDatamight 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::variantis 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::apicould represent single-note (glyph) tremolos but silently dropped the measured, two-noteform written as
<tremolo type="start">/<tremolo type="stop">on adjacent notes. This adds it.MarkTypegainstremoloStartandtremoloStop, marking the first and second note of ameasured tremolo. The existing
tremoloSingleOne..Fiveglyph marks are unchanged.MarkDatagains achoicefield (MarkDataChoice), a variant class (mirroring theTimeChoice/ComplexTimeSignaturepattern) that carries payloads for mark types whose datadoesn't fit the common fields. For
tremoloStart/tremoloStopit holdsTremoloMarkDatawithtremoloMarks(std::optional<int>), the slash count (0-8). ThetremoloSingle*marks keepencoding their count in the enumerator itself, as before.
<tremolo>has nonumberattribute (unlike slur/tie), so there is nothing to key across-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/isTieStopalready work.OrnamentsFunctions.cpp(read) andNotationsWriter.cpp(write) are updated symmetrically;isMarkTremolo/isMarkOrnamentinclude the two new mark types. Unmeasured tremolos are stillnot representable and continue to be dropped as before.
MarkData.hnow documents which of its fields are common to all marks vs. specificto one mark type, and directs future mark-specific additions to a new
MarkDataChoicealternative (
MarkDataChoice.h) rather than a new ad hoc direct field.Testing
NoteDataTest.cpp: an in-memory round trip of two adjacent notes carryingtremoloStart/tremoloStopmarks, and a read test againstdata/synthetic/tremolo.3.0.xmlmake test: all pass (4821 assertions in 392 test cases, plus the three examples)make test-api-roundtrip: 164 passed, 0 failed (of 164 pinned; no regressions)make discover-api-roundtrip: no newly-attributable passes; the two corpus files with realstart/stop tremolo content (
musuite/testTremolo.xml,lysuite/ly23e_Tuplets_Tremolo.xml)still fail on unrelated pre-existing subset gaps (an
identification/rightsmismatch and atime-modificationgap, respectively), so noroundtrip-baseline.txtentry was addedmake fmt/make checkcleanReferences
mx::api#329