fix: narrow an oversized octave shift to the closest ottava line

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;
     }

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 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
+// 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";
 

src/private/mxtest/api/OttavaSizeApiTest.cpp

@@ -343,4 +343,101 @@ 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 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"(<?xml version="1.0" encoding="UTF-8"?>)"
+        R"(<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 4.0 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">)"
+        R"(<score-partwise version="4.0"><part-list><score-part id="P1"><part-name>P</part-name></score-part></part-list>)"
+        R"(<part id="P1"><measure number="1"><attributes><divisions>1</divisions></attributes>)"
+        R"(<direction><direction-type><octave-shift type="down" size="11"/></direction-type></direction>)"
+        R"(<note><pitch><step>C</step><octave>4</octave></pitch><duration>1</duration><type>quarter</type></note>)"
+        R"(<direction><direction-type><octave-shift type="stop" size="11"/></direction-type></direction>)"
+        R"(</measure></part></score-partwise>)";
+
+    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 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"(<?xml version="1.0" encoding="UTF-8"?>)"
+        R"(<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 4.0 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">)"
+        R"(<score-partwise version="4.0"><part-list><score-part id="P1"><part-name>P</part-name></score-part></part-list>)"
+        R"(<part id="P1"><measure number="1"><attributes><divisions>1</divisions></attributes>)"
+        R"(<direction><direction-type><octave-shift type="down" size="27"/></direction-type></direction>)"
+        R"(<note><pitch><step>C</step><octave>4</octave></pitch><duration>1</duration><type>quarter</type></note>)"
+        R"(<direction><direction-type><octave-shift type="stop" size="27"/></direction-type></direction>)"
+        R"(</measure></part></score-partwise>)";
+
+    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)
+{
+    const std::string sourceXml =
+        R"(<?xml version="1.0" encoding="UTF-8"?>)"
+        R"(<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 4.0 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">)"
+        R"(<score-partwise version="4.0"><part-list><score-part id="P1"><part-name>P</part-name></score-part></part-list>)"
+        R"(<part id="P1"><measure number="1"><attributes><divisions>1</divisions></attributes>)"
+        R"(<direction><direction-type><octave-shift type="up" size="1"/></direction-type></direction>)"
+        R"(<note><pitch><step>C</step><octave>4</octave></pitch><duration>1</duration><type>quarter</type></note>)"
+        R"(</measure></part></score-partwise>)";
+
+    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

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