feat: add notehead filled and smufl to mx::api - #377
Merged
Conversation
Expose the <notehead> element's filled and smufl attributes, which mx::core already modeled but mx::api dropped in both directions. NoteWriter now emits <notehead> when either is set, even for a normal notehead value. Also adds the missing MXAPI_EQUALS_MEMBER(notehead), which had never been in the equality block. Six tests in NoteDataTest; no roundtrip-baseline.txt additions.
The test bound a reference to firstNoteheadNote(mxtest::fromXml(xml)). Lifetime extension does not apply when a reference binds to one returned by a function, so the temporary ScoreData was destroyed at the end of the statement and outNote dangled. Hold the score in a local first, as the neighbouring notehead tests already do. MSVC's debug CRT caught this by filling the freed block with 0xDD, which read back as -572662307; libc++ happened not to reuse the block, so the Linux and macOS jobs passed.
webern
approved these changes
Jul 30, 2026
webern
left a comment
webern
left a comment
Owner
There was a problem hiding this comment.
Nice, thank you for all these contributions.
You can merge them in the order you prefer. Only one of them is not approved yet, the one that I mentioned seems to not have the desired diff at the moment.
Contributor
Author
|
Just a reminder: I don't have merge privilege (and am not asking for it). |
Contributor
Author
|
These should be mergeable in any order. (That was the intent.) But the safest order would be from least-to-most recent. |
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
The title says it all. Add notehead filled and smufl to mx::api. Also cleanup as identified by Claude.
Summary
The
<notehead>element'sfilledandsmuflattributes were modeled inmx::corebut dropped bymx::apiin both directions.NoteDatanow carries them alongsidenotehead:noteheadFilledis anapi::Booldefaulting to unspecified, so the attribute is only written when set and MusicXML's duration-based default (hollow for a half note and longer) otherwise applies.noteheadSmuflis an optional glyph name, following the precedent inPercussionDataandOtherDirectionData. An empty string is treated as absent rather than lettingcore::SmuflGlyphNamerepair it into the placeholder "X".NoteWriter::setNoteheadpreviously skipped the element entirely whenever the notehead value wasnormal. It now emits when either attribute is set, so<notehead filled="no">normal</notehead>survives a round trip.
NoteReader::setNoteheadValuebecomessetNoteheadItemsand reads allthree off
core::Notehead;NoteFunctions::parseNotecopies them onto the api struct.Also adds the missing
MXAPI_EQUALS_MEMBER(notehead). That field had never been inNoteData'sequality block, so two notes with different noteheads compared equal and the field was invisible
to round-trip checks.
No
roundtrip-baseline.txtadditions.lysuite/ly22a_Noteheads.xmlgets past its noteheadattribute mismatch but still fails further along on three explicit
<notehead>normal</notehead>elements the writer drops, which needs a separate fidelity knob. The synthetic notehead files now
fail on
parentheses, font and color, which mx::api does not model.Testing
Notehead::other, both together oncluster, absence by default, and a read ofdata/synthetic/notehead.3.1.xmlthat pins the core to api pathnotehead*: 38 assertions in 9 test cases)*_NoteData: 285 assertions in 37 test cases)make api-roundtrippasses (295 of 295 pinned)