refactor(editor): share atomic PDF write via app/pdf_io (R3)

app/base.py

@@ -1,6 +1,5 @@
 """PDFApps – BasePage: standard page layout (header + scroll + action bar)."""
 
-import contextlib
 import os
 import subprocess
 import sys
@@ -13,6 +12,7 @@
 from PySide6.QtWidgets import (QWidget, QVBoxLayout, QHBoxLayout, QFileDialog,
                                QPushButton, QLabel)
 
+from app import pdf_io
 from app.constants import DESKTOP, ACCENT
 from app.i18n import t
 from app.utils import ToolHeader, ActionBar, scrolled, _paint_bg
@@ -413,31 +413,31 @@ def _check_not_same_path(dst: str,
         input and output, opening the output for writing truncates the
         input before the writer's lazy stream reads complete and we
         get silent dataloss + corrupted output.
+
+        Thin wrapper around :func:`app.pdf_io.check_not_same_path`
+        (R3): the logic now lives in the low-level ``pdf_io`` module so
+        the visual editor can reuse the exact same guard without
+        importing this Qt-heavy page base. Kept as a ``@staticmethod``
+        so ``BasePage._check_not_same_path`` / ``self._check_not_same_path``
+        call sites and the unit tests stay unchanged.
         """
-        try:
-            dst_real = os.path.realpath(dst)
-        except OSError:
-            return
-        for src in (sources or ()):
-            if not src:
-                continue
-            try:
-                if os.path.realpath(src) == dst_real:
-                    raise RuntimeError(t("tool.err.same_source_output"))
-            except OSError:
-                continue
+        pdf_io.check_not_same_path(dst, sources)
 
     @staticmethod
     def _atomic_pdf_write(writer, dst: str, *,
                           sources: "Iterable[str] | None" = None,
                           save_opts: "dict | None" = None) -> None:
         """Write a PdfWriter (pypdf) or fitz.Document to ``dst`` atomically.
 
-        Two defensive layers fix the silent dataloss bug where opening
-        ``open(dst, "wb")`` truncates the input file BEFORE the writer's
-        lazy stream reads complete (PdfWriter holds references into
-        the PdfReader; same applies to fitz.Document.save() with
-        incremental flags).
+        Thin wrapper around :func:`app.pdf_io.atomic_pdf_write` (R3):
+        the tempfile + ``os.replace`` + same-source-guard logic now
+        lives in the low-level ``pdf_io`` module so ``TabEditar._run``
+        reuses the identical write path instead of duplicating it.
+        Kept as a ``@staticmethod`` with the same signature so the
+        ~20 ``self._atomic_pdf_write`` / ``BasePage._atomic_pdf_write``
+        call sites and the regression tests are untouched.
+
+        The two defensive layers are unchanged:
 
         1. Reject up-front if ``dst`` resolves to any path in
            ``sources`` (via ``os.path.realpath``) — this catches the
@@ -449,44 +449,17 @@ def _atomic_pdf_write(writer, dst: str, *,
 
         ``writer`` may be a pypdf ``PdfWriter`` (uses ``writer.write(fh)``)
         or a PyMuPDF ``fitz.Document`` (uses ``writer.save(tmp)``).
-        Anything else with a ``.write(fh)`` method is accepted.
+        Anything else with a ``.write(fh)`` method is accepted. The
+        writer is left OPEN (BasePage tools never save back onto the
+        input handle); the editor opts into ``close_writer`` directly
+        via ``pdf_io.atomic_pdf_write``.
 
         Raises :class:`RuntimeError` with a translated message when the
         same-source check fails; the caller's existing ``show_error``
         path surfaces it as a friendly dialog.
         """
-        BasePage._check_not_same_path(dst, sources)
-
-        dst_dir = os.path.dirname(dst) or os.getcwd()
-        # mkstemp returns an OS-level fd; close via os.fdopen so the
-        # writer can stream into it. Same-volume placement guarantees
-        # os.replace() stays atomic.
-        fd, tmp = tempfile.mkstemp(suffix=".pdf", dir=dst_dir)
-        # Detect fitz.Document via its module to avoid importing fitz
-        # at base.py load time (every page imports BasePage). Modern
-        # PyMuPDF reports module="pymupdf"; legacy versions used "fitz".
-        # Both expose Document.save(path, ...).
-        writer_mod = type(writer).__module__
-        is_fitz_doc = (writer_mod.startswith("pymupdf")
-                       or writer_mod.startswith("fitz")) and hasattr(writer, "save")
-        try:
-            if is_fitz_doc:
-                # fitz.Document.save(path, ...) accepts a filesystem
-                # path and writes through cleanly. We close the fd
-                # we opened first so save() can take exclusive access.
-                os.close(fd)
-                writer.save(tmp, **(save_opts or {}))
-            else:
-                # pypdf.PdfWriter (and anything else with .write(fh))
-                # streams into the open file handle.
-                with os.fdopen(fd, "wb") as fh:
-                    writer.write(fh)
-            os.replace(tmp, dst)
-        except Exception:
-            with contextlib.suppress(Exception):
-                if os.path.exists(tmp):
-                    os.unlink(tmp)
-            raise
+        pdf_io.atomic_pdf_write(writer, dst, sources=sources,
+                                save_opts=save_opts)
 
     # ── background-task helper ────────────────────────────────────────────
 

app/editor/tab.py

@@ -25,6 +25,7 @@
 from app.editor.canvas import PdfEditCanvas, _get_icon_cursor
 from app.editor.dialogs import _NoteDialog
 from app.editor.apply_edits import apply_pending_edits
+from app.pdf_io import atomic_pdf_write
 
 
 _log = logging.getLogger(__name__)
@@ -1266,37 +1267,39 @@ def _run(self):
             # unexplained illegibly-shrunk line.
             _apply_result = apply_pending_edits(doc, self._pending)
             text_fit_warnings = _apply_result.text_fit_warnings
-            fd, tmp = tempfile.mkstemp(prefix=".pdfapps_save_", suffix=".pdf",
-                                       dir=os.path.dirname(out) or ".")
-            os.close(fd)
-            try:
-                if encrypt_choice == "protect" and self._pdf_password:
-                    # Documented limitation: owner_pw == user_pw because we
-                    # only captured a single password from the load prompt
-                    # — the original owner password is not recoverable from
-                    # the input file. Future enhancement: ask the user for
-                    # a separate owner password.
-                    # ``_fitz_permissions_of`` already returns -1 on any
-                    # internal failure (PyMuPDF sentinel for "all perms"),
-                    # so a wrapping try/except here would be dead code.
-                    perms = self._fitz_permissions_of(doc)
-                    doc.save(
-                        tmp, garbage=4, deflate=True,
-                        encryption=fitz.PDF_ENCRYPT_AES_256,
-                        user_pw=self._pdf_password,
-                        owner_pw=self._pdf_password,
-                        permissions=perms,
-                    )
-                    _log.info(
-                        "Re-encrypted output with user password as owner")
-                else:
-                    doc.save(tmp, garbage=4, deflate=True)
-                doc.close()
-                os.replace(tmp, out)
-            except Exception:
-                try: os.unlink(tmp)
-                except OSError: pass
-                raise
+            # Atomic save via the shared low-level writer (R3): the exact
+            # tempfile + os.replace path every BasePage tool already uses,
+            # reused here instead of a duplicated mkstemp/save/replace
+            # block. ``close_writer=True`` preserves the editor's original
+            # save→close→replace ordering — ``doc`` was opened from
+            # ``self._doc_path`` so on Windows its handle must be released
+            # before the rename (the user may be overwriting the input).
+            # ``save_opts`` is built with ``dict(...)`` so the encryption
+            # kwargs (encryption=…, user_pw=…, owner_pw=…) read exactly as
+            # the direct ``doc.save`` call did before.
+            reencrypt = bool(encrypt_choice == "protect" and self._pdf_password)
+            if reencrypt:
+                # Documented limitation: owner_pw == user_pw because we
+                # only captured a single password from the load prompt
+                # — the original owner password is not recoverable from
+                # the input file. Future enhancement: ask the user for
+                # a separate owner password.
+                # ``_fitz_permissions_of`` already returns -1 on any
+                # internal failure (PyMuPDF sentinel for "all perms"),
+                # so a wrapping try/except here would be dead code.
+                perms = self._fitz_permissions_of(doc)
+                save_opts = dict(
+                    garbage=4, deflate=True,
+                    encryption=fitz.PDF_ENCRYPT_AES_256,
+                    user_pw=self._pdf_password,
+                    owner_pw=self._pdf_password,
+                    permissions=perms,
+                )
+            else:
+                save_opts = dict(garbage=4, deflate=True)
+            atomic_pdf_write(doc, out, save_opts=save_opts, close_writer=True)
+            if reencrypt:
+                _log.info("Re-encrypted output with user password as owner")
             self._pending.clear(); self._pending_list.clear()
             self._status(t("edit.status.saved", path=out))
             if text_fit_warnings:

app/pdf_io.py

@@ -0,0 +1,124 @@
+"""PDFApps – pdf_io: low-level atomic PDF write helpers.
+
+Extracted from :class:`app.base.BasePage` (R3) so the same safe-write
+logic is shared by BOTH the tool pages (via
+``BasePage._atomic_pdf_write``) and the visual editor
+(``TabEditar._run``) WITHOUT either side re-implementing the tempfile +
+``os.replace`` dance, the same-source guard, or the fitz/pypdf writer
+dispatch.
+
+This module is deliberately pure and low-level: it imports only stdlib
+at load time (``app.i18n`` is imported lazily inside the one function
+that needs a translated message). It pulls in no Qt and no other app
+module, so it sits at the bottom of the dependency graph and is safe to
+import from ``app.base`` and ``app.editor.tab`` alike with no import
+cycle.
+"""
+
+import contextlib
+import os
+import tempfile
+from typing import Iterable
+
+__all__ = ["atomic_pdf_write", "check_not_same_path"]
+
+
+def check_not_same_path(dst: str,
+                        sources: "Iterable[str] | None" = None) -> None:
+    """Raise RuntimeError if ``dst`` resolves to any of ``sources``.
+
+    Shared invariant for every tool that takes a PDF in and writes
+    a result back to disk: if the user picks the same path for
+    input and output, opening the output for writing truncates the
+    input before the writer's lazy stream reads complete and we
+    get silent dataloss + corrupted output.
+    """
+    from app.i18n import t
+    try:
+        dst_real = os.path.realpath(dst)
+    except OSError:
+        return
+    for src in (sources or ()):
+        if not src:
+            continue
+        try:
+            if os.path.realpath(src) == dst_real:
+                raise RuntimeError(t("tool.err.same_source_output"))
+        except OSError:
+            continue
+
+
+def atomic_pdf_write(writer, dst: str, *,
+                     sources: "Iterable[str] | None" = None,
+                     save_opts: "dict | None" = None,
+                     close_writer: bool = False) -> None:
+    """Write a PdfWriter (pypdf) or fitz.Document to ``dst`` atomically.
+
+    Two defensive layers fix the silent dataloss bug where opening
+    ``open(dst, "wb")`` truncates the input file BEFORE the writer's
+    lazy stream reads complete (PdfWriter holds references into
+    the PdfReader; same applies to fitz.Document.save() with
+    incremental flags).
+
+    1. Reject up-front if ``dst`` resolves to any path in
+       ``sources`` (via ``os.path.realpath``) — this catches the
+       "user picked the same path for input and output" case which
+       was producing corrupt output + losing the original.
+
+    2. Write to a same-directory tempfile and atomically rename to
+       ``dst`` via :func:`os.replace` (works on POSIX and Windows).
+
+    ``writer`` may be a pypdf ``PdfWriter`` (uses ``writer.write(fh)``)
+    or a PyMuPDF ``fitz.Document`` (uses ``writer.save(tmp)``).
+    Anything else with a ``.write(fh)`` method is accepted.
+
+    ``close_writer`` closes the writer AFTER a successful save but
+    BEFORE ``os.replace``. The visual editor needs this: it opens the
+    document from the SAME file it may be overwriting, so on Windows
+    the handle must be released before the rename or ``os.replace``
+    fails with a sharing violation. The tool pages leave the writer
+    open (default ``False``) — they never save back onto the input
+    handle, and some reuse the writer/doc after the call.
+
+    Raises :class:`RuntimeError` with a translated message when the
+    same-source check fails; the caller's existing ``show_error``
+    path surfaces it as a friendly dialog.
+    """
+    check_not_same_path(dst, sources)
+
+    dst_dir = os.path.dirname(dst) or os.getcwd()
+    # mkstemp returns an OS-level fd; close via os.fdopen so the
+    # writer can stream into it. Same-volume placement guarantees
+    # os.replace() stays atomic.
+    fd, tmp = tempfile.mkstemp(suffix=".pdf", dir=dst_dir)
+    # Detect fitz.Document via its module to avoid importing fitz
+    # here (this module is imported by every page through BasePage).
+    # Modern PyMuPDF reports module="pymupdf"; legacy versions used
+    # "fitz". Both expose Document.save(path, ...).
+    writer_mod = type(writer).__module__
+    is_fitz_doc = (writer_mod.startswith("pymupdf")
+                   or writer_mod.startswith("fitz")) and hasattr(writer, "save")
+    try:
+        if is_fitz_doc:
+            # fitz.Document.save(path, ...) accepts a filesystem
+            # path and writes through cleanly. We close the fd
+            # we opened first so save() can take exclusive access.
+            os.close(fd)
+            writer.save(tmp, **(save_opts or {}))
+        else:
+            # pypdf.PdfWriter (and anything else with .write(fh))
+            # streams into the open file handle.
+            with os.fdopen(fd, "wb") as fh:
+                writer.write(fh)
+        # Release the writer's file lock (if any) before the rename —
+        # see the ``close_writer`` note in the docstring. Placed
+        # between save and replace to preserve the editor's original
+        # save→close→replace ordering exactly.
+        if close_writer:
+            writer.close()
+        os.replace(tmp, dst)
+    except Exception:
+        with contextlib.suppress(Exception):
+            if os.path.exists(tmp):
+                os.unlink(tmp)
+        raise

tests/test_atomic_pdf_write.py

@@ -128,7 +128,12 @@ def spy(a, b):
         calls.append((a, b))
         return real_replace(a, b)
 
-    monkeypatch.setattr("app.base.os.replace", spy)
+    # R3: the atomic-rename logic moved from BasePage into the low-level
+    # app.pdf_io module (BasePage._atomic_pdf_write now delegates to it),
+    # so os.replace runs in the pdf_io namespace. Patch it there — the
+    # assertion is otherwise unchanged: calling BasePage._atomic_pdf_write
+    # must still ultimately invoke os.replace for the atomic rename.
+    monkeypatch.setattr("app.pdf_io.os.replace", spy)
     BasePage._atomic_pdf_write(w, str(dst), sources=[str(src)])
     assert calls, "os.replace was not called — the rename isn't atomic"
     assert calls[-1][1] == str(dst)