feat: round-trip the midi-device port and id attributes#348
Merged
Conversation
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36648 |
| Functions | 74.2% | 6352 / 8556 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 84.4% | 7242 / 8577 |
| Functions | 74.9% | 2512 / 3353 |
| Branches | 52.4% | 6320 / 12072 |
Core HTML report | API HTML report
Commit 7c27b1634eabc62ef7f8035335203433841c3884.
gen-quality
|
webern
force-pushed
the
claude/grace-chord-boundary
branch
from
2989c24 to
de0d806
Compare
webern
force-pushed
the
claude/midi-device-attributes
branch
from
43ba4e4 to
037a1e6
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36648 |
| Functions | 74.2% | 6352 / 8556 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 84.6% | 7256 / 8579 |
| Functions | 74.9% | 2512 / 3353 |
| Branches | 52.5% | 6334 / 12076 |
Core HTML report | API HTML report
Commit 24d4f74ba1ed108b8c786defb96e04d1baa4e3d7.
gen-quality
|
webern
force-pushed
the
claude/grace-chord-boundary
branch
from
de0d806 to
541a835
Compare
webern
force-pushed
the
claude/midi-device-attributes
branch
from
037a1e6 to
b37d969
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36648 |
| Functions | 74.2% | 6352 / 8556 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 84.6% | 7256 / 8579 |
| Functions | 74.9% | 2512 / 3353 |
| Branches | 52.5% | 6334 / 12076 |
Core HTML report | API HTML report
Commit 2ccc0e33209fb693384d41b567d21a30ae225158.
gen-quality
|
webern
commented
Jul 12, 2026
Comment on lines
+33
to
+39
| // The midi-device port attribute: a number from 1 to 16 for the unofficial MIDI 1.0 | ||
| // port (cable) meta event. | ||
| std::optional<int> devicePort; | ||
|
|
||
| // The midi-device element's id attribute, which refers to the score-instrument the | ||
| // device assignment belongs to. | ||
| std::optional<std::string> deviceId; |
Owner
Author
There was a problem hiding this comment.
Hmm first of all there's something about saying "The midi-device attribute" and in the next one saying "The midi-device element's attribute" that throws me off. It makes me ask, wait, are these on the same element? And if so why are these worded differently.
Secondly, I don't like things that point somewhere else, like deviceId is doing here, when it could simply be a dangling pointer and forces bookkeeping on the client to make sure they mark the same deviceId here as they do somewhere else. Is there another way to model it?
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36648 |
| Functions | 74.2% | 6352 / 8556 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 84.6% | 7256 / 8579 |
| Functions | 74.9% | 2512 / 3353 |
| Branches | 52.5% | 6334 / 12076 |
Core HTML report | API HTML report
Commit 5a8230c80117e3f83c0c800d44ef9270a66653b7.
gen-quality
|
MidiData modeled only the midi-device element text; the port and id attributes were dropped, and the common attribute-only form <midi-device id="P1-I1" port="1"></midi-device> 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).
webern
force-pushed
the
claude/midi-device-attributes
branch
from
b23637b to
bff85c8
Compare
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36648 |
| Functions | 74.2% | 6352 / 8556 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 84.5% | 7237 / 8560 |
| Functions | 74.9% | 2511 / 3352 |
| Branches | 52.4% | 6325 / 12066 |
Core HTML report | API HTML report
Commit eaf101d39b6fdf221f52c83cbb816016e9d80752.
gen-quality
|
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.
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36648 |
| Functions | 74.2% | 6352 / 8556 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 84.5% | 7237 / 8560 |
| Functions | 74.9% | 2511 / 3352 |
| Branches | 52.4% | 6327 / 12068 |
Core HTML report | API HTML report
Commit d1573c2961e628396f8a8f5c4cabd1c8b7f8d1bc.
gen-quality
|
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
TODO: human writes here
Summary
The midi-device port and id attributes were dropped on read, and the common attribute-only form
(
<midi-device id="P1-I1" port="1"></midi-device>, emitted by MuseScore and Finale) vanished onwrite because the emit condition required non-empty device text. This exposes the port, keeps the
id round-tripping, and emits the element whenever device text or a port is present.
Issue #347 suggested storing the id as a
deviceIdfield. Instead the id is modeled bycontainment. The midi-device id 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— so a stored idwould just duplicate the instrument's
uniqueIdand force callers to keep the two in sync.uniqueId, the same id thesibling midi-instrument already writes. There is no separate id for a caller to manage.
MidiData::writeDeviceId(unspecified/yes/no, default unspecified) records only whether thesource spelled the link out, so an id-less midi-device stays id-less and an id-bearing one is
preserved. Same fidelity-knob convention as
ClefData::writeStaffNumber; authors can ignore it.MidiData::devicePortcarries the 1–16 port.While here,
InstrumentDatais documented to explain a real limitation: mx::api models oneinstrument per part and does not support instrument changes mid-piece. MusicXML allows several
instruments per part (notably drum kits, where each sound is its own instrument); only the first is
brought in. That gap is left for a future change, since there is no clean way yet to expose it
without complicating the common single-instrument case.
Testing
midiDeviceRoundTrip.portSurvivesWriteAndReadandwriteDeviceIdSurvivesWriteAndReadmidi-device in ksuite/k008a_Beaming.xml continues to pass
References