feat: model grace and cue as independent bools on NoteData#301
Conversation
webern
added
feature
Claude
Replace the mx::api NoteType enum (normal/grace/cue) with two independent bools on NoteData, isGrace and isCue, so a MusicXML <grace/><cue/> note -- a grace note inside a cue passage -- is representable. Previously such a note round-tripped as grace-only: the reader never set the cue bit for core's GraceCueNoteGroup and the writer had no branch emitting it. - NoteData: remove enum NoteType; add bool isGrace / bool isCue with doc comments stating the schema facts (grace notes carry no wire <duration>; cue and grace-cue notes cannot carry <tie>). Update the equality block and constructor defaults. - NoteReader: set the cue flag when a graceNoteGroup's choice is graceCueNoteGroup. - NoteFunctions: assign the two bools directly instead of collapsing to an enum. - NoteWriter: branch on the bool pair -- (F,F) NormalNoteGroup, (F,T) CueNoteGroup, (T,F) GraceNoteGroup/GraceNormalNoteGroup, (T,T) GraceNoteGroup/GraceCueNoteGroup. Ties on cue and grace-cue notes are silently dropped (unchanged behavior for cue). - Tests: GraceCueApiTest covers all four combinations through the api round-trip, the reader path, exact writer output, tie dropping on cue, and tie preservation on grace-normal. - Corpus: add data/synthetic/grace-cue.4.0.xml (validates against the 4.0 XSD), bump the corert pinned count to 833, regenerate audit artifacts, and pin the fixture in roundtrip-baseline.txt (it passes the strict compare). make audit also backfilled the missing sidecar for data/rpatters1/timesigs_composite-ref.musicxml from #287. BREAKING CHANGE: removes enum class mx::api::NoteType and the NoteData::noteType field. Closes #288.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28499 / 36624 |
| Functions | 74.3% | 6352 / 8550 |
| Branches | 50.7% | 22655 / 44725 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 78.9% | 6122 / 7760 |
| Functions | 64.3% | 2090 / 3251 |
| Branches | 48.0% | 5218 / 10867 |
Core HTML report | API HTML report
Commit d75b1dfdff6cb1e84e26bb7cd8f96e597612c54e.
gen-quality
|
webern
left a comment
webern
left a comment
There was a problem hiding this comment.
Spot-checked the grace/cue split against the XSD and traced both the reader and writer paths. Three notes inline: the "schema has no <tie>" claim checks out against the actual spec text, round-tripping is lossless by construction (not just by observed test results), and the authoring-time silent-drop for schema-incompatible combinations is a deliberate, pre-existing policy this PR extends consistently rather than a new gap. Comment only, not blocking.
Generated by Claude Code
| // MusicXML's four <note> flavors -- normal, grace, cue, and grace-cue -- | ||
| // are the four combinations of these two independent flags. Schema facts: | ||
| // a grace note carries no <duration> on the wire (durationTimeTicks reads | ||
| // as 0 and is ignored on write), and cue notes -- including grace-cue | ||
| // notes -- cannot carry <tie> (ties on them are silently dropped on write). |
There was a problem hiding this comment.
Checked this against the XSD directly (docs/musicxml-4.0-ed15c23.xsd, the note complexType). Its own doc annotation says it outright: "grace notes do not have a duration element... Cue notes have a duration element... but no tie elements." Confirmed on the mx::core side too: NormalNoteGroup/GraceNormalNoteGroup have a tie() accessor, CueNoteGroup/GraceCueNoteGroup don't. So this is describing a real schema constraint, not something mx::api is choosing to impose.
Generated by Claude Code
| switch (myNoteData.noteType) | ||
| // The schema has no <tie> on cue and grace-cue notes, so ties on them are | ||
| // silently dropped. Normal and grace-normal notes carry their ties. | ||
| if (!myNoteData.isCue) |
There was a problem hiding this comment.
Worth being explicit about the gap this leaves. Round-tripping is safe (see the NoteReader.cpp comment), but nothing stops an API consumer from authoring a NoteData with isCue=true plus isTieStart/isTieStop set, or isGrace=true plus a nonzero durationTimeTicks -- those get silently dropped here (and the grace-duration case a few lines down in assembleNoteChoice) with no exception or signal. That mirrors the pre-existing behavior for cue-note ties (previously handled by the old NoteType enum's case cue: break;), just now extended consistently to grace-cue. Given there's no validation layer elsewhere in the writer either, I think that's the right call for the api's current shape. Flagging as a comment, not asking for a change.
Generated by Claude Code
| else | ||
| { | ||
| // <grace/> + <cue/>: a grace note inside a cue passage. The | ||
| // grace-cue group carries no <tie> in the schema. | ||
| myIsCue = true; | ||
| } |
There was a problem hiding this comment.
Traced this against NoteWriter's assembleNoteChoice/setNoteChoiceAndFullNoteGroup. Round-tripping is lossless by construction here, not just by luck: this function can only ever set myDurationValue = 0 for a grace note (a few lines up, unconditional -- GraceNoteGroup's core type has no duration() to read), and can only ever leave tie unset for cue/grace-cue -- as here, where the grace-cue branch sets myIsCue but never calls setTie(), since CueNoteGroup/GraceCueNoteGroup have no tie() to read from. So the four combinations this reader can produce are exactly the four the writer can re-emit -- a legitimately-parsed file can't lose data on round trip.
Generated by Claude Code
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 | 78.9% | 6134 / 7772 |
| Functions | 64.3% | 2093 / 3254 |
| Branches | 48.1% | 5233 / 10887 |
Core HTML report | API HTML report
Commit 7de533bffdb026fa7ca75b4a5443b72dc15281a6.
gen-quality
|
1 participant
Human Summary
This seems ok, I guess, for now. We re-model
graceandcuenotes as bool flags onNoteData, even though the spec is a little weird (perhaps with good reason) in thatgracecannot have duration andcuecannot have a tie (that's the one that seems really weird to me). So when reading and writing from MusicXML there is no problem.The only issue is that when writing hand-authored
NoteData, an author does not have any way to know that these would be dropped on write. It seems better than not supportinggraceandcuecombinations, which is what the issue was about, so this seems like a good-enough "for now" fix to me.Summary
MusicXML permits four
<note>flavors: normal, cue, grace, and grace-cue (a grace note inside a cue passage). The api modeled this as a three-value enum (NoteType), which cannot represent grace-cue, so a<grace/><cue/>note round-tripped as grace-only:NoteReadernever set the cue bit for core'sGraceCueNoteGroupandNoteWriterhad no branch emitting it.Per the discussion on the issue,
NoteDatanow carries two independent bools,isGraceandisCue, whose four combinations map one-to-one onto the four note flavors.This is a BREAKING api change (removes
enum class mx::api::NoteTypeand theNoteData::noteTypefield), intended for the batched breaking release. Migration:noteType == NoteType::gracebecomesisGrace,noteType == NoteType::cuebecomesisCue,noteType == NoteType::normalbecomes!isGrace && !isCue.Details:
NoteData: enum deleted; the new bools carry doc comments stating the schema facts (grace notes have no wire<duration>-- ticks read as 0 and are ignored on write; cue and grace-cue notes cannot carry<tie>-- ties on them are silently dropped on write, which preserves the existing behavior for cue notes). Equality block and constructor defaults updated.NoteReader: sets the cue flag when a grace note group's choice isGraceCueNoteGroup.NoteFunctions: assigns the two bools directly instead of collapsing to an enum.NoteWriter: branches on the bool pair -- (F,F)NormalNoteGroup, (F,T)CueNoteGroup, (T,F)GraceNoteGroup/GraceNormalNoteGroup, (T,T)GraceNoteGroup/GraceCueNoteGroup.data/synthetic/grace-cue.4.0.xml(validates against the 4.0 XSD) exercising all four combinations in one measure; corert pinned count bumped to 833; audit artifacts regenerated (this also backfilled the sidecar fordata/rpatters1/timesigs_composite-ref.musicxml, which test: add rpatters1 composite time-sig sample #287 added without one); the fixture is pinned inroundtrip-baseline.txtsince it survives the strict DOM compare.Testing
GraceCueApiTestcovers all four combinations through the api round-trip, the reader path (embedded<grace/><cue/>fixture), exact writer output (grace before cue, no<duration>on grace notes), tie dropping on cue and grace-cue, and tie preservation on grace-normal notesmake test: all pass (4497 assertions in 351 test cases, plus the three examples)make test-core-dev: all pass (834 test cases; pinned count 833, 0 skipped)make test-api-roundtrip: 122 passed, 0 failed (of 122 pinned, including the newsynthetic/grace-cue.4.0.xml)make fmt/make checkcleanxmllint --nonet --schema docs/musicxml-4.0-ed15c23.xsd data/synthetic/grace-cue.4.0.xmlvalidatesReferences