fix: preserve background color for odt - #122
Conversation
f3a2319 to
641b729
Compare
chrip
left a comment
There was a problem hiding this comment.
Overview
When converting ODT → DOCX, office_body::docx_convert only inspected the page layout tied to the default page properties (Context.get_page_properties()) for fo:background-color. If the background was defined on a non-default master page's layout, it was dropped. This PR keeps the default-layout lookup as the primary path and, when that layout has no fo_background_color_, iterates over all master pages, resolves each one's page layout, and uses the first layout that carries a background color. The serialization logic itself is unchanged — it's just been re-homed under the new backgroundLayout selection. The change is well null-guarded and the fix direction is sound.
Correctness
- Lookup chain is correct.
page_layout_name_by_style(masterPageStyle)→page_layout_by_name(layoutName)mirrors the resolution used elsewhere (office_text.cpp,docx_conversion_context.cpp), and the null checks (pl && pl->properties() && …) are thorough. The default layout is retained asbackgroundLayoutwhen the fallback finds nothing, so the pre-existing default-page behavior is preserved. ⚠️ Image/graphic-fill backgrounds on non-default master pages are still ignored. The fallback loop only tests…common_background_color_attlist_.fo_background_color_. But the serialization block downstream honors both a color and a graphic fill (fill.type != 0— background images viaCompute_GraphicFill). So a non-default master page whose layout has only a background image still gets silently dropped — the exact class of bug this PR fixes, just for the image case. Given the PR title is specifically about color this may be acceptable scope, but the asymmetry is worth a comment or a follow-up note.⚠️ Possible regression: color on another page can override the default page's image. The fallback is entered whenever the default layout lacksfo_background_color_— even if the default layout has a valid background image. If some other master page then has a color,backgroundLayoutis reassigned to it and the default page's image is lost (previously it would have been serialized). Narrow edge case, but a genuine behavior change. Consider only entering the fallback when the default layout has neither a color nor a fill, or checking the fill attlist presence before overriding.- Heuristic "first match wins." Among multiple master pages with different colors, insertion order decides the winner. DOCX only supports a single document-wide
w:background, so some lossiness is unavoidable, but the choice is arbitrary — a doc with a colored cover page + white body could pick the wrong one. Worth a one-line comment explaining the DOCX single-background limitation so the heuristic doesn't read as accidental.
Code quality / conventions
- Reuse the existing helper.
page_layout_container::page_layout_by_style(StyleName)(odfcontext.cpp:491) already does exactly the two-stepname_by_style+by_namelookup.properties()is aconstmethod returning a non-constpointer, so you can declareconst page_layout_instance * backgroundLayoutand collapse the loop body toconst page_layout_instance * pl = pgContainer.page_layout_by_style(mpName);— dropping the intermediatelpName/mpNamedance. Everything downstream (properties(),docx_background_serialize) still works through the const pointer. - Minor duplication.
backgroundLayout->properties()and…fo_background_color_are evaluated in the entry condition and re-fetched in the final block. Not a correctness issue (properties()is cheap), just slightly repetitive. - Whitespace nits. The reworked
//background (for all pages)comment switched from tab to space indentation, breaking with the surrounding tab-indented block, and there's a blank line with trailing tabs after thebackgroundLayoutdeclaration. Removing the oldfirtsPageLayouttypo is a nice incidental cleanup.
Performance
Negligible. The fallback is O(number of master pages) with map lookups, runs once per document, and only when the default layout has no background color.
Test coverage
- No automated test or fixture added — verification is "manually tested end-to-end and with x2t." These conversions are hard to unit-test, but if the repo has a document round-trip / regression corpus, adding a small ODT with a background color on a non-default master page (and ideally one on an image) would lock in the fix and guard the override edge case above.
Security
No concerns. No new external input, allocations are guarded, and the media add_or_find path is unchanged from the original.
Summary
Solid, well-guarded fix for a real gap. Before merge I'd suggest: (1) confirm the image-only override edge case (color on another master page clobbering the default page's image) is acceptable or guard against it; (2) optionally extend the fallback to graphic fills for symmetry, or note it as out-of-scope; (3) simplify via the existing page_layout_by_style helper; (4) a one-line comment on the single-background heuristic. None are blocking on their own.
🤖 Draft review generated with Claude Code
Aiiaiiio
left a comment
There was a problem hiding this comment.
I can barely follow the logic. Probably because I don't know the code nor the formats (so that part is lgtm).
I added a few notes that are subjective, up to you.
| layout_properties->style_background_image_, Context.root(), fill); | ||
|
|
||
| if (layout_properties->attlist_.common_background_color_attlist_.fo_background_color_ || fill.type != 0) | ||
| std::vector<style_master_page*> & masterPagesAll = pgContainer.master_pages(); |
There was a problem hiding this comment.
I think this could and should be const:
| std::vector<style_master_page*> & masterPagesAll = pgContainer.master_pages(); | |
| const std::vector<style_master_page*> & masterPagesAll = pgContainer.master_pages(); |
| for (size_t i = 0; i < masterPagesAll.size(); ++i) | ||
| { | ||
| if ((fill.bitmap) && (fill.bitmap->rId.empty())) | ||
| if (!masterPagesAll[i]) continue; |
There was a problem hiding this comment.
This is a matter of preference, but omitting braces can lead to so many hard-to-find issues that it makes me uneasy to see this. I recommend adding braces. At the end of the day, your choice.
| if (!masterPagesAll[i]) continue; | |
| if (!masterPagesAll[i]){ continue; } |
Signed-off-by: dsyzov <dmytro.syzov@nextcloud.com>
641b729 to
0e37280
Compare
Summary
When reading an ODT, the code only checked the default page layout for fo:background-color. If the background was defined on a non-default master page it was silently ignored and nothing got written to the DOCY binary.
With this change it iterates over all master pages until one with fo_background_color_ is found and use that.
Test
Manually tested both end-to-end and with x2t only.
Notes
Assisted-by: ClaudeCode:claude-sonnet-4-6