diff --git a/src/include/mx/api/PartData.h b/src/include/mx/api/PartData.h index 2fe793b8..b2a7c44e 100644 --- a/src/include/mx/api/PartData.h +++ b/src/include/mx/api/PartData.h @@ -4,6 +4,7 @@ #pragma once +#include "mx/api/ApiCommon.h" #include "mx/api/MeasureData.h" #include "mx/api/SoundID.h" #include "mx/api/TransposeData.h" @@ -83,23 +84,62 @@ class PartData public: std::string uniqueId; - // because the MuscXML specification says this "Formatting - // attributes for the part-name element are deprecated in - // Version 2.0 in favor of the new part-name-display and - // part-abbreviation-display elements" the name will always - // be written with 'print-object="no". You must populate - // the displayName field in order to have a name displayed. + // --------------------------------------------------------------------- + // Part name / abbreviation, and the MusicXML "two places" problem + // --------------------------------------------------------------------- + // + // MusicXML can express how a part is named in two overlapping ways, and + // that overlap is a perennial source of confusion. mx::api deliberately + // collapses it into a single, simpler model. The reasoning: + // + // 1. (and ) carry the canonical, plain- + // text name. is REQUIRED by the schema, so mx always + // writes it (even when empty). Its *formatting* attributes -- font, + // color, justify and the print-style position attributes -- were + // deprecated in MusicXML 2.0 in favor of the *-display elements. + // + // 2. (and ) are the + // modern, non-deprecated home for a formatted rendering of the name. + // + // So the same *formatting* has two possible homes. mx::api keeps exactly + // one -- the display* fields below -- and follows these rules: + // + // * On read, formatting is taken from wherever it lives. Formatting + // found on the deprecated attributes is migrated into the + // display* model. If a *-display element is also present it WINS over + // the deprecated attributes (the non-deprecated location is canonical). + // * On write, formatting is emitted ONLY to the non-deprecated *-display + // elements. mx never writes the deprecated formatting attributes back + // onto . A document that used the deprecated attributes is + // therefore normalized to the modern form; by design this does not + // round-trip byte-for-byte. + // + // This is purely about *formatting*. The two name strings are not pure + // duplicates: is the canonical text, while + // may hold a different, richer rendering (e.g. with accidental glyphs). + // That is why both `name` and `displayName` are retained. + + // The canonical part name: the text of the required element. + // May be empty; see effectiveName() for the "fall back to the display + // name when this is empty" convenience. std::string name; - // because the MuscXML specification says this "Formatting - // attributes for the part-name element are deprecated in - // Version 2.0 in favor of the new part-name-display and - // part-abbreviation-display elements" the abbreviation - // will always be written with 'print-object="no". You - // must populate the displayAbbreviation field in order to - // have an abbreviation displayed. + // The canonical part abbreviation: the text of . The + // element is optional, so this is only written when non-empty. std::string abbreviation; + // Visibility of / via the print-object + // attribute. print-object is NOT part of the 2.0 formatting deprecation: + // it is an independent property controlling whether the name is rendered, + // and mx round-trips it faithfully: + // unspecified -> no print-object attribute is written (defaults to shown) + // yes / no -> print-object="yes" / "no" is written verbatim + // (Historically mx force-wrote print-object="no" on every part, silently + // hiding otherwise-visible names. That conflated print-object with the + // formatting deprecation; it was incorrect and has been removed.) + Bool namePrintObject = Bool::unspecified; + Bool abbreviationPrintObject = Bool::unspecified; + std::string displayName; PrintData displayNamePrintData; PositionData displayNamePositionData; @@ -119,6 +159,22 @@ class PartData std::vector measures; + // MusicXML requires , but real files often leave it empty and + // place the human-readable name only in . These helpers + // return the name to actually show: the canonical `name` when present, + // otherwise the `displayName` (and likewise for the abbreviation). The + // stored fields are left untouched, so the original document still + // round-trips. + inline const std::string &effectiveName() const + { + return !name.empty() ? name : displayName; + } + + inline const std::string &effectiveAbbreviation() const + { + return !abbreviation.empty() ? abbreviation : displayAbbreviation; + } + inline int getNumStaves() const { int numStaves = 0; @@ -190,6 +246,8 @@ MXAPI_EQUALS_BEGIN(PartData) MXAPI_EQUALS_MEMBER(uniqueId) MXAPI_EQUALS_MEMBER(name) MXAPI_EQUALS_MEMBER(abbreviation) +MXAPI_EQUALS_MEMBER(namePrintObject) +MXAPI_EQUALS_MEMBER(abbreviationPrintObject) MXAPI_EQUALS_MEMBER(displayName) MXAPI_EQUALS_MEMBER(displayNamePrintData) MXAPI_EQUALS_MEMBER(displayNamePositionData) diff --git a/src/private/mx/impl/NameDisplayFunctions.cpp b/src/private/mx/impl/NameDisplayFunctions.cpp index d34f3530..bc30776f 100644 --- a/src/private/mx/impl/NameDisplayFunctions.cpp +++ b/src/private/mx/impl/NameDisplayFunctions.cpp @@ -8,6 +8,8 @@ #include "mx/core/generated/FormattedText.h" #include "mx/core/generated/NameDisplay.h" #include "mx/core/generated/NameDisplayChoice.h" +#include "mx/impl/PositionFunctions.h" +#include "mx/impl/PrintFunctions.h" #include @@ -39,11 +41,38 @@ std::string extractDisplayText(const core::NameDisplay &nameDisplay) return ss.str(); } +void extractDisplayFormatting(const core::NameDisplay &nameDisplay, api::PrintData &outPrintData, + api::PositionData &outPositionData) +{ + for (const auto &c : nameDisplay.choice()) + { + if (c.isDisplayText()) + { + const auto &ft = c.asDisplayText(); + outPrintData = getPrintData(ft); + outPositionData = getPositionData(ft); + return; // the api keeps one run's formatting; the first run wins + } + } +} + core::NameDisplay makeNameDisplay(const std::string &text) +{ + return makeNameDisplay(text, api::PrintData{}, api::PositionData{}); +} + +core::NameDisplay makeNameDisplay(const std::string &text, const api::PrintData &printData, + const api::PositionData &positionData) { core::NameDisplay nameDisplay{}; core::FormattedText ft{}; ft.setValue(text); + // Emit font/color and the print-style position attributes onto the + // run -- the modern, non-deprecated home for this + // formatting. (print-object is not a attribute, so the + // PrintData::printObject field is intentionally not applied here.) + setAttributesFromPrintData(printData, ft); + setAttributesFromPositionData(positionData, ft); nameDisplay.addChoice(core::NameDisplayChoice::displayText(ft)); return nameDisplay; } diff --git a/src/private/mx/impl/NameDisplayFunctions.h b/src/private/mx/impl/NameDisplayFunctions.h index 0884c46e..7f6a1a5b 100644 --- a/src/private/mx/impl/NameDisplayFunctions.h +++ b/src/private/mx/impl/NameDisplayFunctions.h @@ -4,6 +4,9 @@ #pragma once +#include "mx/api/PositionData.h" +#include "mx/api/PrintData.h" + #include namespace mx @@ -20,12 +23,25 @@ namespace impl { // Best-effort plain-text extraction from a MusicXML name-display element // (, , etc.): the concatenated -// runs, with rendered as "b"/"#". Formatting -// attributes are not preserved -- the api models display names as plain text. +// runs, with rendered as "b"/"#". std::string extractDisplayText(const core::NameDisplay &nameDisplay); -// Build a minimal name-display carrying a single run. The -// inverse of extractDisplayText for plain-text display names. +// Reads the formatting (font, color, and print-style position attributes) of a +// name-display element from its first run. The api models a +// display name as a single formatted string, so a multi-run display collapses +// to the formatting of its first run. +void extractDisplayFormatting(const core::NameDisplay &nameDisplay, api::PrintData &outPrintData, + api::PositionData &outPositionData); + +// Build a minimal name-display carrying a single, unformatted +// run. The inverse of extractDisplayText for plain-text display names. core::NameDisplay makeNameDisplay(const std::string &text); + +// As makeNameDisplay(text), but also applies the given formatting to the +// run. This is how mx emits name formatting at the modern, +// non-deprecated *-display location instead of on the deprecated +// attributes (see api/PartData.h for the full rationale). +core::NameDisplay makeNameDisplay(const std::string &text, const api::PrintData &printData, + const api::PositionData &positionData); } // namespace impl } // namespace mx diff --git a/src/private/mx/impl/PartReader.cpp b/src/private/mx/impl/PartReader.cpp index b46fd3ce..253a80c3 100644 --- a/src/private/mx/impl/PartReader.cpp +++ b/src/private/mx/impl/PartReader.cpp @@ -29,6 +29,7 @@ #include "mx/impl/Converter.h" #include "mx/impl/MeasureReader.h" #include "mx/impl/NameDisplayFunctions.h" +#include "mx/impl/PositionFunctions.h" #include "mx/impl/PrintFunctions.h" #include "mx/utility/Throw.h" #include "mx/utility/Unused.h" @@ -39,6 +40,42 @@ namespace mx { namespace impl { +namespace +{ +// True when a / carries any of the formatting +// attributes that MusicXML 2.0 deprecated in favor of the *-display elements. +bool nameHasDeprecatedFormatting(const core::PartName &n) +{ + return n.fontFamily().has_value() || n.fontStyle().has_value() || n.fontSize().has_value() || + n.fontWeight().has_value() || n.color().has_value() || n.defaultX().has_value() || + n.defaultY().has_value() || n.relativeX().has_value() || n.relativeY().has_value() || + n.justify().has_value(); +} + +// Reads a name/abbreviation's display text and formatting into the api's single +// display model, following the deprecation rules documented in api/PartData.h: +// a present *-display element is canonical and wins; otherwise any deprecated +// formatting on the name element itself is migrated into the display model so it +// is re-emitted at the modern location. +void readNameDisplay(const core::PartName &nameElement, const std::optional &display, + std::string &outText, api::PrintData &outPrintData, api::PositionData &outPositionData) +{ + if (display.has_value()) + { + outText = extractDisplayText(*display); + extractDisplayFormatting(*display, outPrintData, outPositionData); + } + else if (nameHasDeprecatedFormatting(nameElement)) + { + outText = nameElement.value(); + outPrintData = getPrintData(nameElement); + // print-object belongs to the name element, not the display run. + outPrintData.printObject = api::Bool::unspecified; + outPositionData = getPositionData(nameElement); + } +} +} // namespace + PartReader::PartReader(const core::ScorePart &inScorePart, const core::PartwisePart &inPartwisePartRef, int globalTicksPerMeasure, const core::ScorePartwise &inScore, int inDivisionsValue) : myPartwisePart{inPartwisePartRef}, myScorePart{inScorePart}, myNumStaves{-1}, @@ -164,21 +201,26 @@ int PartReader::calculateNumStaves() const void PartReader::parseScorePart() const { myOutPartData.uniqueId = myScorePart.id().value(); - myOutPartData.name = myScorePart.partName().value(); - if (myScorePart.partNameDisplay().has_value()) - { - myOutPartData.displayName = extractDisplayText(*myScorePart.partNameDisplay()); - } + const auto &corePartName = myScorePart.partName(); + myOutPartData.name = corePartName.value(); + myOutPartData.namePrintObject = getPrintObject(corePartName); + readNameDisplay(corePartName, myScorePart.partNameDisplay(), myOutPartData.displayName, + myOutPartData.displayNamePrintData, myOutPartData.displayNamePositionData); if (myScorePart.partAbbreviation().has_value()) { - myOutPartData.abbreviation = myScorePart.partAbbreviation()->value(); + const auto &coreAbbreviation = *myScorePart.partAbbreviation(); + myOutPartData.abbreviation = coreAbbreviation.value(); + myOutPartData.abbreviationPrintObject = getPrintObject(coreAbbreviation); + readNameDisplay(coreAbbreviation, myScorePart.partAbbreviationDisplay(), myOutPartData.displayAbbreviation, + myOutPartData.displayAbbreviationPrintData, myOutPartData.displayAbbreviationPositionData); } - - if (myScorePart.partAbbreviationDisplay().has_value()) + else if (myScorePart.partAbbreviationDisplay().has_value()) { myOutPartData.displayAbbreviation = extractDisplayText(*myScorePart.partAbbreviationDisplay()); + extractDisplayFormatting(*myScorePart.partAbbreviationDisplay(), myOutPartData.displayAbbreviationPrintData, + myOutPartData.displayAbbreviationPositionData); } if (!myScorePart.scoreInstrument().empty()) diff --git a/src/private/mx/impl/PartWriter.cpp b/src/private/mx/impl/PartWriter.cpp index de41a370..bc6b99c7 100644 --- a/src/private/mx/impl/PartWriter.cpp +++ b/src/private/mx/impl/PartWriter.cpp @@ -42,6 +42,21 @@ namespace mx { namespace impl { +namespace +{ +// Writes the print-object attribute onto a / only +// when the api specifies it; api::Bool::unspecified leaves the attribute off so +// the element defaults to shown. +void applyPrintObject(api::Bool printObject, core::PartName &out) +{ + if (printObject != api::Bool::unspecified) + { + Converter converter; + out.setPrintObject(converter.convert(printObject)); + } +} +} // namespace + PartWriter::PartWriter(const api::PartData &inPartData, int inPartIndex, int inTicksPerQuarter, const ScoreWriter &inScoreWriter) : myPartData{inPartData}, myPartIndex{inPartIndex}, myTicksPerQuarter{inTicksPerQuarter}, myMutex{}, @@ -54,27 +69,33 @@ core::ScorePart PartWriter::getScorePart() const core::ScorePart scorePart{}; scorePart.setID(core::Token{myPartData.uniqueId}); + // is required, so always write it. print-object is round- + // tripped from the model (not force-hidden); deprecated formatting is never + // written here -- it goes to below. See api/PartData.h. core::PartName partName{}; partName.setValue(myPartData.name); - partName.setPrintObject(core::YesNo::no()); + applyPrintObject(myPartData.namePrintObject, partName); scorePart.setPartName(partName); if (myPartData.abbreviation.size() > 0) { core::PartName abbrev{}; abbrev.setValue(myPartData.abbreviation); - abbrev.setPrintObject(core::YesNo::no()); + applyPrintObject(myPartData.abbreviationPrintObject, abbrev); scorePart.setPartAbbreviation(abbrev); } if (myPartData.displayName.size() > 0) { - scorePart.setPartNameDisplay(makeNameDisplay(myPartData.displayName)); + scorePart.setPartNameDisplay(makeNameDisplay(myPartData.displayName, myPartData.displayNamePrintData, + myPartData.displayNamePositionData)); } if (myPartData.displayAbbreviation.size() > 0) { - scorePart.setPartAbbreviationDisplay(makeNameDisplay(myPartData.displayAbbreviation)); + scorePart.setPartAbbreviationDisplay(makeNameDisplay(myPartData.displayAbbreviation, + myPartData.displayAbbreviationPrintData, + myPartData.displayAbbreviationPositionData)); } core::ScoreInstrument scoreInstrument{}; diff --git a/src/private/mxtest/api/ApiK009bSlurScoreData.h b/src/private/mxtest/api/ApiK009bSlurScoreData.h index 58791ccb..412bc20a 100644 --- a/src/private/mxtest/api/ApiK009bSlurScoreData.h +++ b/src/private/mxtest/api/ApiK009bSlurScoreData.h @@ -24,6 +24,7 @@ inline mx::api::ScoreData apiK009bSlurAttributesScoreData() auto &part = score.parts.front(); part.uniqueId = "P1"; part.name = "MusicXML Part"; + part.namePrintObject = Bool::no; // the source has // 1 part.measures.emplace_back(MeasureData{}); diff --git a/src/private/mxtest/api/ApiK014aFermatasScoreData.h b/src/private/mxtest/api/ApiK014aFermatasScoreData.h index 22906417..9776dce4 100644 --- a/src/private/mxtest/api/ApiK014aFermatasScoreData.h +++ b/src/private/mxtest/api/ApiK014aFermatasScoreData.h @@ -20,6 +20,7 @@ inline mx::api::ScoreData apiK014aFermatasScoreData() auto &part = score.parts.front(); part.uniqueId = "P1"; part.name = "MusicXML Part"; + part.namePrintObject = Bool::no; // the source has // 1 part.measures.emplace_back(MeasureData{}); diff --git a/src/private/mxtest/api/ApiK016aMiscScoreData.h b/src/private/mxtest/api/ApiK016aMiscScoreData.h index aa46d4cf..c9efde1b 100644 --- a/src/private/mxtest/api/ApiK016aMiscScoreData.h +++ b/src/private/mxtest/api/ApiK016aMiscScoreData.h @@ -20,6 +20,7 @@ inline mx::api::ScoreData apiK016aMiscScoreData() scoreData.parts.emplace_back(PartData{}); auto &part = scoreData.parts.back(); part.name = "hello world"; + part.namePrintObject = Bool::no; // the source has part.uniqueId = "ID"; part.measures.emplace_back(MeasureData{}); auto &measure = part.measures.back(); diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index b35300aa..176f4138 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -24,3 +24,38 @@ ksuite/k016a_Miscellaneous_Fields.xml lysuite/ly33c_Spanners_Slurs.xml lysuite/ly33g_Slur_ChordedNotes.xml lysuite/ly52b_Breaks.xml + +# Unblocked by #227: mx no longer force-writes print-object="no" on every +# /. print-object is now round-tripped faithfully +# from the api model (tri-state yes/no/unspecified), and deprecated name +# formatting is migrated to the *-display elements. Visible part names -- and +# the many files whose only divergence was the spurious print-object="no" -- +# now round-trip. +ksuite/k007a_Notations_Dynamics.xml +ksuite/k007c_Directions_Dynamics.xml +lysuite/ly01b_Pitches_Intervals.xml +lysuite/ly01d_Pitches_Microtones.xml +lysuite/ly03a_Rhythm_Durations.xml +lysuite/ly03b_Rhythm_Backup.xml +lysuite/ly11c_TimeSignatures_CompoundSimple.xml +lysuite/ly11g_TimeSignatures_SingleNumber.xml +lysuite/ly12b_Clefs_NoKeyOrClef.xml +lysuite/ly13a_KeySignatures.xml +lysuite/ly13d_KeySignatures_Microtones.xml +lysuite/ly21a_Chord_Basic.xml +lysuite/ly21b_Chords_TwoNotes.xml +lysuite/ly21c_Chords_ThreeNotesDuration.xml +lysuite/ly21e_Chords_PickupMeasures.xml +lysuite/ly24f_GraceNote_Slur.xml +lysuite/ly41a_MultiParts_Partorder.xml +lysuite/ly41b_MultiParts_MoreThan10.xml +lysuite/ly41i_PartNameDisplay_Override.xml +lysuite/ly43a_PianoStaff.xml +lysuite/ly43b_MultiStaff_DifferentKeys.xml +lysuite/ly45b_RepeatWithAlternatives.xml +lysuite/ly45g_Repeats_NotEnded.xml +lysuite/ly46d_PickupMeasure_ImplicitMeasures.xml +lysuite/ly46f_IncompleteMeasures.xml +lysuite/ly71a_Chordnames.xml +lysuite/ly72a_TransposingInstruments.xml +lysuite/ly72b_TransposingInstruments_Full.xml