From bff85c85c116248bc64b66d2c97d0a581bc038f2 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 12 Jul 2026 16:33:27 +0000 Subject: [PATCH 1/2] feat: round-trip the midi-device port and id attributes MidiData modeled only the midi-device element text; the port and id attributes were dropped, and the common attribute-only form vanished entirely because the writer required non-empty text. Adds MidiData::devicePort and MidiData::deviceId and wires them through PartReader/PartWriter. Pins the 3 corpus files this flips to PASS (227 total). --- src/include/mx/api/PartData.h | 17 ++++++++++++++--- src/private/mx/impl/PartReader.cpp | 10 ++++++++++ src/private/mx/impl/PartWriter.cpp | 16 ++++++++++++++-- .../mxtest/api/MidiNameRoundTripTest.cpp | 17 +++++++++++++++++ src/private/mxtest/api/roundtrip-baseline.txt | 7 +++++++ 5 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/include/mx/api/PartData.h b/src/include/mx/api/PartData.h index e0d69975f..343348eed 100644 --- a/src/include/mx/api/PartData.h +++ b/src/include/mx/api/PartData.h @@ -29,6 +29,15 @@ struct MidiData std::string virtualLibrary; std::string virtualName; std::string device; + + // The midi-device port attribute: a number from 1 to 16 for the unofficial MIDI 1.0 + // port (cable) meta event. + std::optional devicePort; + + // The midi-device element's id attribute, which refers to the score-instrument the + // device assignment belongs to. + std::optional deviceId; + std::string name; // VALUE_UNSPECIFIED indicates absence of value @@ -56,9 +65,9 @@ struct MidiData bool isElevationSpecified; MidiData() - : virtualLibrary{}, virtualName{}, device{}, name{}, bank{VALUE_UNSPECIFIED}, channel{VALUE_UNSPECIFIED}, - program{VALUE_UNSPECIFIED}, unpitched{VALUE_UNSPECIFIED}, volume{0.0}, isVolumeSpecified{false}, pan{0.0}, - isPanSpecified{false}, elevation{0.0}, isElevationSpecified{false} + : virtualLibrary{}, virtualName{}, device{}, devicePort{}, deviceId{}, name{}, bank{VALUE_UNSPECIFIED}, + channel{VALUE_UNSPECIFIED}, program{VALUE_UNSPECIFIED}, unpitched{VALUE_UNSPECIFIED}, volume{0.0}, + isVolumeSpecified{false}, pan{0.0}, isPanSpecified{false}, elevation{0.0}, isElevationSpecified{false} { } }; @@ -230,6 +239,8 @@ MXAPI_EQUALS_BEGIN(MidiData) MXAPI_EQUALS_MEMBER(virtualLibrary) MXAPI_EQUALS_MEMBER(virtualName) MXAPI_EQUALS_MEMBER(device) +MXAPI_EQUALS_MEMBER(devicePort) +MXAPI_EQUALS_MEMBER(deviceId) MXAPI_EQUALS_MEMBER(name) MXAPI_EQUALS_MEMBER(bank) MXAPI_EQUALS_MEMBER(channel) diff --git a/src/private/mx/impl/PartReader.cpp b/src/private/mx/impl/PartReader.cpp index 27cc090e4..efbf7c9f1 100644 --- a/src/private/mx/impl/PartReader.cpp +++ b/src/private/mx/impl/PartReader.cpp @@ -299,6 +299,16 @@ void PartReader::parseMidiDeviceInstrumentGroup(const core::ScorePartMIDIGroup & if (grp.midiDevice().has_value()) { myOutPartData.instrumentData.midiData.device = grp.midiDevice()->value(); + + if (grp.midiDevice()->port().has_value()) + { + myOutPartData.instrumentData.midiData.devicePort = grp.midiDevice()->port()->value(); + } + + if (grp.midiDevice()->id().has_value()) + { + myOutPartData.instrumentData.midiData.deviceId = grp.midiDevice()->id()->value(); + } } if (grp.midiInstrument().has_value()) diff --git a/src/private/mx/impl/PartWriter.cpp b/src/private/mx/impl/PartWriter.cpp index 502918e9c..27a8d3e75 100644 --- a/src/private/mx/impl/PartWriter.cpp +++ b/src/private/mx/impl/PartWriter.cpp @@ -177,10 +177,22 @@ core::ScorePart PartWriter::getScorePart() const core::MIDIInstrument midiInstrument{}; midiInstrument.setID(core::Token{myPartData.instrumentData.uniqueId}); - if (myPartData.instrumentData.midiData.device.size() > 0) + const auto &apiMidiData = myPartData.instrumentData.midiData; + if (apiMidiData.device.size() > 0 || apiMidiData.devicePort.has_value() || apiMidiData.deviceId.has_value()) { addMidiElement = true; - midiDevice.setValue(myPartData.instrumentData.midiData.device); + midiDevice.setValue(apiMidiData.device); + + if (apiMidiData.devicePort.has_value()) + { + midiDevice.setPort(core::MIDI16{*apiMidiData.devicePort}); + } + + if (apiMidiData.deviceId.has_value()) + { + midiDevice.setID(core::Token{*apiMidiData.deviceId}); + } + midiGroup.setMIDIDevice(midiDevice); } diff --git a/src/private/mxtest/api/MidiNameRoundTripTest.cpp b/src/private/mxtest/api/MidiNameRoundTripTest.cpp index d247bbccc..44931228b 100644 --- a/src/private/mxtest/api/MidiNameRoundTripTest.cpp +++ b/src/private/mxtest/api/MidiNameRoundTripTest.cpp @@ -53,4 +53,21 @@ TEST(midiNameRoundTrip, survivesWriteAndRead) CHECK_EQUAL(expected, out.parts.at(0).instrumentData.midiData.name); } +TEST(midiDeviceRoundTrip, portAndIdSurviveWriteAndRead) +{ + // A midi-device carrying only attributes (empty text) must still round-trip, e.g. + // . + auto in = makeScoreWithMidiName("Flute Player One"); + in.parts.at(0).instrumentData.midiData.devicePort = 1; + in.parts.at(0).instrumentData.midiData.deviceId = "P1-I1"; + + const auto out = mxtest::roundTrip(in); + REQUIRE(out.parts.size() == 1); + const auto &midiData = out.parts.at(0).instrumentData.midiData; + REQUIRE(midiData.devicePort.has_value()); + CHECK_EQUAL(1, *midiData.devicePort); + REQUIRE(midiData.deviceId.has_value()); + CHECK_EQUAL(std::string{"P1-I1"}, *midiData.deviceId); +} + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index f0c4d98c4..ad7bfe8d3 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -425,3 +425,10 @@ musuite/testTempo1.xml # the chord instead of absorbing the main chord's first note. lysuite/ly24b_ChordAsGraceNote.xml musuite/testGrace2.xml + +# Unblocked by MidiData::devicePort/deviceId: the midi-device port and id +# attributes now round-trip, and an attribute-only (empty-text) midi-device +# is no longer dropped. +custom/musescore-slur-start-stop.musicxml +musuite/testChord.xml +musuite/testPiano.xml From 06a1b57ad0bf319517a8d235b141f2bb22d6305b Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Mon, 13 Jul 2026 09:55:48 +0000 Subject: [PATCH 2/2] feat: round-trip the midi-device port and id attributes Model the midi-device id by containment instead of a stored reference. The midi-device id attribute is a back-reference to the score-instrument the device belongs to, and mx::api already models that instrument as the part's single InstrumentData. Storing it as MidiData::deviceId duplicated the instrument's uniqueId and forced callers to keep the two in sync. - Remove MidiData::deviceId. The writer derives the midi-device id from the part instrument's uniqueId, the same id the sibling midi-instrument already uses. - Add MidiData::writeDeviceId, a fidelity flag (unspecified/yes/no, default unspecified) recording whether the source spelled out the link, so an id-less midi-device stays id-less on write and an id-bearing one is preserved. Same convention as ClefData::writeStaffNumber. - Keep MidiData::devicePort; emit the midi-device when device text or port is present, so an attribute-only (empty-text) midi-device is no longer dropped. - Document on InstrumentData that mx::api models one instrument per part and does not support instrument changes mid-piece. Round-trip baseline 224 -> 227 (musescore-slur-start-stop, testChord, testPiano); the id-less midi-device in ksuite/k008a_Beaming.xml continues to pass. --- src/include/mx/api/PartData.h | 50 +++++++++++++++---- src/private/mx/impl/PartReader.cpp | 2 +- src/private/mx/impl/PartWriter.cpp | 9 ++-- .../mxtest/api/MidiNameRoundTripTest.cpp | 22 ++++++-- src/private/mxtest/api/roundtrip-baseline.txt | 6 +-- 5 files changed, 68 insertions(+), 21 deletions(-) diff --git a/src/include/mx/api/PartData.h b/src/include/mx/api/PartData.h index 343348eed..4a1e66242 100644 --- a/src/include/mx/api/PartData.h +++ b/src/include/mx/api/PartData.h @@ -28,15 +28,26 @@ struct MidiData { std::string virtualLibrary; std::string virtualName; + + // The name of the playback device or sound bank for this instrument — for example the name of a + // MIDI output device, or a sound or bank label. This is often left empty. std::string device; - // The midi-device port attribute: a number from 1 to 16 for the unofficial MIDI 1.0 - // port (cable) meta event. + // An optional port (or cable) number, from 1 to 16, that says which output the device uses. It + // has no value when the file doesn't specify one, and most files leave it unset — you only need + // it when you are aiming at a particular MIDI port. You don't have to attach the device to this + // part's instrument yourself; the library keeps them together automatically when the music is + // written out. std::optional devicePort; - // The midi-device element's id attribute, which refers to the score-instrument the - // device assignment belongs to. - std::optional deviceId; + // Most users can ignore this; leave it unspecified. It only controls whether the saved file + // spells out the little link that ties this part's playback device to its instrument. unspecified + // (the default) does the sensible thing on its own: it leaves that link out, because each part + // here has a single instrument and so the link isn't needed. yes/no force it to be written or left + // out. You never have to set this when you are creating music — when the library reads a file that + // did spell the link out, it sets this for you, so that saving the file brings it back the same way + // it came in. + Bool writeDeviceId; std::string name; @@ -65,13 +76,34 @@ struct MidiData bool isElevationSpecified; MidiData() - : virtualLibrary{}, virtualName{}, device{}, devicePort{}, deviceId{}, name{}, bank{VALUE_UNSPECIFIED}, - channel{VALUE_UNSPECIFIED}, program{VALUE_UNSPECIFIED}, unpitched{VALUE_UNSPECIFIED}, volume{0.0}, - isVolumeSpecified{false}, pan{0.0}, isPanSpecified{false}, elevation{0.0}, isElevationSpecified{false} + : virtualLibrary{}, virtualName{}, device{}, devicePort{}, writeDeviceId{Bool::unspecified}, name{}, + bank{VALUE_UNSPECIFIED}, channel{VALUE_UNSPECIFIED}, program{VALUE_UNSPECIFIED}, unpitched{VALUE_UNSPECIFIED}, + volume{0.0}, isVolumeSpecified{false}, pan{0.0}, isPanSpecified{false}, elevation{0.0}, + isElevationSpecified{false} { } }; +// Describes the instrument that a part is played on. In this library every part has exactly one +// instrument, and the name and playback settings you set here (the playback settings live in +// midiData) apply to the whole part, from the first measure to the last. +// +// MusicXML itself allows a single part to carry more than one instrument. The most common case is +// a drum kit or percussion part, where the bass drum, snare, ride cymbal, and so on are each +// treated as a separate instrument. You also see it when a part layers or doubles two sounds that +// play together — two sampled instruments sounding at once, or a single "Clarinet 1 & 2" line that +// really holds two separate instruments. +// +// For now this library brings in only the first instrument of such a part. Any additional +// instruments in the file are not available here and are not written back out. We're sorry for the +// limitation: we looked for a way to offer several instruments per part without making the ordinary +// single-instrument case — which covers the vast majority of music — harder to understand and use, +// and we didn't find one we were happy with, so it stays one instrument per part for the time +// being. +// +// One more thing worth knowing: MusicXML also lets a piece switch a part's instrument, or change +// its playback settings, partway through the music. This library does not offer those mid-piece +// changes. Whatever you set here is in effect for the entire part, start to finish. struct InstrumentData { std::string uniqueId; @@ -240,7 +272,7 @@ MXAPI_EQUALS_MEMBER(virtualLibrary) MXAPI_EQUALS_MEMBER(virtualName) MXAPI_EQUALS_MEMBER(device) MXAPI_EQUALS_MEMBER(devicePort) -MXAPI_EQUALS_MEMBER(deviceId) +MXAPI_EQUALS_MEMBER(writeDeviceId) MXAPI_EQUALS_MEMBER(name) MXAPI_EQUALS_MEMBER(bank) MXAPI_EQUALS_MEMBER(channel) diff --git a/src/private/mx/impl/PartReader.cpp b/src/private/mx/impl/PartReader.cpp index efbf7c9f1..748dc74b0 100644 --- a/src/private/mx/impl/PartReader.cpp +++ b/src/private/mx/impl/PartReader.cpp @@ -307,7 +307,7 @@ void PartReader::parseMidiDeviceInstrumentGroup(const core::ScorePartMIDIGroup & if (grp.midiDevice()->id().has_value()) { - myOutPartData.instrumentData.midiData.deviceId = grp.midiDevice()->id()->value(); + myOutPartData.instrumentData.midiData.writeDeviceId = api::Bool::yes; } } diff --git a/src/private/mx/impl/PartWriter.cpp b/src/private/mx/impl/PartWriter.cpp index 27a8d3e75..cec80b887 100644 --- a/src/private/mx/impl/PartWriter.cpp +++ b/src/private/mx/impl/PartWriter.cpp @@ -178,7 +178,7 @@ core::ScorePart PartWriter::getScorePart() const midiInstrument.setID(core::Token{myPartData.instrumentData.uniqueId}); const auto &apiMidiData = myPartData.instrumentData.midiData; - if (apiMidiData.device.size() > 0 || apiMidiData.devicePort.has_value() || apiMidiData.deviceId.has_value()) + if (apiMidiData.device.size() > 0 || apiMidiData.devicePort.has_value()) { addMidiElement = true; midiDevice.setValue(apiMidiData.device); @@ -188,9 +188,12 @@ core::ScorePart PartWriter::getScorePart() const midiDevice.setPort(core::MIDI16{*apiMidiData.devicePort}); } - if (apiMidiData.deviceId.has_value()) + // The midi-device attaches to this part's instrument, the same instrument the + // midi-instrument below is written for. Emit that link only when the source stated it; + // for a single-instrument part it is otherwise implied. + if (apiMidiData.writeDeviceId == api::Bool::yes && myPartData.instrumentData.uniqueId.size() > 0) { - midiDevice.setID(core::Token{*apiMidiData.deviceId}); + midiDevice.setID(core::Token{myPartData.instrumentData.uniqueId}); } midiGroup.setMIDIDevice(midiDevice); diff --git a/src/private/mxtest/api/MidiNameRoundTripTest.cpp b/src/private/mxtest/api/MidiNameRoundTripTest.cpp index 44931228b..a33c0ca25 100644 --- a/src/private/mxtest/api/MidiNameRoundTripTest.cpp +++ b/src/private/mxtest/api/MidiNameRoundTripTest.cpp @@ -53,21 +53,33 @@ TEST(midiNameRoundTrip, survivesWriteAndRead) CHECK_EQUAL(expected, out.parts.at(0).instrumentData.midiData.name); } -TEST(midiDeviceRoundTrip, portAndIdSurviveWriteAndRead) +TEST(midiDeviceRoundTrip, portSurvivesWriteAndRead) { // A midi-device carrying only attributes (empty text) must still round-trip, e.g. - // . + // . writeDeviceId defaults to unspecified, so no id is + // emitted here; only the port is exercised. auto in = makeScoreWithMidiName("Flute Player One"); in.parts.at(0).instrumentData.midiData.devicePort = 1; - in.parts.at(0).instrumentData.midiData.deviceId = "P1-I1"; const auto out = mxtest::roundTrip(in); REQUIRE(out.parts.size() == 1); const auto &midiData = out.parts.at(0).instrumentData.midiData; REQUIRE(midiData.devicePort.has_value()); CHECK_EQUAL(1, *midiData.devicePort); - REQUIRE(midiData.deviceId.has_value()); - CHECK_EQUAL(std::string{"P1-I1"}, *midiData.deviceId); +} + +TEST(midiDeviceRoundTrip, writeDeviceIdSurvivesWriteAndRead) +{ + // When a source spells out the device-to-instrument link, mx re-emits it as + // , taking the id from the part's instrument (uniqueId "P1-I1" + // here), so the flag survives the round-trip. + auto in = makeScoreWithMidiName("Flute Player One"); + in.parts.at(0).instrumentData.midiData.devicePort = 1; + in.parts.at(0).instrumentData.midiData.writeDeviceId = Bool::yes; + + const auto out = mxtest::roundTrip(in); + REQUIRE(out.parts.size() == 1); + CHECK(out.parts.at(0).instrumentData.midiData.writeDeviceId == Bool::yes); } #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index ad7bfe8d3..662abba89 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -426,9 +426,9 @@ musuite/testTempo1.xml lysuite/ly24b_ChordAsGraceNote.xml musuite/testGrace2.xml -# Unblocked by MidiData::devicePort/deviceId: the midi-device port and id -# attributes now round-trip, and an attribute-only (empty-text) midi-device -# is no longer dropped. +# Unblocked by MidiData::devicePort: the midi-device port attribute now round-trips +# (its id is written automatically from the part's instrument), and an attribute-only +# (empty-text) midi-device is no longer dropped. custom/musescore-slur-start-stop.musicxml musuite/testChord.xml musuite/testPiano.xml