From 799b214a79ab24326ee4075265b3ed1adcf3c6cd Mon Sep 17 00:00:00 2001 From: Steve Canny Date: Fri, 2 Aug 2024 16:17:29 -0700 Subject: [PATCH] fix: #972 next-slide-id fails when max used https://github.com/scanny/python-pptx/issues/972 Naive "max + 1" slide-id allocation algorithm assumed that slide-ids were assigned from bottom up. Apparently some client assigns slide ids from the top (2,147,483,647) down and the naive algorithm would assign an invalid slide-id in that case. Detect when the assigned id is out-of-range and fall-back to a robust algorithm for assigning a valid id based on a "first unused starting at bottom" policy. --- src/pptx/oxml/presentation.py | 25 +++++++++++++++++++++---- tests/oxml/test_presentation.py | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/pptx/oxml/presentation.py b/src/pptx/oxml/presentation.py index 12c6751f..25447249 100644 --- a/src/pptx/oxml/presentation.py +++ b/src/pptx/oxml/presentation.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Callable +from typing import TYPE_CHECKING, Callable, cast from pptx.oxml.simpletypes import ST_SlideId, ST_SlideSizeCoordinate, XsdString from pptx.oxml.xmlchemy import BaseOxmlElement, RequiredAttribute, ZeroOrMore, ZeroOrOne @@ -67,14 +67,31 @@ def add_sldId(self, rId: str) -> CT_SlideId: return self._add_sldId(id=self._next_id, rId=rId) @property - def _next_id(self): + def _next_id(self) -> int: """The next available slide ID as an `int`. Valid slide IDs start at 256. The next integer value greater than the max value in use is chosen, which minimizes that chance of reusing the id of a deleted slide. """ - id_str_lst = self.xpath("./p:sldId/@id") - return max([255] + [int(id_str) for id_str in id_str_lst]) + 1 + MIN_SLIDE_ID = 256 + MAX_SLIDE_ID = 2147483647 + + used_ids = [int(s) for s in cast(list[str], self.xpath("./p:sldId/@id"))] + simple_next = max([MIN_SLIDE_ID - 1] + used_ids) + 1 + if simple_next <= MAX_SLIDE_ID: + return simple_next + + # -- fall back to search for next unused from bottom -- + valid_used_ids = sorted(id for id in used_ids if (MIN_SLIDE_ID <= id <= MAX_SLIDE_ID)) + return ( + next( + candidate_id + for candidate_id, used_id in enumerate(valid_used_ids, start=MIN_SLIDE_ID) + if candidate_id != used_id + ) + if valid_used_ids + else 256 + ) class CT_SlideMasterIdList(BaseOxmlElement): diff --git a/tests/oxml/test_presentation.py b/tests/oxml/test_presentation.py index 1607ab5c..d2a47b27 100644 --- a/tests/oxml/test_presentation.py +++ b/tests/oxml/test_presentation.py @@ -38,3 +38,26 @@ def it_can_add_a_sldId_element_as_a_child(self): def it_knows_the_next_available_slide_id(self, sldIdLst_cxml: str, expected_value: int): sldIdLst = cast(CT_SlideIdList, element(sldIdLst_cxml)) assert sldIdLst._next_id == expected_value + + @pytest.mark.parametrize( + ("sldIdLst_cxml", "expected_value"), + [ + ("p:sldIdLst/p:sldId{id=2147483646}", 2147483647), + ("p:sldIdLst/p:sldId{id=2147483647}", 256), + # -- 2147483648 is not a valid id but shouldn't stop us from finding a one that is -- + ("p:sldIdLst/p:sldId{id=2147483648}", 256), + ("p:sldIdLst/(p:sldId{id=256},p:sldId{id=2147483647})", 257), + ("p:sldIdLst/(p:sldId{id=256},p:sldId{id=2147483647},p:sldId{id=257})", 258), + # -- 245 is also not a valid id but that shouldn't change the result either -- + ("p:sldIdLst/(p:sldId{id=245},p:sldId{id=2147483647},p:sldId{id=256})", 257), + ], + ) + def and_it_chooses_a_valid_slide_id_when_max_slide_id_is_used_for_a_slide( + self, sldIdLst_cxml: str, expected_value: int + ): + sldIdLst = cast(CT_SlideIdList, element(sldIdLst_cxml)) + + slide_id = sldIdLst._next_id + + assert 256 <= slide_id <= 2147483647 + assert slide_id == expected_value