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