Repository navigation
fix(quick): read the deck language and theme fonts from the roster's first page - #292
Conversation
…first page
Three probes that mean "the first svg_output page" picked it with
sorted(glob('*.svg')), which is filename order, while the pages that get
exported are ordered by discover_slide_svgs, which reads the numeric
segments. On a roster that is numbered but not zero padded, 10_end.svg
sorts ahead of 1_cover.svg, so the exporter drew slide 1 from the cover
while the language probe, the theme-font probe and the Quick advisory all
read the last page.
All three now call discover_slide_svgs, the helper the exporter and the
rest of the checker already use. A zero-padded roster of NN_name.svg
pages is unaffected, because numeric order and filename order agree
there. Return values and the parse-error paths are untouched.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Thanks, @MaxFreedomPollard — another careful catch, and the write-up again made it fast to verify. I reproduced the three probes reading Merged as 76db071, diff exactly as submitted. That review did turn up three pre-existing siblings outside your scope, which I am fixing separately: |
…r; Quick deck-language advisory tolerates an empty roster Follow-ups to #292, found by an adversarial review of that change. template_preview_pptx.py and register_template.py::_list_pages still picked pages by filename order, so an unpadded Layout roster (1_cover.svg ... 13_closing.svg) previewed with process_timeline as slide 1 and failed page_types validation against a roster that started at the end. Both now use discover_slide_svgs, the roster-order authority the exporter already uses; zero-padded rosters are unchanged. The Quick advisory in checker.py indexed [0] of an unguarded roster, so a single-file check of an uppercase .SVG (not matched by *.svg) failed with "list index out of range"; the roster is now only read in Quick mode and an empty roster skips the advisory. Verified on an unpadded copy of report_core: registration passes and the preview's slide 1 is the Cover layout (main: rejected / Process Timeline); cover.SVG passes the Quick check. Template flow batch 1/3/4, apply-template, checker and roster first-page tests pass. Co-Authored-By: Claude Fable 5.1 <[email protected]>
What & why
_svg_root_language()inskills/ppt-master/scripts/svg_to_pptx/pptx_package/cli.pypromises in its docstring to return thelang/xml:langthat "the first svg_output page" declares, then picks that page withpages = sorted((project_path / 'svg_output').glob('*.svg'))(line 1644). That is filename order. The pages that actually become slides are ordered bydiscover_slide_svgs, reached throughsvg_to_pptx/pptx_package/discovery.py:58, which sorts the numeric filename segments by value. The two orders agree only while every page number has the same digit count. On a ten page roster numbered1_cover.svgthrough10_end.svg,10_end.svgsorts first, because'0'is below'_'in code point order. The export then builds slide 1 from the cover and reads the deck language from the last page.Two more places make the same pick from the same directory.
load_theme_font_spec_from_pages()insvg_to_pptx/drawingml/theme_fonts.py:218derives the major and minor theme faces from what its docstring also calls the first svg_output page. The Quick deck language advisory insvg_quality/checker.py:1317decides which page to complain about by comparingsvg_path.nameagainstsorted(p.name for p in svg_path.parent.glob('*.svg'))[0]. That last one makes the module disagree with itself:checker.pyalready importsdiscover_slide_svgsat line 34 and uses it at 6788, 6992, 6997, 7265 and 7887.On an unpadded roster whose cover carries
lang="ja-JP"exactly as the checker's own advisory instructs, all three go wrong at once. The checker prints "Quick roster declares no deck language" against10_end.svg, and stays silent when page 1 really is missing its declaration. The export prints "no deck language declared ... using legacy per-run language detection". The theme falls back to Calibri with an empty East Asian face instead of the cover's Cover Title / Yu Gothic / Cover Body. Zero padding the same ten files, with byte identical page content, fixes all three, which is what shows the filename and not the content is deciding.The fix is to use the helper the rest of the pipeline already uses, in all three places. Three lines plus two imports, no new logic. The commit just before this one, c6ae489, changed how the first page is opened in
_svg_root_language(); this one changes which page is opened. Slide order was already correct and does not move. Return values, theif not pagesguard, theET.ParseErrorpaths and the advisory wording are untouched, and a zero padded roster ofNN_name.svgpages is a no-op, because numeric order and filename order agree there. Not every directory is: a stray file sorting ahead of a digit, say#ref.svg, is page one by filename order but not by numeric order, so the branch stops picking it, which is the direction the docstrings ask for but still a change. The change is code-only: three modules and one new test module, no prompt or instruction text.Two honest limits. First, the authoring text does ask for one roster wide zero padded width (
references/executor-base.md:331andworkflows/profiles/quick-generate.md:180), so a fully conforming Quick deck is unaffected. The bug bites rosters that are numbered but not padded, which is whatslide_roster.pyexists to order, whichproject_utils.validate_project_structureaccepts without a word (^(slide_\d+_\w+|P?\d+_.+)\.svg$, line 547, and the page number read at line 430 isre.match(r'(\d+)')), and which arrive from hand authored decks, imported decks and decks renumbered after a page is dropped. Second, what the lost declaration costs depends on the text. Legacy per-run detection looks at the runs, so a cover carrying kana still comes outja-JPand only the theme faces are wrong; a Han only Japanese cover comes outzh-CNand PowerPoint proofs slide 1 as Chinese. I built both and saw both.Verification
Environment: macOS 26.2, Python 3.11.9. No packages were installed for this change; the touched modules and the new test import only the standard library and the repo's own helpers, and the export run below uses the python-pptx 1.0.2 already here. Eight of the nineteen
requirements.txtpackages are absent on this machine, which is what the skipped test modules at the end of this section are about.Real project run. Two ten page Quick projects with byte identical page content, confirmed by comparing the pages pairwise, one numbered
1_cover.svg...10_end.svgand the other01_cover.svg...10_end.svg. The cover carrieslang="ja-JP",font-family="Cover Body"and a 72px<text font-family="Cover Title">; pages 2 to 10 carry no lang. From the repo root:On
mainthe checker attaches[WARN] Quick roster declares no deck languageto10_end.svg, and the export opens withWarning: no deck language declared (root <svg lang="..."> on the first page, or --primary-language); using legacy per-run language detection.Reading the two packages back (the cover text in this fixture is Han-only, so legacy per-run detection tags itzh-CN):With this branch the unpadded run prints no warning,
grep -c 'Quick roster declares no deck language'over the checker output is 0, and the package readsmajor Cover Title / Yu Gothic, minor Cover Body, slide1 run langs ['ja-JP'], identical to the padded control. Blanking the cover'slangputs the advisory on1_cover.svg, where it belongs. Slide order was right on both sides: slide 1 is the cover, slide 10 the end page.The theme probe on its own. In the roster above the language probe fails too, so the export skips the page derived faces altogether and lands on Calibri, which hides
theme_fonts.pypicking a page. A second ten page unpadded roster carryinglang="ja-JP"on every page separates them, since the language is found whichever page is read. Exporting that one directory withmainand then with this branch:mainwrites theme majorEnd Titleand minorEnd Body, this branch writesCover TitleandCover Body, both with theYu GothicEast Asian face, and both give slide 1 from1_cover.svgwith run langs['ja-JP'].No-op check. One padded ten page project, exported into the same project directory with
mainand then with this branch: 36 package parts on each side, same names, 35 byte identical. The only part that differs isdocProps/core.xml, and the only difference in it is one second of export timestamp,dcterms:createdanddcterms:modifiedreading2026-09-15T23:16:08Zagainst2026-09-15T23:16:09Z.Unit test, new module
skills/ppt-master/scripts/tests/test_slide_roster_first_page.py, stdlib unittest, fixtures built inside the module, no network:Against the three files as they stand on
main:FAILED (failures=3), reportingAssertionError: None != 'ja-JP',AssertionError: 'End Title' != 'Cover Title', and the advisory missing from1_cover.svg. On this branch:Ran 5 tests ... OK. The two padded cases pass either way and are the no-op guard. The language assertion calls_declared_primary_language, which is underscore private; I kept it becausetests/test_dogfood_quick_language_fixes.pyalready pins that same function next toload_theme_font_spec_from_pages, and it is the narrowest place to see which page was chosen.Full suite, one module at a time, before and after: 29 of 35 modules pass on
main, 30 of 36 on this branch, and the same six fail on both sides. None of the six touch this code, and both causes are local to this machine rather than to the repo.test_confirm_ui_design_spec_depth,test_local_server_security,test_preview_browserandtest_svg_editor_serverstop atModuleNotFoundError: No module named 'flask'.test_source_to_md_clifails 17 times on the macOS tempfile path difference, every one of themLists differ: [PosixPath('/private/var/folders/...')] != [PosixPath('/var/folders/...')], becausetempfile.mkdtemp()hands back the/varspelling and the code resolves the/private/varone.test_audit_r3_sources(1 failure, 10 errors) is a separate cause:source_to_md/doc_to_md.pyexits 1 with[ERROR] mammoth not installed, so those tests find noreport.md.Also clean:
python3 -m py_compileon the three changed modules and the new test, andgit diff --check. The repo ships no linter config, so this is only a side note, but a locally installedruffreports the same 22 pre-existing errors onmainand on this branch, and none in the new test module or intheme_fonts.py.Confirmations
All three of the following must be checked. If any one is left unchecked, the PR is closed without review.
main(purely AI-generated PRs without human review are closed unmerged)SKILL.md,references/*.md,workflows/*.md, and so on) and was discussed and agreed in an issue first, link it here: code-only