From 0912721925b5ecc9a9b4bca3cde5e1a5f4b1b00a Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Sat, 8 Aug 2026 10:00:39 +0100 Subject: [PATCH 1/3] refactor(editor): extract pure apply_pending_edits dispatcher from TabEditar._run Move the edit-application loop (redact / text / image / signature / highlight / note / draw / delete_annot / text_edit branches, plus the post-loop subset_fonts) out of TabEditar._run into a new pure module app/editor/apply_edits.py. apply_pending_edits(doc, pending, *, warn_fn) takes an already-open (and authenticated) fitz.Document, mutates it in place and does no file I/O and no Qt work, so it is unit-testable headless. It returns an ApplyResult carrying the text_fit_warnings and the embedded_font flag. TabEditar._run keeps only UI orchestration: opening the document, password/encryption prompts, the non-latin heads-up, the defensive try/except, the atomic tempfile+os.replace write, encrypted/plain save, reload and the toast/QMessageBox dialogs. Behaviour is identical; the order of operations (apply edits -> subset_fonts -> atomic write -> save -> reload) is unchanged. Add tests/test_apply_edits.py exercising the dispatcher without a GUI (text_edit, redact, text, draw, note, delete_annot, the _existing gate and the warn_fn / ApplyResult warning propagation). Update the three source-scraping regression tests that pinned the moved loop to tab.py so they read the new apply_edits.py module. Co-Authored-By: Claude Opus 4.8 --- app/editor/apply_edits.py | 146 ++++++++++++++++++ app/editor/tab.py | 76 ++-------- tests/test_apply_edits.py | 278 ++++++++++++++++++++++++++++++++++ tests/test_editor_audit_r9.py | 19 ++- tests/test_pdfapps.py | 9 +- 5 files changed, 451 insertions(+), 77 deletions(-) create mode 100644 app/editor/apply_edits.py create mode 100644 tests/test_apply_edits.py diff --git a/app/editor/apply_edits.py b/app/editor/apply_edits.py new file mode 100644 index 0000000..7ead3f0 --- /dev/null +++ b/app/editor/apply_edits.py @@ -0,0 +1,146 @@ +"""PDFApps – pure edit-application dispatcher for the PDF editor. + +This module holds the pure PDF/``fitz`` logic that applies a list of pending +edits to an already-open (and, if encrypted, already-authenticated) +``fitz.Document``. It performs NO file I/O (it does not open, save or reload +the document) and touches NO Qt / UI state (no ``QMessageBox``, no ``self``, +no ``self._status`` and no password prompts), so it can be unit-tested +headless. See ``TabEditar._run`` for the surrounding orchestration. + +Extracted verbatim from ``TabEditar._run`` (R1 refactor). The per-edit branch +logic is a faithful copy of the original ``for e in self._pending:`` loop; the +only adaptations are: + +* ``self._pending`` became the ``pending`` parameter; +* ``text_fit_warnings.append`` became result accumulation (plus an optional + ``warn_fn`` callback that preserves the previous semantics); +* ``import fitz`` is done locally, mirroring how ``_run`` imported it. + +``subset_fonts()`` stays here (not in ``_run``): the original ran it +unconditionally after the loop, gated on ``embedded_font``, operating solely +on ``doc`` with no Qt involvement — so it belongs to the pure edit-application +step. The order of operations therefore remains identical: apply edits -> +``subset_fonts`` -> (back in ``_run``) atomic write -> save -> reload. +""" + +import logging +from dataclasses import dataclass, field + +from app.editor.text_reinsert import _reinsert_edited_text + + +_log = logging.getLogger(__name__) + + +@dataclass +class ApplyResult: + """Outcome of :func:`apply_pending_edits`. + + ``text_fit_warnings``: the edit dicts whose reinserted text could not keep + its original size and had to be scaled below the legibility floor (S1). A + non-empty list lets the caller raise a non-blocking heads-up after saving. + + ``embedded_font``: whether any ``text_edit`` re-embedded the original span + font (i.e. whether ``subset_fonts`` was run). Exposed mainly for testing; + the caller no longer needs it because subsetting already happened here. + """ + + text_fit_warnings: list = field(default_factory=list) + embedded_font: bool = False + + +def apply_pending_edits(doc, pending, *, warn_fn=None) -> ApplyResult: + """Apply ``pending`` edits to the already-open ``doc`` (pure; no I/O, no UI). + + ``doc``: an open ``fitz.Document`` (already authenticated if it was + encrypted). This function mutates it in place and does NOT save or close it. + + ``pending``: the list of edit dicts (``TabEditar._pending``). Each carries a + ``type`` and ``page`` plus type-specific keys. + + ``warn_fn`` (optional): called with the offending edit dict when reinserted + text had to be shrunk below the legibility floor. This preserves the + previous ``warn_fn=text_fit_warnings.append`` behaviour for callers that + want a live callback; the same edits are always accumulated into the + returned :class:`ApplyResult` regardless. + + Returns an :class:`ApplyResult` with the accumulated text-fit warnings and + the ``embedded_font`` flag. + """ + import fitz + + result = ApplyResult() + + # Collector passed to ``_reinsert_edited_text``: always records into the + # result (so the caller can read result.text_fit_warnings) AND forwards to + # the caller's optional live callback, matching the old append semantics. + def _collect_warning(edit): + result.text_fit_warnings.append(edit) + if warn_fn is not None: + warn_fn(edit) + + embedded_font = False # any text_edit that re-embedded its font + for e in pending: + if e.get("_existing") and e.get("type") != "delete_annot": + continue # already saved in the PDF + pg = doc[e["page"]] + if e["type"] == "redact": + pg.add_redact_annot(e["rect"], fill=e["fill"]); pg.apply_redactions() + elif e["type"] == "text": + fname = (e.get("font", "") or "").lower() + if "times" in fname or "serif" in fname or "roman" in fname: + fontname = "tiro" + elif "mono" in fname or "courier" in fname or "consol" in fname: + fontname = "cour" + else: + fontname = "helv" + pg.insert_text(e["point"], e["text"], fontsize=e["size"], + color=e["color"], fontname=fontname) + elif e["type"] in ("image", "signature"): + pg.insert_image(e["rect"], filename=e["path"]) + elif e["type"] == "highlight": + a = pg.add_highlight_annot(e["rect"]); a.set_colors(stroke=e["color"]); a.update() + elif e["type"] == "note": + pg.add_text_annot(e["point"], e["text"]) + elif e["type"] == "draw": + # PyMuPDF's add_ink_annot expects a list of strokes, where + # each stroke is a list of (x, y) float pairs — NOT a list + # of fitz.Point. Passing Points raises + # `ValueError: arg must be seq of seq of float pairs`. + stroke = [(float(x), float(y)) + for x, y in e.get("points", [])] + if len(stroke) >= 2: + annot = pg.add_ink_annot([stroke]) + annot.set_colors(stroke=e.get("color", (1, 0, 0))) + annot.set_border(width=max(1, int(e.get("width", 2)))) + annot.update() + elif e["type"] == "delete_annot": + # Match by annot type + bbox (xref isn't stable across + # the canvas-release / fitz.open round-trip used here). + target_type = e.get("annot_type") + target_bbox = e.get("bbox") + if target_bbox is not None: + target_rect = fitz.Rect(target_bbox) + for annot in list(pg.annots() or []): + if (annot.type[0] == target_type + and abs(annot.rect.x0 - target_rect.x0) < 1 + and abs(annot.rect.y0 - target_rect.y0) < 1): + pg.delete_annot(annot) + break + elif e["type"] == "text_edit": + # High-fidelity reinsertion: transparent redaction (no white + # box) + insert_htmlbox preserving the original size, weight, + # colour and — when the source font is embeddable — the exact + # typeface, with a defensive base-14 fallback. See #147. + if _reinsert_edited_text(fitz, doc, pg, e, + warn_fn=_collect_warning): + embedded_font = True + result.embedded_font = embedded_font + if embedded_font: + # Subset the freshly embedded fonts to keep the file small. + # Best-effort: never let optimisation abort a valid save. + try: + doc.subset_fonts() + except Exception: + _log.exception("subset_fonts after text edit failed") + return result diff --git a/app/editor/tab.py b/app/editor/tab.py index fe0f12b..5aeee28 100644 --- a/app/editor/tab.py +++ b/app/editor/tab.py @@ -24,7 +24,7 @@ from app.widgets import DropFileEdit, ColorPickerButton from app.editor.canvas import PdfEditCanvas, _get_icon_cursor from app.editor.dialogs import _NoteDialog -from app.editor.text_reinsert import _reinsert_edited_text +from app.editor.apply_edits import apply_pending_edits _log = logging.getLogger(__name__) @@ -1256,74 +1256,16 @@ def _run(self): ) if _non_latin: self._status(t("tool.warn.font_latin_only")) - embedded_font = False # any text_edit that re-embedded its font - # Edits whose new text could not keep its original size (S1). Each - # entry is the edit dict; a non-empty list raises a non-blocking + # Apply every pending edit to the open doc via the pure dispatcher + # (redact / text / image / signature / highlight / note / draw / + # delete_annot / text_edit) and run subset_fonts when a text edit + # re-embedded its font — all PDF-only work, no Qt. The returned + # ``text_fit_warnings`` are edits whose new text could not keep its + # original size (S1); a non-empty list raises a non-blocking # heads-up after the save so the user is never left with an # unexplained illegibly-shrunk line. - text_fit_warnings = [] - for e in self._pending: - if e.get("_existing") and e.get("type") != "delete_annot": - continue # already saved in the PDF - pg = doc[e["page"]] - if e["type"] == "redact": - pg.add_redact_annot(e["rect"], fill=e["fill"]); pg.apply_redactions() - elif e["type"] == "text": - fname = (e.get("font", "") or "").lower() - if "times" in fname or "serif" in fname or "roman" in fname: - fontname = "tiro" - elif "mono" in fname or "courier" in fname or "consol" in fname: - fontname = "cour" - else: - fontname = "helv" - pg.insert_text(e["point"], e["text"], fontsize=e["size"], - color=e["color"], fontname=fontname) - elif e["type"] in ("image", "signature"): - pg.insert_image(e["rect"], filename=e["path"]) - elif e["type"] == "highlight": - a = pg.add_highlight_annot(e["rect"]); a.set_colors(stroke=e["color"]); a.update() - elif e["type"] == "note": - pg.add_text_annot(e["point"], e["text"]) - elif e["type"] == "draw": - # PyMuPDF's add_ink_annot expects a list of strokes, where - # each stroke is a list of (x, y) float pairs — NOT a list - # of fitz.Point. Passing Points raises - # `ValueError: arg must be seq of seq of float pairs`. - stroke = [(float(x), float(y)) - for x, y in e.get("points", [])] - if len(stroke) >= 2: - annot = pg.add_ink_annot([stroke]) - annot.set_colors(stroke=e.get("color", (1, 0, 0))) - annot.set_border(width=max(1, int(e.get("width", 2)))) - annot.update() - elif e["type"] == "delete_annot": - # Match by annot type + bbox (xref isn't stable across - # the canvas-release / fitz.open round-trip used here). - target_type = e.get("annot_type") - target_bbox = e.get("bbox") - if target_bbox is not None: - target_rect = fitz.Rect(target_bbox) - for annot in list(pg.annots() or []): - if (annot.type[0] == target_type - and abs(annot.rect.x0 - target_rect.x0) < 1 - and abs(annot.rect.y0 - target_rect.y0) < 1): - pg.delete_annot(annot) - break - elif e["type"] == "text_edit": - # High-fidelity reinsertion: transparent redaction (no white - # box) + insert_htmlbox preserving the original size, weight, - # colour and — when the source font is embeddable — the exact - # typeface, with a defensive base-14 fallback. See #147. - if _reinsert_edited_text(fitz, doc, pg, e, - warn_fn=text_fit_warnings.append): - embedded_font = True - if embedded_font: - # Subset the freshly embedded fonts to keep the file small. - # Best-effort: never let optimisation abort a valid save. - try: - doc.subset_fonts() - except Exception: - _log.exception("subset_fonts after text edit failed") + _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) diff --git a/tests/test_apply_edits.py b/tests/test_apply_edits.py new file mode 100644 index 0000000..34c531a --- /dev/null +++ b/tests/test_apply_edits.py @@ -0,0 +1,278 @@ +"""Headless tests for the pure edit-application dispatcher (R1 refactor). + +``app.editor.apply_edits.apply_pending_edits`` is the pure PDF/``fitz`` logic +extracted from ``TabEditar._run``: it applies a list of pending edits to an +already-open ``fitz.Document`` with NO file I/O and NO Qt. These tests exercise +it directly, without any widget, proving the refactor's headline win — +testability. Each edit type is applied to a real in-memory document and the +resulting document (via a save+reopen round-trip through bytes) is asserted. + +Run with ``QT_QPA_PLATFORM=offscreen`` (no widgets are instantiated). +""" + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +pymupdf = pytest.importorskip("pymupdf") +fitz = pymupdf + +from app.editor.apply_edits import apply_pending_edits, ApplyResult # noqa: E402 + + +# ── helpers ────────────────────────────────────────────────────────────── + +def _page_with_text(text, *, fontname="helv", size=14, at=(30, 60), + width=400, height=200, color=(0, 0, 0)): + doc = fitz.open() + page = doc.new_page(width=width, height=height) + page.insert_text(at, text, fontsize=size, fontname=fontname, color=color) + return doc, page + + +def _first_span(page): + for block in page.get_text("dict")["blocks"]: + if block.get("type") != 0: + continue + for line in block.get("lines", []): + for span in line.get("spans", []): + return span + return None + + +def _text_edit(span, new_text, page_idx=0): + """Build a text_edit dict as canvas._commit_inline emits it.""" + bb = span["bbox"] + return { + "type": "text_edit", "page": page_idx, + "bbox": list(bb), "old_text": span.get("text", ""), + "new_text": new_text, + "size": max(float(span.get("size") or 0), float(bb[3] - bb[1])), + "font_size": float(span.get("size") or 0), + "color": span.get("color", 0), + "font": span.get("font", ""), + "flags": int(span.get("flags", 0) or 0), + "ascender": float(span.get("ascender") or 0), + "descender": float(span.get("descender") or 0), + "origin": list(span.get("origin", (bb[0], bb[3]))), + } + + +def _reopen(doc): + """Save+reopen through bytes so we assert the persisted result, not the + live in-memory object.""" + data = doc.tobytes(garbage=4, deflate=True) + doc.close() + return fitz.open("pdf", data) + + +# ── return type ────────────────────────────────────────────────────────── + +def test_returns_apply_result_empty_for_no_edits(): + doc, _page = _page_with_text("Untouched") + result = apply_pending_edits(doc, []) + assert isinstance(result, ApplyResult) + assert result.text_fit_warnings == [] + assert result.embedded_font is False + assert "Untouched" in doc[0].get_text() + doc.close() + + +# ── text_edit: new text in, old text out ───────────────────────────────── + +def test_text_edit_reinserts_new_and_removes_old(): + doc, page = _page_with_text("OriginalWord", size=14) + span = _first_span(page) + result = apply_pending_edits(doc, [_text_edit(span, "ReplacedWord")]) + assert isinstance(result, ApplyResult) + reopened = _reopen(doc) + txt = reopened[0].get_text() + reopened.close() + assert "ReplacedWord" in txt # new text persisted + assert "OriginalWord" not in txt # old span removed + + +# ── redact: removes the covered text ───────────────────────────────────── + +def test_redact_removes_covered_text(): + doc, page = _page_with_text("SECRETdata", size=16, at=(30, 60)) + span = _first_span(page) + rect = fitz.Rect(span["bbox"]) + result = apply_pending_edits( + doc, [{"type": "redact", "page": 0, "rect": rect, "fill": (1, 1, 1)}]) + assert isinstance(result, ApplyResult) + reopened = _reopen(doc) + txt = reopened[0].get_text() + reopened.close() + assert "SECRET" not in txt + + +# ── plain text insert ──────────────────────────────────────────────────── + +def test_text_insert_adds_new_text(): + doc, _page = _page_with_text("Base") + edit = {"type": "text", "page": 0, "point": (30, 120), + "text": "InsertedLine", "size": 12, "color": (0, 0, 0), + "font": "Helvetica"} + apply_pending_edits(doc, [edit]) + reopened = _reopen(doc) + txt = reopened[0].get_text() + reopened.close() + assert "InsertedLine" in txt + + +# ── draw: creates an ink annotation ────────────────────────────────────── + +def test_draw_creates_ink_annot(): + doc, page = _page_with_text("Canvas") + before = len(list(page.annots() or [])) + edit = {"type": "draw", "page": 0, + "points": [(40, 40), (60, 60), (80, 40)], + "color": (1, 0, 0), "width": 3} + apply_pending_edits(doc, [edit]) + annots = list(page.annots() or []) + count = len(annots) + last_kind = annots[-1].type[1] if annots else None + doc.close() + assert count == before + 1 + assert last_kind == "Ink" + + +def test_draw_ignored_when_fewer_than_two_points(): + doc, page = _page_with_text("Canvas") + edit = {"type": "draw", "page": 0, "points": [(40, 40)], + "color": (1, 0, 0), "width": 3} + apply_pending_edits(doc, [edit]) + annots = list(page.annots() or []) + doc.close() + assert annots == [] + + +# ── note: creates a text annotation ────────────────────────────────────── + +def test_note_creates_text_annot(): + doc, page = _page_with_text("Sheet") + edit = {"type": "note", "page": 0, "point": (50, 50), + "text": "a comment"} + apply_pending_edits(doc, [edit]) + annots = list(page.annots() or []) + count = len(annots) + first_kind = annots[0].type[1] if annots else None + doc.close() + assert count == 1 + assert first_kind == "Text" + + +# ── delete_annot: removes a matching annotation ────────────────────────── + +def test_delete_annot_removes_matching_annotation(): + doc, page = _page_with_text("Sheet") + annot = page.add_text_annot((50, 50), "to be deleted") + target_type = annot.type[0] + target_bbox = list(annot.rect) + assert len(list(page.annots() or [])) == 1 + edit = {"type": "delete_annot", "page": 0, + "annot_type": target_type, "bbox": target_bbox} + apply_pending_edits(doc, [edit]) + remaining = list(page.annots() or []) + doc.close() + assert remaining == [] + + +def test_delete_annot_leaves_non_matching_annotation(): + doc, page = _page_with_text("Sheet") + page.add_text_annot((50, 50), "keep me") + # bbox nowhere near the real annot -> no match, nothing deleted + edit = {"type": "delete_annot", "page": 0, + "annot_type": 0, "bbox": [300, 300, 320, 320]} + apply_pending_edits(doc, [edit]) + remaining = list(page.annots() or []) + doc.close() + assert len(remaining) == 1 + + +# ── _existing gate: only delete_annot survives the skip ─────────────────── + +def test_existing_non_delete_edits_are_skipped(): + doc, page = _page_with_text("KeepThis") + span = _first_span(page) + edit = _text_edit(span, "ShouldNotAppear") + edit["_existing"] = True # already saved in the PDF -> must be skipped + apply_pending_edits(doc, [edit]) + reopened = _reopen(doc) + txt = reopened[0].get_text() + reopened.close() + assert "KeepThis" in txt + assert "ShouldNotAppear" not in txt + + +def test_existing_delete_annot_still_applied(): + doc, page = _page_with_text("Sheet") + annot = page.add_text_annot((50, 50), "existing note") + edit = {"type": "delete_annot", "page": 0, "_existing": True, + "annot_type": annot.type[0], "bbox": list(annot.rect)} + apply_pending_edits(doc, [edit]) + remaining = list(page.annots() or []) + doc.close() + assert remaining == [] # delete_annot bypasses the _existing skip + + +# ── warnings: accumulated in result AND forwarded to warn_fn ────────────── + +def test_unfittable_text_edit_warns_via_result_and_callback(): + """A replacement far too large for a tiny page must raise the non-blocking + text-fit warning: recorded in ApplyResult.text_fit_warnings AND forwarded + to the optional warn_fn callback (preserving the previous semantics).""" + doc, page = _page_with_text("x", size=14, width=120, height=90, at=(20, 40)) + span = _first_span(page) + huge = "word " * 400 + edit = _text_edit(span, huge) + live = [] + result = apply_pending_edits(doc, [edit], warn_fn=live.append) + reopened = _reopen(doc) + txt = reopened[0].get_text() + reopened.close() + assert result.text_fit_warnings, "no warning accumulated in the result" + assert result.text_fit_warnings[0] is edit + assert live and live[0] is edit, "warn_fn callback was not forwarded" + assert "word" in txt, "text was dropped instead of shrunk" + + +def test_fitting_text_edit_produces_no_warnings(): + doc, page = _page_with_text("Short", size=12, width=400, height=300) + span = _first_span(page) + result = apply_pending_edits(doc, [_text_edit(span, "AlsoShort")]) + assert result.text_fit_warnings == [] + doc.close() + + +# ── batch: several edit types in one call ──────────────────────────────── + +def test_mixed_batch_applies_all_edits(): + doc, page = _page_with_text("FirstLine", size=14, at=(30, 50), + width=420, height=260) + span = _first_span(page) + edits = [ + _text_edit(span, "EditedFirst"), + {"type": "text", "page": 0, "point": (30, 120), + "text": "AddedText", "size": 12, "color": (0, 0, 0), + "font": "Helvetica"}, + {"type": "note", "page": 0, "point": (200, 80), "text": "note"}, + {"type": "draw", "page": 0, + "points": [(40, 200), (80, 220), (120, 200)], + "color": (0, 0, 1), "width": 2}, + ] + result = apply_pending_edits(doc, edits) + annots = list(page.annots() or []) + annot_kinds = sorted(a.type[1] for a in annots) + reopened = _reopen(doc) + txt = reopened[0].get_text() + reopened.close() + assert "EditedFirst" in txt + assert "FirstLine" not in txt + assert "AddedText" in txt + assert annot_kinds == ["Ink", "Text"] + assert isinstance(result, ApplyResult) diff --git a/tests/test_editor_audit_r9.py b/tests/test_editor_audit_r9.py index 143d1c7..fe9f4ef 100644 --- a/tests/test_editor_audit_r9.py +++ b/tests/test_editor_audit_r9.py @@ -114,19 +114,24 @@ def test_canvas_late_discovered_notes_also_tag_match_fields(): def test_run_loop_applies_delete_annot_edits(): - src = _read("app/editor/tab.py") - run_block = src[src.find("def _run("): - src.find("def _apply_forms")] - assert 'e["type"] == "delete_annot"' in run_block - assert "page.delete_annot" in run_block or "pg.delete_annot" in run_block + # R1 refactor: the edit-application loop moved from ``TabEditar._run`` to + # the pure ``app.editor.apply_edits.apply_pending_edits`` dispatcher. The + # delete_annot wiring must still be present there. + src = _read("app/editor/apply_edits.py") + loop_block = src[src.find("def apply_pending_edits("):] + assert 'e["type"] == "delete_annot"' in loop_block + assert "page.delete_annot" in loop_block or "pg.delete_annot" in loop_block def test_existing_filter_does_not_drop_delete_annot(): """The pre-existing ``if e.get("_existing"): continue`` guard skipped every entry already saved in the PDF. delete_annot edits are flagged ``_existing=True`` but MUST be processed, so the guard now - excludes them explicitly.""" - src = _read("app/editor/tab.py") + excludes them explicitly. + + R1 refactor: the guard moved with the loop into + ``app.editor.apply_edits.apply_pending_edits``.""" + src = _read("app/editor/apply_edits.py") assert 'e.get("_existing") and e.get("type") != "delete_annot"' in src diff --git a/tests/test_pdfapps.py b/tests/test_pdfapps.py index b4656bb..54dfdfb 100644 --- a/tests/test_pdfapps.py +++ b/tests/test_pdfapps.py @@ -759,12 +759,15 @@ def test_long_running_tools_use_background_runner(self): def test_draw_ink_annot_uses_tuple_points(self): # PyMuPDF 1.27+ rejects fitz.Point as ink-annot input with # ValueError: arg must be seq of seq of float pairs. - # tab.py builds the stroke as plain (float, float) tuples now; + # The draw branch builds the stroke as plain (float, float) tuples; # this test fails if anyone reintroduces fitz.Point wrapping. - src = open(_REPO_ROOT / "app" / "editor" / "tab.py", encoding="utf-8").read() + # R1 refactor: the edit-application loop moved from TabEditar._run to + # the pure app/editor/apply_edits.py dispatcher. + src = open(_REPO_ROOT / "app" / "editor" / "apply_edits.py", + encoding="utf-8").read() # Locate the draw branch i = src.find('elif e["type"] == "draw":') - assert i > 0, "draw branch missing in tab.py" + assert i > 0, "draw branch missing in apply_edits.py" block = src[i:i + 600] assert "[fitz.Point(x, y) for x, y in" not in block, \ "ink-annot strokes must be (x,y) tuples, not fitz.Point" From 5a55834486884fcd7f263807667deabe86fbb000 Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Sat, 8 Aug 2026 10:18:27 +0100 Subject: [PATCH 2/3] test(editor): cover image and highlight branches of apply_pending_edits Add direct headless tests for the two previously-uncovered branches of apply_pending_edits: * image/signature: parametrised over both type strings (they share one branch); writes a real PNG via fitz.Pixmap, applies the edit and asserts exactly one raster is embedded after a bytes round-trip. * highlight: asserts a single Highlight annotation is added over the text rect and stamped with the requested stroke colour. Co-Authored-By: Claude Opus 4.8 --- tests/test_apply_edits.py | 60 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/test_apply_edits.py b/tests/test_apply_edits.py index 34c531a..232021a 100644 --- a/tests/test_apply_edits.py +++ b/tests/test_apply_edits.py @@ -249,6 +249,66 @@ def test_fitting_text_edit_produces_no_warnings(): doc.close() +# ── image / signature: embeds a raster into the page ───────────────────── + +def _write_png(dir_path, name="stamp.png", size=12, value=90): + """Write a tiny valid PNG to ``dir_path`` and return its path string. + + Uses only ``fitz`` (no PIL dependency): a solid-grey RGB pixmap saved as + PNG. Small but real, so ``insert_image`` genuinely embeds a raster. + """ + pix = fitz.Pixmap(fitz.csRGB, fitz.IRect(0, 0, size, size)) + pix.clear_with(value) + path = dir_path / name + pix.save(str(path)) + return str(path) + + +@pytest.mark.parametrize("edit_type", ["image", "signature"]) +def test_image_and_signature_embed_raster(tmp_path, edit_type): + """Both ``image`` and ``signature`` route through the same branch and must + embed the on-disk raster into the target page. Parametrising over the two + type strings makes this discriminative: dropping either from the branch's + ``in ("image", "signature")`` tuple would fail here.""" + doc, page = _page_with_text("Backdrop", width=300, height=200) + assert page.get_images() == [], "page unexpectedly had an image to start" + img_path = _write_png(tmp_path, name=f"{edit_type}.png") + edit = {"type": edit_type, "page": 0, + "rect": fitz.Rect(40, 40, 140, 140), "path": img_path} + result = apply_pending_edits(doc, [edit]) + assert isinstance(result, ApplyResult) + reopened = _reopen(doc) # persist + reload through bytes + images = reopened[0].get_images() + reopened.close() + assert len(images) == 1, ( + f"{edit_type} edit did not embed exactly one raster: {images}") + + +# ── highlight: creates a coloured highlight annotation ─────────────────── + +def test_highlight_creates_coloured_highlight_annot(): + """A ``highlight`` edit must add exactly one Highlight annotation over the + target rect and stamp it with the requested stroke colour (proving both + ``add_highlight_annot`` and the ``set_colors(stroke=...)`` call run).""" + doc, page = _page_with_text("HighlightMe", size=16, at=(30, 60)) + span = _first_span(page) + before = len(list(page.annots() or [])) + colour = (0.1, 0.7, 0.3) # distinct from the default yellow + edit = {"type": "highlight", "page": 0, + "rect": fitz.Rect(span["bbox"]), "color": colour} + apply_pending_edits(doc, [edit]) + annots = list(page.annots() or []) + count = len(annots) + kind = annots[-1].type[1] if annots else None + stroke = annots[-1].colors.get("stroke") if annots else None + doc.close() + assert count == before + 1 + assert kind == "Highlight" + assert stroke is not None + assert all(abs(a - b) < 0.05 for a, b in zip(stroke, colour, strict=True)), ( + f"highlight stroke colour {stroke} != requested {colour}") + + # ── batch: several edit types in one call ──────────────────────────────── def test_mixed_batch_applies_all_edits(): From a1678e848312d4e125dec7a4c8365d4be7a63c77 Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Sat, 8 Aug 2026 10:27:49 +0100 Subject: [PATCH 3/3] test: close file handles in test_pdfapps to satisfy CodeQL Wrap the source-inspection open().read() calls in context managers so the file handle is always closed. Fixes CodeQL alert #303 (File is not always closed) at tests/test_pdfapps.py and the same pattern in nine sibling audit-regression tests. No test logic or assertions changed. Co-Authored-By: Claude Opus 4.8 --- tests/test_pdfapps.py | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/tests/test_pdfapps.py b/tests/test_pdfapps.py index 54dfdfb..cf9e39e 100644 --- a/tests/test_pdfapps.py +++ b/tests/test_pdfapps.py @@ -598,7 +598,8 @@ def test_toast_guard_uses_shiboken_not_qpointer(self): # PySide6 has no QPointer — the toast hide-timer must guard the # widget liveness check via shiboken6.isValid(). Importing # QPointer from PySide6 would fail with ImportError at load. - src = open(_REPO_ROOT / "app" / "base.py", encoding="utf-8").read() + with open(_REPO_ROOT / "app" / "base.py", encoding="utf-8") as f: + src = f.read() assert "from shiboken6 import isValid" in src assert "isValid(t)" in src # No QPointer import or instantiation — only the explanatory @@ -613,7 +614,8 @@ def test_restart_app_handles_pyinstaller_frozen(self): # _restart_app must branch on sys.frozen — using # os.path.dirname(__file__) + "pdfapps.py" breaks in frozen # bundles because __file__ points inside _MEIPASS. - src = open(_REPO_ROOT / "app" / "window.py", encoding="utf-8").read() + with open(_REPO_ROOT / "app" / "window.py", encoding="utf-8") as f: + src = f.read() # The fixed implementation references sys.frozen. assert 'getattr(sys, "frozen"' in src, \ "_restart_app must check sys.frozen" @@ -625,7 +627,8 @@ def test_restart_app_handles_pyinstaller_frozen(self): def test_pdfapps_spec_reads_version_dynamically(self): # Avoids drift between APP_VERSION and the macOS BUNDLE # CFBundleVersion / CFBundleShortVersionString. - spec = open(_REPO_ROOT / "pdfapps.spec", encoding="utf-8").read() + with open(_REPO_ROOT / "pdfapps.spec", encoding="utf-8") as f: + spec = f.read() assert "_app_version" in spec assert "APP_VERSION" in spec # parsed from app/constants.py assert "CFBundleVersion': '1.13" not in spec, \ @@ -634,7 +637,8 @@ def test_pdfapps_spec_reads_version_dynamically(self): def test_installer_pins_third_party_hashes(self): # Tesseract and Ghostscript exes are downloaded and run with # admin — they MUST be hash-pinned in installer.py. - src = open(_REPO_ROOT / "installer.py", encoding="utf-8").read() + with open(_REPO_ROOT / "installer.py", encoding="utf-8") as f: + src = f.read() assert "TESSERACT_SHA256" in src assert "GHOSTSCRIPT_SHA256" in src assert "hmac.compare_digest" in src @@ -701,7 +705,8 @@ def test_editor_handles_encrypted_pdfs(self): # The editor's _load_pdf must prompt for a password and pass it # through to the canvas. The audit flagged this as broken — the # job opened with fitz.open without authenticate(). - src = open(_REPO_ROOT / "app" / "editor" / "tab.py", encoding="utf-8").read() + with open(_REPO_ROOT / "app" / "editor" / "tab.py", encoding="utf-8") as f: + src = f.read() # _load_pdf integrates the password prompt. PR-H/PR-I inflated # the body of _load_pdf past the original 1500-char slice (now # ~2 KB), so slice to the next function boundary instead of a @@ -716,7 +721,8 @@ def test_editor_handles_encrypted_pdfs(self): assert "password=self._pdf_password" in block, \ "canvas.load must receive the password" # The canvas job must accept and apply the password - canvas_src = open(_REPO_ROOT / "app" / "editor" / "canvas.py", encoding="utf-8").read() + with open(_REPO_ROOT / "app" / "editor" / "canvas.py", encoding="utf-8") as f: + canvas_src = f.read() assert "doc.authenticate(self._password)" in canvas_src, \ "_EditPageJob.run must authenticate the document" @@ -728,7 +734,8 @@ def test_taskrunner_cancel_uses_lambda_wrap(self): # queue never drains. The lambda wrap forces a plain Python # call on the dialog's thread (main), which mutates the flag # immediately. Pin this so it can't be "simplified" back. - worker_src = open(_REPO_ROOT / "app" / "worker.py", encoding="utf-8").read() + with open(_REPO_ROOT / "app" / "worker.py", encoding="utf-8") as f: + worker_src = f.read() assert "lambda: runner.cancel()" in worker_src, \ "cancel must be wrapped in a lambda; bare bound method gets queued" @@ -763,8 +770,9 @@ def test_draw_ink_annot_uses_tuple_points(self): # this test fails if anyone reintroduces fitz.Point wrapping. # R1 refactor: the edit-application loop moved from TabEditar._run to # the pure app/editor/apply_edits.py dispatcher. - src = open(_REPO_ROOT / "app" / "editor" / "apply_edits.py", - encoding="utf-8").read() + with open(_REPO_ROOT / "app" / "editor" / "apply_edits.py", + encoding="utf-8") as f: + src = f.read() # Locate the draw branch i = src.find('elif e["type"] == "draw":') assert i > 0, "draw branch missing in apply_edits.py" @@ -779,9 +787,11 @@ def test_flatpak_manifest_tag_is_current(self): # Bump script now keeps it in sync; this test ensures it matches # APP_VERSION at any given point. import re - const = open(_REPO_ROOT / "app" / "constants.py", encoding="utf-8").read() + with open(_REPO_ROOT / "app" / "constants.py", encoding="utf-8") as f: + const = f.read() version = re.search(r'APP_VERSION\s*=\s*"([^"]+)"', const).group(1) - manifest = open(_REPO_ROOT / "flatpak" / "io.github.nelsonduarte.PDFApps.yml", - encoding="utf-8").read() + with open(_REPO_ROOT / "flatpak" / "io.github.nelsonduarte.PDFApps.yml", + encoding="utf-8") as f: + manifest = f.read() assert f"tag: v{version}" in manifest, \ f"Flatpak manifest tag must match APP_VERSION ({version})"