diff --git a/app/editor/canvas.py b/app/editor/canvas.py index 68e5404..bae78d7 100644 --- a/app/editor/canvas.py +++ b/app/editor/canvas.py @@ -1,5 +1,7 @@ """PDFApps – PdfEditCanvas: continuous-scroll visual PDF edit canvas.""" +import contextlib + from PySide6.QtCore import Qt, Signal, QRect, QPoint, QObject, QRunnable, QThreadPool, QEvent from PySide6.QtWidgets import QWidget, QSizePolicy, QLineEdit @@ -458,6 +460,30 @@ def close_doc(self): self.setMaximumSize(16777215, 16777215) self.update() + # ── DPR change handling (R8/D1) ────────────────────────────────────── + def showEvent(self, event): + """Re-render pages when the top-level window crosses a screen + with a different devicePixelRatio. ``_schedule_visible`` only + sampled the DPR at scroll/zoom time, so dragging the window from + a 100 % monitor to a 200 % monitor left previously rendered + pages blurry until the user changed zoom (R8/D1).""" + super().showEvent(event) + win = self.window().windowHandle() if self.window() else None + if win: + # Disconnect before reconnecting — re-show events can stack + # the same handler multiple times. + with contextlib.suppress(TypeError, RuntimeError): + win.screenChanged.disconnect(self._on_screen_changed) + win.screenChanged.connect(self._on_screen_changed) + + def _on_screen_changed(self, _screen): + """Drop cached pixmaps and re-queue visible pages at the new DPR.""" + self._gen += 1 + self._pending.clear() + self._page_pixmaps = [None] * len(self._page_pixmaps) + self._schedule_visible() + self.update() + def on_scroll(self): """Called when scroll position changes — renders newly visible pages.""" self._schedule_visible() diff --git a/app/tools/encrypt.py b/app/tools/encrypt.py index 7aa5f3a..9a8ffb1 100644 --- a/app/tools/encrypt.py +++ b/app/tools/encrypt.py @@ -1,5 +1,6 @@ """PDFApps – TabEncriptar: encrypt/decrypt PDF tool.""" +import contextlib import os from PySide6.QtCore import Qt @@ -107,6 +108,22 @@ def _load_input(self, p: str): def auto_load(self, path: str): if path and not self.drop_in.path(): self._load_input(path) + def _clear_password_fields(self) -> None: + """Best-effort wipe of password QLineEdits after encrypt/decrypt run. + + QLineEdit text persisted in memory for the entire session prior + to R8-H1. Clearing the field both via ``setText('')`` and + ``clear()`` drops the cached display string and any pending + completer state; the underlying QString allocation may still + linger in Qt's heap until GC, same caveat as + ``wipe_pdf_password``. + """ + for field in (self.edit_owner, self.edit_owner_confirm, + self.edit_user, self.edit_pwd): + with contextlib.suppress(Exception): + field.setText("") + field.clear() + def _run(self): pdf_path = self.drop_in.path() if not pdf_path or not os.path.isfile(pdf_path): @@ -153,4 +170,10 @@ def _run(self): self._pipeline_success(msg, out_path) else: QMessageBox.information(self, t("msg.done"), msg) - except Exception as e: show_error(self, e) + except Exception as e: + show_error(self, e) + finally: + # R8-H1: wipe password fields whether _run() succeeded or + # raised. Keeping the user's password in the QLineEdit text + # buffer for the whole session was a needless heap leak. + self._clear_password_fields() diff --git a/app/tools/watermark.py b/app/tools/watermark.py index 3a2e4f8..2e970a0 100644 --- a/app/tools/watermark.py +++ b/app/tools/watermark.py @@ -81,6 +81,24 @@ def _load_input(self, p: str): def auto_load(self, path: str): if path and not self.drop_in.path(): self._load_input(path) + def _prompt_watermark_password(self, wm_path: str) -> str | None: + """Resolve an encryption password for the watermark PDF. + + Returns: + "" — watermark PDF is plaintext (no password needed) + "" — user supplied a valid password + None — user cancelled the prompt (caller aborts) + + Kept separate from ``self._pdf_password`` (which holds the + *source* PDF's password) so a corporate stamp PDF and the user's + own document don't leak credentials into one another. + """ + from app.utils import prompt_pdf_password + ok, pwd = prompt_pdf_password(wm_path, self) + if not ok: + return None + return pwd + def _run(self): pdf_path = self.drop_in.path(); wm_path = self.drop_wm.path() if not pdf_path or not os.path.isfile(pdf_path): @@ -90,10 +108,23 @@ def _run(self): out_path = self._resolve_output_file(self.drop_out, pdf_path) if not out_path: return + # R8 bonus #6: the watermark PDF can itself be encrypted (rare + # but observed when users borrow a corporate stamp PDF). + # PdfReader(wm_path) used to raise PdfReadError cryptically; + # prompt for the watermark's password explicitly here so the + # error surface matches the source-PDF flow. Tracked separately + # from self._pdf_password (which holds the *source* PDF's + # password) so neither leaks into the other. + wm_pwd = self._prompt_watermark_password(wm_path) + if wm_pwd is None: + return # user cancelled + # Pre-flight on the main thread: validate inputs and resolve page # targets so the worker can be a tight loop with no Qt calls. try: wm_reader = PdfReader(wm_path) + if wm_reader.is_encrypted and wm_pwd: + wm_reader.decrypt(wm_pwd) if not wm_reader.pages: QMessageBox.warning(self, t("msg.warning"), t("tool.watermark.empty_wm")) return @@ -116,6 +147,8 @@ def do_work(worker): if r.is_encrypted and pwd: r.decrypt(pwd) wm = PdfReader(wm_path) + if wm.is_encrypted and wm_pwd: + wm.decrypt(wm_pwd) wm_page = wm.pages[0] w = PdfWriter() n = len(r.pages) diff --git a/app/viewer/canvas.py b/app/viewer/canvas.py index 7cf57b4..8c914d0 100644 --- a/app/viewer/canvas.py +++ b/app/viewer/canvas.py @@ -2,6 +2,8 @@ from __future__ import annotations +import contextlib + from PySide6.QtCore import Qt, Signal, QRect, QObject, QRunnable, QThreadPool from PySide6.QtWidgets import QWidget, QApplication from PySide6.QtGui import QPixmap, QColor, QPainter, QPen, QFont @@ -203,6 +205,36 @@ def close_doc(self): self.setFixedSize(300, 400) self.update() + # ── DPR change handling (D1) ───────────────────────────────────────── + def showEvent(self, event): + """Hook into the top-level QWindow.screenChanged signal so a + screen migration (drag to another monitor) or DPR mutation + (Windows Display Settings change) invalidates the cached + pixmaps and re-renders at the new device pixel ratio. + + Without this handler ``_schedule_visible`` only sampled the DPR + on zoom changes, leaving pages blurry until the user interacted + (R8/D1).""" + super().showEvent(event) + win = self.window().windowHandle() if self.window() else None + if win: + # Re-show events may fire after a tab switch — disconnect + # first to avoid stacking duplicate handlers. + with contextlib.suppress(TypeError, RuntimeError): + win.screenChanged.disconnect(self._on_screen_changed) + win.screenChanged.connect(self._on_screen_changed) + + def _on_screen_changed(self, _screen): + """Invalidate cached pixmaps and re-render at the new DPR.""" + # Bump generation so any in-flight render jobs are discarded + # by _on_page_ready when they finally land on the main thread. + self._gen += 1 + self._pending.clear() + for entry in self._entries: + entry.pixmap = None + self._schedule_visible() + self.update() + # ── Layout ─────────────────────────────────────────────────────────────── def _invalidate_and_relayout(self): diff --git a/app/viewer/panel.py b/app/viewer/panel.py index d59dc7e..24ca77b 100644 --- a/app/viewer/panel.py +++ b/app/viewer/panel.py @@ -369,30 +369,52 @@ def current_path(self) -> str: # ── TOC / Bookmarks ───────────────────────────────────────────────── def _populate_toc(self, doc): - """Read the PDF outline and build the tree. Hides the panel if empty.""" + """Read the PDF outline and build the tree. Hides the panel if empty. + + Wrapped in try/except: a malformed outline (cyclic refs, bad + page indexes, unexpected entry shape) used to leave the TOC + panel half-populated and could raise mid-build, surfacing as a + cryptic stack trace. Now the failure is logged and the panel + hides gracefully so the rest of the viewer stays usable + (R8 bonus #7). + """ self._toc_tree.clear() try: toc = doc.get_toc() - except Exception: + except Exception as exc: + import logging + logging.getLogger(__name__).warning( + "Failed to read TOC for %s: %s", self._current_path, exc) toc = [] if not toc: self._toc_tree.setVisible(False) self._toc_btn.setVisible(False) return - # toc is a list of [level, title, page] (page is 1-indexed) - stack = [(0, self._toc_tree.invisibleRootItem())] - for level, title, page in toc: - while stack and stack[-1][0] >= level: - stack.pop() - parent = stack[-1][1] if stack else self._toc_tree.invisibleRootItem() - item = QTreeWidgetItem(parent, [title]) - item.setData(0, Qt.ItemDataRole.UserRole, max(0, page - 1)) - item.setToolTip(0, title) - stack.append((level, item)) - self._toc_tree.expandToDepth(1) - self._toc_tree.setVisible(True) - self._toc_btn.setVisible(True) - self._toc_btn.setEnabled(True) + try: + # toc is a list of [level, title, page] (page is 1-indexed) + stack = [(0, self._toc_tree.invisibleRootItem())] + for level, title, page in toc: + while stack and stack[-1][0] >= level: + stack.pop() + parent = stack[-1][1] if stack else self._toc_tree.invisibleRootItem() + item = QTreeWidgetItem(parent, [title]) + item.setData(0, Qt.ItemDataRole.UserRole, max(0, page - 1)) + item.setToolTip(0, title) + stack.append((level, item)) + self._toc_tree.expandToDepth(1) + self._toc_tree.setVisible(True) + self._toc_btn.setVisible(True) + self._toc_btn.setEnabled(True) + except Exception as exc: + import logging + logging.getLogger(__name__).warning( + "Failed to build TOC tree for %s: %s", + self._current_path, exc) + # Reset to a known-empty state so a partial build does not + # leave dangling QTreeWidgetItems pointing at invalid pages. + self._toc_tree.clear() + self._toc_tree.setVisible(False) + self._toc_btn.setVisible(False) def _on_toc_clicked(self, item, column): page_idx = item.data(0, Qt.ItemDataRole.UserRole) diff --git a/app/window.py b/app/window.py index ec5211e..31fadb3 100644 --- a/app/window.py +++ b/app/window.py @@ -79,8 +79,6 @@ def _build_nav_items(): class MainWindow(QMainWindow): - _update_ready = Signal() - def __init__(self): super().__init__() self.setWindowTitle(t("app.name")) @@ -268,7 +266,8 @@ def _a11y(btn, tip): self._update_btn.clicked.connect(self._show_update_dialog) wb_h.addWidget(self._update_btn) self._update_release = None - self._update_ready.connect(self._notify_update) + self._update_thread = None + self._update_worker = None self._check_for_updates_async() root_v.addWidget(self._workspace_bar) @@ -1155,13 +1154,11 @@ def closeEvent(self, event): wait_fn() # Same for the update-check thread (usually a short HTTP # request, but the user can close the app immediately on - # launch and Qt will warn if it's still running). - upd = getattr(self, "_update_thread", None) - if upd is not None: - with contextlib.suppress(RuntimeError): - if upd.isRunning(): - upd.quit() - upd.wait(1000) + # launch and Qt will warn if it's still running). Also drops + # the QObject worker to release its closure / release dict + # if the user closes the window before the check completes + # (R8-H2 defensive path). + self._release_update_worker() try: from app.i18n import _update_config sizes = self._splitter.sizes() @@ -1339,8 +1336,32 @@ def run(self): self._update_thread.start() def _on_update_found(self): - self._update_release = self._update_worker.release + if self._update_worker is not None: + self._update_release = self._update_worker.release self._notify_update() + # R8-H2: the worker QObject lived for the lifetime of the + # application before this — only the QThread was scheduled for + # deleteLater. Drop the worker after the check completes so the + # closure (and its captured release dict) is freed. + self._release_update_worker() + + def _release_update_worker(self): + """Tear down the update worker/thread defensively. + + Safe to call from both ``_on_update_found`` (the happy path) and + ``closeEvent`` (in case the worker never emitted ``done`` — e.g. + no update available, network failure).""" + worker = getattr(self, "_update_worker", None) + if worker is not None: + with contextlib.suppress(RuntimeError): + worker.deleteLater() + self._update_worker = None + thread = getattr(self, "_update_thread", None) + if thread is not None: + with contextlib.suppress(RuntimeError): + if thread.isRunning(): + thread.quit() + thread.wait(1000) def _notify_update(self): """Show update notification dialog automatically.""" diff --git a/pdfapps.py b/pdfapps.py index 28ec517..754a5b7 100644 --- a/pdfapps.py +++ b/pdfapps.py @@ -1,4 +1,5 @@ """PDFApps – entry point.""" +import argparse import sys import os @@ -21,6 +22,7 @@ "Install PyMuPDF:\n\npip install pymupdf") sys.exit(1) +from app.constants import APP_VERSION from app.window import MainWindow from app.styles import STYLE, STYLE_LIGHT from app.utils import _make_palette, setup_logging @@ -36,8 +38,39 @@ def _load_dark_pref() -> bool: return True +def _parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse the PDFApps command-line. + + Accepting *multiple* positional PDF paths fixes R8-M1: previously + only ``sys.argv[1]`` was loaded, so dragging-and-dropping more than + one file onto the executable (or "Open With" multi-selection on + Windows / macOS) silently dropped every file after the first. + ``argparse`` also brings standard ``--help`` / ``--version`` for + free; the old hand-rolled parser treated ``-h`` as an invalid path + and launched into the welcome screen. + """ + parser = argparse.ArgumentParser( + prog="pdfapps", + description="PDFApps — fast desktop PDF editor.", + add_help=True, + ) + parser.add_argument( + "files", nargs="*", metavar="PDF", + help=("One or more PDF files to open. Each file opens in its " + "own tab. Defaults to the welcome screen."), + ) + parser.add_argument( + "-v", "--version", action="version", + version=f"PDFApps {APP_VERSION}", + ) + return parser.parse_args(argv) + + def main(): setup_logging() + # Parse BEFORE QApplication so --help / --version exit cleanly + # without bringing up the Qt event loop (and the splash screen). + args = _parse_args(sys.argv[1:]) app = QApplication(sys.argv) app.setApplicationName(" ") app.setApplicationDisplayName(" ") @@ -49,11 +82,13 @@ def main(): window = MainWindow() window.show() - # Open PDF passed as argument (e.g.: double-click on a .pdf file) - if len(sys.argv) > 1: - pdf_arg = sys.argv[1] + # Open PDFs passed as arguments (e.g.: double-click on a .pdf file + # or multi-select "Open With" on Windows/macOS). Each valid path + # opens through _load_and_track so it lands in a new tab and is + # appended to the recents list, matching drag-and-drop semantics. + for pdf_arg in args.files: if os.path.isfile(pdf_arg) and pdf_arg.lower().endswith(".pdf"): - window._viewer.load(pdf_arg) + window._load_and_track(pdf_arg) sys.exit(app.exec()) diff --git a/tests/test_round8_fixes.py b/tests/test_round8_fixes.py new file mode 100644 index 0000000..a9f7681 --- /dev/null +++ b/tests/test_round8_fixes.py @@ -0,0 +1,151 @@ +"""Source-level regression tests for PR-C / Round 8 selective fixes. + +These do not exercise the Qt event loop — instead they read the source +of the touched files and assert that the new wiring is in place, so a +future refactor that drops a guard reintroduces an obvious test +failure rather than a silent regression. + +Mapped fixes: + - R8-H1 : password QLineEdits wiped after encrypt _run() + - R8-H2 : _update_worker.deleteLater() + _release_update_worker + - R8-M1 : argparse CLI with multi-PDF support + - R8/D1 : screenChanged handler on viewer + editor canvases + - R8 dead-code : _update_ready signal removed + - R8 bonus #6 : encrypted watermark PDF prompts for password + - R8 bonus #7 : _populate_toc wrapped in try/except + logging +""" + +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent + + +def _read(rel: str) -> str: + return (ROOT / rel).read_text(encoding="utf-8") + + +# ── R8-H1 ──────────────────────────────────────────────────────────────── + + +def test_encrypt_clears_password_fields(): + """encrypt._run() must wipe the password QLineEdits via try/finally.""" + src = _read("app/tools/encrypt.py") + assert "import contextlib" in src + assert "_clear_password_fields" in src + # The helper must touch all four password fields + for fld in ("self.edit_owner", "self.edit_owner_confirm", + "self.edit_user", "self.edit_pwd"): + assert fld in src, f"password field {fld} missing from clear loop" + # _run() must wire the wipe through try/finally so it fires on + # both happy and exception paths. + assert re.search( + r"def _run\(self\):.*?finally:\s*\n\s*#.*?\n\s*self\._clear_password_fields\(\)", + src, re.DOTALL), "expected try/finally calling _clear_password_fields" + + +# ── R8-H2 + dead signal removal ────────────────────────────────────────── + + +def test_update_worker_release_helper_exists(): + src = _read("app/window.py") + assert "_release_update_worker" in src + # Both _on_update_found and closeEvent must invoke the helper. + assert src.count("_release_update_worker()") >= 2, ( + "expected _release_update_worker to be called from both " + "_on_update_found and closeEvent") + assert "worker.deleteLater()" in src + + +def test_update_ready_signal_removed(): + src = _read("app/window.py") + assert "_update_ready = Signal()" not in src, ( + "_update_ready was declared but never emitted — should be gone") + assert "self._update_ready.connect" not in src + assert "self._update_ready.emit" not in src + + +# ── R8-M1 ──────────────────────────────────────────────────────────────── + + +def test_pdfapps_uses_argparse(): + src = _read("pdfapps.py") + assert "import argparse" in src + assert "argparse.ArgumentParser" in src + assert "nargs=\"*\"" in src or "nargs='*'" in src + # Multi-file loop, not just argv[1] + assert "for pdf_arg in args.files" in src + assert "_load_and_track" in src + + +def test_pdfapps_version_flag_runs(): + """python pdfapps.py --version must print the APP_VERSION and exit 0.""" + result = subprocess.run( + [sys.executable, str(ROOT / "pdfapps.py"), "--version"], + capture_output=True, text=True, timeout=30, + ) + assert result.returncode == 0, result.stderr + out = (result.stdout + result.stderr).strip() + assert out.startswith("PDFApps "), out + # Sanity-check format: PDFApps X.Y.Z + assert re.match(r"^PDFApps \d+\.\d+\.\d+", out), out + + +def test_pdfapps_help_flag_runs(): + """python pdfapps.py --help must print usage and exit 0.""" + result = subprocess.run( + [sys.executable, str(ROOT / "pdfapps.py"), "--help"], + capture_output=True, text=True, timeout=30, + ) + assert result.returncode == 0, result.stderr + out = result.stdout + result.stderr + assert "usage:" in out.lower() + assert "PDF" in out # positional metavar + assert "--version" in out + + +# ── R8/D1 ──────────────────────────────────────────────────────────────── + + +def test_viewer_canvas_handles_screen_changed(): + src = _read("app/viewer/canvas.py") + assert "def showEvent" in src + assert "screenChanged" in src + assert "_on_screen_changed" in src + # The handler must invalidate cached pixmaps + assert "entry.pixmap = None" in src + + +def test_editor_canvas_handles_screen_changed(): + src = _read("app/editor/canvas.py") + assert "def showEvent" in src + assert "screenChanged" in src + assert "_on_screen_changed" in src + assert "_page_pixmaps" in src + + +# ── R8 bonus #6 — watermark encrypted PDF ──────────────────────────────── + + +def test_watermark_prompts_for_encrypted_wm(): + src = _read("app/tools/watermark.py") + assert "_prompt_watermark_password" in src + assert "prompt_pdf_password" in src + # Worker decrypts the watermark reader with the prompted password + assert "wm.is_encrypted" in src + assert "wm.decrypt(wm_pwd)" in src + + +# ── R8 bonus #7 — TOC try/except ───────────────────────────────────────── + + +def test_populate_toc_logs_on_failure(): + src = _read("app/viewer/panel.py") + # The whole build loop is wrapped, not just doc.get_toc() + assert "Failed to build TOC tree" in src + assert "Failed to read TOC" in src + assert "logging.getLogger(__name__).warning" in src