Repository navigation
fix: write the right valign-image for images - #451
Merged
Merged
Conversation
webern
added
bug
webern
added this pull request to stack #453
September 18, 2026 05:26
<credit-image> and direction <image> 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<core::ValignImage> 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.
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.
webern
force-pushed
the
m/mxdev-valign
branch
from
September 18, 2026 06:56
7fde6c3 to
c53b22f
Compare
webern
marked this pull request as ready for review
September 18, 2026 16:36
This was referenced Sep 18, 2026
webern
added a commit
that referenced
this pull request
Sep 19, 2026
## Human Summary Seems plausible that this is the correct way to read valign. This one was both found and fixed by AI. ## Summary Reading a `<credit-image>` threw its `valign` attribute away, so a credit-image alignment did not survive a round trip. `getImageData` in `src/private/mx/impl/PageTextFunctions.cpp` discarded it on purpose, because the generic `getPositionData` cannot read a picture's alignment: a picture uses `valign-image` (top, middle, bottom, no baseline) rather than the `valign` a text element uses, and the generic value getter is compiled out on the type mismatch, so a present attribute came back as the default `baseline`. The read now happens off the element, in one helper shared with the direction `<image>` reader: `getImageValign` in `src/private/mx/impl/PositionFunctions.h`, next to the writer's `setImageValignFromVerticalAlignment`, called from both readers so they cannot drift. `DirectionReader::parseImage` loses its own inline switch, and the `ValignImage.h` include it no longer needs. An absent attribute still reads as unspecified, and this reader never produces baseline. The writer is unchanged. The comment in `PageImageData.h` that said vertical alignment was not modeled is corrected. ## Testing - [x] New `creditRoundTrip.imageValignSurvives` fails before the fix (top, middle and bottom all read back unspecified) and passes after - [x] New `creditRoundTrip.imageValignIsReadFromXml` reads a spelled-out `valign="middle"`; fails before, passes after - [x] New `creditRoundTrip.imageValignAbsentStaysUnspecified` - [x] `make api-test` passes (5891 assertions in 660 test cases) - [x] `make api-roundtrip` passes with the newly pinned `synthetic/credit-image.3.0.xml` (415 of 415 pinned) - [x] `make api-roundtrip-discover`: 415 pass, 425 fail; before the fix 414 pass, 426 fail, credit-image.3.0.xml being the file the fix unlocked - [x] `make test-all` passes (core round trip, core unit, api-test, api-roundtrip) - [x] `make fmt` and `make fmt-check` ## References - Closes #454 - Related to #444, the writer side, merged as #451
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
Honestly, this is pretty hard to understand, but there was a bug with
valignattribute handling that was detected as a result of an audit looking for places to addDiagnosticmessages to themx::implwriter code.Summary
<image>and<credit-image>write their vertical alignment with thevalign-imagevocabulary(top, middle, bottom), not
valign.setAttributesFromPositionDataengaged the presence setter forany authored alignment, which left
valign-imageat its natural zero oftop, and its value setterwas silently compiled out because a
core::Valigndoes not convert to thestd::optional<core::ValignImage>these elements hold. So<credit-image>wrotevalign="top"forevery authored alignment, and a direction
<image>wrotetopforbaseline.A dedicated
valign-imagesetter now writes the alignment at both call sites.baselinehas novalign-imagecounterpart, so it is left out rather than mapped to something else, and that case isreported as
droppedData. The reader never sets image valign, so round-trips did not show any ofthis; the tests assert on the written XML instead.
Stacked on #450, a one-line build fix found while running these builds; this PR retargets to
mainwhen that merges.
Testing
creditRoundTrip_imageValignMiddleWritesMiddle,creditRoundTrip_imageValignBaselineOmitsAttributeandImageValignBaselineUnwritable_DirectionMarksRoundTripall fail before the fix, each writingvalign="top", and pass aftercreditImageValignBaselineIsDroppedchecks the code, severity and message of the reportmake test-all: core round-trip 841 test cases, core unit 322 assertions in 55 test casesmake fmtReferences
<image>and<credit-image>writevalign="top"for other alignments #444