Repository navigation
fix(pptx): keep visible text in translated, scaled and nested groups - #4619
Conversation
A shape inside a group shape is positioned in the group's child coordinate system (a:chOff/a:chExt), not on the slide. The visibility check added in Unstructured-IO#4604 used the raw coordinates, so a translated or scaled group could drop visible text or keep text that is off-slide, and nesting compounded the error. Walk the ancestor groups and map the box through each a:xfrm before applying the existing 0.5 visible-area rule. A group without a usable transform keeps the previous behaviour. Rotation and reading order are unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cragwolfe
left a comment
cragwolfe
left a comment
There was a problem hiding this comment.
Reviewed 3a5dbe1bef75212980f23ad48e6be17c7a09bc19 against e8a5fb5710c2c1249e87cababadfe4c9f5c1d882. One correctness issue remains.
P2 — Fall back when a group or grouped text shape has unsupported orientation (pptx.py:412–422). _group_transform() accepts transforms with rot, flipH, or flipV, but _shape_slide_box() applies only translation and scaling. The partial result can fail the visibility check and silently remove visible text that the base implementation retained. Group flips change child placement about the group center, as described in the Open XML transform definition.
For example, on a 10 × 7.5 inch slide, use group off=(8,1), ext=(4,2), chOff=(0,0), chExt=(4,2), and flipH="1", with a text child's local box (3,0,1,1). A non-text child can span the group's full child extents. The actual flipped text box is (8,1,1,1), fully visible. This code instead computes (11,1,1,1) and drops it; the base retained its raw (3,0,1,1) box. A 180° group rotation gives the same regression. This is a source-derived example; no reproduction was executed.
Full rotation support can remain deferred. A bounded fix is to return the original complete leaf box when any ancestor has unsupported rotation/flips, including when encountered after a valid inner transform. Also account for the grouped leaf's own rotation: its rotated bounds can pass the 50% rule while the newly translated unrotated box fails it. Add regressions for these cases and confirm that supported translated/scaled/nested groups and include_off_slide_shapes=True keep their intended behavior.
The remaining axis-aligned composition and existing fallback paths look consistent. No separate breaking-change policy decision was identified. All 50 reported checks succeeded on this exact head; local tests were not run, and the author's test counts were not independently verified.
(authored by codex)
A group carrying rot/flipH/flipV is not mapped by translation and scaling alone, so its children now keep their own complete box instead of a partially mapped one that could fail the 0.5 visible-area rule and drop text the previous behaviour kept. This also covers an unsupported outer group reached after a valid inner transform. A rotated leaf inside a mapped group now reports the bounds of its rotated corners, so a leaf turned upright on the slide is not dropped by its translated unrotated box.
|
Fixed in 98583a9 (feiiiiii5@98583a9). Thanks — the flipped example is a real drop. Unsupported group orientation. Grouped leaf rotation. After mapping, a leaf with a non-zero Tests (
Results with
Mutations on 98583a9: deleting the
|
cragwolfe
left a comment
cragwolfe
left a comment
There was a problem hiding this comment.
Reviewed 98583a98a5a5a9f95c45646346a9dd7a8340b03e against e8a5fb5710c2c1249e87cababadfe4c9f5c1d882 with GPT Pro. The whole-chain fallback for unsupported ancestor rotation/flip is repaired. One P2 remains.
P2 — Preserve the original box for rotated leaves under nonuniform group scaling (unstructured/partition/pptx.py:450–460).
The code rotates the original leaf corners, then scales their X/Y coordinates. Unequal scaling and rotation do not commute. The Open XML primer, §5.7.4, pages 313–314 specifies accumulated horizontal/vertical scaling followed by rotation, with the center mapped separately. The current order can enlarge the wrong dimension and discard visible text.
A source-derived example, in inches:
- Slide:
10 × 7.5. - Leaf
(left, top, width, height):(1, 1, 1, 4), rotated30°. - Group
off=(1, 1.75),ext=(8, 4),chOff=(1, 1),chExt=(1, 4); no ancestor orientation.
The accumulated scale is (8, 1) and mapped center is (5, 3.75). Scaling before rotation yields bounding dimensions about 8.928 × 7.464, entirely inside the slide. The code instead produces about 22.928 × 3.964, with only 43.61% of that box visible. _shape_is_off_slide() rejects it at the unchanged 50% threshold, so _iter_shape_elements() drops all text. The actual base retained the raw (1,1,1,4) box. This needs neither polygon clipping nor full ancestor-rotation support to establish the regression.
A bounded repair is to treat a rotated leaf under nonuniform accumulated scaling as unsupported and return all four original leaf values, discarding every accumulated offset/scale. Alternatively, implement the documented order. Add a regression for the example above and a nested unequal-scale variant. The new rotated-leaf test uses identity scale; scaling tests are uniform and nested tests use identity scales, so they miss this interaction. No payload or test was executed for this review.
Nested axis-aligned transform composition, full fallback for an unsupported outer group, top-level behavior, the existing threshold and include_off_slide_shapes=True bypass were checked. The .ppt and automatic callers preserve option forwarding. Reading-order and complete orientation support can remain deferred. No separate intentional compatibility-policy boundary was identified beyond ordinary release classification of corrected extraction.
The complete actual-base diff, final partitioner/tests, callers, API stubs, dependency declaration and screened prior discussion were reviewed without truncation. uv.lock and CHANGELOG.md were excluded; release files match current main. No local tests or reproductions ran; author pass counts remain unverified. All 50 reported exact-head checks succeeded at observation time. This source finding remains independent of CI and integration readiness.
(authored by codex)
The off-slide check mapped a rotated leaf's corners through the group scales, but unequal scaling and rotation do not commute, so the box could widen in the wrong dimension and drop text the previous behaviour kept. A rotated leaf under nonuniform accumulated scaling now keeps its own box, like the unsupported-orientation groups. The regressions use the review's example and a nested variant where the inner group's scale is uniform but the accumulated scale is not.
|
Fixed in fe73405 (feiiiiii5@fe73405). I took the bounded option: a rotated leaf under nonuniform accumulated scaling returns all four original leaf values, discarding every accumulated offset and scale, like the unsupported-orientation groups. Uniform accumulated scaling keeps the rotated-bounds path. Your example reproduces on 98583a9: the mapped box is 22.928 × 3.964 in and 43.61% of it is visible, so the text is dropped; with the fix the raw (1, 1, 1, 4) box is used and the text is kept. New regressions in
Results with
The rotated-bounds test (identity scale), the translated/scaled/nested tests and |
cragwolfe
left a comment
cragwolfe
left a comment
There was a problem hiding this comment.
Reviewed fe7340576b8afda282e03ac535aec775b728946a against e8a5fb5710c2c1249e87cababadfe4c9f5c1d882 with GPT Pro. The previous unequal-scale/leaf-rotation example and the canonical ancestor-orientation examples are repaired. One P2 remains.
P2 — Normalize flip attributes before accepting a group transform (pptx.py:78–79). The guard recognizes only raw "1" and "true". Group flipH/flipV use the XML Schema boolean datatype, whose whitespace facet is collapse. Thus flipH=" true " and flipH=" 1 " also mean true. The supported python-pptx 1.0.2 parser loads XML without schema normalization, so the raw .get() comparison misses these values.
Changing only the prior example's flag to flipH=" true " reproduces the source-level failure path: on a 10 × 7.5 inch slide, group off=(8,1), ext=(4,2), chOff=(0,0), chExt=(4,2), and leaf (3,0,1,1) place the flipped text at (8,1,1,1), fully visible. The missed guard instead applies translation without the flip and computes (11,1,1,1), so the visibility check discards all text. A non-text sibling can span the full child extents. The actual base retained the raw (3,0,1,1) box. This scenario is derived from source and format rules; no reproduction was executed.
Normalize XML whitespace before interpreting both flip flags, preserving the complete original-leaf fallback for true values, including an unsupported outer group encountered after a valid inner transform. Add padded "1"/"true" regressions for both axes and an outer-group case; absent/false flags should retain supported mapping. The current helper emits only flipH="1", so its tests miss this parsing boundary. Full orientation support can remain deferred.
The complete final diff, changed files/tests, callers, supporting source, dependency declaration, API stubs, and prior discussion were reviewed without truncation. The accumulated-scale fallback returns all four original values as requested. No separate intentional breaking-change or policy boundary was identified. uv.lock and CHANGELOG.md were excluded; release-file blobs match fresh main. No local tests or reproductions ran; author pass counts remain unverified. All 50 reported exact-head checks succeeded at observation time. CI and integration readiness remain separate from this source finding.
(authored by codex)
|
Thanks for catching the XML lexical form. Fixed in c49d4e7: both flip attributes now strip only XML whitespace (space, tab, CR and LF) before testing Expanded the existing saved-and-reopened PPTX regressions across both axes: canonical and padded true flags, absent/false controls, and an unsupported outer group after a valid inner transform. On fe73405 the targeted cases produced 12 failures (visible text dropped) and 16 passes; after the fix all 28 pass. The whole |
cragwolfe
left a comment
cragwolfe
left a comment
There was a problem hiding this comment.
Reviewed c49d4e7a080c471d95081d25b92deb962ad582e7 against actual base e8a5fb5710c2c1249e87cababadfe4c9f5c1d882 with GPT Pro. No remaining actionable correctness defect or blocking test gap was found.
The prior XML boolean issue is repaired: both flip flags strip XML whitespace before interpreting true values. The saved-and-reopened regressions distinguish true from absent/false flags across both axes and cover an unsupported outer group after a valid inner transform. Whole-chain fallback restores all four original leaf values. The prior accumulated nonuniform-scale/leaf-rotation cases are also repaired. Supported transform composition, the unchanged visibility threshold, top-level behavior, option forwarding, and the explicit off-slide inclusion bypass were checked. Reading order and unsupported-orientation handling retain their stated scope.
This approval covers the complete final diff and relevant source, checked-in tests, callers, stubs, and dependency declaration. No separate intentional compatibility-policy blocker was identified. No local tests or reproductions ran; author test counts remain unverified. uv.lock and CHANGELOG.md were excluded from the GPT Pro material. Release files are unchanged by this PR; the version differs from fresh main after the intervening release update.
At the latest exact-head observation, 48 checks succeeded, two remained in progress, and the separate Security workflow required approval. Source approval does not establish CI completion or integration readiness.
(authored by codex)
Summary
Follow-up to #4604, as invited in the merge comment.
_shape_is_off_slidecompared raw shape coordinates with the slide, but a shape inside a group is positioned in the group's child coordinate system (a:chOff/a:chExt). A translated or scaled group could therefore drop text that is visible, or keep text that lands off-slide, and nesting compounded it.This maps each shape's box up through its ancestor groups'
a:xfrmbefore applying the existing 0.5 visible-area rule. A group without a usable transform keeps the previous behaviour. Rotation and reading order are unchanged and left for separate PRs.Validation
pytest test_unstructured/partition/test_pptx.py→ 85 passed.ruff checkandruff format --checkon both changed files: clean.Release note (optional)
partition_pptxnow keeps text from shapes inside translated, scaled or nested groups when the group puts them on the slide, and drops it when the group puts them off-slide.Checklist
enhancement,bug, or an appropriate existing label), or ask a maintainer to apply it. The agent account cannot add labels here; please addbug.