feat: extend direction text formatting in mx::api and unify the enclosure enum - #378
Merged
Merged
Conversation
- WordsData, SymbolData and RehearsalData gain justify, mapping the attribute of the same name on <words>, <symbol> and <rehearsal>. - DirectionData gains systemRelation, mapping the system attribute on <direction> and <harmony>. - Enclosure in ApiCommon.h replaces RehearsalEnclosure and PercussionEnclosure, completing the enclosure-shape vocabulary and collapsing three conversion paths into one Converter table. The two old enum names are removed; the vocabularies are unchanged.
5 tasks
Contributor
Author
|
This is the same as #374 but based on the correct source branch. |
webern
approved these changes
Aug 4, 2026
webern
left a comment
webern
left a comment
Owner
There was a problem hiding this comment.
Looks good. I think having enums where certain members are only applicable in certain scenarios is not ideal, but you are not wrong to choose this simplification. I think I balked at it at first which is why it took longer to review. Sorry for the delay.
| // the system attribute of <measure-numbering> (MeasureData) and of <direction> and <harmony> | ||
| // (DirectionData). only... means the item is drawn on the top or bottom part of the system instead | ||
| // of this part; also... means it is drawn on both. unspecified means the attribute is absent. | ||
| // onlyBottom and alsoBottom are measure-numbering only; MusicXML has no bottom-of-system direction. |
Owner
There was a problem hiding this comment.
I think this is unfortunate, but I don't disagree with the choice you made. There's often a tradeoff like this when trying to simplify the MusicXML surface area.
rpatters1
added a commit
to rpatters1/mx
that referenced
this pull request
Aug 4, 2026
## Human Summary
There were two enums for the same 26 dynamic markings: the dynamics block of `MarkType`, and
`StandardDynamic`, which the previous commit added for compound dynamics. They are now one.
`MarkType` keeps a single `dynamics` value and the symbol itself moves into `MarkDataChoice`, so
`<dynamics>` has one shape in the api instead of three. Writing a dynamic is unchanged in length --
`MarkData{StandardDynamic::ff}` -- and `ffz` now spells itself in `MarkData::name`, which it could
not do before.
## Summary
`StandardDynamic` won and the 29 dynamics values in `MarkType` were removed (`p` through `sfzp`,
`otherDynamics`, `compoundDynamics`, `unknownDynamics`), replaced by one `dynamics` value.
`StandardDynamic` was chosen despite being the newer type because it had not shipped -- it exists
only on this branch -- while removing `MarkType::ff` and friends would break every downstream
caller. The `Enclosure` unification in webern#378 is the precedent for accepting a breaking api change
where the model improves.
The symbol lives in `MarkDataChoice`, which gains `Kind::dynamic` holding a `StandardDynamic`
alongside the existing `Kind::compoundDynamics`. `MarkDataChoice(CompoundDynamicsData)` collapses
a lone standard component to `Kind::dynamic`, following `TimeChoice(ComplexTimeSignature)`. The
collapse is what makes two alternatives safe rather than confusing: `ff` has exactly one
representation however it was built, so code reading a plain dynamic never has to look inside a
compound for it. A lone other-dynamics does not collapse -- it has no dedicated MusicXML element,
so it stays a compound of one, which keeps `Kind::dynamic` meaning exactly "a standard
abbreviation" and lets `dynamic()` return a bare `StandardDynamic`.
Before this change one `<dynamics>` element had three api shapes: `markType == ff`; or
`markType == otherDynamics` with the text in `MarkData::name` and the glyph in
`choice.otherMark().smufl`; or `markType == compoundDynamics` with both facts restated inside
`DynamicsComponent`. All three collapse to one component list. `DynamicsReader` loses its
single-child special case and `DynamicsWriter` loses its entire non-compound branch.
`MarkData` gains `MarkData(StandardDynamic)` and `MarkData(CompoundDynamicsData)`, which set
`markType`, `choice`, and `name` together. Dynamics is the one mark family where a payload
determines its mark type uniquely -- `Kind::tremolo` covers `tremoloStart`/`tremoloStop`,
`Kind::arpeggiate` covers three mark types -- so it is the one family where a constructor can set
both without guessing. Every dynamics mark in the tree, including the reader, now goes through
these constructors; there are no direct `choice = CompoundDynamicsData{...}` assignments left.
`MarkData::name` keeps working. It spells the whole mark -- "ff", "z", or "ffz" -- derived by one
rule with no special cases, the way articulations and fermatas already name themselves. The writer
ignores it for dynamics, so it cannot contradict the output. A single standard component yields
what the old reader yielded, and a lone other-dynamics yields its text, so compatibility falls out
of the general rule rather than being special-cased. `name` is documented for the first time,
including that a self-naming mark derives it at construction and that replacing `choice` afterwards
means re-deriving it.
Mapping tables went from two to one, and the survivor is public. `markDataDynamicsKindToString`
and `dynamicsKindToName` were near-identical private Kind-to-string switches differing only in
their default arm; both are deleted, along with `Converter::dynamicsMap` and both `convertDynamic`
overloads. `standardDynamicsMap` remains as the only dynamics map. In their place
`DynamicsData.h` exports `toString(StandardDynamic)`, `toString(const DynamicsComponent &)` and
`toString(const CompoundDynamicsData &)`, so clients can spell a mark they built rather than
reaching for `name`. The core cannot supply these strings: it hard-codes the element literals
inline in the generated parse and serialize routines with no accessor, and `mx::impl` never sees
the XML -- by the time `DynamicsReader` runs, `<ff/>` is already `DynamicsChoice::Kind::ff`.
`isMarkDynamic` is now a single comparison and no longer has its duplicated `MarkType::p` test.
Dispatch written as `if (isMarkDynamic(...))` keeps working verbatim, so notations and directions
are untouched, as are `NoteAttachmentData::marks`, `DirectionChoice`, and every other mark family.
MusicXML output is unchanged; this is an api-shape change only.
## Breaking changes
- The 29 dynamics values of `MarkType` are gone. Use `MarkType::dynamics` with
`MarkData(StandardDynamic)` or `MarkData(CompoundDynamicsData)`. Every site is a compile error.
- `MarkData::name` is no longer the other-dynamics text on the write path; that text is
`OtherDynamicsData::text`. The reader still fills `name` in, so read-only clients are unaffected.
- Building a `CompoundDynamicsData` with a single standard component yields `Kind::dynamic`, not
`Kind::compoundDynamics`.
## Testing
- make test-all MX_RUNNING_IN_DOCKER=1 (837 + 41 + 502 test cases, 5541 assertions; 305 pinned
round-trip files, 0 failed)
- strict unity build of target mx (CMAKE_UNITY_BUILD_BATCH_SIZE=0)
- git diff --check
- make fmt MX_RUNNING_IN_DOCKER=1. Formatting was verified against the host clang-format only;
Docker was unavailable, so CI's clang-format has not seen these files. ApiEquality.h is
deliberately left as-is (host/Docker version drift, see 3a99c92).
rpatters1
added a commit
to rpatters1/mx
that referenced
this pull request
Aug 4, 2026
## Human Summary
There were two enums for the same 26 dynamic markings: the dynamics block of `MarkType`, and
`StandardDynamic`, which the previous commit added for compound dynamics. They are now one.
`MarkType` keeps a single `dynamics` value and the symbol itself moves into `MarkDataChoice`, so
`<dynamics>` has one shape in the api instead of three. Writing a dynamic is unchanged in length --
`MarkData{StandardDynamic::ff}` -- and `ffz` now spells itself in `MarkData::name`, which it could
not do before.
## Summary
`StandardDynamic` won and the 29 dynamics values in `MarkType` were removed (`p` through `sfzp`,
`otherDynamics`, `compoundDynamics`, `unknownDynamics`), replaced by one `dynamics` value.
`StandardDynamic` was chosen despite being the newer type because it had not shipped -- it exists
only on this branch -- while removing `MarkType::ff` and friends would break every downstream
caller. The `Enclosure` unification in webern#378 is the precedent for accepting a breaking api change
where the model improves.
The symbol lives in `MarkDataChoice`, which gains `Kind::dynamic` holding a `StandardDynamic`
alongside the existing `Kind::compoundDynamics`. `MarkDataChoice(CompoundDynamicsData)` collapses
a lone standard component to `Kind::dynamic`, following `TimeChoice(ComplexTimeSignature)`. The
collapse is what makes two alternatives safe rather than confusing: `ff` has exactly one
representation however it was built, so code reading a plain dynamic never has to look inside a
compound for it. A lone other-dynamics does not collapse -- it has no dedicated MusicXML element,
so it stays a compound of one, which keeps `Kind::dynamic` meaning exactly "a standard
abbreviation" and lets `dynamic()` return a bare `StandardDynamic`.
Before this change one `<dynamics>` element had three api shapes: `markType == ff`; or
`markType == otherDynamics` with the text in `MarkData::name` and the glyph in
`choice.otherMark().smufl`; or `markType == compoundDynamics` with both facts restated inside
`DynamicsComponent`. All three collapse to one component list. `DynamicsReader` loses its
single-child special case and `DynamicsWriter` loses its entire non-compound branch.
`MarkData` gains `MarkData(StandardDynamic)` and `MarkData(CompoundDynamicsData)`, which set
`markType`, `choice`, and `name` together. Dynamics is the one mark family where a payload
determines its mark type uniquely -- `Kind::tremolo` covers `tremoloStart`/`tremoloStop`,
`Kind::arpeggiate` covers three mark types -- so it is the one family where a constructor can set
both without guessing. Every dynamics mark in the tree, including the reader, now goes through
these constructors; there are no direct `choice = CompoundDynamicsData{...}` assignments left.
`MarkData::name` keeps working. It spells the whole mark -- "ff", "z", or "ffz" -- derived by one
rule with no special cases, the way articulations and fermatas already name themselves. The writer
ignores it for dynamics, so it cannot contradict the output. A single standard component yields
what the old reader yielded, and a lone other-dynamics yields its text, so compatibility falls out
of the general rule rather than being special-cased. `name` is documented for the first time,
including that a self-naming mark derives it at construction and that replacing `choice` afterwards
means re-deriving it.
Mapping tables went from two to one, and the survivor is public. `markDataDynamicsKindToString`
and `dynamicsKindToName` were near-identical private Kind-to-string switches differing only in
their default arm; both are deleted, along with `Converter::dynamicsMap` and both `convertDynamic`
overloads. `standardDynamicsMap` remains as the only dynamics map. In their place
`DynamicsData.h` exports `toString(StandardDynamic)`, `toString(const DynamicsComponent &)` and
`toString(const CompoundDynamicsData &)`, so clients can spell a mark they built rather than
reaching for `name`. The core cannot supply these strings: it hard-codes the element literals
inline in the generated parse and serialize routines with no accessor, and `mx::impl` never sees
the XML -- by the time `DynamicsReader` runs, `<ff/>` is already `DynamicsChoice::Kind::ff`.
`isMarkDynamic` is now a single comparison and no longer has its duplicated `MarkType::p` test.
Dispatch written as `if (isMarkDynamic(...))` keeps working verbatim, so notations and directions
are untouched, as are `NoteAttachmentData::marks`, `DirectionChoice`, and every other mark family.
MusicXML output is unchanged; this is an api-shape change only.
## Breaking changes
- The 29 dynamics values of `MarkType` are gone. Use `MarkType::dynamics` with
`MarkData(StandardDynamic)` or `MarkData(CompoundDynamicsData)`. Every site is a compile error.
- `MarkData::name` is no longer the other-dynamics text on the write path; that text is
`OtherDynamicsData::text`. The reader still fills `name` in, so read-only clients are unaffected.
- Building a `CompoundDynamicsData` with a single standard component yields `Kind::dynamic`, not
`Kind::compoundDynamics`.
## Testing
- make test-all MX_RUNNING_IN_DOCKER=1 (837 + 41 + 502 test cases, 5541 assertions; 305 pinned
round-trip files, 0 failed)
- strict unity build of target mx (CMAKE_UNITY_BUILD_BATCH_SIZE=0)
- git diff --check
- make fmt MX_RUNNING_IN_DOCKER=1. Formatting was verified against the host clang-format only;
Docker was unavailable, so CI's clang-format has not seen these files. ApiEquality.h is
deliberately left as-is (host/Docker version drift, see 3a99c92).
9 tasks
webern
pushed a commit
that referenced
this pull request
Aug 7, 2026
## Human Summary This is a omnibus PR that cleans up some minor issues. - clean up stale comments - add enclosure to `PageTextData` - correctly read/write lyricist, arranger, and publisher credits ## Summary Three small gaps in `mx::api`, collected into one fix-up PR. Each is confined to `mx::impl` plus a single new field; none needs a new API model. **Arranger and publisher were accepted but never written.** `ScoreData::arranger` and `ScoreData::publisher` have existed, with equality members, but nothing in `mx::impl` ever touched them -- a scan of every `MXAPI_EQUALS_MEMBER` name against `src/private/mx/impl/` showed these two as the only dead public fields. Worse than dead: `ScoreReader` read an incoming `<creator type="arranger">` into `ScoreData::lyricist`, so a file carrying both an arranger and a lyricist lost the lyricist, and `ScoreWriter` then re-emitted the arranger's text mislabelled as `type="lyricist"`. Both now read into their own field and are written beside composer and lyricist. Where a file has several creators of one type (`recsuite/Echigo_Jishi.xml` has two arrangers) the first wins, since the api has one slot. Before, on `musuite/testMetaData.xml`: `lyricist == "MetaArranger"`, `arranger == ""`, and the round trip emitted `<creator type="lyricist">MetaArranger</creator>`. After: composer, lyricist and arranger each land in their own field and are rewritten with the right type. **Page text carried no enclosure.** `<credit-words>` is type `formatted-text-id`, the same core class as `<words>` and `<rehearsal>`, so it carries the whole `text-formatting` attribute group. `core::FormattedTextID` already exposes `enclosure()`/`setEnclosure()`, and `Converter` already has both directions from #378, so only the api field was missing. `PageTextData` gains `Enclosure enclosure`, converted in both directions in `PageTextFunctions.cpp` beside the existing `justify` handling -- the same shape #273 used for justify. **A stale comment about `<pedal>`.** `SpannerNumberResolver` asserted in two places that `<pedal>` has no `number` attribute. True through MusicXML 3.0; 3.1 added it and `core::Pedal` supports it. Pedals still stay out of number resolution, because `mx::api` does not model the attribute -- behaviour unchanged, only the stated reason corrected. ## Testing - [x] Two new `DocumentManager` tests: all four creator types survive a round trip independently and serialize with the right `type`, and a read of `musuite/testMetaData.xml` keeps composer, lyricist and arranger apart. The second fails before the fix (`lyricist` comes back as `MetaArranger`). - [x] Two new `ROUND_TRIP_TEST_SCALAR` entries for `arranger` and `publisher` - [x] Three new `creditRoundTrip` tests: `enclosure="rectangle"` survives, an explicit `none` survives, and an unset enclosure writes no attribute and reads back `unspecified` - [x] Full api suite passes (5922 assertions in 528 test cases) - [x] `make api-roundtrip` passes (363 of 363 pinned) - [x] `make api-roundtrip-discover` still shows 363 PASS -- no file changed status, so the baseline is untouched - [x] `make core-roundtrip-test` passes (839 test cases) - [x] `make core-unit` passes (212 assertions in 41 test cases) - [x] `make fmt-check` clean on touched files No corpus files added or removed, so no pinned-count bump and no audit regeneration. ## References - Related to #273 (credit-words `justify`) and #187 (credit gaps) -- same family, and the enclosure work follows the pattern both established - Builds on #378, which unified the `Enclosure` enum
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
2 participants
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
Extends direction text formatting with
justifyandsystemRelationadded where available in the musicxml spec. Also unified theEnclosureenum which resulted in a breaking change, but it seems like the best approach moving forward. (See below for more details.)Summary
Four MusicXML attributes that mx::api could not express are now modeled, and the two duplicate enclosure enums are merged into one.
justify on words, symbol and rehearsal. All three elements carry both justify and halign, which MusicXML treats as different things: halign says which edge of the text the position refers to, justify says how the lines sit relative to each other when the text runs to more than one line. halign was already reaching the api through positionData.horizontalAlignment; justify was dropped. PageTextData and TempoData already had a justify field, so the new ones follow that precedent. Elements carrying justify without halign (lyric, part-name, group-name) are deliberately left alone -- the spec says justify doubles as alignment there, which is how the lyric reader already treats it.
systemRelation on DirectionData, mapping the system attribute of both direction and harmony. The api::SystemRelation enum already existed for measure numbering. The direction vocabulary is narrower (only-top, also-top, none), so the api-to-core conversion returns an optional and writes no attribute for the two measure-numbering-only values instead of coercing them into something wrong.
The enclosure vocabulary is now complete. RehearsalEnclosure was missing invertedBracket and pentagon through decagon, while PercussionEnclosure already had all fifteen. Since both described the same MusicXML enclosure-shape type with identical values, they are replaced by a single Enclosure in ApiCommon.h. That also collapses three parallel conversion paths -- two hand-written switches in DirectionReader and DirectionWriter plus the Converter table -- into the Converter table alone, which is where the drift came from in the first place.
Breaking change:
RehearsalEnclosureandPercussionEnclosureare removed rather than aliased. The vocabularies are unchanged, so the downstream fix is a mechanical rename toEnclosure. Otherwise there is no structural API change; the additions are new fields on existing types with defaults that preserve current behavior.Testing
No corpus fixture was pinned to roundtrip-baseline.txt: the files exercising these attributes (synthetic/direction.4.0.xml, synthetic/words.3.x.xml) still fail the api round trip on unrelated gaps, directive on direction and dir on words.