Skip to content

fix: write back tuplet position data, midi-unpitched and accidental marks - #462

Merged
webern merged 1 commit into
mainfrom
m/mxdev-writedrop
Sep 19, 2026
Merged

webern merged 1 commit into
mainfrom
m/mxdev-writedrop

Conversation

@webern

@webern webern commented Sep 19, 2026

Copy link
Copy Markdown
Owner

Human Summary

Fixes a few random writer bugs. Found and fixed by AI.

Summary

Three values that mx::api reads were never written back. Each was a silent drop: the value came in
on a read and nothing went out on a write.

TupletStart::positionData and TupletStop::positionData are read by the tuplet reader, but neither
writer path set a position attribute on the emitted <tuplet>, so a tuplet's placement
(default-x, default-y, relative-x, relative-y, placement) was lost. Both paths now call
setAttributesFromPositionData, the way the mark emitters in the same file already do.

MidiData::unpitched is read from <midi-unpitched>, and PartWriter never wrote it. It is now
written when the value is present; VALUE_UNSPECIFIED still means absent, so an ordinary pitched
instrument writes no element. As with the sibling midi fields, a value the core type clamps is
reported through the diagnostics.

Accidental marks were read from two places into the same mark values, <notations> and, inside an
ornament, <ornaments>, while the mark loop had no branch for them, so they vanished.
isMarkAccidentalMark now covers the accidental values and the writer emits them.

This is worth being plain about, because it is where the deferred half of #443 begins: the api does
not record which element a mark came from, so the writer cannot put it back where it came from. The
mark is written at the notations level. That is not a preference but a schema constraint. MusicXML's
ornaments group is a sequence whose first member is a required ornament choice, followed by optional
accidental-marks, and both the schema and mx::core enforce it: an <ornaments> group must carry
an ornament, and the parser rejects a group without one. Writing an accidental mark into
<ornaments> would therefore mean inventing an ornament to hang it on.
<notations><accidental-mark> needs no such invention, is legal in every case, and the reader
already reads it, so a mark survives a write and a read either way.

This is non-breaking: no api type, field or vocabulary changes. isMarkAccidentalMark is added
beside the other mark predicates.

What is deferred

The mark-placement half of #443 stays open, so this does not close it. A mark read from <notations>
can only be written back to <notations>, and one read from <ornaments> back to <ornaments>,
once the api can tell the two apart, which today it cannot: both become the same MarkType in the
same flat list, with no field recording the source. That needs an api-visible distinction, likely a
MarkDataChoice alternative per the doctrine, plus a split in the reader and its own tests. It is a
design pass over the public model rather than a writer branch, so it is deliberately not attempted
here.

Testing

  • New tests fail before the change, pass after: with the five implementation files reverted, 4 of
    the 5 new cases fail, 25 of 33 assertions. midi-unpitched is absent from the written XML and
    reads back as -1, the written tuplet carries no position attribute, the accidental mark is
    absent and reads back as 0 marks, and the mark attached to an ornament reads back as 1 mark
    instead of 2
  • make fmt and make fmt-check pass
  • make api-test: all tests pass (5949 assertions in 670 test cases)
  • make api-roundtrip: 415 passed, 0 failed (of 415 pinned)
  • make api-roundtrip-discover: 415 PASS, 425 FAIL; no file unlocked by this change, so nothing
    was added to the baseline
  • make test-all: core round trip passes (841 test cases), core unit passes (603 assertions in
    69 test cases), plus the api suites above

The tests added are: a tuplet carried through a write and a read with all four position attributes
and a placement; a midi-unpitched that survives a write and a read, plus a guard that an ordinary
pitched part writes no <midi-unpitched>; an authored accidental mark that survives a write and a
read; and a source fixture whose accidental mark is attached to a trill-mark, checked to keep both
marks once the ornament attachment is normalized away.

References

@webern webern added bug software defect non-breaking fixes or implementation that do not require breaking changes impl Affects the mx::impl layer ai Issues opened by, or through, a coding agent. labels Sep 19, 2026
@webern
webern merged commit 19df0ab into main Sep 19, 2026
8 checks passed
@webern
webern deleted the m/mxdev-writedrop branch September 19, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai Issues opened by, or through, a coding agent. bug software defect impl Affects the mx::impl layer non-breaking fixes or implementation that do not require breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant