Skip to content

Commit 35f592e

Browse files
committed
fix(oxml): scope CT_GroupShape.max_shape_id to spTree descendants
The xpath `//@id` in `CT_GroupShape.max_shape_id` is absolute — it scopes to the whole document, not the spTree subtree. On slide masters, that pulls in the sentinel ids on `<p:sldLayoutIdLst>/<p:sldLayoutId>` which start at 2147483649 (PowerPoint's authoring convention for layout-master pointer ids — a deliberately-segregated namespace). The next-shape-id calculation `max_shape_id + 1` then returns a value just above `xs:int` (2147483660). PowerPoint quarantines shapes with ids in that range on open, surfacing as a 'Repair' dialog that removes the offending shape and synthesizes replacement parts. The latent bug was harmless until Phase 5 (#52) introduced `MasterShapes.add_textbox` and `SlideMaster.add_text_watermark` — the first public API that *writes* a shape to a slide master. Reads off masters didn't exercise `_next_shape_id`. Fix: change `self.xpath('//@id')` to `self.xpath('.//@id')`. `.//` is the descendant-or-self axis relative to the context node — scopes the search to the shape tree only, leaving sibling-element ids (`p:sldLayoutId`, etc.) outside the max. Tests: - `it_returns_zero_for_max_shape_id_on_an_empty_spTree` — baseline. - `it_finds_the_max_id_among_shape_descendants` — sanity, picks max=42 across three sp ids. - `but_it_ignores_sibling_ids_outside_the_shape_tree` — regression pin: `<p:sldLayoutIdLst>` siblings with ids in the 2147483649+ range do NOT inflate the result. spTree.max_shape_id stays at 2. Verification (local, CPython 3.14.4): - python3 -m pytest tests/ -q → 3654 passed (+3 vs Phase 5 baseline) - python3 -m ruff check src tests → All checks passed! - python3 -m ruff format --check src tests → 216 files already formatted - python3 -m behave features/ --no-color → 1048 scenarios, 0 failed - python3 uat/uat_headers_footers_phase5.py → script runs clean; watermark shape_id=7 (was 2147483660 pre-fix). UAT signoff is maintainer-only per CLAUDE.md §6a — please re-open the regenerated uat/out_headers_footers_phase5.pptx in PowerPoint to confirm. Refs #52.
1 parent dba6ca4 commit 35f592e

2 files changed

Lines changed: 52 additions & 9 deletions

File tree

src/pptx/oxml/shapes/groupshape.py

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -149,16 +149,24 @@ def iter_shape_elms(self) -> Iterator[ShapeElement]:
149149

150150
@property
151151
def max_shape_id(self) -> int:
152-
"""Maximum int value assigned as @id in this slide.
153-
154-
This is generally a shape-id, but ids can be assigned to other
155-
objects so we just check all @id values anywhere in the document
156-
(XML id-values have document scope).
157-
158-
In practice, its minimum value is 1 because the spTree element itself
159-
is always assigned id="1".
152+
"""Maximum int value assigned as @id within this shape tree.
153+
154+
This is the maximum @id value on any descendant of this `<p:spTree>` (or
155+
`<p:grpSp>`) element. Scoped strictly to the shape tree because OOXML
156+
uses sibling element-types (notably `<p:sldLayoutIdLst>/<p:sldLayoutId>`
157+
on a slide master) whose `@id` values live in a deliberately-segregated
158+
namespace starting at 2147483649. Pulling those values into this max
159+
would push the next shape id beyond `xs:int`, which PowerPoint quarantines
160+
on open ("Repair", with the offending shape removed).
161+
162+
In practice the minimum return is 1 because the spTree element itself is
163+
always assigned `id="1"`.
160164
"""
161-
id_str_lst = self.xpath("//@id")
165+
# ---`.//@id` (not `//@id`) — `//` is absolute in XPath and scopes to the
166+
# whole document, picking up sibling-element ids that share neither the
167+
# semantic namespace nor the id range with shape ids. See the docstring
168+
# for the failure mode that surfaced via SlideMaster.add_text_watermark.
169+
id_str_lst = self.xpath(".//@id")
162170
used_ids = [int(id_str) for id_str in id_str_lst if id_str.isdigit()]
163171
return max(used_ids) if used_ids else 0
164172

tests/oxml/shapes/test_groupshape.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,41 @@ def it_calculates_its_child_extents_to_help(self, child_exts_fixture):
8888
x, y, cx, cy = xSp._child_extents
8989
assert (x, y, cx, cy) == expected_values
9090

91+
def it_returns_zero_for_max_shape_id_on_an_empty_spTree(self):
92+
spTree = element("p:spTree")
93+
assert spTree.max_shape_id == 0
94+
95+
def it_finds_the_max_id_among_shape_descendants(self):
96+
spTree = element(
97+
"p:spTree/(p:nvGrpSpPr/p:cNvPr{id=1,name=root}"
98+
",p:sp/p:nvSpPr/p:cNvPr{id=2,name=a}"
99+
",p:sp/p:nvSpPr/p:cNvPr{id=42,name=b}"
100+
",p:sp/p:nvSpPr/p:cNvPr{id=7,name=c})"
101+
)
102+
assert spTree.max_shape_id == 42
103+
104+
def but_it_ignores_sibling_ids_outside_the_shape_tree(self):
105+
# ---Regression for the slide-master shape-id-overflow bug surfaced by
106+
# SlideMaster.add_text_watermark: `<p:sldLayoutIdLst>` lives as a
107+
# sibling of `<p:cSld>` on a slide master and uses sentinel ids
108+
# starting at 2147483649. A whole-document xpath would pull those
109+
# values in and push the next shape id past `xs:int`, which
110+
# PowerPoint quarantines on open ("Repair" path).
111+
sldMaster = element(
112+
"p:sldMaster/(p:cSld/p:spTree/(p:nvGrpSpPr/p:cNvPr{id=1,name=root}"
113+
",p:sp/p:nvSpPr/p:cNvPr{id=2,name=title})"
114+
",p:sldLayoutIdLst/(p:sldLayoutId{id=2147483649,r:id=rId1}"
115+
",p:sldLayoutId{id=2147483650,r:id=rId2}))"
116+
)
117+
spTree = sldMaster.find(
118+
"{http://schemas.openxmlformats.org/presentationml/2006/main}cSld/"
119+
"{http://schemas.openxmlformats.org/presentationml/2006/main}spTree"
120+
)
121+
# ---scoped to spTree descendants; 2147483649 etc. are NOT counted---
122+
assert spTree.max_shape_id == 2
123+
# ---and `_next_shape_id` (max + 1) stays in the safe shape-id range---
124+
assert spTree.max_shape_id + 1 == 3
125+
91126
# fixtures ---------------------------------------------
92127

93128
@pytest.fixture

0 commit comments

Comments
 (0)