Skip to content

fix: preserve background color for odt - #122

Merged
DmySyz merged 1 commit into
mainfrom
fix/background-color-odf
Aug 3, 2026
Merged

fix: preserve background color for odt#122
DmySyz merged 1 commit into
mainfrom
fix/background-color-odf

Conversation

@DmySyz

@DmySyz DmySyz commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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

@DmySyz
DmySyz requested a review from a team as a code owner July 15, 2026 15:43
@DmySyz
DmySyz requested review from Aiiaiiio and chrip and removed request for a team July 15, 2026 15:43
@DmySyz
DmySyz force-pushed the fix/background-color-odf branch 2 times, most recently from f3a2319 to 641b729 Compare July 15, 2026 15:49

@chrip chrip left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 as backgroundLayout when 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 via Compute_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 lacks fo_background_color_even if the default layout has a valid background image. If some other master page then has a color, backgroundLayout is 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-step name_by_style + by_name lookup. properties() is a const method returning a non-const pointer, so you can declare const page_layout_instance * backgroundLayout and collapse the loop body to const page_layout_instance * pl = pgContainer.page_layout_by_style(mpName); — dropping the intermediate lpName/mpName dance. 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 the backgroundLayout declaration. Removing the old firtsPageLayout typo 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 Aiiaiiio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could and should be const:

Suggested change
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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (!masterPagesAll[i]) continue;
if (!masterPagesAll[i]){ continue; }

Signed-off-by: dsyzov <dmytro.syzov@nextcloud.com>
@DmySyz
DmySyz force-pushed the fix/background-color-odf branch from 641b729 to 0e37280 Compare August 3, 2026 08:52
@DmySyz
DmySyz merged commit 0c00321 into main Aug 3, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants