fix: scanny#972 next-slide-id fails when max used

scanny#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.

Author: scanny

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):

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