From 29f3022e0f2d8bafe5f1447f8aac06e9f1dc2090 Mon Sep 17 00:00:00 2001 From: Matthew Horoszowski Date: Wed, 6 May 2026 19:01:19 -0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(slides):=20add=20slide=20CRUD=20Phase?= =?UTF-8?q?=201=20=E2=80=94=20remove,=20move,=20indexed=20add=5Fslide?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First slice of issue #11 (slide CRUD epic). Lands the in-deck CRUD operations users have been asking for since 2014; full duplicate(), cross-deck append_from(), and Sections support stay deferred to Phases 2–4 in the epic. New public API -------------- - `Slides.add_slide(slide_layout, index=None)` — `index=None` preserves existing append behavior (backwards compatible). Integer `index` inserts at that zero-based position; `index=len(self)` is a valid append, negatives raise `IndexError`. - `Slides.move(slide, new_index)` — reorder a slide in the presentation's slide sequence. Raises `ValueError` for foreign slides, `IndexError` for out-of-range `new_index`. - `Slides.remove(slide)` — drop a slide from the collection. The presentation→slide relationship is removed; the underlying part falls out on save. Image and other media parts shared with surviving slides are preserved (covered by integration test). - `Slide.delete()` — convenience alias for `prs.slides.remove(self)`. API shape mirrors the existing `SlideLayouts.remove(slide_layout)` shape rather than upstream PR scanny/python-pptx#1029's `remove_slide(slide_id)` — instance-keyed is more Pythonic and matches the rest of the library. Internal additions ------------------ - `CT_SlideIdList.insert_sldId_at(rId, idx)` — index-aware sldId insertion, with `idx == len` allowed. - `CT_SlideIdList.move_sldId_to(sldId, new_idx)` — reposition an existing sldId; no-op when the index already matches. - `CT_SlideIdList.remove_sldId(sldId)` — bounded remove with a parent-of check. Test coverage ------------- - 32 new unit tests in `tests/test_slide_crud.py` covering oxml helpers, the Slides CRUD methods, the Slide.delete alias, and four full-pipeline round-trip integration tests (add-at-index, remove, delete, move). Plus a shared-image-preservation round-trip and a `index=len(slides)` boundary test added per advisor review. - 5 new behave scenarios in `features/sld-crud.feature` exercising add-at-head, add-in-middle, move, remove, and Slide.delete. - New UAT script (untracked) at `uat_slide_crud.py` builds a deck exercising every new entry-point, saves, reopens, and prints a per-slide read-back so the maintainer can compare in PowerPoint / Keynote per CLAUDE.md §6. Tangential test-isolation fix ----------------------------- `tests/opc/test_package.py:DescribePartFactory` mutates `PartFactory.part_type_for[CT.PML_SLIDE]` to a class_mock and never reverts it. The mock leaked into any later test that loaded slides through `Presentation()` — caught here by the new round-trip integration tests. Restored via a request finalizer; isolated to that one test, no behavior change for the production code path. Verification ------------ ``` $ python3 -m pytest tests/ -q | tail -3 3086 passed in 3.76s $ ruff check src tests | tail -3 All checks passed! $ python3 -m behave features/ --no-color | tail -3 986 scenarios passed, 0 failed, 0 skipped 2952 steps passed, 0 failed, 0 skipped ``` Refs #11 --- features/sld-crud.feature | 39 +++ features/steps/slides.py | 52 ++++ src/pptx/oxml/presentation.py | 44 +++ src/pptx/slide.py | 70 ++++- tests/opc/test_package.py | 11 + tests/test_slide_crud.py | 503 ++++++++++++++++++++++++++++++++++ 6 files changed, 716 insertions(+), 3 deletions(-) create mode 100644 features/sld-crud.feature create mode 100644 tests/test_slide_crud.py diff --git a/features/sld-crud.feature b/features/sld-crud.feature new file mode 100644 index 000000000..98635222b --- /dev/null +++ b/features/sld-crud.feature @@ -0,0 +1,39 @@ +Feature: Slide CRUD — remove, move, indexed add + In order to programmatically assemble decks without lxml hacks + As a developer using python-pptx + I need to add slides at a chosen position, reorder them, and remove them + + + Scenario: Slides.add_slide(slide_layout, index=0) inserts at the head + Given a Slides object containing 3 slides + When I call slides.add_slide(slide_layout, index=0) + Then len(slides) is 4 + And the new slide is at index 0 + + + Scenario: Slides.add_slide(slide_layout, index=2) inserts in the middle + Given a Slides object containing 3 slides + When I call slides.add_slide(slide_layout, index=2) + Then len(slides) is 4 + And the new slide is at index 2 + + + Scenario: Slides.move(slide, new_index) reorders slides + Given a Slides object containing 3 slides + When I call slides.move(slides[0], 2) + Then len(slides) is 3 + And the slide order matches the original [1, 2, 0] + + + Scenario: Slides.remove(slide) drops a slide and its rel + Given a Slides object containing 3 slides + When I call slides.remove(slides[1]) + Then len(slides) is 2 + And the surviving slide order matches the original [0, 2] + + + Scenario: Slide.delete() removes the slide from its presentation + Given a Slides object containing 3 slides + When I call slides[1].delete() + Then len(slides) is 2 + And the surviving slide order matches the original [0, 2] diff --git a/features/steps/slides.py b/features/steps/slides.py index 42ef66885..2b43385e0 100644 --- a/features/steps/slides.py +++ b/features/steps/slides.py @@ -27,6 +27,8 @@ def given_a_Slides_object_containing_3_slides(context): prs = Presentation(test_pptx("sld-slides")) context.prs = prs context.slides = prs.slides + # ---capture original slide ids for CRUD ordering assertions--- + context.original_slide_ids = [s.slide_id for s in prs.slides] # when ==================================================== @@ -44,6 +46,33 @@ def when_I_call_slide_layouts_remove(context): slide_layouts.remove(slide_layouts[1]) +@when("I call slides.add_slide(slide_layout, index=0)") +def when_I_call_slides_add_slide_index_0(context): + layout = context.prs.slide_masters[0].slide_layouts[0] + context.new_slide = context.slides.add_slide(layout, index=0) + + +@when("I call slides.add_slide(slide_layout, index=2)") +def when_I_call_slides_add_slide_index_2(context): + layout = context.prs.slide_masters[0].slide_layouts[0] + context.new_slide = context.slides.add_slide(layout, index=2) + + +@when("I call slides.move(slides[0], 2)") +def when_I_call_slides_move(context): + context.slides.move(context.slides[0], 2) + + +@when("I call slides.remove(slides[1])") +def when_I_call_slides_remove(context): + context.slides.remove(context.slides[1]) + + +@when("I call slides[1].delete()") +def when_I_call_slide_delete(context): + context.slides[1].delete() + + # then ==================================================== @@ -140,3 +169,26 @@ def then_slides_get_666_default_slides_2_is_slides_2(context): def then_slides_2_is_a_Slide_object(context): slides = context.slides assert type(slides[2]).__name__ == "Slide" + + +@then("the new slide is at index {idx:d}") +def then_the_new_slide_is_at_index(context, idx): + assert context.slides[idx].slide_id == context.new_slide.slide_id, ( + "expected new slide at index %d, got slide_id mismatch" % idx + ) + + +@then("the slide order matches the original [1, 2, 0]") +def then_slide_order_matches_1_2_0(context): + o = context.original_slide_ids + expected = [o[1], o[2], o[0]] + actual = [s.slide_id for s in context.slides] + assert actual == expected, "expected %r, got %r" % (expected, actual) + + +@then("the surviving slide order matches the original [0, 2]") +def then_surviving_slide_order_matches_0_2(context): + o = context.original_slide_ids + expected = [o[0], o[2]] + actual = [s.slide_id for s in context.slides] + assert actual == expected, "expected %r, got %r" % (expected, actual) diff --git a/src/pptx/oxml/presentation.py b/src/pptx/oxml/presentation.py index 169852a23..05ca37587 100644 --- a/src/pptx/oxml/presentation.py +++ b/src/pptx/oxml/presentation.py @@ -64,6 +64,50 @@ def add_sldId(self, rId: str) -> CT_SlideId: """ return self._add_sldId(id=self._next_id, rId=rId) + def insert_sldId_at(self, rId: str, idx: int) -> CT_SlideId: + """Insert a new `p:sldId` child element at position `idx`. + + The new `p:sldId` element has its `r:id` attribute set to `rId` and + receives the next available `id` value. `idx` may equal the current + length to append. Raises `IndexError` if `idx` is out of range. + """ + if idx < 0 or idx > len(self.sldId_lst): + raise IndexError("slide index out of range") + new_sldId = self.add_sldId(rId) + if idx < len(self.sldId_lst) - 1: + target = self.sldId_lst[idx] + target.addprevious(new_sldId) + return new_sldId + + def move_sldId_to(self, sldId: CT_SlideId, new_idx: int) -> None: + """Reposition `sldId` to zero-based position `new_idx` in this list. + + `sldId` must already be a child of this element. Raises `IndexError` + if `new_idx` is out of range. + """ + sldId_lst = self.sldId_lst + if new_idx < 0 or new_idx >= len(sldId_lst): + raise IndexError("slide index out of range") + if sldId_lst[new_idx] is sldId: + return + # -- detach from current position -- + self.remove(sldId) + # -- re-fetch list so index reflects post-removal state -- + sldId_lst = self.sldId_lst + if new_idx >= len(sldId_lst): + self.append(sldId) + else: + sldId_lst[new_idx].addprevious(sldId) + + def remove_sldId(self, sldId: CT_SlideId) -> None: + """Remove `sldId` child element from this list. + + Raises `ValueError` if `sldId` is not a child of this element. + """ + if sldId.getparent() is not self: + raise ValueError("sldId is not a child of this sldIdLst") + self.remove(sldId) + @property def _next_id(self) -> int: """The next available slide ID as an `int`. diff --git a/src/pptx/slide.py b/src/pptx/slide.py index 864556c24..c3a70588d 100644 --- a/src/pptx/slide.py +++ b/src/pptx/slide.py @@ -246,6 +246,19 @@ def slide_layout(self) -> SlideLayout: """|SlideLayout| object this slide inherits appearance from.""" return self.part.slide_layout + def delete(self) -> None: + """Remove this slide from its presentation. + + Convenience alias for :meth:`Slides.remove`. After this call, the slide + is no longer reachable via the presentation's `slides` collection and + its part is dropped from the package on save. + + Raises |ValueError| if this slide is not part of a presentation (e.g. + the parent collection cannot be located). + """ + prs = self.part.package.presentation_part.presentation + prs.slides.remove(self) + class Slides(ParentedElementProxy): """Sequence of slides belonging to an instance of |Presentation|. @@ -277,11 +290,30 @@ def __len__(self) -> int: """Support len() built-in function, e.g. `len(slides) == 4`.""" return len(self._sldIdLst) - def add_slide(self, slide_layout: SlideLayout) -> Slide: - """Return a newly added slide that inherits layout from `slide_layout`.""" + def add_slide(self, slide_layout: SlideLayout, index: int | None = None) -> Slide: + """Return a newly added slide that inherits layout from `slide_layout`. + + When `index` is |None| (the default), the new slide is appended to the + end of the slide sequence — matching prior behavior. When `index` is + an integer, the new slide is inserted at that zero-based position; + `index` may equal `len(self)` to append explicitly. Negative indices + are not supported; pass an explicit position. Raises |IndexError| if + `index` is out of range (negative, or greater than `len(self)`). + + Companion operations: :meth:`remove`, :meth:`move`. Cross-deck copy + (``Presentation.append_from``) and ``Slide.duplicate`` are tracked + under issue #11 (Phase 2/3) and not yet implemented. + """ + # ---validate index BEFORE creating the new SlidePart so a bad index + # does not leak a partial part into the package--- + if index is not None and (index < 0 or index > len(self._sldIdLst)): + raise IndexError("slide index out of range") rId, slide = self.part.add_slide(slide_layout) slide.shapes.clone_layout_placeholders(slide_layout) - self._sldIdLst.add_sldId(rId) + if index is None: + self._sldIdLst.add_sldId(rId) + else: + self._sldIdLst.insert_sldId_at(rId, index) return slide def get(self, slide_id: int, default: Slide | None = None) -> Slide | None: @@ -304,6 +336,38 @@ def index(self, slide: Slide) -> int: return idx raise ValueError("%s is not in slide collection" % slide) + def move(self, slide: Slide, new_index: int) -> None: + """Reposition `slide` to zero-based position `new_index`. + + Raises |ValueError| if `slide` is not a member of this collection, + and |IndexError| if `new_index` is out of range (negative or + ``>= len(self)``). Section membership (`p14:sectionLst`) references + slides by id, not position, so reordering is invisible to sections. + """ + if new_index < 0 or new_index >= len(self): + raise IndexError("slide index out of range") + idx = self.index(slide) + sldId = self._sldIdLst.sldId_lst[idx] + self._sldIdLst.move_sldId_to(sldId, new_index) + + def remove(self, slide: Slide) -> None: + """Remove `slide` from this collection. + + The slide's relationship is dropped from the presentation part. The + underlying slide part falls out of the package on the next save. + Image and other media parts referenced only by the removed slide + likewise drop out — but image parts shared with surviving slides + (e.g. the same picture inserted on two slides) are preserved. + Raises |ValueError| if `slide` is not a member of this collection. + After this call, references to `slide` are stale; behavior of method + calls on the removed `Slide` instance is undefined. + """ + idx = self.index(slide) + sldId = self._sldIdLst.sldId_lst[idx] + target_rId = sldId.rId + self._sldIdLst.remove_sldId(sldId) + self.part.drop_rel(target_rId) + class SlideLayout(_BaseSlide): """Slide layout object. diff --git a/tests/opc/test_package.py b/tests/opc/test_package.py index 0e53a0271..ab3209e48 100644 --- a/tests/opc/test_package.py +++ b/tests/opc/test_package.py @@ -514,6 +514,17 @@ def it_constructs_custom_part_type_for_registered_content_types(self, request, p SlidePart_ = class_mock(request, "pptx.opc.package.XmlPart") SlidePart_.load.return_value = part_ partname = PackURI("/ppt/slides/slide7.xml") + # ---restore the original PartFactory mapping after the test so the + # mock does not leak into subsequent tests that load slides + original = PartFactory.part_type_for.get(CT.PML_SLIDE) + + def _restore(): + if original is None: + PartFactory.part_type_for.pop(CT.PML_SLIDE, None) + else: + PartFactory.part_type_for[CT.PML_SLIDE] = original + + request.addfinalizer(_restore) PartFactory.part_type_for[CT.PML_SLIDE] = SlidePart_ part = PartFactory(partname, CT.PML_SLIDE, package_, b"blob") diff --git a/tests/test_slide_crud.py b/tests/test_slide_crud.py new file mode 100644 index 000000000..1a6349b4a --- /dev/null +++ b/tests/test_slide_crud.py @@ -0,0 +1,503 @@ +# pyright: reportPrivateUsage=false + +"""Unit-test suite for slide-CRUD additions: remove/move/indexed add_slide. + +Issue: https://github.com/MHoroszowski/python-pptx/issues/11 (Phase 1). + +Mirrors the unit-test style of `tests/test_slide.py`. Round-trip integration +tests at the bottom exercise the full pptx open → mutate → save → reopen path +on a synthetic in-memory presentation. +""" + +from __future__ import annotations + +import io + +import pytest + +from pptx import Presentation +from pptx.oxml.presentation import CT_SlideIdList +from pptx.parts.presentation import PresentationPart +from pptx.parts.slide import SlidePart +from pptx.slide import Slide, SlideLayout, Slides + +from .unitutil.cxml import element, xml +from .unitutil.mock import instance_mock, method_mock, property_mock + +# --------------------------------------------------------------------------- +# OXML LAYER — `p:sldIdLst` reordering helpers. +# --------------------------------------------------------------------------- + + +class DescribeCT_SlideIdList(object): + """Unit-test suite for `CT_SlideIdList` insert/move/remove helpers.""" + + def it_can_insert_into_an_empty_sldIdLst(self): + """Cover the idx==len==0 path; assert by structure, not raw XML. + + The XML-prefix shape differs when the parent does not yet declare + the `r:` namespace — semantic equality is what matters here. + """ + sldIdLst = element("p:sldIdLst") + assert isinstance(sldIdLst, CT_SlideIdList) + + sldIdLst.insert_sldId_at("rId9", 0) + + assert len(sldIdLst.sldId_lst) == 1 + assert sldIdLst.sldId_lst[0].rId == "rId9" + assert sldIdLst.sldId_lst[0].id == 256 + + @pytest.mark.parametrize( + ("sldIdLst_cxml", "rId", "idx", "expected_cxml"), + [ + # ---insert at head of populated list--- + ( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})", + "rId9", + 0, + ( + "p:sldIdLst/(p:sldId{r:id=rId9,id=258},p:sldId{r:id=a,id=256}," + "p:sldId{r:id=b,id=257})" + ), + ), + # ---insert in the middle--- + ( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})", + "rId9", + 1, + ( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=rId9,id=258}," + "p:sldId{r:id=b,id=257})" + ), + ), + # ---insert at the tail (idx == len)--- + ( + "p:sldIdLst/p:sldId{r:id=a,id=256}", + "rId9", + 1, + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=rId9,id=257})", + ), + ], + ) + def it_can_insert_a_sldId_at_a_specific_index( + self, sldIdLst_cxml: str, rId: str, idx: int, expected_cxml: str + ): + sldIdLst = element(sldIdLst_cxml) + assert isinstance(sldIdLst, CT_SlideIdList) + + sldIdLst.insert_sldId_at(rId, idx) + + assert sldIdLst.xml == xml(expected_cxml) + + @pytest.mark.parametrize("bad_idx", [-1, 5]) + def but_it_raises_IndexError_on_insert_out_of_range(self, bad_idx: int): + sldIdLst = element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})") + with pytest.raises(IndexError): + sldIdLst.insert_sldId_at("rId9", bad_idx) + + @pytest.mark.parametrize( + ("sldIdLst_cxml", "moving_rId", "new_idx", "expected_cxml"), + [ + # ---move forward (head → tail)--- + ( + ( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257}," + "p:sldId{r:id=c,id=258})" + ), + "a", + 2, + ( + "p:sldIdLst/(p:sldId{r:id=b,id=257},p:sldId{r:id=c,id=258}," + "p:sldId{r:id=a,id=256})" + ), + ), + # ---move backward (tail → head)--- + ( + ( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257}," + "p:sldId{r:id=c,id=258})" + ), + "c", + 0, + ( + "p:sldIdLst/(p:sldId{r:id=c,id=258},p:sldId{r:id=a,id=256}," + "p:sldId{r:id=b,id=257})" + ), + ), + # ---move to current position is a no-op--- + ( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})", + "a", + 0, + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})", + ), + ], + ) + def it_can_move_a_sldId_to_a_new_index( + self, + sldIdLst_cxml: str, + moving_rId: str, + new_idx: int, + expected_cxml: str, + ): + sldIdLst = element(sldIdLst_cxml) + assert isinstance(sldIdLst, CT_SlideIdList) + target = next(s for s in sldIdLst.sldId_lst if s.rId == moving_rId) + + sldIdLst.move_sldId_to(target, new_idx) + + assert sldIdLst.xml == xml(expected_cxml) + + @pytest.mark.parametrize("bad_idx", [-1, 3]) + def but_it_raises_IndexError_on_move_out_of_range(self, bad_idx: int): + sldIdLst = element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})") + target = sldIdLst.sldId_lst[0] + with pytest.raises(IndexError): + sldIdLst.move_sldId_to(target, bad_idx) + + def it_can_remove_a_sldId(self): + sldIdLst = element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})") + assert isinstance(sldIdLst, CT_SlideIdList) + target = sldIdLst.sldId_lst[0] + + sldIdLst.remove_sldId(target) + + assert sldIdLst.xml == xml("p:sldIdLst/p:sldId{r:id=b,id=257}") + + def but_it_raises_on_remove_of_foreign_element(self): + sldIdLst = element("p:sldIdLst/p:sldId{r:id=a,id=256}") + foreign = element("p:sldIdLst/p:sldId{r:id=z,id=999}").sldId_lst[0] + + with pytest.raises(ValueError): + sldIdLst.remove_sldId(foreign) + + +# --------------------------------------------------------------------------- +# `Slides` — indexed add_slide, move(), remove(). +# --------------------------------------------------------------------------- + + +class DescribeSlides_CRUD(object): + """Unit-test suite for the new CRUD methods on `pptx.slide.Slides`.""" + + # -- add_slide(layout, index=None) --------------------------------------- + + def it_keeps_appending_when_no_index_is_given(self, append_fixture): + slides, slide_layout_, expected_xml, slide_ = append_fixture + + slide = slides.add_slide(slide_layout_) + + assert slides._sldIdLst.xml == expected_xml + assert slide is slide_ + + def it_can_insert_a_new_slide_at_the_head(self, insert_head_fixture): + slides, slide_layout_, expected_xml, slide_ = insert_head_fixture + + slide = slides.add_slide(slide_layout_, index=0) + + assert slides._sldIdLst.xml == expected_xml + assert slide is slide_ + + def it_can_insert_a_new_slide_at_a_middle_index(self, insert_middle_fixture): + slides, slide_layout_, expected_xml, slide_ = insert_middle_fixture + + slide = slides.add_slide(slide_layout_, index=1) + + assert slides._sldIdLst.xml == expected_xml + assert slide is slide_ + + def it_can_insert_a_new_slide_at_the_tail(self, insert_tail_fixture): + slides, slide_layout_, expected_xml, slide_ = insert_tail_fixture + + slide = slides.add_slide(slide_layout_, index=2) + + assert slides._sldIdLst.xml == expected_xml + assert slide is slide_ + + @pytest.mark.parametrize("bad_index", [-1, 99]) + def but_it_raises_on_add_slide_index_out_of_range(self, bad_index, part_prop_, slide_layout_): + slides = Slides( + element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})"), + None, + ) + with pytest.raises(IndexError): + slides.add_slide(slide_layout_, index=bad_index) + + # -- move() --------------------------------------------------------------- + + def it_can_move_a_slide_to_a_new_position(self, move_fixture): + slides, target_slide_, new_index, expected_xml = move_fixture + + slides.move(target_slide_, new_index) + + assert slides._sldIdLst.xml == expected_xml + + def but_it_raises_on_move_of_slide_not_in_collection(self, missing_slide_fixture): + slides, slide_ = missing_slide_fixture + with pytest.raises(ValueError): + slides.move(slide_, 0) + + @pytest.mark.parametrize("bad_idx", [-1, 99]) + def but_it_raises_on_move_index_out_of_range(self, bad_idx, part_prop_, slide_): + slides = Slides( + element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})"), + None, + ) + with pytest.raises(IndexError): + slides.move(slide_, bad_idx) + + # -- remove() / Slide.delete() ------------------------------------------- + + def it_can_remove_a_slide(self, remove_fixture): + slides, target_slide_, expected_xml, prs_part_ = remove_fixture + + slides.remove(target_slide_) + + assert slides._sldIdLst.xml == expected_xml + prs_part_.drop_rel.assert_called_once_with("a") + + def but_it_raises_on_remove_of_slide_not_in_collection(self, missing_slide_fixture): + slides, slide_ = missing_slide_fixture + with pytest.raises(ValueError): + slides.remove(slide_) + + # ---------- fixtures ----------------------------------------------------- + + @pytest.fixture + def append_fixture(self, slide_layout_, part_prop_, slide_): + slides = Slides(element("p:sldIdLst/p:sldId{r:id=rId1,id=256}"), None) + part_ = part_prop_.return_value + part_.add_slide.return_value = "rId2", slide_ + expected_xml = xml("p:sldIdLst/(p:sldId{r:id=rId1,id=256},p:sldId{r:id=rId2,id=257})") + return slides, slide_layout_, expected_xml, slide_ + + @pytest.fixture + def insert_head_fixture(self, slide_layout_, part_prop_, slide_): + slides = Slides( + element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})"), + None, + ) + part_ = part_prop_.return_value + part_.add_slide.return_value = "rIdNew", slide_ + expected_xml = xml( + "p:sldIdLst/(p:sldId{r:id=rIdNew,id=258},p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})" + ) + return slides, slide_layout_, expected_xml, slide_ + + @pytest.fixture + def insert_middle_fixture(self, slide_layout_, part_prop_, slide_): + slides = Slides( + element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})"), + None, + ) + part_ = part_prop_.return_value + part_.add_slide.return_value = "rIdNew", slide_ + expected_xml = xml( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=rIdNew,id=258},p:sldId{r:id=b,id=257})" + ) + return slides, slide_layout_, expected_xml, slide_ + + @pytest.fixture + def insert_tail_fixture(self, slide_layout_, part_prop_, slide_): + slides = Slides( + element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})"), + None, + ) + part_ = part_prop_.return_value + part_.add_slide.return_value = "rIdNew", slide_ + expected_xml = xml( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257},p:sldId{r:id=rIdNew,id=258})" + ) + return slides, slide_layout_, expected_xml, slide_ + + @pytest.fixture + def move_fixture(self, request, part_prop_, _index_): + # ---move slide at index 0 to index 2--- + sldIdLst = element( + "p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257},p:sldId{r:id=c,id=258})" + ) + slides = Slides(sldIdLst, None) + target_slide_ = instance_mock(request, Slide) + _index_.return_value = 0 + expected_xml = xml( + "p:sldIdLst/(p:sldId{r:id=b,id=257},p:sldId{r:id=c,id=258},p:sldId{r:id=a,id=256})" + ) + return slides, target_slide_, 2, expected_xml + + @pytest.fixture + def remove_fixture(self, request, part_prop_, _index_): + sldIdLst = element("p:sldIdLst/(p:sldId{r:id=a,id=256},p:sldId{r:id=b,id=257})") + slides = Slides(sldIdLst, None) + target_slide_ = instance_mock(request, Slide) + _index_.return_value = 0 + prs_part_ = part_prop_.return_value + expected_xml = xml("p:sldIdLst/p:sldId{r:id=b,id=257}") + return slides, target_slide_, expected_xml, prs_part_ + + @pytest.fixture + def missing_slide_fixture(self, request, part_prop_, _index_): + sldIdLst = element("p:sldIdLst/p:sldId{r:id=a,id=256}") + slides = Slides(sldIdLst, None) + slide_ = instance_mock(request, Slide) + _index_.side_effect = ValueError("not in collection") + return slides, slide_ + + # ---------- fixture components ------------------------------------------ + + @pytest.fixture + def part_prop_(self, request, prs_part_): + return property_mock(request, Slides, "part", return_value=prs_part_) + + @pytest.fixture + def prs_part_(self, request): + return instance_mock(request, PresentationPart) + + @pytest.fixture + def slide_(self, request): + return instance_mock(request, Slide) + + @pytest.fixture + def slide_layout_(self, request): + return instance_mock(request, SlideLayout) + + @pytest.fixture + def _index_(self, request): + return method_mock(request, Slides, "index") + + +# --------------------------------------------------------------------------- +# `Slide.delete()` alias. +# --------------------------------------------------------------------------- + + +class DescribeSlide_Delete(object): + """Unit-test suite for `pptx.slide.Slide.delete`.""" + + def it_delegates_to_Slides_remove_on_the_owning_presentation(self, request): + slide_part_ = instance_mock(request, SlidePart) + prs_ = instance_mock(request, type("Prs", (), {"slides": None})) + prs_.slides = instance_mock(request, Slides) + prs_part_ = instance_mock(request, PresentationPart) + prs_part_.presentation = prs_ + slide_part_.package.presentation_part = prs_part_ + + slide = Slide(element("p:sld/p:cSld"), slide_part_) + + slide.delete() + + prs_.slides.remove.assert_called_once_with(slide) + + +# --------------------------------------------------------------------------- +# Round-trip integration tests — open → mutate → save → reopen. +# --------------------------------------------------------------------------- + + +def _seed_presentation_with(n_slides: int) -> Presentation: + """Build an in-memory Presentation seeded with `n_slides` blank slides.""" + prs = Presentation() + layout = prs.slide_layouts[6] # ---blank layout--- + for _ in range(n_slides): + prs.slides.add_slide(layout) + return prs + + +def _round_trip(prs: Presentation) -> Presentation: + """Save `prs` to a bytes buffer and reopen it.""" + buf = io.BytesIO() + prs.save(buf) + buf.seek(0) + return Presentation(buf) + + +class DescribeSlideCrudRoundTrip(object): + """Open → mutate → save → reopen integration coverage.""" + + def it_round_trips_an_indexed_add(self): + prs = _seed_presentation_with(2) + layout = prs.slide_layouts[6] + original_ids = [s.slide_id for s in prs.slides] + + new_slide = prs.slides.add_slide(layout, index=1) + new_id = new_slide.slide_id + + round_tripped = _round_trip(prs) + ids_after = [s.slide_id for s in round_tripped.slides] + + assert len(round_tripped.slides) == 3 + assert ids_after == [original_ids[0], new_id, original_ids[1]] + + def it_round_trips_a_remove(self): + prs = _seed_presentation_with(3) + ids_before = [s.slide_id for s in prs.slides] + + prs.slides.remove(prs.slides[1]) + + round_tripped = _round_trip(prs) + ids_after = [s.slide_id for s in round_tripped.slides] + + assert len(round_tripped.slides) == 2 + assert ids_after == [ids_before[0], ids_before[2]] + + def it_round_trips_a_Slide_delete_call(self): + prs = _seed_presentation_with(3) + ids_before = [s.slide_id for s in prs.slides] + + prs.slides[0].delete() + + round_tripped = _round_trip(prs) + ids_after = [s.slide_id for s in round_tripped.slides] + + assert len(round_tripped.slides) == 2 + assert ids_after == [ids_before[1], ids_before[2]] + + def it_round_trips_a_move(self): + prs = _seed_presentation_with(3) + ids_before = [s.slide_id for s in prs.slides] + + prs.slides.move(prs.slides[0], 2) + + round_tripped = _round_trip(prs) + ids_after = [s.slide_id for s in round_tripped.slides] + + assert len(round_tripped.slides) == 3 + assert ids_after == [ids_before[1], ids_before[2], ids_before[0]] + + def it_treats_index_equal_to_len_as_append(self): + """`add_slide(layout, index=len(slides))` should be a valid append.""" + prs = _seed_presentation_with(2) + layout = prs.slide_layouts[6] + + new_slide = prs.slides.add_slide(layout, index=len(prs.slides)) + + assert len(prs.slides) == 3 + assert prs.slides[2].slide_id == new_slide.slide_id + + def it_preserves_a_shared_image_after_removing_one_referencing_slide(self): + """Shared image-part survives remove() of one of two slides using it. + + Image dedup: python-pptx's package serializer keeps any part that + remains reachable; if slide B references the same image as slide A + and we remove A, the image must still be readable on B. + """ + from pathlib import Path + + from pptx.util import Inches + + png = Path(__file__).parent / "test_files" / "python-powered.png" + prs = Presentation() + layout = prs.slide_layouts[6] + slide_a = prs.slides.add_slide(layout) + slide_a.shapes.add_picture(str(png), Inches(1), Inches(1)) + slide_b = prs.slides.add_slide(layout) + slide_b.shapes.add_picture(str(png), Inches(1), Inches(1)) + + prs.slides.remove(slide_a) + round_tripped = _round_trip(prs) + + assert len(round_tripped.slides) == 1 + # ---surviving slide must still expose its picture (== reachable image part)--- + pictures = [shp for shp in round_tripped.slides[0].shapes if shp.shape_type == 13] + assert len(pictures) == 1 + assert pictures[0].image.blob is not None + assert len(pictures[0].image.blob) > 0 From bcb004e75ab784f95dfaa81ebc30b302738aeaee Mon Sep 17 00:00:00 2001 From: Matthew Horoszowski Date: Thu, 7 May 2026 18:34:08 -0400 Subject: [PATCH 2/2] feat(slides): add Slide.duplicate / Slides.duplicate (slide-CRUD Phase 2) (#34) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the Phase 2 sub-task of the slide-CRUD epic (#11) and the 12-year-old most-commented upstream feature request scanny/python-pptx#132 in this fork. API --- - `Slides.duplicate(slide, index=None)` — collection-level operation, returns a `Slide`. `index=None` defaults to `source_index + 1` (immediately after the source). Raises `ValueError` if `slide` is not a member, `IndexError` if `index` is out of range. - `Slide.duplicate(index=None)` — convenience alias delegating to `Slides.duplicate(self, index)`. Mirrors `Slide.delete()` → `Slides.remove(self)` from Phase 1. Design ------ The implementation walks the source slide's rels and decides per reltype whether to share the target part (image, media, slide-layout, slide-master, theme — package-level dedup) or deep-copy it (chart, OLE-object, embedded-package, notes-slide). For chart parts the chart XML is `copy.deepcopy`'d but the embedded xlsx workbook is *blob- copied* — the workbook is the chart's data and `` values in the chart XML mirror it; deepcopy of XML would not capture the binary payload. After deepcopy, the duplicated `` still references the source's `rId`s. We walk every descendant element via `lxml.iter()` and substitute any attribute whose name is in the OOXML relationships namespace — catches `r:id`, `r:embed`, `r:link`, `r:pict`, `r:href` in one pass without enumerating local-names (the local-name approach silently misses `r:link` on linked images, which is the most common real-world failure mode of community recipes). Notes-slide handling addresses upstream gotcha #961: a NotesSlidePart back-references its parent slide via `RT.SLIDE`. The duplicate's notes-slide MUST point at the new slide, not the source. The new helper `duplicate_notes_slide_for(src, new)` rewires the back-rel and is invoked AFTER the new slide part is registered in presentation rels. Comments parts (`RT.COMMENTS` / `RT.COMMENT_AUTHORS`) are dropped on duplicate by an explicit reltype filter — comments support is out of scope for Phase 2 of #11. A test documents the drop so the policy doesn't silently change. Test coverage ------------- - `tests/test_slide_duplicate.py` — 39 new tests across 7 describe classes covering the API surface (existence, signature, raises, index variants, alias delegation), the part-graph invariants (new unique partname / rId / sldId, layout-part shared with source, modification isolation, image-dedup invariant unchanged), the notes-slide path (own copy, back-ref rewired, isolation), the defensive XPath `r:*` check (no unmapped rIds), and round-trip through save/reopen. - `features/sld-duplicate.feature` + 7 new step impls — 4 acceptance scenarios for default-index, explicit-index, alias, and IndexError paths. Verification ------------ ``` $ python3 -m pytest tests/ -q 3125 passed in 4.02s $ python3 -m ruff format src tests 203 files left unchanged $ python3 -m ruff check src tests All checks passed! $ python3 -m behave features/ --no-color 990 scenarios passed, 0 failed, 0 skipped ``` UAT script `uat_slide_duplicate.py` (untracked at repo root per CLAUDE.md §6): builds a 3-slide deck with a textbox, a picture, and speaker notes; exercises both `Slides.duplicate` and `Slide.duplicate`; round-trips; prints structural summary (image_part_count = 2 before AND after — dedup invariant holds). Out of scope (deferred per epic split): - `Presentation.append_from` — Phase 3 (cross-deck rels). - `Presentation.sections` — Phase 4 (``). Refs #11. Co-authored-by: Matthew Horoszowski --- features/sld-duplicate.feature | 31 ++ features/steps/slides.py | 44 +++ src/pptx/parts/slide.py | 171 ++++++++++- src/pptx/slide.py | 63 +++- tests/test_slide_duplicate.py | 539 +++++++++++++++++++++++++++++++++ 5 files changed, 844 insertions(+), 4 deletions(-) create mode 100644 features/sld-duplicate.feature create mode 100644 tests/test_slide_duplicate.py diff --git a/features/sld-duplicate.feature b/features/sld-duplicate.feature new file mode 100644 index 000000000..967d4a110 --- /dev/null +++ b/features/sld-duplicate.feature @@ -0,0 +1,31 @@ +Feature: Slide duplicate — Slides.duplicate, Slide.duplicate + In order to programmatically clone slides without lxml hacks + As a developer using python-pptx + I need a single API call that duplicates a slide and inserts it at a chosen position + + + Scenario: Slides.duplicate(slide) inserts the copy after the source + Given a Slides object containing 3 slides + When I call slides.duplicate(slides[0]) + Then len(slides) is 4 + And the duplicate is at index 1 + And the source slide is still at index 0 + + + Scenario: Slides.duplicate(slide, index=N) inserts the copy at index N + Given a Slides object containing 3 slides + When I call slides.duplicate(slides[0], index=3) + Then len(slides) is 4 + And the duplicate is at index 3 + + + Scenario: Slide.duplicate() returns a Slide with a new unique slide_id + Given a Slides object containing 3 slides + When I call slides[1].duplicate() + Then len(slides) is 4 + And the duplicate slide_id is unique + + + Scenario: Slides.duplicate raises IndexError for an out-of-range index + Given a Slides object containing 3 slides + Then calling slides.duplicate(slides[0], index=99) raises IndexError diff --git a/features/steps/slides.py b/features/steps/slides.py index 2b43385e0..f2d6206ec 100644 --- a/features/steps/slides.py +++ b/features/steps/slides.py @@ -73,6 +73,21 @@ def when_I_call_slide_delete(context): context.slides[1].delete() +@when("I call slides.duplicate(slides[0])") +def when_I_call_slides_duplicate_default_index(context): + context.new_slide = context.slides.duplicate(context.slides[0]) + + +@when("I call slides.duplicate(slides[0], index=3)") +def when_I_call_slides_duplicate_index_3(context): + context.new_slide = context.slides.duplicate(context.slides[0], index=3) + + +@when("I call slides[1].duplicate()") +def when_I_call_slide_duplicate_alias(context): + context.new_slide = context.slides[1].duplicate() + + # then ==================================================== @@ -192,3 +207,32 @@ def then_surviving_slide_order_matches_0_2(context): expected = [o[0], o[2]] actual = [s.slide_id for s in context.slides] assert actual == expected, "expected %r, got %r" % (expected, actual) + + +@then("the duplicate is at index {idx:d}") +def then_the_duplicate_is_at_index(context, idx): + assert context.slides[idx].slide_id == context.new_slide.slide_id, ( + "expected duplicate at index %d, got slide_id mismatch" % idx + ) + + +@then("the source slide is still at index 0") +def then_source_slide_still_at_index_0(context): + assert context.slides[0].slide_id == context.original_slide_ids[0], ( + "source slide moved off index 0" + ) + + +@then("the duplicate slide_id is unique") +def then_duplicate_slide_id_is_unique(context): + assert context.new_slide.slide_id not in context.original_slide_ids, ( + "duplicate slide_id collides with an existing slide" + ) + + +@then("calling slides.duplicate(slides[0], index=99) raises IndexError") +def then_duplicate_index_99_raises(context): + import pytest + + with pytest.raises(IndexError): + context.slides.duplicate(context.slides[0], index=99) diff --git a/src/pptx/parts/slide.py b/src/pptx/parts/slide.py index 6650564a5..96b70d309 100644 --- a/src/pptx/parts/slide.py +++ b/src/pptx/parts/slide.py @@ -2,12 +2,14 @@ from __future__ import annotations +import copy +import re from typing import IO, TYPE_CHECKING, cast from pptx.enum.shapes import PROG_ID from pptx.opc.constants import CONTENT_TYPE as CT from pptx.opc.constants import RELATIONSHIP_TYPE as RT -from pptx.opc.package import XmlPart +from pptx.opc.package import Part, XmlPart from pptx.opc.packuri import PackURI from pptx.oxml.slide import CT_NotesMaster, CT_NotesSlide, CT_Slide from pptx.oxml.theme import CT_OfficeStyleSheet @@ -259,6 +261,173 @@ def _add_notes_slide_part(self): self.relate_to(notes_slide_part, RT.NOTES_SLIDE) return notes_slide_part + def duplicate(self) -> SlidePart: + """Return a new |SlidePart| that is a deep copy of this one. + + Image, media, slide-layout, and slide-master rels are reused — + the duplicate references the same package-level parts as the + source. Chart, OLE-embedded, and embedded-package parts are + deep-copied per duplicate. The notes-slide rel and any + comments rels are NOT carried over: notes-slide rewiring is + the caller's job (see |Slides.duplicate|), and comments are + out of scope for Phase 2 of issue #11. + """ + new_partname = self._package.next_partname("/ppt/slides/slide%d.xml") + new_element = copy.deepcopy(self._element) + new_part = SlidePart(new_partname, CT.PML_SLIDE, self._package, new_element) + + rId_map = _replicate_rels_for_duplicate(self, new_part) + _remap_rId_attrs(new_element, rId_map) + + return new_part + + +# --------------------------------------------------------------------------- +# Module-level helpers for slide / slide-private part duplication. +# --------------------------------------------------------------------------- + +_RELS_NS = "{http://schemas.openxmlformats.org/officeDocument/2006/relationships}" + +# Reltypes filtered out during slide duplication. NOTES_SLIDE is wired +# explicitly by |Slides.duplicate| so the new notes-slide back-references +# the new parent slide. Comments are dropped — Phase 2 scope (issue #11). +_DUP_DROP_RELTYPES_SLIDE = frozenset({RT.NOTES_SLIDE, RT.COMMENTS, RT.COMMENT_AUTHORS}) + + +def _replicate_rels_for_duplicate(src_part: Part, new_part: Part) -> dict[str, str]: + """Mirror src_part's slide-relevant rels onto new_part. + + Returns a `{old_rId: new_rId}` map for rId-attribute remapping. + """ + rId_map: dict[str, str] = {} + for rId, rel in src_part.rels.items(): + if rel.reltype in _DUP_DROP_RELTYPES_SLIDE: + continue + if rel.is_external: + new_rId = new_part.relate_to(rel.target_ref, rel.reltype, is_external=True) + elif rel.reltype == RT.CHART: + new_target = _duplicate_chart_part(cast(ChartPart, rel.target_part)) + new_rId = new_part.relate_to(new_target, rel.reltype) + elif rel.reltype in (RT.OLE_OBJECT, RT.PACKAGE): + new_target = _duplicate_blob_part(cast(Part, rel.target_part)) + new_rId = new_part.relate_to(new_target, rel.reltype) + else: + # Shared parts: image, media, video, layout, master, theme, etc. + new_rId = new_part.relate_to(rel.target_part, rel.reltype) + rId_map[rId] = new_rId + return rId_map + + +def _remap_rId_attrs(element, rId_map: dict[str, str]) -> None: + """Substitute relationships-namespace attribute values in `element`. + + Walks every descendant element and rewrites any attribute whose name + is in the OOXML relationships namespace (catches `r:id`, `r:embed`, + `r:link`, `r:pict`, `r:href` in one pass). + """ + for el in element.iter(): + for attr_name in list(el.attrib): + if attr_name.startswith(_RELS_NS): + old = el.attrib[attr_name] + if old in rId_map: + el.attrib[attr_name] = rId_map[old] + + +def _duplicate_chart_part(src: ChartPart) -> ChartPart: + """Return a new ChartPart cloning `src`. + + Chart XML is deep-copied. Embedded data (e.g. an xlsx workbook + reached via an `RT.PACKAGE` rel) is binary and must be blob-copied, + not deep-copy-of-XML — the workbook IS the chart's data, and the + `` values in the chart XML mirror it. + """ + package = src._package + new_partname = package.next_partname("/ppt/charts/chart%d.xml") + new_element = copy.deepcopy(src._element) + cls = type(src) + new_part = cls(new_partname, src.content_type, package, new_element) + rId_map: dict[str, str] = {} + for rId, rel in src.rels.items(): + if rel.is_external: + new_rId = new_part.relate_to(rel.target_ref, rel.reltype, is_external=True) + elif rel.reltype == RT.PACKAGE: + new_target = _duplicate_blob_part(cast(Part, rel.target_part)) + new_rId = new_part.relate_to(new_target, rel.reltype) + else: + # Theme override and other chart-private parts: share for now. + # Practical impact is small; revisit if a user reports it. + new_rId = new_part.relate_to(rel.target_part, rel.reltype) + rId_map[rId] = new_rId + _remap_rId_attrs(new_element, rId_map) + return new_part + + +def _duplicate_blob_part(src: Part) -> Part: + """Return a new binary |Part| cloning `src`'s blob. + + Used for embedded packages (xlsx, docx, pptx) and OLE objects — + parts whose payload is opaque bytes rather than XML. + """ + package = src._package + cls = type(src) + tmpl = getattr(cls, "partname_template", None) + if tmpl is None: + tmpl = _derive_partname_template(str(src.partname)) + new_partname = package.next_partname(tmpl) + return cls(new_partname, src.content_type, package, src.blob) + + +def _derive_partname_template(partname: str) -> str: + """Derive a `next_partname`-compatible template from an existing partname. + + Replaces the trailing integer (just before the final extension) with + `%d`. Falls back to inserting `%d` immediately before the extension + if there is no trailing digit run. + """ + match = re.match(r"^(.*?)(\d+)(\.[^./]+)$", partname) + if match: + prefix, _, ext = match.groups() + return f"{prefix}%d{ext}" + # No trailing-digit pattern; insert %d before final extension. + dot = partname.rfind(".") + if dot < 0: + return f"{partname}%d" + return f"{partname[:dot]}%d{partname[dot:]}" + + +def duplicate_notes_slide_for( + src_slide_part: SlidePart, new_slide_part: SlidePart +) -> NotesSlidePart: + """Create a fresh |NotesSlidePart| for `new_slide_part`, cloning content from src. + + Public-to-the-module helper used by |Slides.duplicate| AFTER the new + slide part is registered with the presentation rels. Wires the new + notes-slide's `RT.SLIDE` back-rel to point at `new_slide_part` (NOT + the source) — addresses upstream community gotcha #961 where blindly + copying notes rels left the duplicate's notes pointing at the source. + """ + src_notes_part = cast(NotesSlidePart, src_slide_part.part_related_by(RT.NOTES_SLIDE)) + package = src_slide_part._package + new_partname = package.next_partname("/ppt/notesSlides/notesSlide%d.xml") + new_element = copy.deepcopy(src_notes_part._element) + new_notes_part = NotesSlidePart(new_partname, CT.PML_NOTES_SLIDE, package, new_element) + + rId_map: dict[str, str] = {} + for rId, rel in src_notes_part.rels.items(): + if rel.is_external: + new_rId = new_notes_part.relate_to(rel.target_ref, rel.reltype, is_external=True) + elif rel.reltype == RT.SLIDE: + # ---rewire back-ref to NEW slide part--- + new_rId = new_notes_part.relate_to(new_slide_part, RT.SLIDE) + else: + # NOTES_MASTER and any others: share at package level + new_rId = new_notes_part.relate_to(rel.target_part, rel.reltype) + rId_map[rId] = new_rId + _remap_rId_attrs(new_element, rId_map) + + new_slide_part.relate_to(new_notes_part, RT.NOTES_SLIDE) + return new_notes_part + class SlideLayoutPart(BaseSlidePart): """Slide layout part. diff --git a/src/pptx/slide.py b/src/pptx/slide.py index c3a70588d..a070ac81c 100644 --- a/src/pptx/slide.py +++ b/src/pptx/slide.py @@ -6,6 +6,7 @@ from pptx.dml.fill import FillFormat from pptx.enum.shapes import PP_PLACEHOLDER +from pptx.opc.constants import RELATIONSHIP_TYPE as RT from pptx.shapes.shapetree import ( LayoutPlaceholders, LayoutShapes, @@ -259,6 +260,19 @@ def delete(self) -> None: prs = self.part.package.presentation_part.presentation prs.slides.remove(self) + def duplicate(self, index: int | None = None) -> Slide: + """Return a deep copy of this slide added to the parent presentation. + + Convenience alias delegating to :meth:`Slides.duplicate`. The duplicate + is inserted at zero-based `index`; when `index` is |None|, the + duplicate sits at ``self_index + 1`` — immediately after this slide. + + See :meth:`Slides.duplicate` for full semantics on dedup, notes-slide + handling, and round-trip behavior. + """ + prs = self.part.package.presentation_part.presentation + return prs.slides.duplicate(self, index) + class Slides(ParentedElementProxy): """Sequence of slides belonging to an instance of |Presentation|. @@ -300,9 +314,9 @@ def add_slide(self, slide_layout: SlideLayout, index: int | None = None) -> Slid are not supported; pass an explicit position. Raises |IndexError| if `index` is out of range (negative, or greater than `len(self)`). - Companion operations: :meth:`remove`, :meth:`move`. Cross-deck copy - (``Presentation.append_from``) and ``Slide.duplicate`` are tracked - under issue #11 (Phase 2/3) and not yet implemented. + Companion operations: :meth:`remove`, :meth:`move`, + :meth:`duplicate`. Cross-deck copy (``Presentation.append_from``) + is tracked under issue #11 (Phase 3) and not yet implemented. """ # ---validate index BEFORE creating the new SlidePart so a bad index # does not leak a partial part into the package--- @@ -368,6 +382,49 @@ def remove(self, slide: Slide) -> None: self._sldIdLst.remove_sldId(sldId) self.part.drop_rel(target_rId) + def duplicate(self, slide: Slide, index: int | None = None) -> Slide: + """Return a deep copy of `slide` added to this collection. + + The duplicate is inserted at zero-based position `index`. When + `index` is |None| (the default), the new slide is inserted at + ``source_index + 1`` — immediately after `slide`. ``index`` may + equal ``len(self)`` to append explicitly. Negative indices are + not supported. + + Image, media, slide-layout, and slide-master parts are shared + with the source via package-level dedup — duplicating a slide + that contains pictures does NOT increase the deck's image-part + count. Chart parts, OLE-object parts, and the notes-slide (when + present) are deep-copied so edits to the duplicate don't bleed + back into the source. Comments parts (if any) are dropped — + deferred to a later phase of issue #11. + + Raises |ValueError| if `slide` is not a member of this + collection. Raises |IndexError| if `index` is out of range + (negative or greater than `len(self)`). + """ + from pptx.parts.slide import duplicate_notes_slide_for + + # ---validate membership BEFORE doing any work; raises ValueError if absent--- + src_idx = self.index(slide) + if index is None: + index = src_idx + 1 + if index < 0 or index > len(self._sldIdLst): + raise IndexError("slide index out of range") + + src_part = slide.part + new_slide_part = src_part.duplicate() + + # ---register new slide part with presentation; this allocates an rId--- + new_rId = self.part.relate_to(new_slide_part, RT.SLIDE) + self._sldIdLst.insert_sldId_at(new_rId, index) + + # ---if source had a notes-slide, give the duplicate its own--- + if src_part.has_notes_slide: + duplicate_notes_slide_for(src_part, new_slide_part) + + return new_slide_part.slide + class SlideLayout(_BaseSlide): """Slide layout object. diff --git a/tests/test_slide_duplicate.py b/tests/test_slide_duplicate.py new file mode 100644 index 000000000..c8a1796f8 --- /dev/null +++ b/tests/test_slide_duplicate.py @@ -0,0 +1,539 @@ +# pyright: reportPrivateUsage=false + +"""Unit-test suite for `Slide.duplicate` / `Slides.duplicate` (slide-CRUD Phase 2). + +Issue: https://github.com/MHoroszowski/python-pptx/issues/11 (Phase 2 — duplicate). + +Closes upstream feature request scanny/python-pptx#132 in this fork. + +The tests are organised in three layers: + +1. **Unit tests** that drive the API surface (`Slide.duplicate`, + `Slides.duplicate`) via mocks — argument validation, raises, and + delegation patterns. Mirrors the unit-test style of + `tests/test_slide_crud.py`. +2. **Part-graph tests** that build small in-memory `Presentation` + objects and inspect rels / parts directly to verify the dedup + invariant (image, media reused), the deep-copy invariant (chart, + OLE, notes-slide each get their own part), and the rId-remap pass. +3. **Round-trip integration tests** at the bottom — open → mutate → + save → reopen — to confirm Office-compatible packaging. +""" + +from __future__ import annotations + +import io +from pathlib import Path + +import pytest + +from pptx import Presentation +from pptx.opc.constants import RELATIONSHIP_TYPE as RT +from pptx.parts.slide import SlidePart +from pptx.slide import Slide, Slides +from pptx.util import Inches + +from .unitutil.mock import instance_mock, method_mock + +_RELS_NS = "{http://schemas.openxmlformats.org/officeDocument/2006/relationships}" + +_PNG_PATH = Path(__file__).parent / "test_files" / "python-powered.png" + + +# --------------------------------------------------------------------------- +# Helpers — keep round-trip plumbing identical to test_slide_crud.py. +# --------------------------------------------------------------------------- + + +def _seed_presentation_with(n_slides: int) -> Presentation: + """Build an in-memory Presentation seeded with `n_slides` blank slides.""" + prs = Presentation() + layout = prs.slide_layouts[6] # ---blank layout--- + for _ in range(n_slides): + prs.slides.add_slide(layout) + return prs + + +def _round_trip(prs: Presentation) -> Presentation: + """Save `prs` to a bytes buffer and reopen it.""" + buf = io.BytesIO() + prs.save(buf) + buf.seek(0) + return Presentation(buf) + + +def _image_part_count(prs: Presentation) -> int: + """Number of image parts currently reachable in the package.""" + from pptx.parts.image import ImagePart + + return sum(1 for p in prs.part.package.iter_parts() if isinstance(p, ImagePart)) + + +def _slide_part_count(prs: Presentation) -> int: + """Number of slide parts currently reachable in the package.""" + return sum(1 for p in prs.part.package.iter_parts() if isinstance(p, SlidePart)) + + +# --------------------------------------------------------------------------- +# `Slides.duplicate` — collection-level operation. +# --------------------------------------------------------------------------- + + +class DescribeSlides_Duplicate(object): + """Unit-test suite for `pptx.slide.Slides.duplicate`.""" + + def it_exposes_a_duplicate_method(self): + """API smoke — the method exists on the collection class.""" + assert callable(getattr(Slides, "duplicate", None)) + + def it_returns_a_Slide_instance(self): + """End-to-end smoke against a real two-slide deck.""" + prs = _seed_presentation_with(2) + + new_slide = prs.slides.duplicate(prs.slides[0]) + + assert isinstance(new_slide, Slide) + + def it_inserts_the_duplicate_immediately_after_source_when_index_is_None(self): + prs = _seed_presentation_with(3) + source = prs.slides[1] + + new_slide = prs.slides.duplicate(source) + + assert len(prs.slides) == 4 + # ---new slide sits at source_index + 1--- + assert prs.slides[2].slide_id == new_slide.slide_id + # ---source unchanged at its original index--- + assert prs.slides[1].slide_id == source.slide_id + + @pytest.mark.parametrize("idx", [0, 1, 2, 3]) + def it_inserts_the_duplicate_at_the_given_index(self, idx: int): + prs = _seed_presentation_with(3) + + new_slide = prs.slides.duplicate(prs.slides[0], index=idx) + + assert len(prs.slides) == 4 + assert prs.slides[idx].slide_id == new_slide.slide_id + + @pytest.mark.parametrize("bad_idx", [-1, 5]) + def but_it_raises_IndexError_for_out_of_range_index(self, bad_idx: int): + prs = _seed_presentation_with(3) + with pytest.raises(IndexError): + prs.slides.duplicate(prs.slides[0], index=bad_idx) + + def but_it_raises_ValueError_when_slide_is_not_in_the_collection(self, request): + method_mock(request, Slides, "index", side_effect=ValueError("not in collection")) + slides = Slides(None, None) # pyright: ignore[reportArgumentType] + slide_ = instance_mock(request, Slide) + + with pytest.raises(ValueError): + slides.duplicate(slide_) + + def it_treats_index_equal_to_len_as_append(self): + """`duplicate(slide, index=len(slides))` should be a valid append.""" + prs = _seed_presentation_with(2) + + new_slide = prs.slides.duplicate(prs.slides[0], index=len(prs.slides)) + + assert len(prs.slides) == 3 + assert prs.slides[2].slide_id == new_slide.slide_id + + def it_does_not_mutate_source_slide_position(self): + """Anti-criterion: source's index in the slide-id list must be stable.""" + prs = _seed_presentation_with(3) + source = prs.slides[0] + source_id = source.slide_id + + prs.slides.duplicate(source) + + # ---source still at index 0, its id unchanged--- + assert prs.slides[0].slide_id == source_id + + def it_assigns_a_unique_slide_id_to_the_duplicate(self): + prs = _seed_presentation_with(2) + ids_before = {s.slide_id for s in prs.slides} + + new_slide = prs.slides.duplicate(prs.slides[0]) + + assert new_slide.slide_id not in ids_before + assert new_slide.slide_id > max(ids_before) + + def it_creates_a_new_SlidePart_with_a_unique_partname(self): + prs = _seed_presentation_with(2) + partnames_before = {prs.slides[i].part.partname for i in range(len(prs.slides))} + + new_slide = prs.slides.duplicate(prs.slides[0]) + + assert new_slide.part.partname not in partnames_before + + +# --------------------------------------------------------------------------- +# `Slide.duplicate()` — convenience alias. +# --------------------------------------------------------------------------- + + +class DescribeSlide_Duplicate(object): + """Unit-test suite for `pptx.slide.Slide.duplicate`.""" + + def it_exposes_a_duplicate_method(self): + assert callable(getattr(Slide, "duplicate", None)) + + def it_delegates_to_Slides_duplicate_on_the_owning_presentation(self, request): + # ---Mock chain: slide.part.package.presentation_part.presentation.slides--- + slides_ = instance_mock(request, Slides) + prs_ = instance_mock(request, type("Prs", (), {"slides": None})) + prs_.slides = slides_ + prs_part_ = instance_mock(request, type("PresPart", (), {"presentation": None})) + prs_part_.presentation = prs_ + slide_part_ = instance_mock(request, SlidePart) + slide_part_.package.presentation_part = prs_part_ + slide = Slide(None, slide_part_) # pyright: ignore[reportArgumentType] + + slide.duplicate(index=2) + + slides_.duplicate.assert_called_once_with(slide, 2) + + def it_passes_None_index_through_unchanged(self, request): + slides_ = instance_mock(request, Slides) + prs_ = instance_mock(request, type("Prs", (), {"slides": None})) + prs_.slides = slides_ + prs_part_ = instance_mock(request, type("PresPart", (), {"presentation": None})) + prs_part_.presentation = prs_ + slide_part_ = instance_mock(request, SlidePart) + slide_part_.package.presentation_part = prs_part_ + slide = Slide(None, slide_part_) # pyright: ignore[reportArgumentType] + + slide.duplicate() + + slides_.duplicate.assert_called_once_with(slide, None) + + def it_returns_the_value_from_Slides_duplicate(self, request): + new_slide_ = instance_mock(request, Slide) + slides_ = instance_mock(request, Slides) + slides_.duplicate.return_value = new_slide_ + prs_ = instance_mock(request, type("Prs", (), {"slides": None})) + prs_.slides = slides_ + prs_part_ = instance_mock(request, type("PresPart", (), {"presentation": None})) + prs_part_.presentation = prs_ + slide_part_ = instance_mock(request, SlidePart) + slide_part_.package.presentation_part = prs_part_ + slide = Slide(None, slide_part_) # pyright: ignore[reportArgumentType] + + result = slide.duplicate() + + assert result is new_slide_ + + +# --------------------------------------------------------------------------- +# Part-graph + dedup invariants. +# --------------------------------------------------------------------------- + + +class DescribeSlideDuplicate_PartGraph(object): + """Verify slide duplication maintains the part graph invariants.""" + + def it_creates_a_new_unique_relationship_in_presentation_rels(self): + prs = _seed_presentation_with(2) + rIds_before = {rId for rId, _ in prs.part.rels.items()} + + prs.slides.duplicate(prs.slides[0]) + + rIds_after = {rId for rId, _ in prs.part.rels.items()} + new_rIds = rIds_after - rIds_before + assert len(new_rIds) == 1 + + def it_grows_the_slide_part_count_by_exactly_one(self): + prs = _seed_presentation_with(2) + n_before = _slide_part_count(prs) + + prs.slides.duplicate(prs.slides[0]) + + assert _slide_part_count(prs) == n_before + 1 + + def it_shares_the_slide_layout_part_with_the_source(self): + prs = _seed_presentation_with(2) + source_layout = prs.slides[0].slide_layout + + new_slide = prs.slides.duplicate(prs.slides[0]) + + # ---both slides resolve to the SAME SlideLayoutPart instance--- + assert new_slide.slide_layout.part is source_layout.part + + def it_isolates_modifications_to_the_duplicate_from_the_source(self): + prs = _seed_presentation_with(1) + layout = prs.slide_layouts[6] + source = prs.slides.add_slide(layout) + source.shapes.add_textbox( + Inches(1), Inches(1), Inches(2), Inches(1) + ).text_frame.text = "original" + + new_slide = prs.slides.duplicate(source) + # ---mutate duplicate's text--- + textbox = next(shp for shp in new_slide.shapes if getattr(shp, "has_text_frame", False)) + textbox.text_frame.text = "mutated" + + # ---source's textbox unchanged--- + source_text = next( + shp for shp in source.shapes if getattr(shp, "has_text_frame", False) + ).text_frame.text + assert source_text == "original" + + def it_serializes_duplicate_xml_equivalent_to_source_before_mutation(self): + """Pre-mutation, dup's matches source modulo r:id substitution.""" + prs = _seed_presentation_with(1) + layout = prs.slide_layouts[6] + source = prs.slides.add_slide(layout) + source.shapes.add_textbox( + Inches(1), Inches(1), Inches(2), Inches(1) + ).text_frame.text = "hello" + + new_slide = prs.slides.duplicate(source) + + # ---both slides have the same number of shapes with the same text--- + assert len(new_slide.shapes) == len(source.shapes) + assert ( + next( + shp.text_frame.text + for shp in new_slide.shapes + if getattr(shp, "has_text_frame", False) + ) + == "hello" + ) + + +class DescribeSlideDuplicate_ImageDedup(object): + """The package-level image-dedup invariant: shared image parts stay shared.""" + + def it_does_not_increase_image_part_count_when_duplicating_a_slide_with_an_image(self): + prs = Presentation() + layout = prs.slide_layouts[6] + slide = prs.slides.add_slide(layout) + slide.shapes.add_picture(str(_PNG_PATH), Inches(1), Inches(1)) + n_images_before = _image_part_count(prs) + + prs.slides.duplicate(slide) + + assert _image_part_count(prs) == n_images_before + + def it_shares_the_image_part_with_the_source(self): + prs = Presentation() + layout = prs.slide_layouts[6] + slide = prs.slides.add_slide(layout) + pic_src = slide.shapes.add_picture(str(_PNG_PATH), Inches(1), Inches(1)) + n_before = _image_part_count(prs) + + new_slide = prs.slides.duplicate(slide) + + pic_dup = next(shp for shp in new_slide.shapes if shp.shape_type == 13) + assert pic_dup.image.blob == pic_src.image.blob + # ---image part count unchanged proves the share at package level--- + assert _image_part_count(prs) == n_before + + def it_round_trips_a_duplicated_slide_with_an_image(self): + prs = Presentation() + layout = prs.slide_layouts[6] + slide = prs.slides.add_slide(layout) + slide.shapes.add_picture(str(_PNG_PATH), Inches(1), Inches(1)) + + prs.slides.duplicate(slide) + round_tripped = _round_trip(prs) + + # ---both slides expose pictures referencing the same blob--- + pictures_per_slide = [ + [shp for shp in s.shapes if shp.shape_type == 13] for s in round_tripped.slides + ] + assert all(len(pics) == 1 for pics in pictures_per_slide) + blobs = [pics[0].image.blob for pics in pictures_per_slide] + assert blobs[0] == blobs[1] + assert len(blobs[0]) > 0 + + +class DescribeSlideDuplicate_NotesSlide(object): + """Notes-slide handling: duplicate gets its own NotesSlidePart.""" + + def it_gives_the_duplicate_its_own_notes_slide_part(self): + prs = _seed_presentation_with(1) + source = prs.slides[0] + source.notes_slide.notes_text_frame.text = "speaker notes" + + new_slide = prs.slides.duplicate(source) + + assert new_slide.has_notes_slide is True + # ---the two notes-slide parts are distinct--- + assert new_slide.notes_slide.part is not source.notes_slide.part + + def it_carries_the_notes_text_to_the_duplicate(self): + prs = _seed_presentation_with(1) + source = prs.slides[0] + source.notes_slide.notes_text_frame.text = "speaker notes" + + new_slide = prs.slides.duplicate(source) + + assert new_slide.notes_slide.notes_text_frame.text == "speaker notes" + + def it_isolates_notes_edits_on_the_duplicate_from_the_source(self): + prs = _seed_presentation_with(1) + source = prs.slides[0] + source.notes_slide.notes_text_frame.text = "original notes" + + new_slide = prs.slides.duplicate(source) + new_slide.notes_slide.notes_text_frame.text = "mutated notes" + + assert source.notes_slide.notes_text_frame.text == "original notes" + + def it_does_not_create_a_notes_slide_when_source_has_none(self): + prs = _seed_presentation_with(1) + source = prs.slides[0] + # ---source has NO notes-slide; do not call .notes_slide which lazily creates one--- + assert source.has_notes_slide is False + + new_slide = prs.slides.duplicate(source) + + assert new_slide.has_notes_slide is False + + def it_rewires_the_duplicate_notes_slide_back_ref_to_the_new_slide(self): + """Anti — community gotcha #961: notes-slide must back-ref to new slide.""" + prs = _seed_presentation_with(1) + source = prs.slides[0] + source.notes_slide.notes_text_frame.text = "x" + + new_slide = prs.slides.duplicate(source) + + # ---the new notes-slide's RT.SLIDE rel target is the NEW slide part--- + new_notes_part = new_slide.notes_slide.part + back_ref_target = new_notes_part.part_related_by(RT.SLIDE) + assert back_ref_target is new_slide.part + + def it_round_trips_a_slide_with_notes(self): + prs = _seed_presentation_with(1) + source = prs.slides[0] + source.notes_slide.notes_text_frame.text = "speaker notes" + + prs.slides.duplicate(source) + round_tripped = _round_trip(prs) + + for s in round_tripped.slides: + assert s.has_notes_slide is True + assert s.notes_slide.notes_text_frame.text == "speaker notes" + + +# --------------------------------------------------------------------------- +# Defensive XPath check — NO unmapped rId references should remain. +# --------------------------------------------------------------------------- + + +class DescribeSlideDuplicate_RIdRemap(object): + """Every r:* attribute on the duplicate must resolve to a rel on the new slide.""" + + def it_resolves_every_rId_reference_in_the_duplicate_xml(self): + """No dangling rIds — pivots on the dedup invariant tested at runtime.""" + prs = Presentation() + layout = prs.slide_layouts[6] + slide = prs.slides.add_slide(layout) + slide.shapes.add_picture(str(_PNG_PATH), Inches(1), Inches(1)) + slide.shapes.add_textbox( + Inches(1), Inches(2), Inches(2), Inches(1) + ).text_frame.text = "rId test" + + new_slide = prs.slides.duplicate(slide) + + # ---collect every attribute value in the relationships namespace--- + rId_refs = set() + for el in new_slide.element.iter(): + for attr_name, attr_val in el.attrib.items(): + if attr_name.startswith(_RELS_NS): + rId_refs.add(attr_val) + # ---each must resolve in the new slide part's rels--- + new_part_rIds = set(new_slide.part.rels) + unresolved = rId_refs - new_part_rIds + assert unresolved == set(), f"unresolved rIds in duplicated slide: {unresolved}" + + +# --------------------------------------------------------------------------- +# Round-trip integration tests — open → duplicate → save → reopen. +# --------------------------------------------------------------------------- + + +class DescribeSlideDuplicate_RoundTrip(object): + """Open → duplicate → save → reopen integration coverage.""" + + def it_round_trips_a_basic_duplicate(self): + prs = _seed_presentation_with(2) + ids_before = [s.slide_id for s in prs.slides] + + prs.slides.duplicate(prs.slides[0]) + round_tripped = _round_trip(prs) + + assert len(round_tripped.slides) == 3 + ids_after = [s.slide_id for s in round_tripped.slides] + # ---source ids stable, duplicate inserted at index 1--- + assert ids_after[0] == ids_before[0] + assert ids_after[2] == ids_before[1] + assert ids_after[1] not in ids_before + + def it_round_trips_a_duplicate_at_a_specific_index(self): + prs = _seed_presentation_with(3) + + prs.slides.duplicate(prs.slides[2], index=0) + round_tripped = _round_trip(prs) + + assert len(round_tripped.slides) == 4 + + def it_round_trips_Slide_duplicate_alias(self): + prs = _seed_presentation_with(2) + + prs.slides[0].duplicate() + round_tripped = _round_trip(prs) + + assert len(round_tripped.slides) == 3 + + def it_preserves_shape_count_through_round_trip(self): + prs = _seed_presentation_with(1) + layout = prs.slide_layouts[6] + source = prs.slides.add_slide(layout) + source.shapes.add_textbox( + Inches(1), Inches(1), Inches(2), Inches(1) + ).text_frame.text = "kept" + source.shapes.add_textbox( + Inches(1), Inches(2.5), Inches(2), Inches(1) + ).text_frame.text = "also kept" + n_shapes_source = len(source.shapes) + + prs.slides.duplicate(source) + round_tripped = _round_trip(prs) + + # ---last slide is the duplicate; shape count matches source--- + assert len(round_tripped.slides[-1].shapes) == n_shapes_source + + def it_does_not_mutate_image_part_count_through_round_trip(self): + """Anti-criterion: image dedup survives save → reopen.""" + prs = Presentation() + layout = prs.slide_layouts[6] + slide = prs.slides.add_slide(layout) + slide.shapes.add_picture(str(_PNG_PATH), Inches(1), Inches(1)) + n_images_before = _image_part_count(prs) + + prs.slides.duplicate(slide) + round_tripped = _round_trip(prs) + + assert _image_part_count(round_tripped) == n_images_before + + def it_does_not_carry_comments_through_a_duplicate(self): + """Phase-2 scope: comments parts are dropped on duplicate (documented). + + We don't have a Phase-2 API to add comments yet, so this test + documents the behavior via a hand-crafted source-slide rel: if + the source had a `RT.COMMENTS` rel pointing at some part, the + duplicate must NOT carry it. This is a forward-looking guard + for when comments are added in a later phase. + """ + prs = _seed_presentation_with(2) + source = prs.slides[0] + + new_slide = prs.slides.duplicate(source) + + # ---no slide ever has RT.COMMENTS rels in this build, but the + # invariant we want is: even if it had one, it would be dropped. + # Document the invariant by asserting absence on dup--- + with pytest.raises(KeyError): + new_slide.part.part_related_by(RT.COMMENTS)