MAINT: Refactor Fit / Zoom parameters (#1437)

Introduce a new `PyPDF2.generic.Fit` class which captures the type and parameter for how a page should be fit in the viewer (e.g. when clicking on a PDF-internal link).

The class has one method for each fit type which allows users to discover the different types via their IDE, e.g. `Fit.xyz(left=123, top=456, zoom=2)`.

## Breaking Change

This **introduces a breaking change** that needs a major version bump. Affected are the following methods:

* `PdfMerger.add_outline_item` used `fit: FitType` and `*args: ZoomArgType` parameters
* `PdfWriter.add_outline_item` used `fit: FitType` and `*args: ZoomArgType` parameters
* `AnnotationBuilder.link`  used `fit: FitType = "/Fit"` and `fit_args: Tuple[ZoomArgType, ...] = tuple()` instead.

Instead of having two arguments, we now have only one. To that argument, an object of the new `Fit` class must be passed.

## Why *args is problematic

Using the `*args` pattern is problematic for two reasons:

* **User-code readability**:
  * People cannot use the more expressive keyword-argument syntax in their code for the non-fit parameters
  * People may or may not use the keyword-parameter for the fit parameters
* **Library extensions**: PyPDF2 cannot easily add new parameter to those functions; #1371 is the latest PR that stumbled over this issue
* **Two parameter for one thing**: In general I don't like if we have two parameters for one topic

PyPDF2/_merger.py

@@ -55,9 +55,11 @@
 from .constants import PagesAttributes as PA
 from .constants import TypArguments, TypFitArguments
 from .generic import (
+    PAGE_FIT,
     ArrayObject,
     Destination,
     DictionaryObject,
+    Fit,
     FloatObject,
     IndirectObject,
     NameObject,
@@ -217,7 +219,7 @@ def merge(
             outline_item_typ = OutlineItem(
                 TextStringObject(outline_item),
                 NumberObject(self.id_count),
-                NameObject(TypFitArguments.FIT),
+                Fit.fit(),
             )
             self.outline += [outline_item_typ, outline]  # type: ignore
         else:
@@ -662,8 +664,7 @@ def add_outline_item(
         color: Optional[Tuple[float, float, float]] = None,
         bold: bool = False,
         italic: bool = False,
-        fit: FitType = "/Fit",
-        *args: ZoomArgType,
+        fit: Fit = PAGE_FIT,
         pagenum: Optional[int] = None,  # deprecated
     ) -> IndirectObject:
         """
@@ -677,8 +678,7 @@ def add_outline_item(
             from 0.0 to 1.0
         :param bool bold: Outline item font is bold
         :param bool italic: Outline item font is italic
-        :param str fit: The fit of the destination page. See
-            :meth:`add_link()<add_link>` for details.
+        :param Fit fit: The fit of the destination page.
         """
         if page_number is not None and pagenum is not None:
             raise ValueError(
@@ -699,7 +699,13 @@ def add_outline_item(
         if writer is None:
             raise RuntimeError(ERR_CLOSED_WRITER)
         return writer.add_outline_item(
-            title, page_number, parent, color, bold, italic, fit, *args
+            title,
+            page_number,
+            parent,
+            color,
+            bold,
+            italic,
+            fit,
         )
 
     def addBookmark(
@@ -719,7 +725,13 @@ def addBookmark(
         """
         deprecate_with_replacement("addBookmark", "add_outline_item")
         return self.add_outline_item(
-            title, pagenum, parent, color, bold, italic, fit, *args
+            title,
+            pagenum,
+            parent,
+            color,
+            bold,
+            italic,
+            Fit(fit_type=fit, fit_args=args),
         )
 
     def add_bookmark(
@@ -739,7 +751,13 @@ def add_bookmark(
         """
         deprecate_with_replacement("addBookmark", "add_outline_item")
         return self.add_outline_item(
-            title, pagenum, parent, color, bold, italic, fit, *args
+            title,
+            pagenum,
+            parent,
+            color,
+            bold,
+            italic,
+            Fit(fit_type=fit, fit_args=args),
         )
 
     def addNamedDestination(self, title: str, pagenum: int) -> None:  # pragma: no cover
@@ -780,8 +798,7 @@ def add_named_destination(
         dest = Destination(
             TextStringObject(title),
             NumberObject(page_number),
-            NameObject(TypFitArguments.FIT_H),
-            NumberObject(826),
+            Fit.fit_horizontally(top=826),
         )
         self.named_dests.append(dest)
 

PyPDF2/_reader.py

@@ -85,6 +85,7 @@
     DictionaryObject,
     EncodedStreamObject,
     Field,
+    Fit,
     FloatObject,
     IndirectObject,
     NameObject,
@@ -892,23 +893,20 @@ def _build_destination(
         ):
 
             page = NullObject()
-            typ = TextStringObject("/Fit")
-            return Destination(title, page, typ)
+            return Destination(title, page, Fit.fit())
         else:
             page, typ = array[0:2]  # type: ignore
             array = array[2:]
             try:
-                return Destination(title, page, typ, *array)  # type: ignore
+                return Destination(title, page, Fit(fit_type=typ, fit_args=array))  # type: ignore
             except PdfReadError:
                 logger_warning(f"Unknown destination: {title} {array}", __name__)
                 if self.strict:
                     raise
                 # create a link to first Page
                 tmp = self.pages[0].indirect_reference
                 indirect_reference = NullObject() if tmp is None else tmp
-                return Destination(
-                    title, indirect_reference, TextStringObject("/Fit")  # type: ignore
-                )
+                return Destination(title, indirect_reference, Fit.fit())  # type: ignore
 
     def _build_outline_item(self, node: DictionaryObject) -> Optional[Destination]:
         dest, title, outline_item = None, None, None

PyPDF2/_writer.py

@@ -83,6 +83,7 @@
 from .constants import TrailerKeys as TK
 from .constants import TypFitArguments, UserAccessPermissions
 from .generic import (
+    PAGE_FIT,
     AnnotationBuilder,
     ArrayObject,
     BooleanObject,
@@ -91,6 +92,7 @@
     DecodedStreamObject,
     Destination,
     DictionaryObject,
+    Fit,
     FloatObject,
     IndirectObject,
     NameObject,
@@ -110,7 +112,6 @@
     LayoutType,
     OutlineItemType,
     PagemodeType,
-    ZoomArgsType,
     ZoomArgType,
 )
 
@@ -454,9 +455,10 @@ def open_destination(
             try:
                 page, typ = oa[0:2]  # type: ignore
                 array = oa[2:]
-                return Destination("OpenAction", page, typ, *array)  # type: ignore
-            except Exception:
-                raise Exception(f"Invalid Destination {oa}")
+                fit = Fit(typ, tuple(array))
+                return Destination("OpenAction", page, fit)
+            except Exception as exc:
+                raise Exception(f"Invalid Destination {oa}: {exc}")
         else:
             return None
 
@@ -477,7 +479,7 @@ def open_destination(self, dest: Union[None, str, Destination, PageObject]) -> N
                 dest.indirect_reference
                 if dest.indirect_reference is not None
                 else NullObject(),
-                TextStringObject("/Fit"),
+                PAGE_FIT,
             ).dest_array
 
     def add_js(self, javascript: str) -> None:
@@ -1344,8 +1346,7 @@ def add_outline_item(
         color: Optional[Union[Tuple[float, float, float], str]] = None,
         bold: bool = False,
         italic: bool = False,
-        fit: FitType = "/Fit",
-        *args: ZoomArgType,
+        fit: Fit = PAGE_FIT,
         pagenum: Optional[int] = None,  # deprecated
     ) -> IndirectObject:
         """
@@ -1359,8 +1360,7 @@ def add_outline_item(
             from 0.0 to 1.0 or as a Hex String (#RRGGBB)
         :param bool bold: Outline item font is bold
         :param bool italic: Outline item font is italic
-        :param str fit: The fit of the destination page. See
-            :meth:`add_link()<add_link>` for details.
+        :param Fit fit: The fit of the destination page.
         """
         if page_number is not None and pagenum is not None:
             raise ValueError(
@@ -1378,14 +1378,11 @@ def add_outline_item(
         if page_number is None:
             raise ValueError("page_number may not be None")
         page_ref = NumberObject(page_number)
-        zoom_args: ZoomArgsType = [
-            NullObject() if a is None else NumberObject(a) for a in args
-        ]
+
         dest = Destination(
             NameObject("/" + title + " outline item"),
             page_ref,
-            NameObject(fit),
-            *zoom_args,
+            fit,
         )
 
         action_ref = self._add_object(
@@ -1420,7 +1417,13 @@ def add_bookmark(
         """
         deprecate_with_replacement("add_bookmark", "add_outline_item")
         return self.add_outline_item(
-            title, pagenum, parent, color, bold, italic, fit, *args
+            title,
+            pagenum,
+            parent,
+            color,
+            bold,
+            italic,
+            Fit(fit_type=fit, fit_args=args),
         )
 
     def addBookmark(
@@ -1441,7 +1444,13 @@ def addBookmark(
         """
         deprecate_with_replacement("addBookmark", "add_outline_item")
         return self.add_outline_item(
-            title, pagenum, parent, color, bold, italic, fit, *args
+            title,
+            pagenum,
+            parent,
+            color,
+            bold,
+            italic,
+            Fit(fit_type=fit, fit_args=args),
         )
 
     def add_outline(self) -> None:
@@ -1813,8 +1822,7 @@ def add_link(
             rect=rect,
             border=border,
             target_page_index=page_destination,
-            fit=fit,
-            fit_args=args,
+            fit=Fit(fit_type=fit, fit_args=args),
         )
         return self.add_annotation(page_number=pagenum, annotation=annotation)
 
@@ -2066,8 +2074,9 @@ def add_annotation(self, page_number: int, annotation: Dict[str, Any]) -> None:
             dest = Destination(
                 NameObject("/LinkName"),
                 tmp["target_page_index"],
-                tmp["fit"],
-                *tmp["fit_args"],
+                Fit(
+                    fit_type=tmp["fit"], fit_args=dict(tmp)["fit_args"]
+                ),  # I have no clue why this dict-hack is necessary
             )
             to_add[NameObject("/Dest")] = dest.dest_array
 

PyPDF2/generic/__init__.py

@@ -58,6 +58,7 @@
     TreeObject,
     read_object,
 )
+from ._fit import Fit
 from ._outline import Bookmark, OutlineItem
 from ._rectangle import RectangleObject
 from ._utils import (
@@ -96,6 +97,9 @@ def createStringObject(
     return create_string_object(string, forced_encoding)
 
 
+PAGE_FIT = Fit.fit()
+
+
 __all__ = [
     # Base types
     "BooleanObject",
@@ -109,6 +113,9 @@ def createStringObject(
     "ByteStringObject",
     # Annotations
     "AnnotationBuilder",
+    # Fit
+    "Fit",
+    "PAGE_FIT",
     # Data structures
     "ArrayObject",
     "DictionaryObject",

PyPDF2/generic/_annotations.py

@@ -4,11 +4,11 @@
     BooleanObject,
     FloatObject,
     NameObject,
-    NullObject,
     NumberObject,
     TextStringObject,
 )
 from ._data_structures import ArrayObject, DictionaryObject
+from ._fit import DEFAULT_FIT, Fit
 from ._rectangle import RectangleObject
 from ._utils import hex_to_rgb
 
@@ -198,8 +198,7 @@ def link(
         border: Optional[ArrayObject] = None,
         url: Optional[str] = None,
         target_page_index: Optional[int] = None,
-        fit: FitType = "/Fit",
-        fit_args: Tuple[ZoomArgType, ...] = tuple(),
+        fit: Fit = DEFAULT_FIT,
     ) -> DictionaryObject:
         """
         Add a link to the document.
@@ -223,30 +222,7 @@ def link(
         :param str url: Link to a website (if you want to make an external link)
         :param int target_page_index: index of the page to which the link should go
                                 (if you want to make an internal link)
-        :param str fit: Page fit or 'zoom' option (see below). Additional arguments may need
-            to be supplied. Passing ``None`` will be read as a null value for that coordinate.
-        :param Tuple[int, ...] fit_args: Parameters for the fit argument.
-
-
-        .. list-table:: Valid ``fit`` arguments (see Table 8.2 of the PDF 1.7 reference for details)
-           :widths: 50 200
-
-           * - /Fit
-             - No additional arguments
-           * - /XYZ
-             - [left] [top] [zoomFactor]
-           * - /FitH
-             - [top]
-           * - /FitV
-             - [left]
-           * - /FitR
-             - [left] [bottom] [right] [top]
-           * - /FitB
-             - No additional arguments
-           * - /FitBH
-             - [top]
-           * - /FitBV
-             - [left]
+        :param Fit fit: Page fit or 'zoom' option.
         """
         from ..types import BorderArrayType
 
@@ -287,15 +263,12 @@ def link(
                 }
             )
         if is_internal:
-            fit_arg_ready = [
-                NullObject() if a is None else NumberObject(a) for a in fit_args
-            ]
             # This needs to be updated later!
             dest_deferred = DictionaryObject(
                 {
                     "target_page_index": NumberObject(target_page_index),
-                    "fit": NameObject(fit),
-                    "fit_args": ArrayObject(fit_arg_ready),
+                    "fit": NameObject(fit.fit_type),
+                    "fit_args": fit.fit_args,
                 }
             )
             link_obj[NameObject("/Dest")] = dest_deferred