fix(pptx): do not emit a heading for a slide with an empty title - #2442
Merged
Conversation
A layout's title placeholder is present on the slide whether or not anything was typed into it, so its presence says nothing about there being a title. The converter emitted "# " unconditionally, leaving a bare "#" in the output.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The OCR PPTX converter still emits bare headings and must receive the equivalent fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents empty PPTX title placeholders from producing bare Markdown headings.
Changes:
- Guards heading emission on non-whitespace title text.
- Adds regression tests for empty, whitespace-only, untouched, and populated titles.
File summaries
| File | Description |
|---|---|
test_pptx_empty_title.py |
Adds title-placeholder regression coverage. |
_pptx_converter.py |
Suppresses headings without title text. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The equivalent OCR converter still emits bare headings for empty titles.
Review details
Suppressed comments (1)
packages/markitdown/src/markitdown/converters/_pptx_converter.py:184
- This fixes only the built-in converter, but the OCR plugin registers
PptxConverterWithOCRahead of it and that converter still unconditionally emits"# " + shape.text.lstrip()for an empty title (packages/markitdown-ocr/src/markitdown_ocr/_pptx_converter_with_ocr.py:148-153). Consequently, enabling the plugin preserves the reported bare heading; please apply the same guard there and update its existing empty-title snapshot (test_pptx_multiple_images).
if text.strip():
md_content += "# " + text.lstrip() + "\n"
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
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.
Converting a deck whose slide has an empty title placeholder produces a bare
#line where the heading should be:expected the slide's text with no heading at all:
A layout's title placeholder is on the slide whether or not anything was typed into it — PowerPoint shows it as "Click to add title" — so
slide.shapes.titlereturning a shape says nothing about there being a title to read.convertemitted"# " + text.lstrip()unconditionally, so an untouched placeholder wrote"# ", and the trailing space is then removed by the output normalization, leaving#on a line of its own. Every slide built from a title layout but headed by a picture, a chart or a table carries one.This is the same shape as the notes fix in #2427: a section header written before the thing it heads is known to exist.
The fix guards the heading on the title having text, matching how the notes heading is guarded a few lines below.
Reproduction
New test file, on unmodified
main(packages/markitdown):with the failure being the heading that should not be there:
The fourth test — a title that does have text still emitting
# Quarterly Results— passes before and after.With the fix:
Verification
The existing PPTX and end-to-end vector tests are unchanged, before and after:
blackreports both touched files unchanged.