feat: model the metronome-note form and beat-unit-tied - #363
Merged
Conversation
Owner
Author
|
/coverage |
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36658 |
| Functions | 74.2% | 6352 / 8561 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 85.7% | 8175 / 9534 |
| Functions | 78.7% | 3244 / 4123 |
| Branches | 53.1% | 7183 / 13535 |
Core HTML report | API HTML report
Commit 88c98f987a82f9fabe1f55e99fddc14cc515d1c0.
Base automatically changed from
claude/direction-types-ordering-9l60vv-e
to
main
July 19, 2026 13:42
Second metronome slice of #324. TempoData's enum-plus-parallel-payload shape is replaced by a TempoChoice choice class (the TimeChoice / PercussionDataChoice pattern) over the three <metronome> bodies: beats-per-minute, metric modulation, and the note-relation form. The unreachable 'unspecified' tempo sentinel is gone -- a default/empty tempo is skipped by the writer on a content check, so nothing throws. The note-relation form -- metric relationships drawn with note figures (metronome-note and metronome-relation, plus per-figure metronome-beam, metronome-dot, metronome-tied, and metronome-tuplet, and the metronome-arrows flag) -- is modeled in NoteRelationData.h with dedicated mini-structs that reuse only the leaf enums (DurationName, Beam); the metronome note vocabulary is far narrower than NoteData. beat-unit-tied (a beat-unit tied to the preceding one) is modeled on both the beats-per-minute and metric-modulation forms. Round-trip baseline 275 -> 284 (all eight metronome-note fixtures plus beat-unit-tied). Every part of <metronome> now round-trips except per-minute's own font override, a documented drop -- the font belongs on the metronome element. Breaking: TempoData::tempoType and the parallel beatsPerMinute / metricModulation fields are replaced by TempoData::choice.
webern
force-pushed
the
claude/direction-types-ordering-9l60vv-f
branch
from
July 19, 2026 13:49
88c98f9 to
028b8ff
Compare
3 tasks
webern
added a commit
that referenced
this pull request
Jul 19, 2026
## Human Summary Changes the way pedals are modeled into two different things. Pedal marks, which are the old fashioned Ped and * marks, and a separate type for more modern lines. ## Summary `DirectionReader::parsePedal` handled only `PedalType` start/stop and silently dropped `sostenuto`, `change`, `continue`, `discontinue`, and `resume`, even though `core::PedalType` carries all seven tags. This was the last unmodeled item in #324. The shape decision (called for in #324) was to store the pedal type as one fact in one field rather than scatter it across start/stop/other vectors. This replaces the `pedalStarts` / `pedalStops` spanner vectors on `DirectionData` with a single `std::vector<PedalData> pedals`, where each `PedalData` carries a `PedalKind` enum covering every pedal-line type. Each pedal event stands on its own and is placed at its own tick, so the model has no neighbor-dependent meaning. - New `PedalData.h`: the `PedalKind` enum (`start`, `stop`, `sostenuto`, `change`, `continueLine`, `discontinue`, `resume`) and the `PedalData` struct. - Reader: line pedals (`line="yes"`, plus the inherently line-formatting types `sostenuto`/`change`/`continue`/`discontinue`/`resume`) go to `pedals`; sign start/stop still route through `MarkData` as `MarkType::pedal` / `damp`, unchanged. - Writer: a single `emitPedal` maps `PedalKind` back to `core::PedalType`, emits `line="yes"`, and emits nothing for an unspecified kind. Only two corpus files use these types (`lysuite/ly31a_Directions.xml`, `lysuite/ly33a_Spanners.xml`) and both fail first on unrelated features, so this does not flip any file to PASS. It is validated by a focused unit test that round-trips each pedal type through the api. This is a breaking change: it reshapes `DirectionData`'s pedal fields. ## Testing - [x] New `PedalAllKinds` round-trips all seven pedal types through the api; `PedalPlacement` covers placement (`*Pedal*`: 44 assertions in 5 test cases) - [x] Full unit suite passes (5130 assertions in 451 test cases) - [x] api round-trip regression unchanged (284 passed, 0 failed of 284 pinned) ## References - Closes #324 (the `symbol` item from that worklist is tracked separately by #294 and is not included here) - Follows the merged direction-type stack #358–#363 - Part of #208 - Spanner-numbering context: #320, #351
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
Unfortunately this is quite complicated, but it appears to correctly model the various forms of metronome markings that MusicXML supports.
Summary
Second and final metronome slice of #324.
TempoData's enum-plus-parallel-payload shape is replaced by aTempoChoicechoice class (theTimeChoice/PercussionDataChoicepattern) over the three<metronome>bodies: beats-per-minute, metric modulation, and the note-relation form. The unreachableunspecifiedtempo sentinel is gone — a default/empty tempo is skipped by the writer on a content check, so nothing throws.The note-relation form — metric relationships drawn with note figures (
metronome-noteandmetronome-relation, plus per-figuremetronome-beam,metronome-dot,metronome-tied, andmetronome-tuplet, and themetronome-arrowsflag) — is modeled in a newNoteRelationData.hwith dedicated mini-structs. They reuse only the leaf enums (DurationName,Beam); a metronome note figure has none of the pitch/stem/staff/voice machinery of a real note, soNoteDatais not reused.beat-unit-tied(a beat-unit tied to the preceding one, e.g. "quarter + eighth = 120") is modeled on both the beats-per-minute and metric-modulation forms.Every part of
<metronome>now round-trips except per-minute's own font override, which is a documented drop — the font belongs on the metronome element, and no real-world file uses the per-minute font.Breaking:
TempoData::tempoTypeand the parallelbeatsPerMinute/metricModulationfields are replaced byTempoData::choice.Testing
roundTripNoteRelation(strong whole-body equality check),roundTripBeatUnitTied, plusroundTripMetricModulation/roundTripBpm/attributes migrated to the choice APIbeat-unit-tied)References