From d3d8b60977cf5c5c94f5757e87c8f686f731112b Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 23 Aug 2026 19:29:05 +0000 Subject: [PATCH 1/2] test: narrow the expected octave-shift size in the api round-trip MusicXML lets octave-shift/@size be any positive integer, and three corpus files use that room for a number that names no line a performer could read: 27, 11 and 1. mx::api narrows each to the nearest ottava it can draw, so the round-trip saw a 27 go in and a 15 come out. The narrowing is the behavior we want. mx::api models the six ottava lines music notation has, and everything downstream is allowed to assume an ottava is one of them. So the harness brings the expected document to the size mx::api writes, the way it already adds mx's attribution stamp to the expected side rather than stripping it from the output. Only the expected side is narrowed, so a write that produced a size other than 8, 15 or 22 still fails. Two api tests state the narrowing on its own, away from the corpus. Pins the three files in the api round-trip baseline. --- .../mxtest/api/CorpusRoundtripMain.cpp | 36 +++++++++++ src/private/mxtest/api/OttavaSizeApiTest.cpp | 63 +++++++++++++++++++ src/private/mxtest/api/roundtrip-baseline.txt | 9 +++ 3 files changed, 108 insertions(+) diff --git a/src/private/mxtest/api/CorpusRoundtripMain.cpp b/src/private/mxtest/api/CorpusRoundtripMain.cpp index 2f6e37fa4..c05963d0a 100644 --- a/src/private/mxtest/api/CorpusRoundtripMain.cpp +++ b/src/private/mxtest/api/CorpusRoundtripMain.cpp @@ -266,6 +266,37 @@ void canonicalizeDirectionSpellings(pugi::xml_document &doc) mxtest::sortAttributes(doc); } +// MusicXML lets octave-shift/@size be any positive integer, so a corpus file can carry a number +// that does not name a line a performer could read: 27, 11, 1. mx::api narrows each of those to +// the nearest ottava it can draw -- 22 stays 22, anything else above 8 becomes 15, and the rest +// become 8 -- because the api models the six ottava lines music notation has rather than the whole +// integer range. That narrowing is what the api is for, so the expected document is brought to the +// same value instead of the api growing a field to carry the original number back out. +// +// Only the expected side is narrowed. mx writes 8, 15, or 22, so a write that produced some other +// size still fails. The size of a stop is taken here from the stop's own attribute, while mx takes +// it from the start the stop closes, so a source whose stop contradicts its start still fails too. +void narrowOctaveShiftSizes(pugi::xml_node el) +{ + if (std::string_view{el.name()} == "octave-shift") + { + pugi::xml_attribute size = el.attribute("size"); + const int stated = size ? size.as_int(0) : 0; + if (stated > 0) + { + size.set_value(stated == 22 ? 22 : (stated > 8 ? 15 : 8)); + } + } + + for (pugi::xml_node c = el.first_child(); c; c = c.next_sibling()) + { + if (c.type() == pugi::node_element) + { + narrowOctaveShiftSizes(c); + } + } +} + bool hasSuffix(const std::string &name, std::string_view suffix) { return name.size() >= suffix.size() && name.compare(name.size() - suffix.size(), suffix.size(), suffix) == 0; @@ -445,6 +476,10 @@ RoundtripResult runRoundtrip(const std::string &absolutePath) canonicalizeDirectionSpellings(expectedDoc); canonicalizeDirectionSpellings(actualDoc); + // mx::api narrows an octave-shift size to a size music notation has, so the expected document + // states the narrowed size rather than the one the source wrote. + narrowOctaveShiftSizes(expectedDoc.document_element()); + // Compare const auto fail = mxtest::corert::compareElements(expectedDoc.document_element(), actualDoc.document_element()); if (fail.isFailure) @@ -530,6 +565,7 @@ void dumpDocuments(const std::string &absolutePath, const std::string &relPath, canonicalizeEncodingChildOrder(expectedDoc.document_element()); collapseEqualPageMargins(expectedDoc.document_element()); canonicalizeDirectionSpellings(expectedDoc); + narrowOctaveShiftSizes(expectedDoc.document_element()); if (!expectedDoc.save_file(expectedPath.c_str())) std::cerr << "dump: failed to write " << expectedPath << "\n"; diff --git a/src/private/mxtest/api/OttavaSizeApiTest.cpp b/src/private/mxtest/api/OttavaSizeApiTest.cpp index 0d57cd6f8..350e402c7 100644 --- a/src/private/mxtest/api/OttavaSizeApiTest.cpp +++ b/src/private/mxtest/api/OttavaSizeApiTest.cpp @@ -343,4 +343,67 @@ TEST(OttavaStopSizeContradictionIsNormalizedToTheStart, OttavaSize) T_END; +// MusicXML's schema allows any positive integer in octave-shift/@size, so a file can ask for a +// line music notation has no name for. The api narrows it to the nearest line it can draw, which +// is what lets everything downstream assume an ottava is one of the six real ones. +TEST(OttavaSizeAboveEightIsNarrowedToTheFifteenthLine, OttavaSize) +{ + const std::string sourceXml = + R"()" + R"()" + R"(P)" + R"(1)" + R"()" + R"(C41quarter)" + R"()" + R"()"; + + const auto score = mxtest::fromXml(sourceXml); + REQUIRE(score.parts.size() == 1); + const auto &directions = score.parts.front().measures.front().staves.front().directions; + REQUIRE(directions.size() == 2); + const auto &startTypes = directions.front().directionTypes; + REQUIRE(startTypes.size() == 1); + REQUIRE(startTypes.front().isOttavaStart()); + CHECK(startTypes.front().ottavaStart().ottavaType == OttavaType::o15ma); + + const auto xml = mxtest::toXml(score); + REQUIRE(!xml.empty()); + const auto shifts = ottavaSizeWrittenShifts(xml); + REQUIRE(shifts.size() == 2); + CHECK_EQUAL(std::string{"15"}, shifts.front().size); + CHECK_EQUAL(std::string{"15"}, shifts.back().size); +} + +T_END; + +// A size below 8 asks for less than an octave, which an ottava cannot draw either. It becomes a +// plain octave line, and the size the source spelled out stays spelled out. +TEST(OttavaSizeBelowEightIsNarrowedToAnOctave, OttavaSize) +{ + const std::string sourceXml = + R"()" + R"()" + R"(P)" + R"(1)" + R"()" + R"(C41quarter)" + R"()"; + + const auto score = mxtest::fromXml(sourceXml); + REQUIRE(score.parts.size() == 1); + const auto &startTypes = score.parts.front().measures.front().staves.front().directions.front().directionTypes; + REQUIRE(startTypes.size() == 1); + REQUIRE(startTypes.front().isOttavaStart()); + CHECK(startTypes.front().ottavaStart().ottavaType == OttavaType::o8vb); + + const auto xml = mxtest::toXml(score); + REQUIRE(!xml.empty()); + const auto shifts = ottavaSizeWrittenShifts(xml); + REQUIRE(shifts.size() == 1); + CHECK_EQUAL(std::string{"8"}, shifts.front().size); +} + +T_END; + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index eb3347da1..dd8cc51eb 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -716,3 +716,12 @@ lysuite/ly33b_Spanners_Tie.xml # was written under the wrong name. musuite/testChordNoVoice.xml musuite/testStringVoiceName.xml + +# Unblocked in the harness rather than in mx::api. Each of these sources states an +# octave-shift size that no ottava line can mean -- 27, 11, 1 -- which MusicXML's +# schema allows and music notation does not. mx::api narrows the size to a line it +# can draw, which is the behavior we want, so the harness narrows the expected +# document the same way instead of the api learning to echo the original number. +lysuite/ly33e_Spanners_OctaveShifts_InvalidSize.xml +synthetic/octave-shift.3.0.xml +synthetic/octave-shift.3.1.xml From 33002fc40d54b9c9c9451344fc07346f22bb0242 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sun, 23 Aug 2026 19:38:16 +0000 Subject: [PATCH 2/2] fix: narrow an oversized octave shift to the widest line, not the 15th The size narrowing matched 22 exactly, so a source asking for 27 fell through to the 15th line even though the 22nd was both available and closer to what it asked for. Match 22 and up instead. lysuite/ly33e_Spanners_OctaveShifts_InvalidSize.xml states size 27, and now round-trips as 22 rather than 15. The round-trip harness narrows the expected document by the same rule. --- src/private/mx/impl/DirectionReader.cpp | 8 ++-- .../mxtest/api/CorpusRoundtripMain.cpp | 8 ++-- src/private/mxtest/api/OttavaSizeApiTest.cpp | 44 ++++++++++++++++--- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/private/mx/impl/DirectionReader.cpp b/src/private/mx/impl/DirectionReader.cpp index 38a4781dd..113a60994 100644 --- a/src/private/mx/impl/DirectionReader.cpp +++ b/src/private/mx/impl/DirectionReader.cpp @@ -755,8 +755,10 @@ void DirectionReader::parseOctaveShift(const core::DirectionType &directionType) bool isUp = octaveShift.type().tag() == core::UpDownStopContinue::Tag::up; // MusicXML does not properly constrain the ottava size to valid music notation values. We do - // so here by interpolating the value into an enum that maps to valid music notation. - if (!isUp && amount == 22) + // so here by interpolating the value into an enum that maps to valid music notation. A size + // larger than 22 takes the widest line we have rather than falling back to the 15th, so the + // line we draw is the closest one to what the source asked for. + if (!isUp && amount >= 22) { ottavaType = api::OttavaType::o22ma; } @@ -768,7 +770,7 @@ void DirectionReader::parseOctaveShift(const core::DirectionType &directionType) { ottavaType = api::OttavaType::o8va; } - else if (isUp && amount == 22) + else if (isUp && amount >= 22) { ottavaType = api::OttavaType::o22mb; } diff --git a/src/private/mxtest/api/CorpusRoundtripMain.cpp b/src/private/mxtest/api/CorpusRoundtripMain.cpp index c05963d0a..21b40ba46 100644 --- a/src/private/mxtest/api/CorpusRoundtripMain.cpp +++ b/src/private/mxtest/api/CorpusRoundtripMain.cpp @@ -268,9 +268,9 @@ void canonicalizeDirectionSpellings(pugi::xml_document &doc) // MusicXML lets octave-shift/@size be any positive integer, so a corpus file can carry a number // that does not name a line a performer could read: 27, 11, 1. mx::api narrows each of those to -// the nearest ottava it can draw -- 22 stays 22, anything else above 8 becomes 15, and the rest -// become 8 -- because the api models the six ottava lines music notation has rather than the whole -// integer range. That narrowing is what the api is for, so the expected document is brought to the +// the closest ottava it can draw -- 22 and up become 22, anything else above 8 becomes 15, and the +// rest become 8 -- because the api models the six ottava lines music notation has rather than the +// whole integer range. That narrowing is what the api is for, so the expected document is brought to the // same value instead of the api growing a field to carry the original number back out. // // Only the expected side is narrowed. mx writes 8, 15, or 22, so a write that produced some other @@ -284,7 +284,7 @@ void narrowOctaveShiftSizes(pugi::xml_node el) const int stated = size ? size.as_int(0) : 0; if (stated > 0) { - size.set_value(stated == 22 ? 22 : (stated > 8 ? 15 : 8)); + size.set_value(stated >= 22 ? 22 : (stated > 8 ? 15 : 8)); } } diff --git a/src/private/mxtest/api/OttavaSizeApiTest.cpp b/src/private/mxtest/api/OttavaSizeApiTest.cpp index 350e402c7..8bbc364bc 100644 --- a/src/private/mxtest/api/OttavaSizeApiTest.cpp +++ b/src/private/mxtest/api/OttavaSizeApiTest.cpp @@ -344,18 +344,19 @@ TEST(OttavaStopSizeContradictionIsNormalizedToTheStart, OttavaSize) T_END; // MusicXML's schema allows any positive integer in octave-shift/@size, so a file can ask for a -// line music notation has no name for. The api narrows it to the nearest line it can draw, which -// is what lets everything downstream assume an ottava is one of the six real ones. -TEST(OttavaSizeAboveEightIsNarrowedToTheFifteenthLine, OttavaSize) +// line music notation has no name for. The api narrows it to the closest line it can draw, which +// is what lets everything downstream assume an ottava is one of the six real ones. A size between +// the 8th and the 15th takes the 15th. +TEST(OttavaSizeBetweenTheLinesIsNarrowedToTheFifteenth, OttavaSize) { const std::string sourceXml = R"()" R"()" R"(P)" R"(1)" - R"()" + R"()" R"(C41quarter)" - R"()" + R"()" R"()"; const auto score = mxtest::fromXml(sourceXml); @@ -377,6 +378,39 @@ TEST(OttavaSizeAboveEightIsNarrowedToTheFifteenthLine, OttavaSize) T_END; +// A size past the 22nd asks for more than the widest line we can draw, so it takes that widest +// line. Falling back to the 15th would move the reader further from what the source asked for. +TEST(OttavaSizePastTheTwentySecondIsNarrowedToTheTwentySecond, OttavaSize) +{ + const std::string sourceXml = + R"()" + R"()" + R"(P)" + R"(1)" + R"()" + R"(C41quarter)" + R"()" + R"()"; + + const auto score = mxtest::fromXml(sourceXml); + REQUIRE(score.parts.size() == 1); + const auto &directions = score.parts.front().measures.front().staves.front().directions; + REQUIRE(directions.size() == 2); + const auto &startTypes = directions.front().directionTypes; + REQUIRE(startTypes.size() == 1); + REQUIRE(startTypes.front().isOttavaStart()); + CHECK(startTypes.front().ottavaStart().ottavaType == OttavaType::o22ma); + + const auto xml = mxtest::toXml(score); + REQUIRE(!xml.empty()); + const auto shifts = ottavaSizeWrittenShifts(xml); + REQUIRE(shifts.size() == 2); + CHECK_EQUAL(std::string{"22"}, shifts.front().size); + CHECK_EQUAL(std::string{"22"}, shifts.back().size); +} + +T_END; + // A size below 8 asks for less than an octave, which an ottava cannot draw either. It becomes a // plain octave line, and the size the source spelled out stays spelled out. TEST(OttavaSizeBelowEightIsNarrowedToAnOctave, OttavaSize)