fix: write the right valign-image for images

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);
-    // <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)
+    // <image>'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::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{};

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);
+    // <credit-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::error, api::DiagnosticCode::droppedData, api::Location{},
+                           "a credit-image valign of baseline is not written; an image has no baseline alignment");
+    }
     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)
         {

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
 // `<credit>` 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

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);
     }
 }
+
+// <image> and <credit-image> 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<core::ValignImage> 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 <typename ATTRIBUTES_TYPE>
+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

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<core::ScorePart, core::PartwisePart>;
     using PartPairs = std::vector<PartPair>;

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();

src/private/mxtest/api/DirectionMarksRoundTripTest.cpp

@@ -46,6 +46,29 @@ static std::vector<DirectionData> roundTripDirectionData(const DirectionData &in
     return oscore.parts.back().measures.back().staves.back().directions;
 }
 
+// Serializes a DirectionData without reading it back, so the raw <image> 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;

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