From 39fb35bbd8243b56dd2e45da0cb798f14b06370a Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 19 Jun 2026 22:14:17 +0000 Subject: [PATCH 1/3] impl: add rehearsal writer in DirectionWriter, fix reader font/color MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #205 and closes #134. `DirectionWriter` was missing a write path for `RehearsalData`, causing rehearsal marks to be silently dropped on output. Add a loop over `directionData.rehearsals` that emits `` elements, mirroring the segno/coda pattern from PR #203. While wiring up the round-trip test, also fix two gaps in `DirectionReader::parseRehearsal`: it was not reading `fontData` (calling `getFontData`) and was not setting `isColorSpecified` before copying color attributes, so those fields were always lost on the read path. Tests added: - `rehearsalRoundTrip` in `DirectionWriterTest.cpp`: api → core → api round-trip at the impl layer, covering text, position, font, color, and enclosure. - `RehearsalSyntheticFileRead` in `DirectionDataTest.cpp`: loads `data/synthetic/rehearsal.3.1.xml` and asserts the read path surfaces text and enclosure. - `RehearsalRoundTripXml` in `DirectionDataTest.cpp`: full MusicXML serialization/deserialization round-trip via `mxtest::roundTrip` covering text, enclosure, and font weight. --- src/private/mx/impl/DirectionReader.cpp | 7 +- src/private/mx/impl/DirectionWriter.cpp | 43 ++++++++++ src/private/mxtest/api/DirectionDataTest.cpp | 79 +++++++++++++++++++ .../mxtest/impl/DirectionWriterTest.cpp | 50 ++++++++++++ 4 files changed, 178 insertions(+), 1 deletion(-) diff --git a/src/private/mx/impl/DirectionReader.cpp b/src/private/mx/impl/DirectionReader.cpp index 7f4a41cd..4241b4ef 100644 --- a/src/private/mx/impl/DirectionReader.cpp +++ b/src/private/mx/impl/DirectionReader.cpp @@ -314,7 +314,12 @@ void DirectionReader::parseRehearsal(const core::DirectionType &directionType) api::RehearsalData outRehearsal; outRehearsal.text = rehearsal.value(); outRehearsal.positionData = getPositionData(rehearsal); - outRehearsal.colorData = getColor(rehearsal); + outRehearsal.fontData = getFontData(rehearsal); + outRehearsal.isColorSpecified = rehearsal.color().has_value(); + if (outRehearsal.isColorSpecified) + { + outRehearsal.colorData = getColor(rehearsal); + } if (rehearsal.enclosure().has_value()) { switch (rehearsal.enclosure()->tag()) diff --git a/src/private/mx/impl/DirectionWriter.cpp b/src/private/mx/impl/DirectionWriter.cpp index 12124bf9..689d97d3 100644 --- a/src/private/mx/impl/DirectionWriter.cpp +++ b/src/private/mx/impl/DirectionWriter.cpp @@ -449,6 +449,49 @@ std::vector DirectionWriter::getDirectionLikeThings() addDirectionType(std::move(dt), direction); } + for (const auto &item : myDirectionData.rehearsals) + { + core::FormattedTextID rehearsal{}; + rehearsal.setValue(item.text); + setAttributesFromPositionData(item.positionData, rehearsal); + setAttributesFromFontData(item.fontData, rehearsal); + if (item.isColorSpecified) + { + setAttributesFromColorData(item.colorData, rehearsal); + } + switch (item.enclosure) + { + case api::RehearsalEnclosure::rectangle: + rehearsal.setEnclosure(core::EnclosureShape::rectangle()); + break; + case api::RehearsalEnclosure::square: + rehearsal.setEnclosure(core::EnclosureShape::square()); + break; + case api::RehearsalEnclosure::oval: + rehearsal.setEnclosure(core::EnclosureShape::oval()); + break; + case api::RehearsalEnclosure::circle: + rehearsal.setEnclosure(core::EnclosureShape::circle()); + break; + case api::RehearsalEnclosure::bracket: + rehearsal.setEnclosure(core::EnclosureShape::bracket()); + break; + case api::RehearsalEnclosure::triangle: + rehearsal.setEnclosure(core::EnclosureShape::triangle()); + break; + case api::RehearsalEnclosure::diamond: + rehearsal.setEnclosure(core::EnclosureShape::diamond()); + break; + case api::RehearsalEnclosure::none: + rehearsal.setEnclosure(core::EnclosureShape::none()); + break; + } + core::DirectionType dt{}; + dt.setChoice( + core::DirectionTypeChoice::rehearsal(core::OneOrMore{std::move(rehearsal)})); + addDirectionType(std::move(dt), direction); + } + if (myIsFirstDirectionTypeAdded) { // The direction has other content; attach the as a child of the . diff --git a/src/private/mxtest/api/DirectionDataTest.cpp b/src/private/mxtest/api/DirectionDataTest.cpp index 404049c6..a6fab816 100644 --- a/src/private/mxtest/api/DirectionDataTest.cpp +++ b/src/private/mxtest/api/DirectionDataTest.cpp @@ -7,9 +7,11 @@ #include "cpul/cpulTestHarness.h" #include "mx/api/DocumentManager.h" +#include "mx/api/RehearsalData.h" #include "mx/api/ScoreData.h" #include "mxtest/api/RoundTrip.h" #include "mxtest/api/TestHelpers.h" +#include "mxtest/file/Path.h" using namespace std; using namespace mx::api; @@ -194,4 +196,81 @@ TEST(OutOfOrderTorture, DirectionData) T_END; +// Parse the synthetic rehearsal file and confirm that mx::api reads the text and enclosure. +// This pins the core -> api read path and would fail if parseRehearsal regresses. +TEST(RehearsalSyntheticFileRead, DirectionData) +{ + const std::string path = mxtest::getResourcesDirectoryPath() + "synthetic/rehearsal.3.1.xml"; + auto &docMgr = DocumentManager::getInstance(); + const auto docIdResult = docMgr.createFromFile(path); + REQUIRE(docIdResult.ok()); + const int docId = docIdResult.value(); + const auto scoreResult = docMgr.getData(docId); + docMgr.destroyDocument(docId); + REQUIRE(scoreResult.ok()); + const auto &score = scoreResult.value(); + REQUIRE(score.parts.size() == 1); + REQUIRE(score.parts.front().measures.size() == 1); + REQUIRE(score.parts.front().measures.front().staves.size() == 1); + const auto &directions = score.parts.front().measures.front().staves.front().directions; + REQUIRE(directions.size() == 1); + REQUIRE(directions.front().rehearsals.size() == 1); + const auto &rehearsal = directions.front().rehearsals.front(); + CHECK_EQUAL("x", rehearsal.text); + CHECK(RehearsalEnclosure::rectangle == rehearsal.enclosure); +} + +T_END; + +// Verify that rehearsal marks survive a full MusicXML serialization/deserialization round trip. +// This catches the bug where DirectionWriter had no rehearsal write path and rehearsals were +// silently dropped on output. +TEST(RehearsalRoundTripXml, DirectionData) +{ + ScoreData oscore; + oscore.ticksPerQuarter = 10; + oscore.parts.emplace_back(); + auto &opart = oscore.parts.back(); + opart.measures.emplace_back(); + auto &omeasure = opart.measures.back(); + omeasure.staves.emplace_back(); + auto &ostaff = omeasure.staves.back(); + auto &ovoice = ostaff.voices[0]; + + NoteData onote{}; + onote.tickTimePosition = 0; + onote.durationData.durationTimeTicks = 10; + onote.durationData.durationName = DurationName::quarter; + ovoice.notes.push_back(onote); + + RehearsalData rehearsal; + rehearsal.text = "B"; + rehearsal.enclosure = RehearsalEnclosure::rectangle; + rehearsal.fontData.fontFamily = {"Times New Roman"}; + rehearsal.fontData.style = FontStyle::normal; + rehearsal.fontData.weight = FontWeight::bold; + rehearsal.fontData.sizeType = FontSizeType::point; + rehearsal.fontData.sizePoint = 12.0; + rehearsal.positionData.isDefaultXSpecified = true; + rehearsal.positionData.defaultX = 5.0; + + DirectionData directionData; + directionData.tickTimePosition = 0; + directionData.rehearsals.push_back(rehearsal); + ostaff.directions.push_back(directionData); + + const auto rscore = mxtest::roundTrip(oscore); + REQUIRE(rscore.parts.size() == 1); + REQUIRE(rscore.parts.front().measures.size() == 1); + REQUIRE(rscore.parts.front().measures.front().staves.size() == 1); + const auto &rdirections = rscore.parts.front().measures.front().staves.front().directions; + REQUIRE(rdirections.size() == 1); + REQUIRE(rdirections.front().rehearsals.size() == 1); + CHECK_EQUAL("B", rdirections.front().rehearsals.front().text); + CHECK(RehearsalEnclosure::rectangle == rdirections.front().rehearsals.front().enclosure); + CHECK(FontWeight::bold == rdirections.front().rehearsals.front().fontData.weight); +} + +T_END; + #endif diff --git a/src/private/mxtest/impl/DirectionWriterTest.cpp b/src/private/mxtest/impl/DirectionWriterTest.cpp index 0f73d91d..df9cea65 100644 --- a/src/private/mxtest/impl/DirectionWriterTest.cpp +++ b/src/private/mxtest/impl/DirectionWriterTest.cpp @@ -7,6 +7,7 @@ #include "cpul/cpulTestHarness.h" #include "mx/api/OttavaData.h" +#include "mx/api/RehearsalData.h" #include "mx/core/generated/Direction.h" #include "mx/core/generated/DirectionType.h" #include "mx/core/generated/MusicDataChoice.h" @@ -117,4 +118,53 @@ TEST(segnoAndCodaRoundTrip, DirectionWriter) T_END +// Build a RehearsalData carrying the full attribute set (text, position, color, font, enclosure), +// write it with DirectionWriter, read it back with DirectionReader, and confirm every field +// survives the api -> core -> api round trip. +TEST(rehearsalRoundTrip, DirectionWriter) +{ + api::RehearsalData rehearsal; + rehearsal.text = "A"; + rehearsal.positionData.isDefaultXSpecified = true; + rehearsal.positionData.defaultX = 10.0; + rehearsal.positionData.isDefaultYSpecified = true; + rehearsal.positionData.defaultY = 20.0; + rehearsal.positionData.isRelativeXSpecified = true; + rehearsal.positionData.relativeX = 3.0; + rehearsal.positionData.isRelativeYSpecified = true; + rehearsal.positionData.relativeY = 4.0; + rehearsal.positionData.horizontalAlignmnet = api::HorizontalAlignment::left; + rehearsal.positionData.verticalAlignment = api::VerticalAlignment::top; + rehearsal.fontData.fontFamily = {"Maestro"}; + rehearsal.fontData.style = api::FontStyle::italic; + rehearsal.fontData.weight = api::FontWeight::bold; + rehearsal.fontData.sizeType = api::FontSizeType::point; + rehearsal.fontData.sizePoint = 14.0; + rehearsal.isColorSpecified = true; + rehearsal.colorData.red = 255; + rehearsal.colorData.green = 0; + rehearsal.colorData.blue = 0; + rehearsal.colorData.isAlphaSpecified = true; + rehearsal.colorData.alpha = 255; + rehearsal.enclosure = api::RehearsalEnclosure::square; + + api::DirectionData directionData; + directionData.rehearsals.push_back(rehearsal); + + Cursor cursor{1, 100}; + DirectionWriter writer{directionData, cursor}; + const auto mdcSet = writer.getDirectionLikeThings(); + REQUIRE(mdcSet.size() >= 1); + CHECK(mdcSet.front().isDirection()); + const auto &direction = mdcSet.front().asDirection(); + + DirectionReader reader{direction, cursor}; + const auto roundTripped = reader.getDirectionData(); + + REQUIRE(roundTripped.rehearsals.size() == 1); + CHECK(rehearsal == roundTripped.rehearsals.front()); +} + +T_END + #endif From a7c6684fa661cec2a3fb5bf03f93e4a20a1ef991 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 19 Jun 2026 22:42:49 +0000 Subject: [PATCH 2/3] fix: add RehearsalEnclosure::unspecified to prevent phantom enclosure on write When a rehearsal element had no enclosure attribute in the source XML, the reader left RehearsalData::enclosure at its default (rectangle), and the writer would then emit enclosure="rectangle", silently modifying the document on round-trip. Fix by adding an `unspecified` sentinel to RehearsalEnclosure, changing the RehearsalData default to `unspecified`, and skipping the enclosure setEnclosure call in DirectionWriter when the value is unspecified. Adds RehearsalUnspecifiedEnclosureNoPhantomAttribute test that asserts no enclosure attribute appears in the serialized XML and that the field round-trips as unspecified. --- src/include/mx/api/RehearsalData.h | 3 +- src/private/mx/impl/DirectionWriter.cpp | 2 + src/private/mxtest/api/DirectionDataTest.cpp | 41 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/include/mx/api/RehearsalData.h b/src/include/mx/api/RehearsalData.h index eece7090..e45f9016 100644 --- a/src/include/mx/api/RehearsalData.h +++ b/src/include/mx/api/RehearsalData.h @@ -14,6 +14,7 @@ namespace api { enum class RehearsalEnclosure { + unspecified, rectangle, square, oval, @@ -36,7 +37,7 @@ class RehearsalData RehearsalData() : text{}, positionData{}, isColorSpecified{false}, colorData{}, fontData{}, - enclosure{RehearsalEnclosure::rectangle} + enclosure{RehearsalEnclosure::unspecified} { } }; diff --git a/src/private/mx/impl/DirectionWriter.cpp b/src/private/mx/impl/DirectionWriter.cpp index 689d97d3..d4b04b2f 100644 --- a/src/private/mx/impl/DirectionWriter.cpp +++ b/src/private/mx/impl/DirectionWriter.cpp @@ -461,6 +461,8 @@ std::vector DirectionWriter::getDirectionLikeThings() } switch (item.enclosure) { + case api::RehearsalEnclosure::unspecified: + break; case api::RehearsalEnclosure::rectangle: rehearsal.setEnclosure(core::EnclosureShape::rectangle()); break; diff --git a/src/private/mxtest/api/DirectionDataTest.cpp b/src/private/mxtest/api/DirectionDataTest.cpp index a6fab816..a901c117 100644 --- a/src/private/mxtest/api/DirectionDataTest.cpp +++ b/src/private/mxtest/api/DirectionDataTest.cpp @@ -273,4 +273,45 @@ TEST(RehearsalRoundTripXml, DirectionData) T_END; +// Verify that a rehearsal with no enclosure set (RehearsalEnclosure::unspecified) does not emit +// an enclosure attribute in the serialized XML, and that the field round-trips as unspecified. +TEST(RehearsalUnspecifiedEnclosureNoPhantomAttribute, DirectionData) +{ + ScoreData oscore; + oscore.ticksPerQuarter = 10; + oscore.parts.emplace_back(); + auto &opart = oscore.parts.back(); + opart.measures.emplace_back(); + auto &omeasure = opart.measures.back(); + omeasure.staves.emplace_back(); + auto &ostaff = omeasure.staves.back(); + auto &ovoice = ostaff.voices[0]; + + NoteData onote{}; + onote.tickTimePosition = 0; + onote.durationData.durationTimeTicks = 10; + onote.durationData.durationName = DurationName::quarter; + ovoice.notes.push_back(onote); + + RehearsalData rehearsal; + rehearsal.text = "C"; + // enclosure left at default (unspecified) — must not appear in XML or round-trip as rectangle + + DirectionData directionData; + directionData.tickTimePosition = 0; + directionData.rehearsals.push_back(rehearsal); + ostaff.directions.push_back(directionData); + + const auto xml = mxtest::toXml(oscore); + CHECK(xml.find("enclosure") == std::string::npos); + + const auto rscore = mxtest::roundTrip(oscore); + REQUIRE(rscore.parts.front().measures.front().staves.front().directions.size() == 1); + REQUIRE(rscore.parts.front().measures.front().staves.front().directions.front().rehearsals.size() == 1); + CHECK(RehearsalEnclosure::unspecified == + rscore.parts.front().measures.front().staves.front().directions.front().rehearsals.front().enclosure); +} + +T_END; + #endif From 87bb2079fd83bd8e7890104137d2b046ab4713b5 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sat, 20 Jun 2026 17:18:22 +0200 Subject: [PATCH 3/3] chore: edit agents.md --- AGENTS.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index fa04cefd..c97defa6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,3 +122,28 @@ locally. | `src/private/mx/core/generated/Document.h` | The core document model (parse, serialize, Document class) | | `src/private/mxtest/corert/CoreRoundtripImpl.cpp` | The corert test runner (discover, parse, serialize, compare) | | `src/private/mxtest/corert/Compare.cpp` | DOM normalization and comparison engine used by corert and api tests | + +## Git Authorship + +The git commit Author and Committer fields are for the user's information, not yours. No Co-Authored-By either. + +Correct: + +```text +Author: The User +Date: Sat Jun 20 10:31:06 2026 +0000 + + Blah +``` + +Incorrect: + +```text +Author: Claude +Date: Sat Jun 20 10:31:06 2026 +0000 + + Blah + + Co-Authored-By: Claude Opus 4.8 (1M context) + Claude-Session: https://claude.ai/code/session_01S42a2LXrZb5GUk9cY7Hdru +```