From c947f5f61ab05b223d44f6aff985c968882844d2 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 18 Sep 2026 02:13:57 +0200 Subject: [PATCH 1/2] fix: write correct valign-image for images and direction use the valign-image vocabulary (top, middle, bottom), not valign. setAttributesFromPositionData engaged valign's presence flag for any authored alignment, but its value setter silently no-ops for these elements (a core::Valign does not convert to the std::optional they expect), so every authored alignment serialized as the leftover default "top". Write the alignment through a dedicated valign-image setter instead, omitting the attribute when unspecified or, for baseline, which valign-image cannot express, and report the dropped baseline case as a diagnostic rather than silently mapping it to something wrong. Fixes #444. --- src/private/mx/impl/DirectionWriter.cpp | 24 +++--------- src/private/mx/impl/PageTextFunctions.cpp | 14 +++++-- src/private/mx/impl/PageTextFunctions.h | 4 +- src/private/mx/impl/PositionFunctions.h | 33 ++++++++++++++++ src/private/mx/impl/ScoreWriter.cpp | 2 +- .../mxtest/api/CreditRoundTripTest.cpp | 32 +++++++++++++++ .../api/DirectionMarksRoundTripTest.cpp | 39 +++++++++++++++++++ 7 files changed, 125 insertions(+), 23 deletions(-) diff --git a/src/private/mx/impl/DirectionWriter.cpp b/src/private/mx/impl/DirectionWriter.cpp index cbab43f73..b2c090b1f 100644 --- a/src/private/mx/impl/DirectionWriter.cpp +++ b/src/private/mx/impl/DirectionWriter.cpp @@ -103,7 +103,6 @@ #include "mx/core/generated/TimeModificationGroup.h" #include "mx/core/generated/Timpani.h" #include "mx/core/generated/TuningGroup.h" -#include "mx/core/generated/ValignImage.h" #include "mx/core/generated/Wedge.h" #include "mx/core/generated/WedgeType.h" #include "mx/core/generated/YesNo.h" @@ -113,6 +112,7 @@ #include "mx/impl/LineFunctions.h" #include "mx/impl/MarkDataFunctions.h" #include "mx/impl/OttavaFunctions.h" +#include "mx/impl/PositionFunctions.h" #include "mx/impl/PrintFunctions.h" #include "mx/impl/SoundFunctions.h" #include "mx/impl/SpannerFunctions.h" @@ -1026,24 +1026,12 @@ void DirectionWriter::emitImage(const api::ImageData &item, core::Direction &dir image.setWidth(core::Tenths{core::Decimal{*item.width}}); } setAttributesFromPositionData(item.positionData, image); - // 's valign is the valign-image type (no baseline), which the generic position - // helper cannot write; set it on the element directly. A baseline value cannot be - // expressed on an image and is not written. - switch (item.positionData.verticalAlignment) + // 's valign is the valign-image type, not valign; see PositionFunctions.h. + setImageValignFromVerticalAlignment(item.positionData.verticalAlignment, image); + if (item.positionData.verticalAlignment == api::VerticalAlignment::baseline) { - case api::VerticalAlignment::top: - image.setValign(core::ValignImage::top()); - break; - case api::VerticalAlignment::middle: - image.setValign(core::ValignImage::middle()); - break; - case api::VerticalAlignment::bottom: - image.setValign(core::ValignImage::bottom()); - break; - case api::VerticalAlignment::baseline: - case api::VerticalAlignment::unspecified: - default: - break; + myDiagnostics.report(api::Severity::warning, api::DiagnosticCode::droppedData, cursorLocation(myCursor), + "image valign baseline has no valign-image counterpart; omitted"); } setId(item.id, image, myDiagnostics, cursorLocation(myCursor)); core::DirectionType dt{}; diff --git a/src/private/mx/impl/PageTextFunctions.cpp b/src/private/mx/impl/PageTextFunctions.cpp index 12d92d9d9..93c7e089b 100644 --- a/src/private/mx/impl/PageTextFunctions.cpp +++ b/src/private/mx/impl/PageTextFunctions.cpp @@ -48,7 +48,7 @@ api::PageImageData getImageData(const core::Image &image, int pageNumber) return out; } -core::Image makeCoreImage(const api::PageImageData &in) +core::Image makeCoreImage(const api::PageImageData &in, const DiagnosticsContext &diagnostics) { core::Image image{}; image.setSource(in.source); @@ -65,11 +65,19 @@ core::Image makeCoreImage(const api::PageImageData &in) } setAttributesFromPositionData(in.positionData, image); + // writes valign-image, not valign; see PositionFunctions.h. + setImageValignFromVerticalAlignment(in.positionData.verticalAlignment, image); + if (in.positionData.verticalAlignment == api::VerticalAlignment::baseline) + { + diagnostics.report(api::Severity::warning, api::DiagnosticCode::droppedData, api::Location{}, + "credit-image valign baseline has no valign-image counterpart; omitted"); + } return image; } } // namespace -void createCredits(const api::ScoreData &inScoreData, core::ScoreHeaderGroup &outHeader) +void createCredits(const api::ScoreData &inScoreData, core::ScoreHeaderGroup &outHeader, + const DiagnosticsContext &diagnostics) { for (const auto &p : inScoreData.pageTextItems) { @@ -128,7 +136,7 @@ void createCredits(const api::ScoreData &inScoreData, core::ScoreHeaderGroup &ou for (const auto &img : inScoreData.pageImageItems) { core::Credit credit; - credit.setChoice(core::CreditChoice::creditImage(makeCoreImage(img))); + credit.setChoice(core::CreditChoice::creditImage(makeCoreImage(img, diagnostics))); if (img.pageNumber > 0) { diff --git a/src/private/mx/impl/PageTextFunctions.h b/src/private/mx/impl/PageTextFunctions.h index 5dc867064..b43c876b0 100644 --- a/src/private/mx/impl/PageTextFunctions.h +++ b/src/private/mx/impl/PageTextFunctions.h @@ -5,6 +5,7 @@ #pragma once #include "mx/api/PageTextData.h" +#include "mx/impl/DiagnosticsContext.h" namespace mx { @@ -27,6 +28,7 @@ void createCredits(const core::ScoreHeaderGroup &inHeader, api::ScoreData &outSc // Writes the score's pageTextItems and pageImageItems back out as // `` elements on the header. -void createCredits(const api::ScoreData &inScoreData, core::ScoreHeaderGroup &outHeader); +void createCredits(const api::ScoreData &inScoreData, core::ScoreHeaderGroup &outHeader, + const DiagnosticsContext &diagnostics); } // namespace impl } // namespace mx diff --git a/src/private/mx/impl/PositionFunctions.h b/src/private/mx/impl/PositionFunctions.h index bb7035a66..1a936c5b3 100644 --- a/src/private/mx/impl/PositionFunctions.h +++ b/src/private/mx/impl/PositionFunctions.h @@ -5,6 +5,7 @@ #pragma once #include "mx/api/PositionData.h" +#include "mx/core/generated/ValignImage.h" #include "mx/impl/Converter.h" #include "mx/utility/OptionalMembers.h" @@ -193,5 +194,37 @@ void setAttributesFromPositionData(const api::PositionData &positionData, ATTRIB lookForAndSetPlacement(converter.convert(positionData.placement), &outAttributes); } } + +// and write vertical alignment as valign-image (top, middle, bottom -- +// no baseline), not valign. setAttributesFromPositionData above cannot write it: its presence +// setter engages the field with its natural zero (top) for any specified alignment, and its +// value setter is compiled out because a core::Valign does not convert to the +// std::optional that valign-image elements expect, so the natural-zero top +// is left in place no matter what was authored. Call this afterward on the same element to +// write the correct value. Vertical alignments unspecified are omitted, and so is baseline: +// valign-image has no baseline value, so a baseline alignment cannot be represented and is +// omitted rather than mapped to something misleading. Callers that want to report the dropped +// baseline case should check for it before calling this. +template +void setImageValignFromVerticalAlignment(api::VerticalAlignment verticalAlignment, ATTRIBUTES_TYPE &outAttributes) +{ + switch (verticalAlignment) + { + case api::VerticalAlignment::top: + outAttributes.setValign(core::ValignImage::top()); + break; + case api::VerticalAlignment::middle: + outAttributes.setValign(core::ValignImage::middle()); + break; + case api::VerticalAlignment::bottom: + outAttributes.setValign(core::ValignImage::bottom()); + break; + case api::VerticalAlignment::baseline: + case api::VerticalAlignment::unspecified: + default: + outAttributes.setValign(std::nullopt); + break; + } +} } // namespace impl } // namespace mx diff --git a/src/private/mx/impl/ScoreWriter.cpp b/src/private/mx/impl/ScoreWriter.cpp index 969fb0116..da08d1d93 100644 --- a/src/private/mx/impl/ScoreWriter.cpp +++ b/src/private/mx/impl/ScoreWriter.cpp @@ -159,7 +159,7 @@ core::ScorePartwise ScoreWriter::getScorePartwise() const createEncoding(myScoreData.encoding, header, myDiagnostics); addDefaultsData(myScoreData.defaults, header, myDiagnostics); - createCredits(myScoreData, header); + createCredits(myScoreData, header, myDiagnostics); using PartPair = std::pair; using PartPairs = std::vector; diff --git a/src/private/mxtest/api/CreditRoundTripTest.cpp b/src/private/mxtest/api/CreditRoundTripTest.cpp index a0eb66ca3..050ef37ec 100644 --- a/src/private/mxtest/api/CreditRoundTripTest.cpp +++ b/src/private/mxtest/api/CreditRoundTripTest.cpp @@ -112,6 +112,38 @@ TEST(creditRoundTrip, creditImage) CHECK_EQUAL(60.0, got.positionData.defaultY); } +TEST(creditRoundTrip, imageValignMiddleWritesMiddle) +{ + // The reader always reports credit-image valign as unspecified (it has no + // vertical-alignment vocabulary for valign-image), so a round trip would not show a + // writer bug here; check the written XML instead (#444). + auto in = makeMinimalScore(); + PageImageData img{}; + img.source = "logo.png"; + img.type = "image/png"; + img.positionData.verticalAlignment = VerticalAlignment::middle; + in.pageImageItems.push_back(img); + + const auto xml = mxtest::toXml(in); + CHECK(xml.find("valign=\"middle\"") != std::string::npos); + CHECK(xml.find("valign=\"top\"") == std::string::npos); +} + +TEST(creditRoundTrip, imageValignBaselineOmitsAttribute) +{ + // valign-image has no baseline value, so it must not be written at all -- and in + // particular not defaulted to "top" (#444). + auto in = makeMinimalScore(); + PageImageData img{}; + img.source = "logo.png"; + img.type = "image/png"; + img.positionData.verticalAlignment = VerticalAlignment::baseline; + in.pageImageItems.push_back(img); + + const auto xml = mxtest::toXml(in); + CHECK(xml.find("valign=") == std::string::npos); +} + TEST(creditRoundTrip, justifySurvives) { auto in = makeMinimalScore(); diff --git a/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp b/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp index cfb709875..99b1d6fc6 100644 --- a/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp +++ b/src/private/mxtest/api/DirectionMarksRoundTripTest.cpp @@ -46,6 +46,29 @@ static std::vector roundTripDirectionData(const DirectionData &in return oscore.parts.back().measures.back().staves.back().directions; } +// Serializes a DirectionData without reading it back, so the raw valign attribute +// can be checked. The image valign-image vocabulary has no baseline value, so the reader +// converting a written attribute back would never see a baseline value it could get wrong +// -- the bug is in what gets written, not what round-trips (#444). +static std::string xmlForDirectionData(const DirectionData &inDirectionData) +{ + ScoreData score; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + auto &staff = measure.staves.back(); + staff.directions.push_back(inDirectionData); + + auto r1 = fromScore(score); + if (!r1.ok()) + return {}; + std::stringstream ss; + std::move(r1).value().writeToStream(ss); + return ss.str(); +} + TEST(Damp, DirectionMarksRoundTrip) { DirectionData direction; @@ -179,6 +202,22 @@ TEST(Image, DirectionMarksRoundTrip) T_END; +TEST(ImageValignBaselineUnwritable, DirectionMarksRoundTrip) +{ + // valign-image has no baseline value, so it must not be written -- and in particular + // not defaulted to "top" (#444). + DirectionData direction; + ImageData image; + image.source = "logo.png"; + image.type = "image/png"; + image.positionData.verticalAlignment = VerticalAlignment::baseline; + direction.directionTypes.emplace_back(DirectionChoice{image}); + const auto xml = xmlForDirectionData(direction); + CHECK(xml.find("valign=") == std::string::npos); +} + +T_END; + TEST(AccordionRegistration, DirectionMarksRoundTrip) { DirectionData direction; From c53b22f54825d2034560fe4c3ac216f0ebaeb25f Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 18 Sep 2026 02:20:54 +0200 Subject: [PATCH 2/2] fix: correct image valign diagnostic severity droppedData is always reported as an error elsewhere; the two new image-valign reports used warning. Reword the messages to say what is not written, matching existing droppedData messages, instead of naming the internal valign-image type. Add a WriteDiagnostics test asserting the credit-image case's code, severity and message. --- src/private/mx/impl/DirectionWriter.cpp | 4 ++-- src/private/mx/impl/PageTextFunctions.cpp | 4 ++-- src/private/mxtest/api/WriteDiagnosticsTest.cpp | 17 +++++++++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/private/mx/impl/DirectionWriter.cpp b/src/private/mx/impl/DirectionWriter.cpp index b2c090b1f..ee84ddc42 100644 --- a/src/private/mx/impl/DirectionWriter.cpp +++ b/src/private/mx/impl/DirectionWriter.cpp @@ -1030,8 +1030,8 @@ void DirectionWriter::emitImage(const api::ImageData &item, core::Direction &dir setImageValignFromVerticalAlignment(item.positionData.verticalAlignment, image); if (item.positionData.verticalAlignment == api::VerticalAlignment::baseline) { - myDiagnostics.report(api::Severity::warning, api::DiagnosticCode::droppedData, cursorLocation(myCursor), - "image valign baseline has no valign-image counterpart; omitted"); + myDiagnostics.report(api::Severity::error, api::DiagnosticCode::droppedData, cursorLocation(myCursor), + "an image valign of baseline is not written; an image has no baseline alignment"); } setId(item.id, image, myDiagnostics, cursorLocation(myCursor)); core::DirectionType dt{}; diff --git a/src/private/mx/impl/PageTextFunctions.cpp b/src/private/mx/impl/PageTextFunctions.cpp index 93c7e089b..80448a01a 100644 --- a/src/private/mx/impl/PageTextFunctions.cpp +++ b/src/private/mx/impl/PageTextFunctions.cpp @@ -69,8 +69,8 @@ core::Image makeCoreImage(const api::PageImageData &in, const DiagnosticsContext setImageValignFromVerticalAlignment(in.positionData.verticalAlignment, image); if (in.positionData.verticalAlignment == api::VerticalAlignment::baseline) { - diagnostics.report(api::Severity::warning, api::DiagnosticCode::droppedData, api::Location{}, - "credit-image valign baseline has no valign-image counterpart; omitted"); + diagnostics.report(api::Severity::error, api::DiagnosticCode::droppedData, api::Location{}, + "a credit-image valign of baseline is not written; an image has no baseline alignment"); } return image; } diff --git a/src/private/mxtest/api/WriteDiagnosticsTest.cpp b/src/private/mxtest/api/WriteDiagnosticsTest.cpp index 0e57f2cc7..f4f11c8a0 100644 --- a/src/private/mxtest/api/WriteDiagnosticsTest.cpp +++ b/src/private/mxtest/api/WriteDiagnosticsTest.cpp @@ -148,6 +148,23 @@ TEST(pedalMarkOnANoteIsDropped, WriteDiagnostics) T_END +TEST(creditImageValignBaselineIsDropped, WriteDiagnostics) +{ + auto score = writeDiagnosticsScore(1); + PageImageData img{}; + img.source = "logo.png"; + img.type = "image/png"; + img.positionData.verticalAlignment = VerticalAlignment::baseline; + score.pageImageItems.push_back(img); + + const auto diagnostics = writeDiagnostics(score); + REQUIRE(diagnostics.all().size() == 1); + writeDiagnosticsCheck(diagnostics.all().front(), DiagnosticCode::droppedData, + "a credit-image valign of baseline is not written; an image has no baseline alignment"); +} + +T_END + TEST(unmatchedSlurIsReportedAtItsNote, WriteDiagnostics) { auto score = writeDiagnosticsScore(2);