From 8816a0a7ae2cba4009b07b9ef3eeb51ef4e939dc Mon Sep 17 00:00:00 2001 From: Robert Patterson Date: Thu, 6 Aug 2026 21:54:22 -0500 Subject: [PATCH] fix: write arranger and publisher, add page-text enclosure Three small gaps from Denigma's mx::api gap list, each confined to mx::impl plus one new field. ScoreData::arranger and ScoreData::publisher existed with equality members but nothing in mx::impl ever touched them. ScoreReader read an incoming into ScoreData::lyricist, so a file carrying both an arranger and a lyricist lost the lyricist, and ScoreWriter then re-emitted the arranger's text mislabelled as type="lyricist". Both now read into their own field (first of each type wins, as the api has one slot) and are written on the same path as composer and lyricist. is type formatted-text-id, the same core class as and , so it carries the whole text-formatting group. core::FormattedTextID already exposes enclosure()/setEnclosure() and Converter already has both directions; only the api field was missing. PageTextData gains Enclosure enclosure, converted in both directions in PageTextFunctions.cpp beside the existing justify handling. SpannerNumberResolver's comments claimed has no number attribute. That was true through MusicXML 3.0; 3.1 added it. Pedals stay out of number resolution because mx::api does not model the attribute -- behaviour unchanged, only the stated reason corrected. --- src/include/mx/api/ApiCommon.h | 8 +-- src/include/mx/api/PageTextData.h | 5 ++ src/private/mx/impl/PageTextFunctions.cpp | 12 ++++ src/private/mx/impl/ScoreReader.cpp | 23 ++++--- src/private/mx/impl/ScoreWriter.cpp | 18 ++++++ src/private/mx/impl/SpannerNumberResolver.cpp | 2 +- src/private/mx/impl/SpannerNumberResolver.h | 5 +- .../mxtest/api/CreditRoundTripTest.cpp | 55 ++++++++++++++++ .../mxtest/api/DocumentManagerTest.cpp | 62 +++++++++++++++++++ 9 files changed, 173 insertions(+), 17 deletions(-) diff --git a/src/include/mx/api/ApiCommon.h b/src/include/mx/api/ApiCommon.h index 4827c916a..3ff4433aa 100644 --- a/src/include/mx/api/ApiCommon.h +++ b/src/include/mx/api/ApiCommon.h @@ -87,10 +87,10 @@ enum class SystemRelation }; // The shape drawn around a piece of text or a symbol -- MusicXML's enclosure attribute, carried by -// RehearsalData, WordsData, SymbolData and PercussionData. unspecified means the attribute is -// absent, which draws no enclosure; none states explicitly that there is none. A bracket is a -// rectangle with the bottom line missing, as is common in jazz notation, and an invertedBracket is -// one with the top line missing. +// RehearsalData, WordsData, SymbolData, PercussionData and PageTextData. unspecified means the +// attribute is absent, which draws no enclosure; none states explicitly that there is none. A +// bracket is a rectangle with the bottom line missing, as is common in jazz notation, and an +// invertedBracket is one with the top line missing. enum class Enclosure { unspecified, diff --git a/src/include/mx/api/PageTextData.h b/src/include/mx/api/PageTextData.h index 93d0d50e3..d4fcf991f 100644 --- a/src/include/mx/api/PageTextData.h +++ b/src/include/mx/api/PageTextData.h @@ -40,6 +40,10 @@ class PageTextData // carried in `positionData.horizontalAlignment`; MusicXML defines both // attributes on ``. HorizontalAlignment justify = HorizontalAlignment::unspecified; + + // A shape drawn around the text. Enclosure::unspecified draws no enclosure; Enclosure::none + // states explicitly that there is none. + Enclosure enclosure = Enclosure::unspecified; }; MXAPI_EQUALS_BEGIN(PageTextData) @@ -54,6 +58,7 @@ if (!areVectorsEqual(lhs.creditTypes, rhs.creditTypes)) return false; } MXAPI_EQUALS_MEMBER(justify) +MXAPI_EQUALS_MEMBER(enclosure) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(PageTextData); } // namespace api diff --git a/src/private/mx/impl/PageTextFunctions.cpp b/src/private/mx/impl/PageTextFunctions.cpp index 368b02d83..12d92d9d9 100644 --- a/src/private/mx/impl/PageTextFunctions.cpp +++ b/src/private/mx/impl/PageTextFunctions.cpp @@ -91,6 +91,12 @@ void createCredits(const api::ScoreData &inScoreData, core::ScoreHeaderGroup &ou words.setJustify(converter.convert(p.justify)); } + if (p.enclosure != api::Enclosure::unspecified) + { + const Converter converter; + words.setEnclosure(converter.convert(p.enclosure)); + } + core::CreditChoiceGroupChoice groupChoice = core::CreditChoiceGroupChoice::creditWords(words); core::CreditChoiceGroup group; group.setChoice(groupChoice); @@ -164,6 +170,12 @@ void createCredits(const core::ScoreHeaderGroup &inHeader, api::ScoreData &outSc const Converter converter; pageText.justify = converter.convert(*words.justify()); } + + if (words.enclosure().has_value()) + { + const Converter converter; + pageText.enclosure = converter.convert(*words.enclosure()); + } } for (const auto &ct : c.creditType()) diff --git a/src/private/mx/impl/ScoreReader.cpp b/src/private/mx/impl/ScoreReader.cpp index 4dad7d8df..61348f9d4 100644 --- a/src/private/mx/impl/ScoreReader.cpp +++ b/src/private/mx/impl/ScoreReader.cpp @@ -171,13 +171,15 @@ api::ScoreData ScoreReader::getScoreData() const myOutScoreData.movementNumber = *myHeaderGroup.movementNumber(); } - bool isComposerFound = false; - bool isCopyrightFound = false; - if (myHeaderGroup.identification().has_value()) { const auto &ident = *myHeaderGroup.identification(); + bool isComposerFound = false; + bool isArrangerFound = false; + bool isPublisherFound = false; + bool isCopyrightFound = false; + for (const auto &i : ident.creator()) { const bool hasType = i.type().has_value(); @@ -199,17 +201,18 @@ api::ScoreData ScoreReader::getScoreData() const myOutScoreData.lyricist = i.value(); } - // TODO: arranger/publisher overwrite lyricist (ScoreData has no fields for them), - // and ScoreWriter re-emits the value as type="lyricist" -- lossy and mislabeled. - // Issue: add arranger/publisher fields to ScoreData. - if (typeStr == "arranger") + // ScoreData has one slot each for arranger and publisher, but MusicXML permits any + // number of elements of a given type. Keep the first of each. + if (typeStr == "arranger" && !isArrangerFound) { - myOutScoreData.lyricist = i.value(); + myOutScoreData.arranger = i.value(); + isArrangerFound = true; } - if (typeStr == "publisher") + if (typeStr == "publisher" && !isPublisherFound) { - myOutScoreData.lyricist = i.value(); + myOutScoreData.publisher = i.value(); + isPublisherFound = true; } } diff --git a/src/private/mx/impl/ScoreWriter.cpp b/src/private/mx/impl/ScoreWriter.cpp index 447f1bbf0..da0ae5b6a 100644 --- a/src/private/mx/impl/ScoreWriter.cpp +++ b/src/private/mx/impl/ScoreWriter.cpp @@ -117,6 +117,24 @@ core::ScorePartwise ScoreWriter::getScorePartwise() const hasIdentification = true; } + if (!myScoreData.arranger.empty()) + { + core::TypedText arranger{}; + arranger.setType(std::string{"arranger"}); + arranger.setValue(myScoreData.arranger); + identification.addCreator(arranger); + hasIdentification = true; + } + + if (!myScoreData.publisher.empty()) + { + core::TypedText publisher{}; + publisher.setType(std::string{"publisher"}); + publisher.setValue(myScoreData.publisher); + identification.addCreator(publisher); + hasIdentification = true; + } + if (!myScoreData.copyright.empty()) { core::TypedText copyright{}; diff --git a/src/private/mx/impl/SpannerNumberResolver.cpp b/src/private/mx/impl/SpannerNumberResolver.cpp index 8b6c87247..b564a7f78 100644 --- a/src/private/mx/impl/SpannerNumberResolver.cpp +++ b/src/private/mx/impl/SpannerNumberResolver.cpp @@ -85,7 +85,7 @@ class SpannerNumberEventCollector // Mirrors DirectionWriter::emitDirectionTypes: one pass over the ordered direction-type // content, registering each spanner event with the address of its DirectionChoice -- the // same identity the writer presents when it asks for the emitted number. Pedals are - // skipped: has no number attribute. + // skipped: mx::api does not model 's number attribute. void addDirection(const api::DirectionData &inDirection) { for (const auto &choice : inDirection.directionTypes) diff --git a/src/private/mx/impl/SpannerNumberResolver.h b/src/private/mx/impl/SpannerNumberResolver.h index b1d1b88d1..4df1af6f7 100644 --- a/src/private/mx/impl/SpannerNumberResolver.h +++ b/src/private/mx/impl/SpannerNumberResolver.h @@ -40,8 +40,9 @@ namespace impl // // Identity ids are scoped per part and per spanner class: events in the same // part sharing a class and id are one logical spanner, even across staves. -// Pedal starts/stops carry SpannerNumber but the element has no -// number attribute, so pedals are ignored here. +// Pedal starts/stops carry SpannerNumber, but mx::api does not model the +// number attribute (added in MusicXML 3.1), so pedals are ignored +// here and no number is ever emitted for one. // // If more than 16 spanners of one class are open at once in a part (which no // real score approaches), resolution fails loudly with an exception rather diff --git a/src/private/mxtest/api/CreditRoundTripTest.cpp b/src/private/mxtest/api/CreditRoundTripTest.cpp index 51621c6ca..54fb09af4 100644 --- a/src/private/mxtest/api/CreditRoundTripTest.cpp +++ b/src/private/mxtest/api/CreditRoundTripTest.cpp @@ -149,4 +149,59 @@ TEST(creditRoundTrip, justifyAbsentStaysAbsent) CHECK(HorizontalAlignment::unspecified == out.pageTextItems.at(0).justify); } +TEST(creditRoundTrip, enclosureSurvives) +{ + auto in = makeMinimalScore(); + PageTextData credit{}; + credit.text = "Framed title"; + credit.pageNumber = 1; + credit.enclosure = Enclosure::rectangle; + in.pageTextItems.push_back(credit); + + const auto xml = mxtest::toXml(in); + CHECK(xml.find("enclosure=\"rectangle\"") != std::string::npos); + + const auto out = mxtest::roundTrip(in); + + REQUIRE(out.pageTextItems.size() == 1); + const auto &got = out.pageTextItems.at(0); + CHECK_EQUAL("Framed title", got.text); + CHECK(Enclosure::rectangle == got.enclosure); +} + +TEST(creditRoundTrip, enclosureNoneIsExplicit) +{ + auto in = makeMinimalScore(); + PageTextData credit{}; + credit.text = "Deliberately unframed"; + credit.pageNumber = 1; + credit.enclosure = Enclosure::none; + in.pageTextItems.push_back(credit); + + const auto xml = mxtest::toXml(in); + CHECK(xml.find("enclosure=\"none\"") != std::string::npos); + + const auto out = mxtest::roundTrip(in); + + REQUIRE(out.pageTextItems.size() == 1); + CHECK(Enclosure::none == out.pageTextItems.at(0).enclosure); +} + +TEST(creditRoundTrip, enclosureAbsentStaysAbsent) +{ + auto in = makeMinimalScore(); + PageTextData credit{}; + credit.text = "no enclosure here"; + credit.pageNumber = 1; + in.pageTextItems.push_back(credit); + + const auto xml = mxtest::toXml(in); + CHECK(xml.find("enclosure=") == std::string::npos); + + const auto out = mxtest::roundTrip(in); + + REQUIRE(out.pageTextItems.size() == 1); + CHECK(Enclosure::unspecified == out.pageTextItems.at(0).enclosure); +} + #endif diff --git a/src/private/mxtest/api/DocumentManagerTest.cpp b/src/private/mxtest/api/DocumentManagerTest.cpp index 5f62495e2..4c7ffdf49 100644 --- a/src/private/mxtest/api/DocumentManagerTest.cpp +++ b/src/private/mxtest/api/DocumentManagerTest.cpp @@ -261,6 +261,8 @@ ROUND_TRIP_TEST_SCALAR(std::string, movementTitle, movementTitle, "value", 0); ROUND_TRIP_TEST_SCALAR(std::string, movementNumber, movementNumber, "value", 0); ROUND_TRIP_TEST_SCALAR(std::string, composer, composer, "value", 0); ROUND_TRIP_TEST_SCALAR(std::string, lyricist, lyricist, "value", 0); +ROUND_TRIP_TEST_SCALAR(std::string, arranger, arranger, "value", 0); +ROUND_TRIP_TEST_SCALAR(std::string, publisher, publisher, "value", 0); ROUND_TRIP_TEST_SCALAR(std::string, copyright, copyright, "value", 0); ROUND_TRIP_TEST_SCALAR(std::string, encoding.encoder, encoder, "value", 0); ROUND_TRIP_TEST_SCALAR(std::string, encoding.encodingDescription, encodingDescription, "value", 0); @@ -268,6 +270,66 @@ ROUND_TRIP_TEST_SCALAR(int, encoding.encodingDate.year, year, 2016, 0); ROUND_TRIP_TEST_SCALAR(int, encoding.encodingDate.month, month, 9, 0); ROUND_TRIP_TEST_SCALAR(int, encoding.encodingDate.day, day, 12, 0); +// The four types mx::api models are independent slots. The arranger used to be read +// into ScoreData::lyricist and rewritten as type="lyricist", so a file carrying both lost the +// lyricist; the publisher was dropped on the way out entirely. + +TEST(RoundTrip_AllCreatorTypes, DocumentManager) +{ + auto input = ScoreData{}; + input.composer = "MetaComposer"; + input.lyricist = "MetaLyricist"; + input.arranger = "MetaArranger"; + input.publisher = "MetaPublisher"; + + auto &docMngr = DocumentManager::getInstance(); + const auto createResult = docMngr.createFromScore(input); + REQUIRE(createResult.ok()); + const int writeId = createResult.value(); + std::ostringstream oss; + const auto writeResult = docMngr.writeToStream(writeId, oss); + REQUIRE(writeResult.ok()); + docMngr.destroyDocument(writeId); + + const auto xml = oss.str(); + CHECK(xml.find("MetaComposer") != std::string::npos); + CHECK(xml.find("MetaLyricist") != std::string::npos); + CHECK(xml.find("MetaArranger") != std::string::npos); + CHECK(xml.find("MetaPublisher") != std::string::npos); + + const auto output = roundTripScore(input); + CHECK_EQUAL("MetaComposer", output.composer); + CHECK_EQUAL("MetaLyricist", output.lyricist); + CHECK_EQUAL("MetaArranger", output.arranger); + CHECK_EQUAL("MetaPublisher", output.publisher); +} + +T_END + +// Reading the same shape from a real file: musuite/testMetaData.xml carries an arranger, a +// composer and a lyricist, plus creator types mx::api does not model, which must not disturb +// the ones it does. + +TEST(ReadAllCreatorTypes, DocumentManager) +{ + auto &docMngr = DocumentManager::getInstance(); + const auto createResult = docMngr.createFromFile(std::string{mxtest::getResourcesDirectoryPath()} + + std::string{"/musuite/testMetaData.xml"}); + REQUIRE(createResult.ok()); + const int documentId = createResult.value(); + const auto dataResult = docMngr.getData(documentId); + REQUIRE(dataResult.ok()); + const auto score = dataResult.value(); + docMngr.destroyDocument(documentId); + + CHECK_EQUAL("MetaComposer", score.composer); + CHECK_EQUAL("MetaLyricist", score.lyricist); + CHECK_EQUAL("MetaArranger", score.arranger); + CHECK_EQUAL("", score.publisher); +} + +T_END + // --- Page margin coalescing ------------------------------------------------- // Equal odd/even margins collapse to a single ; // unequal margins emit separate odd and even entries. This rule is exercised