pptx: stop losing content on a customer template (#195)

[dvejsada]

Fixes #195.

A deck built on a corporate template came out badly broken. Investigating it turned up six defects, five of which failed silently — the caller got a success response and no warning. Two of them affect the templates this repo ships, not just the customer one.

Every fix is verified against the real template, and the test for each one fails on the code before it.

What was wrong, and what changed

Two-column slides lost a column and both headings

_build_two_column_slide addressed placeholders by idx, assuming PowerPoint's built-in numbering. The customer template numbered its two cards 4 and 2 — left and right in that order — and its comparison layout 4, 13, 14, 15. The right column was written into the left card, the left column and both headings were dropped, three placeholders were left showing "Click to add text", and nothing was reported. The no-headings path lost a column too.

content_columns() reads the layout's shape instead: placeholders overlapping horizontally are one column, a column's tallest is its body, a shorter box above it is the heading strip.

Nothing warned

Per the warnings rule in AGENTS.md, every shortfall now reports itself — heading_inlined (info: no heading strip, so it becomes a bold first line), columns_merged (warning: one content area, columns merged in order), column_dropped (error: nothing to write into). A test asserts the invariant directly: the content is in the file, or a warning says why not.

Autofit measured against a hardcoded 18pt

_fit_text always passed DEFAULT_BODY_FONT_SIZE. Both shipped templates set 28pt, so every estimate was low by the square of the ratio (~2.4×). Text needing 1.9× its box measured as 0.86×, no fontScale was written, and PowerPoint renders a bare <a:normAutofit/> at full size until someone clicks into it — so the text ran off the slide. The overflow warning comes from the same number, so it never fired either.

read_body_font_size() resolves the size the way PowerPoint inherits it. test_shipped_template_body_is_not_the_built_in_default pins the 28pt so the assumption cannot drift back unnoticed.

"Comparison" was decided by counting placeholders

Four content placeholders claimed ROLE_COMPARISON. The customer template spends four on three cards plus a caption bar, so every two-column slide landed on three cards. A layout now has to resolve to exactly two columns each with a heading strip; anything else returns None and is selectable by name only.

That change alone made things worse at first: with comparison unprovided the fallback was the positional index, and position 4 there is Title Only — no body placeholder at all — so the slide lost both columns rather than merely looking wrong. ROLE_ALTERNATIVES now tries a near-neighbour role the template actually has first (comparison → two_column → content), each of which can still hold the text.

Empty placeholders were left in the file

_drop_unused_placeholders() sweeps them after the build — the third card, an unused heading strip, a Section Header's body, a title slide's absent subtitle. Placeholders holding a picture, table or chart are kept; footer chrome is left for the footer pass, which runs afterwards.

Drawn text and table fills ignored the template

Text the builder draws is a plain text box, so it inherited <p:defaultTextStyle> (tx1, black) — on a dark template the KPI figures and timeline captions were black on near-black. Chart text was worse: it lives in its own part and inherits nothing. Both now take the master's body colour, kept as an MSO_THEME_COLOR so it keeps tracking the theme.

Table colours were literals (TABLE_HEADER_FILL = 4172C4), so a table came out Office blue on every template. They are now the theme names accent1 and bg2 — _set_cell_fill() already wrote a theme name as schemeClr, only the default was not one. Per-slide header_color/fills and the registry's table defaults still win, and a test pins that.

Separately: closing had no role

It was hardcoded to ROLE_TITLE, so closing slides always used the cover layout with no way to redirect them per template. ROLE_CLOSING is the first entry in CONFIGURED_ROLE_DEFAULT: a role classification never assigns, supplied by the registry's layouts: mapping. Unmapped it resolves to title without a warning — that is its documented default, not a substitution. missing_roles() and the admin's analysis both exclude such roles, or every template would be reported as lacking something no placeholder arrangement can supply.

Verification

• 2512 tests pass, up from 2407; ruff clean. Four new test files, each written to fail against the code it fixes.
• All 14 slide types build on the corrected customer template with zero warnings, checked by rendering through LibreOffice and inspecting every slide.
• On the original, uncorrected template the same deck now keeps every word, with each compromise reported — that is the check that these are code fixes and not template workarounds.

Notes for the reviewer

• Behaviour change: table headers move from a fixed blue to the template's accent1. On the shipped templates that is 156082 rather than 4172C4, so existing decks will look different — deliberately, toward the template's own palette. TABLE_HEADER_TEXT stays an explicit white; choosing it from the theme needs a luminance decision the tool cannot make safely.
• Docs are in the same commits, per AGENTS.md. The idx assumption was written down as both an invariant and a known limitation in docs/development/tools/powerpoint.md; both are replaced. AGENTS.md gains two rules, and docs/templates.md and docs/powerpoint-slides.md cover the caller-facing parts.
• History was rewritten once before this PR opened: the commits originally cited #194, which is an unrelated admin PR merged before this branch existed. No one else had the branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UiV7z58sAHrFbTr5VXF6ie

---

Generated by Claude Code

[dvejsada]

@claude review

Worth a close look at:

• content_columns() in pptx_tools/placeholder_style.py — the clustering thresholds (50% horizontal overlap for "same column", 60% height ratio for "heading strip"). Are there real template shapes these get wrong?
• _drop_unused_placeholders() — it removes any non-chrome placeholder with an empty text frame. I believe filled picture/table/chart placeholders are safe because they are no longer <p:sp> with a text frame, but that is the riskiest assumption in the PR.
• ROLE_ALTERNATIVES — whether any chain can put a slide somewhere that cannot hold its content.
• The table colour change is user-visible on existing decks (fixed blue → the template's accent1).

---

Generated by Claude Code

[claude]

Claude finished @dvejsada's task in 5m 2s —— View job

---

Review

• Gather context (PR description, changed files list)
• Read content_columns() in pptx_tools/placeholder_style.py and evaluate clustering thresholds
• Read _drop_unused_placeholders() in pptx_tools/slide_builder.py and verify the picture/table/chart-placeholder safety assumption
• Read ROLE_ALTERNATIVES in pptx_tools/constants.py/layouts.py and trace each chain
• Review the table colour change (accent1 vs fixed blue) for user-facing impact
• Post final review

Overall this is a careful, well-tested fix — I traced each of the four areas you flagged and didn't find a case where content silently disappears. Findings below, none blocking.

content_columns() (pptx_tools/placeholder_style.py:136-176)

The clustering compares each new placeholder only against column[0] — the first placeholder assigned to that column — rather than against every member already in it or a running bounding box. In principle a three-placeholder row where the middle box overlaps the third by >50% but overlaps the first by <50% would misfile it. In practice this doesn't bite you here, because the failure mode is self-limiting: classify_layout() (pptx_tools/layouts.py:236-239) requires the result to be exactly two columns with a heading each, so any degenerate clustering (extra columns, a merged caption bar, a missing heading) resolves to None rather than a wrong-but-confident match. test_three_cards_and_a_bar_is_not_a_comparison and test_four_stacked_bodies_are_not_a_comparison cover the two shapes most likely to trip this up, and both correctly return None. The one real template shape this heuristic would still miss: a genuine 2-column layout whose caption sits below each body rather than above it (above = [... ph.top < body.top ...] only looks upward) — that would classify as headingless rather than as Comparison. Worth a code comment noting the "heading is always above" assumption, but not a bug since the fallback (None → resolved by explicit layout name or ROLE_TWO_COLUMN) is safe.

_drop_unused_placeholders() (pptx_tools/slide_builder.py:346-371)

The picture case is verified: insert_picture() replaces the placeholder's <p:sp> with a <p:pic> element in place, and Picture/GraphicFrame don't override has_text_frame (default False in python-pptx), so a filled picture placeholder is correctly skipped — and it's the one case with a dedicated test (test_a_filled_picture_placeholder_survives in tests/test_pptx_unused_placeholders.py). Table and chart placeholders, though, are never actually filled via placeholder.insert_table()/insert_chart() anywhere in this codebase — _add_title_content_slide (pptx_tools/helpers.py:392-405) always removes the content placeholder outright and adds a fresh table/chart shape at its rectangle. So the "holding a picture, table or chart" docstring language is a little broader than what's exercised; it's not incorrect (nothing currently could leave a filled table/chart placeholder behind), just worth tightening if a future change starts inserting into placeholders directly.

ROLE_ALTERNATIVES (pptx_tools/layouts.py:98-108)

Traced all nine chains. None land content somewhere it can't go, because title and content placement are decoupled from whichever layout the slide actually resolves to: title_style()/content_area() are resolved once per template from whichever layout best answers the question (pptx_tools/layouts.py:400-444), not from the slide's own layout. So even ROLE_TITLE_ONLY → (ROLE_CONTENT, ROLE_BLANK) landing all the way on a Blank layout still draws a title and (for kpi/timeline) a body rectangle correctly. The one place content really can be lost is _build_content_slide/_build_agenda_slide/_build_two_column_slide exhausting their alternatives down to a layout with no body placeholder at all — but that's the explicitly-designed last resort, and it's always reported (BULLETS_DROPPED/AGENDA_DROPPED/COLUMN_DROPPED), matching the PR's stated invariant rather than violating it.

Table colour (pptx_tools/constants.py:109-120)

The accent1/bg2/white-text combination is well-tested (test_table_header_uses_the_template_accent, test_an_explicit_header_colour_still_wins) and the user-visible behaviour change is already called out plainly in the PR description, so no surprise there. One thing to flag for awareness rather than fix: TABLE_ALT_ROW_FILL = "bg2" for zebra striping has the same theoretical luminance risk you already called out for TABLE_HEADER_TEXT — on a template whose bg2 sits close in luminance to the table's own base row fill, the zebra stripe could read as barely-there. You've clearly already accepted that class of risk as unavoidable without a luminance heuristic; I'd only suggest a one-line note next to TABLE_ALT_ROW_FILL making that trade-off explicit the way TABLE_HEADER_TEXT's comment already does, so a future reader doesn't have to rediscover it.

No blocking issues found. Nice test coverage — the two-column and unused-placeholder test files in particular exercise exactly the pathological template shapes described in the PR body.

[dvejsada]

Thanks — all four addressed in e8247b4. One turned out to be a real fix rather than a comment.

Clustering against column[0] — fixed, with a test that fails on the old code.

You were right that it's a live gap, but the self-limiting argument only covers the classification path. classify_layout() is indeed safe, because it demands exactly two columns with a heading each. But _build_two_column_slide() also calls content_columns() — on whatever layout the slide actually landed on, which may never have been vetted by classify_layout() at all (an explicit layout: override, or a layout resolved for ROLE_TWO_COLUMN). It then fills columns[0] and columns[1]. A spurious column there puts content in the wrong box, silently, which is the exact failure this PR exists to remove.

The reproducer is a column headed by a narrow strip: it pins the column's identity to the heading's width, so a box sitting inside the wider body below it starts a third column.

(1.0, 1.5, 2.0, 0.5)   narrow heading, spans 1.0–3.0
(1.0, 2.2, 5.0, 3.5)   its body, wider, spans 1.0–6.0
(3.5, 2.2, 2.5, 3.5)   inside the body's span, outside the heading's
(7.5, 2.2, 4.0, 3.5)   the genuine second column

Before: three columns ([(1.0, 5.0), (3.5, 2.5), (7.5, 4.0)]). After: two. Each column now carries a running span that widens as boxes join it. test_a_column_is_matched_on_its_whole_extent_not_its_first_box covers it.

Heading is only looked for above the body — documented in content_columns(), including what happens instead: a below-captioned layout reads as headingless, the captions become body text, and a slide that supplied its own headings gets heading_inlined. I left the behaviour alone deliberately — looking downward as well would risk reading a genuine caption as a heading, and the headingless path already keeps every word.

_drop_unused_placeholders() docstring — tightened. It now describes the actual test (has_text_frame plus empty), names the picture path as the only one that produces a graphic-holding placeholder today, notes that _add_title_content_slide() removes the placeholder outright for tables and charts, and says why a future insert-into-placeholder change would be safe for the same reason.

TABLE_ALT_ROW_FILL luminance caveat — added next to it, matching the note TABLE_HEADER_TEXT already carries.

On ROLE_ALTERNATIVES: agreed, and your reasoning matches mine — title_style() and content_area() resolve per template rather than per slide, so the chains cannot strand a title or a body. The genuine last resort (a layout with no body placeholder at all) is always reported.

2513 tests pass; ruff clean. Also re-verified against the real customer template: the corrected one still resolves every role and builds the original deck with zero warnings, and the uncorrected one still keeps every word with each compromise reported.

---

Generated by Claude Code