diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 3c471ae..1655d67 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -94,8 +94,12 @@ jobs: ], True), ("aur/pdfapps/.SRCINFO", [ (rf'pkgver = {ANY}', f'pkgver = {new}'), - (rf'pdfapps-{ANY}', f'pdfapps-{new}'), - (rf'v{ANY}', f'v{new}'), + # R10 #9: anchor the source-tarball replacement to + # the ``.tar.gz`` suffix + URL contexts. The previous + # unanchored ``v{ANY}`` would happily rewrite a + # future comment line like ``# since v1.10.0``. + (rf'pdfapps-{ANY}\.tar\.gz', f'pdfapps-{new}.tar.gz'), + (rf'/v{ANY}\.tar\.gz', f'/v{new}.tar.gz'), ], True), ("aur/pdfapps-bin/PKGBUILD", [ (rf'pkgver={ANY}', f'pkgver={new}'), @@ -103,8 +107,11 @@ jobs: ("aur/pdfapps-bin/.SRCINFO", [ (rf'pkgver = {ANY}', f'pkgver = {new}'), (rf'provides = pdfapps={ANY}', f'provides = pdfapps={new}'), - (rf'v{ANY}', f'v{new}'), - (rf'pdfapps-{ANY}', f'pdfapps-{new}'), + # R10 #9: as for the source AUR variant — anchor on + # path/tarball context so a future "# since v1.10.0" + # comment is not rewritten by an unanchored v{ANY}. + (rf'/v{ANY}/', f'/v{new}/'), + (rf'pdfapps-{ANY}\.tar\.gz', f'pdfapps-{new}.tar.gz'), ], True), ("winget/nelsonduarte.PDFApps.installer.yaml", [ (rf'PackageVersion:\s*{ANY}', f'PackageVersion: {new}'), diff --git a/app/editor/canvas.py b/app/editor/canvas.py index 992befd..e513198 100644 --- a/app/editor/canvas.py +++ b/app/editor/canvas.py @@ -2,7 +2,6 @@ import contextlib import os -from functools import lru_cache from PySide6.QtCore import Qt, Signal, QRect, QPoint, QObject, QRunnable, QThreadPool, QEvent from PySide6.QtGui import QPixmap @@ -18,19 +17,50 @@ _ICON_CURSORS: dict = {} +# Manual FIFO cache for overlay QPixmaps (R10 #3). +# The previous ``@lru_cache(maxsize=64)`` had no clear() hook the tab +# close path could call — across long-running sessions with several +# tabs each cache slot may hold a multi-MB QPixmap, leaking until +# process exit. The explicit dict gives us: +# - eviction by FIFO insertion order (next(iter(dict)) is O(1)); +# - a clear_overlay_pixmap_cache() public helper for tab teardown; +# - an "uncached" code path for mtime lookup failures (R10 #11) so an +# OSError doesn't poison the cache with a null QPixmap that stays +# forever even after the file reappears on disk. +_OVERLAY_PIXMAP_CACHE: "dict[tuple[str, float], QPixmap]" = {} +_OVERLAY_PIXMAP_CACHE_MAX = 64 -@lru_cache(maxsize=64) -def _load_overlay_pixmap(path: str, _mtime: float) -> QPixmap: - """LRU-cached QPixmap loader for overlay image/signature stamps. + +def _load_overlay_pixmap(path: str, mtime: float) -> QPixmap: + """Explicit FIFO-cached QPixmap loader for overlay image/signature stamps. The previous implementation built a fresh ``QPixmap(path)`` on every ``paintEvent`` — once per overlay — which became the dominant cost of scrolling a document containing dozens of inserted images. The - ``_mtime`` parameter (which the caller passes verbatim) participates - in the cache key so the cache auto-invalidates when the underlying - file is rewritten (e.g. signature regenerated on disk). + ``mtime`` parameter participates in the cache key so the cache + auto-invalidates when the underlying file is rewritten (e.g. + signature regenerated on disk). + """ + key = (path, mtime) + pix = _OVERLAY_PIXMAP_CACHE.get(key) + if pix is None: + pix = QPixmap(path) + if len(_OVERLAY_PIXMAP_CACHE) >= _OVERLAY_PIXMAP_CACHE_MAX: + # Evict the oldest entry (insertion order is preserved + # from Python 3.7+). + _OVERLAY_PIXMAP_CACHE.pop(next(iter(_OVERLAY_PIXMAP_CACHE))) + _OVERLAY_PIXMAP_CACHE[key] = pix + return pix + + +def clear_overlay_pixmap_cache() -> None: + """Drop every cached overlay QPixmap. + + Called from :meth:`PdfEditCanvas.close_doc` so closing a tab + releases the (potentially many MB worth of) pixmaps held by the + module-level cache. Safe to call multiple times. """ - return QPixmap(path) + _OVERLAY_PIXMAP_CACHE.clear() def _get_icon_cursor(icon_name: str, hx: int, hy: int, @@ -488,6 +518,11 @@ def close_doc(self): self._page_pixmaps.clear() self._page_offsets.clear() self._overlays = []; self._open_note = None + # R10 #3: drop module-level overlay QPixmap cache so closing + # a tab actually releases the (potentially many MB) of cached + # image/signature pixmaps. The previous lru_cache had no + # clear hook and grew unboundedly across long sessions. + clear_overlay_pixmap_cache() self.setMinimumSize(300, 400) self.setMaximumSize(16777215, 16777215) self.update() @@ -684,11 +719,16 @@ def paintEvent(self, _): r = e["rect"] qr = QRect(int(r.x0*z), yo+int(r.y0*z), max(1,int(r.width*z)), max(1,int(r.height*z))) path = e["path"] + # R10 #11: if getmtime fails (file removed, permission + # error, transient FS issue) we MUST NOT cache the + # resulting null QPixmap — otherwise the cache would + # keep returning the empty pixmap even after the file + # comes back. Load uncached in that case. try: mtime = os.path.getmtime(path) + img_px = _load_overlay_pixmap(path, mtime) except OSError: - mtime = 0.0 - img_px = _load_overlay_pixmap(path, mtime) + img_px = QPixmap(path) if not img_px.isNull(): p.drawPixmap(qr, img_px) border = "#22C55E" if etype == "signature" else ACCENT @@ -860,11 +900,24 @@ def contextMenuEvent(self, e): if hit < 0: hit, _ = self._annot_note_at(pos) if hit >= 0: - from PySide6.QtWidgets import QMenu + from PySide6.QtWidgets import QMenu, QMessageBox menu = QMenu(self) delete_action = menu.addAction(t("viewer.delete_comment")) action = menu.exec(e.globalPos()) if action == delete_action: + # R10 #8: match the viewer's UX — delete is destructive + # (especially for _existing notes which persist on save), + # so confirm before pulling the trigger. defaultButton + # is No so a stray Enter cannot wipe a note. + reply = QMessageBox.question( + self, t("msg.confirm"), + t("viewer.confirm_delete_comment"), + QMessageBox.StandardButton.Yes + | QMessageBox.StandardButton.No, + QMessageBox.StandardButton.No, + ) + if reply != QMessageBox.StandardButton.Yes: + return overlay = self._overlays[hit] if self._doc and overlay.get("_existing"): import fitz diff --git a/app/editor/tab.py b/app/editor/tab.py index f3dd422..71db5b9 100644 --- a/app/editor/tab.py +++ b/app/editor/tab.py @@ -1122,22 +1122,35 @@ def _run(self): QMessageBox.warning(self, t("msg.warning"), t("msg.no_pending")); return try: import fitz - # Release the file lock without resetting the canvas - self._canvas.release_doc() - doc = fitz.open(self._doc_path) - was_encrypted = bool(doc.needs_pass) - if doc.needs_pass and self._pdf_password: - doc.authenticate(self._pdf_password) - # If the input was encrypted, ask the user whether to preserve - # protection on the output. Defaults to "Keep protection" so - # silent down-grading never happens. We can only re-encrypt if - # we still hold the password from the load prompt. + # CRIT-2 (R10): peek the encryption status BEFORE releasing + # the canvas. PR-D moved release_doc() ahead of the prompt + # so the canvas dropped its _doc reference even when the + # user then cancelled the encryption dialog — leaving the + # canvas stuck on the placeholder until the user manually + # reloaded the file. Now we open a short-lived peek doc, + # ask the user how to save, and only release the canvas + # once we know we will proceed. + peek = fitz.open(self._doc_path) + was_encrypted = bool(peek.needs_pass) + if was_encrypted and self._pdf_password: + peek.authenticate(self._pdf_password) encrypt_choice = "plaintext" if was_encrypted and self._pdf_password: encrypt_choice = self._prompt_encryption_choice() if encrypt_choice is None: - doc.close() + # User cancelled — peek must be closed BUT the + # canvas must still hold the original doc so the + # editor view survives the dismiss. + peek.close() return + peek.close() + # Encryption choice confirmed (or no prompt needed) — + # now safe to release the canvas's file lock so the + # real save reopen can take exclusive access. + self._canvas.release_doc() + doc = fitz.open(self._doc_path) + if doc.needs_pass and self._pdf_password: + doc.authenticate(self._pdf_password) for e in self._pending: if e.get("_existing") and e.get("type") != "delete_annot": continue # already saved in the PDF @@ -1276,6 +1289,17 @@ def _apply_forms(self, out): fields = {self._form_table.item(r, 0).text(): (self._form_table.item(r, 1).text() if self._form_table.item(r, 1) else "") for r in range(self._form_table.rowCount())} + # R10 #6: pypdf's update_page_form_field_values raises + # PyPdfError("No /AcroForm dictionary in PDF…") on PDFs + # without form fields. The user hits this whenever they + # click Apply in Forms mode on a regular PDF; the cryptic + # error message looked like an internal crash. Detect + # up-front and short-circuit with a friendly status + # instead, leaving the file untouched. + if "/AcroForm" not in writer._root_object: + self._status(t("editor.forms.no_fields")) + self._form_status.setText(t("editor.forms.no_fields")) + return for page in writer.pages: # auto_regenerate=True so the rendered widget appearance # actually picks up the new value when viewed in a third- diff --git a/app/translations.json b/app/translations.json index 5cd06d2..5ac7138 100644 --- a/app/translations.json +++ b/app/translations.json @@ -60,6 +60,7 @@ "viewer.error_open_msg": "Could not open the file:\n{ex}", "viewer.delete_comment": "Delete comment", "viewer.confirm_delete_comment": "Are you sure you want to delete this comment? This change is saved immediately and cannot be undone.", + "viewer.drop_url_not_supported": "Online URLs are not supported. Please download the PDF first, then drag the local file here.", "viewer.copy_chars": " Copy ({n} chars)", "search.placeholder": "Search in PDF...", "search.prev": "Previous match", @@ -657,6 +658,7 @@ "viewer.error_open_msg": "Não foi possível abrir o ficheiro:\n{ex}", "viewer.delete_comment": "Apagar comentário", "viewer.confirm_delete_comment": "Tens a certeza que queres apagar este comentário? Esta alteração é guardada imediatamente e não pode ser desfeita.", + "viewer.drop_url_not_supported": "URLs da internet não são suportados. Descarrega primeiro o PDF e depois arrasta o ficheiro local para aqui.", "viewer.copy_chars": " Copiar ({n} car.)", "search.placeholder": "Pesquisar no PDF...", "search.prev": "Resultado anterior", @@ -1254,6 +1256,7 @@ "viewer.error_open_msg": "No se pudo abrir el archivo:\n{ex}", "viewer.delete_comment": "Eliminar comentario", "viewer.confirm_delete_comment": "¿Seguro que quieres eliminar este comentario? Este cambio se guarda inmediatamente y no se puede deshacer.", + "viewer.drop_url_not_supported": "Las URLs en línea no son compatibles. Descarga primero el PDF y luego arrastra el archivo local aquí.", "viewer.copy_chars": " Copiar ({n} caract.)", "search.placeholder": "Buscar en PDF...", "search.prev": "Resultado anterior", @@ -1851,6 +1854,7 @@ "viewer.error_open_msg": "Impossible d'ouvrir le fichier :\n{ex}", "viewer.delete_comment": "Supprimer le commentaire", "viewer.confirm_delete_comment": "Voulez-vous vraiment supprimer ce commentaire ? Cette modification est enregistrée immédiatement et ne peut pas être annulée.", + "viewer.drop_url_not_supported": "Les URL en ligne ne sont pas prises en charge. Téléchargez d'abord le PDF, puis faites glisser le fichier local ici.", "viewer.copy_chars": " Copier ({n} caract.)", "search.placeholder": "Rechercher dans le PDF...", "search.prev": "Résultat précédent", @@ -2448,6 +2452,7 @@ "viewer.error_open_msg": "Die Datei konnte nicht geöffnet werden:\n{ex}", "viewer.delete_comment": "Kommentar löschen", "viewer.confirm_delete_comment": "Soll dieser Kommentar wirklich gelöscht werden? Diese Änderung wird sofort gespeichert und kann nicht rückgängig gemacht werden.", + "viewer.drop_url_not_supported": "Online-URLs werden nicht unterstützt. Laden Sie die PDF zuerst herunter und ziehen Sie dann die lokale Datei hierher.", "viewer.copy_chars": " Kopieren ({n} Zeichen)", "search.placeholder": "Im PDF suchen...", "search.prev": "Vorheriges Ergebnis", @@ -3045,6 +3050,7 @@ "viewer.error_open_msg": "无法打开文件：\n{ex}", "viewer.delete_comment": "删除批注", "viewer.confirm_delete_comment": "确定要删除此批注吗？此更改将立即保存且无法撤销。", + "viewer.drop_url_not_supported": "不支持在线 URL。请先下载 PDF，然后将本地文件拖到此处。", "viewer.copy_chars": " 复制 （{n} 个字符）", "search.placeholder": "在 PDF 中搜索...", "search.prev": "上一个匹配", @@ -3642,6 +3648,7 @@ "viewer.error_open_msg": "Impossibile aprire il file:\n{ex}", "viewer.delete_comment": "Elimina commento", "viewer.confirm_delete_comment": "Sei sicuro di voler eliminare questo commento? Questa modifica viene salvata immediatamente e non può essere annullata.", + "viewer.drop_url_not_supported": "Gli URL online non sono supportati. Scarica prima il PDF, poi trascina il file locale qui.", "viewer.copy_chars": " Copia ({n} caratteri)", "search.placeholder": "Cerca nel PDF...", "search.prev": "Corrispondenza precedente", @@ -4239,6 +4246,7 @@ "viewer.error_open_msg": "Kon het bestand niet openen:\n{ex}", "viewer.delete_comment": "Opmerking verwijderen", "viewer.confirm_delete_comment": "Weet je zeker dat je deze opmerking wilt verwijderen? Deze wijziging wordt onmiddellijk opgeslagen en kan niet ongedaan worden gemaakt.", + "viewer.drop_url_not_supported": "Online-URL's worden niet ondersteund. Download eerst de PDF en sleep vervolgens het lokale bestand hierheen.", "viewer.copy_chars": " Kopiëren ({n} tekens)", "search.placeholder": "Zoeken in PDF...", "search.prev": "Vorige overeenkomst", diff --git a/app/viewer/canvas.py b/app/viewer/canvas.py index 8c914d0..cf3a5f0 100644 --- a/app/viewer/canvas.py +++ b/app/viewer/canvas.py @@ -3,6 +3,9 @@ from __future__ import annotations import contextlib +import os +import shutil +import tempfile from PySide6.QtCore import Qt, Signal, QRect, QObject, QRunnable, QThreadPool from PySide6.QtWidgets import QWidget, QApplication @@ -615,23 +618,105 @@ def contextMenuEvent(self, e): # Remove annotation from fitz doc if self._doc: import fitz + from app.utils import show_error + # CRIT-1: backup the file before saveIncr so a + # power loss / write failure leaves the original + # intact. Same-directory tempfile keeps the + # shutil.move(restore) atomic on POSIX/Windows. + backup_path = None + if self._path and os.path.isfile(self._path): + try: + src_dir = os.path.dirname(self._path) or "." + fd, backup_path = tempfile.mkstemp( + prefix=".pdfapps_backup_", + suffix=".pdf.bak", + dir=src_dir, + ) + os.close(fd) + shutil.copy2(self._path, backup_path) + except Exception as exc: + if backup_path: + with contextlib.suppress(Exception): + os.unlink(backup_path) + backup_path = None + show_error(self, exc) + return + # Step 1 (HIGH A2): mutate the in-memory doc. + # If this raises, no disk state changes and we + # bail before touching saveIncr(). + target_annot = None try: page = self._doc[page_idx] for annot in page.annots() or []: - if annot.type[0] == fitz.PDF_ANNOT_TEXT: - content = annot.info.get("content", "") or "" + if annot.type[0] != fitz.PDF_ANNOT_TEXT: + continue + content = annot.info.get("content", "") or "" + if content.strip() != txt.strip(): + continue + # HIGH A3: 2 notes with identical content + # would silently delete the wrong one. + # Tiebreak with bbox match (1pt tol). + ar = annot.rect + if (abs(ar.x0 - rect.x0) < 1 + and abs(ar.y0 - rect.y0) < 1): + target_annot = annot + break + if target_annot is None: + # Content-only fallback for legacy callers + # that don't have a usable bbox (e.g. + # rect was synthesised). Preserves the + # old behaviour rather than no-op'ing. + for annot in page.annots() or []: + if annot.type[0] != fitz.PDF_ANNOT_TEXT: + continue + content = annot.info.get( + "content", "") or "" if content.strip() == txt.strip(): - page.delete_annot(annot) + target_annot = annot break - # Save the doc. saveIncr() raises on read-only - # files / permission errors; surface a friendly - # dialog instead of crashing the Qt event loop. - if self._path: - self._doc.saveIncr() + if target_annot is not None: + page.delete_annot(target_annot) except Exception as exc: - from app.utils import show_error + if backup_path: + with contextlib.suppress(Exception): + os.unlink(backup_path) show_error(self, exc) return + # Step 2 (HIGH A2): persist to disk. If this + # fails (read-only file, ENOSPC, power loss + # mid-write), restore from the backup so the + # user does not lose the original AND reload + # the in-memory doc so the next paint event + # reflects the on-disk state. + if self._path: + try: + self._doc.saveIncr() + except Exception as exc: + # CRIT-1: restore backup. shutil.move + # is best-effort — if it fails we still + # have the .bak on disk for the user. + if backup_path: + with contextlib.suppress(Exception): + shutil.move(backup_path, self._path) + backup_path = None + # HIGH A2: discard the in-memory delete + # by reopening the file. Best-effort: + # if reopen fails we still surface the + # original write error. + with contextlib.suppress(Exception): + saved_path = self._path + saved_password = self._password + self._doc.close() + new_doc = fitz.open(saved_path) + if new_doc.needs_pass and saved_password: + new_doc.authenticate(saved_password) + self._doc = new_doc + show_error(self, exc) + return + # Both steps succeeded — drop the backup. + if backup_path: + with contextlib.suppress(Exception): + os.unlink(backup_path) # Remove from entry annots list entry.annots.pop(annot_idx) # The _open_note tuple stores (page_idx, annot_idx). diff --git a/app/viewer/panel.py b/app/viewer/panel.py index 24ca77b..980dac5 100644 --- a/app/viewer/panel.py +++ b/app/viewer/panel.py @@ -128,49 +128,16 @@ def _a11y(btn, tip): ph_lay.addWidget(ph_drag) # Recent files section - from app.i18n import get_recent_files, add_recent_file self._recents_container = QWidget() self._recents_container.setMaximumWidth(400) self._recent_links: list[QPushButton] = [] self._recent_del_btns: list[QPushButton] = [] - rc_lay = QVBoxLayout(self._recents_container) - rc_lay.setContentsMargins(0, 16, 0, 0); rc_lay.setSpacing(4) - recents = get_recent_files() - if recents: - rec_title = QLabel(t("viewer.recent")) - rec_title.setAlignment(Qt.AlignmentFlag.AlignCenter) - rec_title.setStyleSheet("font-size: 10pt; font-weight: 600; opacity: 0.7;") - rc_lay.addWidget(rec_title) - for rp in recents[:5]: - if not os.path.isfile(rp): - continue - fname = os.path.basename(rp) - row = QWidget() - row_h = QHBoxLayout(row); row_h.setContentsMargins(0, 0, 0, 0); row_h.setSpacing(0) - link = QPushButton(f"📄 {fname}") - link.setObjectName("recent_link") - link.setToolTip(rp) - link.setCursor(Qt.CursorShape.PointingHandCursor) - link.setFlat(True) - link.setStyleSheet(self._recent_link_style(dark=True)) - link.clicked.connect(lambda checked, p=rp: self.load(p)) - self._recent_links.append(link) - del_btn = QPushButton() - del_btn.setIcon(qta.icon("fa5s.trash-alt", color=TEXT_SEC)) - del_btn.setFixedSize(28, 28) - del_btn.setCursor(Qt.CursorShape.PointingHandCursor) - del_btn.setFlat(True) - del_btn.setToolTip(t("btn.remove")) - del_btn.setAccessibleName(t("btn.remove")) - del_btn.setStyleSheet( - f"QPushButton {{ border: 1px solid transparent; background: transparent; }}" - f"QPushButton:hover {{ background: rgba(239,68,68,0.15); border-radius: 4px; }}" - f"QPushButton:focus {{ border: 1px solid {ACCENT}; border-radius: 4px; }}") - del_btn.clicked.connect(lambda checked, p=rp, r=row: self._remove_recent(p, r)) - self._recent_del_btns.append(del_btn) - row_h.addWidget(link, 1) - row_h.addWidget(del_btn) - rc_lay.addWidget(row) + self._recents_layout = QVBoxLayout(self._recents_container) + self._recents_layout.setContentsMargins(0, 16, 0, 0) + self._recents_layout.setSpacing(4) + # R10 #5: populate via the refresh helper so the same code + # path drives both initial draw and post-load updates. + self._refresh_recents() ph_lay.addWidget(self._recents_container, 0, Qt.AlignmentFlag.AlignCenter) self._placeholder = ph_widget @@ -336,6 +303,74 @@ def update_theme(self, dark: bool) -> None: # Drag & drop is handled at the MainWindow level (see window.py). + # ── Recents ──────────────────────────────────────────────────────────── + def _refresh_recents(self): + """Rebuild the recents section under the placeholder (R10 #5). + + ``add_recent_file()`` is called from ``window._load_and_track`` + AFTER a file is opened, but the recents UI was built once in + ``__init__`` and never re-read the config. Users opening a new + file saw the stale list until the next launch. Rebuild from + scratch so the next time the placeholder is shown (i.e. after + the user closes the open file) the list is up to date. + """ + from app.i18n import get_recent_files + lay = self._recents_layout + # Wipe existing rows. takeAt(0) detaches and deleteLater + # the widget — including nested layouts via the recursive + # _drop_layout_item helper below. + while lay.count(): + item = lay.takeAt(0) + w = item.widget() + if w is not None: + w.deleteLater() + self._recent_links = [] + self._recent_del_btns = [] + recents = get_recent_files() + if not recents: + return + rec_title = QLabel(t("viewer.recent")) + rec_title.setAlignment(Qt.AlignmentFlag.AlignCenter) + rec_title.setStyleSheet( + "font-size: 10pt; font-weight: 600; opacity: 0.7;") + lay.addWidget(rec_title) + for rp in recents[:5]: + if not os.path.isfile(rp): + continue + fname = os.path.basename(rp) + row = QWidget() + row_h = QHBoxLayout(row) + row_h.setContentsMargins(0, 0, 0, 0) + row_h.setSpacing(0) + link = QPushButton(f"📄 {fname}") + link.setObjectName("recent_link") + link.setToolTip(rp) + link.setCursor(Qt.CursorShape.PointingHandCursor) + link.setFlat(True) + link.setStyleSheet(self._recent_link_style(dark=True)) + link.clicked.connect(lambda checked, p=rp: self.load(p)) + self._recent_links.append(link) + del_btn = QPushButton() + del_btn.setIcon(qta.icon("fa5s.trash-alt", color=TEXT_SEC)) + del_btn.setFixedSize(28, 28) + del_btn.setCursor(Qt.CursorShape.PointingHandCursor) + del_btn.setFlat(True) + del_btn.setToolTip(t("btn.remove")) + del_btn.setAccessibleName(t("btn.remove")) + del_btn.setStyleSheet( + f"QPushButton {{ border: 1px solid transparent; " + f"background: transparent; }}" + f"QPushButton:hover {{ background: rgba(239,68,68,0.15); " + f"border-radius: 4px; }}" + f"QPushButton:focus {{ border: 1px solid {ACCENT}; " + f"border-radius: 4px; }}") + del_btn.clicked.connect( + lambda checked, p=rp, r=row: self._remove_recent(p, r)) + self._recent_del_btns.append(del_btn) + row_h.addWidget(link, 1) + row_h.addWidget(del_btn) + lay.addWidget(row) + # ── Open dialog ──────────────────────────────────────────────────────── def _remove_recent(self, path: str, row_widget): """Remove a file from recents and hide its row.""" diff --git a/app/window.py b/app/window.py index f61005b..90fefab 100644 --- a/app/window.py +++ b/app/window.py @@ -807,6 +807,13 @@ def _load_and_track(self, path: str): else: self._viewer.load(path) add_recent_file(path) + # R10 #5: rebuild the recents list across every open viewer + # so the next return-to-placeholder shows the up-to-date list. + for v in self._viewers: + refresh = getattr(v, "_refresh_recents", None) + if callable(refresh): + with contextlib.suppress(Exception): + refresh() self._refresh_viewer_top_buttons() def _open_in_new_tab(self): @@ -817,6 +824,12 @@ def _open_in_new_tab(self): if path: self._add_viewer_tab(path) add_recent_file(path) + # R10 #5: keep the recents list in sync with config. + for v in self._viewers: + refresh = getattr(v, "_refresh_recents", None) + if callable(refresh): + with contextlib.suppress(Exception): + refresh() def _show_recent_menu(self): menu = QMenu(self) @@ -965,14 +978,45 @@ def _restart_app(self): # ── Drag & drop PDF on window ────────────────────────────────────────── def dragEnterEvent(self, e): - if e.mimeData().hasUrls(): - urls = e.mimeData().urls() - if urls and urls[0].toLocalFile().lower().endswith(".pdf"): + if not e.mimeData().hasUrls(): + return + for url in e.mimeData().urls(): + # R10 #12: accept folders + .pdf files. Drop iterates the + # folder for PDFs at dropEvent time so we can't validate + # contents here, but a folder drop should not be silently + # ignored either. + local = url.toLocalFile() + if not local: + continue + if (local.lower().endswith(".pdf") + or os.path.isdir(local)): e.acceptProposedAction() + return def dropEvent(self, e): + # R10 #12: accept folders (open every .pdf inside) and warn + # the user when a web URL is dropped instead of silently + # ignoring it. + import glob for url in e.mimeData().urls(): path = url.toLocalFile() + if not path: + # Non-file URL (http/https/ftp...). Surface a friendly + # one-shot warning rather than swallowing the drop. + scheme = url.scheme().lower() if url.isValid() else "" + if scheme in ("http", "https", "ftp"): + QMessageBox.warning( + self, t("msg.warning"), + t("viewer.drop_url_not_supported")) + return + continue + if os.path.isdir(path): + for pdf in sorted(glob.glob(os.path.join(path, "*.pdf"))): + self._load_and_track(pdf) + # Also pick up uppercase .PDF on case-sensitive FSes + for pdf in sorted(glob.glob(os.path.join(path, "*.PDF"))): + self._load_and_track(pdf) + continue if path.lower().endswith(".pdf"): self._load_and_track(path) diff --git a/tests/test_audit_r10.py b/tests/test_audit_r10.py new file mode 100644 index 0000000..1f73335 --- /dev/null +++ b/tests/test_audit_r10.py @@ -0,0 +1,285 @@ +"""Source-level regression tests for PR-F / Round 10 audit fixes. + +Bug map (matches PR-F worklist): + #1 Viewer saveIncr without backup / atomic write (CRIT) + #2 Editor release_doc before encryption prompt (CRIT - PR-D regression) + #3 Editor _load_overlay_pixmap LRU leak (HIGH) + #4 Viewer try/except annot race (delete OK, save fail) (HIGH) + #5 Recents UI stale after add_recent_file (HIGH) + #6 Forms apply on PDF without /AcroForm crashes (HIGH) + #7 Viewer note match by content-only (collision) (HIGH) + #8 Editor delete without confirmation (MED) + #9 Snap/.SRCINFO regex too greedy (MED) + #10 Tab close during OCR race (pre-existing, audited) (MED) + #11 _load_overlay_pixmap caches null QPixmap on OSError (LOW) + #12 Drop folder / web URL silently ignored (LOW) +""" + +import json +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") + + +# ── #1 — saveIncr backup + confirmation ────────────────────────────────── + + +def test_viewer_save_incr_uses_backup_before_persisting(): + """The delete-comment context menu must copy the PDF to a same- + directory .bak before page.delete_annot+saveIncr so a power loss + leaves the original recoverable.""" + src = _read("app/viewer/canvas.py") + # backup_path is the variable we introduced; shutil.copy2 + bak + # suffix are the markers for the backup step. + assert "backup_path" in src + assert "shutil.copy2" in src + assert ".pdf.bak" in src + + +def test_viewer_save_incr_restores_backup_on_failure(): + """saveIncr() error path must shutil.move(backup_path, src) so + we never silently corrupt the user's PDF.""" + src = _read("app/viewer/canvas.py") + assert "shutil.move(backup_path, self._path)" in src + + +# ── #2 — release_doc deferred past encryption prompt ───────────────────── + + +def test_editor_run_releases_canvas_after_prompt(): + """release_doc() must be reached only AFTER + _prompt_encryption_choice() so cancelling does not strand the + canvas with _doc=None.""" + src = _read("app/editor/tab.py") + run_body_start = src.find("def _run(self)") + run_body_end = src.find("def _fitz_permissions_of", run_body_start) + body = src[run_body_start:run_body_end] + prompt_pos = body.find("_prompt_encryption_choice") + release_pos = body.find("self._canvas.release_doc()") + assert prompt_pos > 0 and release_pos > 0 + # The encryption peek must happen BEFORE the canvas release. + assert prompt_pos < release_pos, ( + "release_doc() must run after the encryption prompt (CRIT-2)" + ) + + +def test_editor_run_uses_peek_for_encryption_check(): + """The peek doc pattern is the marker that we read needs_pass + without releasing the canvas-held doc.""" + src = _read("app/editor/tab.py") + assert "peek = fitz.open(self._doc_path)" in src + assert "peek.close()" in src + + +# ── #3 — Overlay pixmap cache eviction + clear hook ────────────────────── + + +def test_overlay_pixmap_cache_has_clear_hook(): + src = _read("app/editor/canvas.py") + assert "_OVERLAY_PIXMAP_CACHE" in src + assert "def clear_overlay_pixmap_cache" in src + assert "clear_overlay_pixmap_cache()" in src # called from close_doc + + +def test_overlay_pixmap_cache_has_fifo_eviction(): + """When the cache hits the cap, we evict the oldest entry — + next(iter(_OVERLAY_PIXMAP_CACHE)) returns the first inserted key + in CPython 3.7+ dicts.""" + src = _read("app/editor/canvas.py") + assert "_OVERLAY_PIXMAP_CACHE_MAX" in src + assert "next(iter(_OVERLAY_PIXMAP_CACHE))" in src + + +def test_overlay_pixmap_no_longer_uses_lru_cache(): + src = _read("app/editor/canvas.py") + # Old decorator path must be gone — the manual cache replaces it. + assert "from functools import lru_cache" not in src + # Strip comments before checking — the migration note legitimately + # mentions the old decorator by name. + code_only = "\n".join( + line.split("#", 1)[0] + for line in src.splitlines() + ) + assert "@lru_cache" not in code_only + + +# ── #4 — saveIncr separated from delete_annot + reload on error ───────── + + +def test_viewer_reloads_doc_on_save_failure(): + """When saveIncr() raises, the in-memory delete is discarded by + reopening the file so the next paintEvent reflects on-disk state.""" + src = _read("app/viewer/canvas.py") + # Reload-on-error markers: re-open via fitz.open + re-auth. + assert "self._doc.close()" in src + assert "new_doc = fitz.open(saved_path)" in src + assert "saved_password" in src + + +# ── #5 — Recents UI refresh hook ──────────────────────────────────────── + + +def test_viewer_panel_has_refresh_recents(): + src = _read("app/viewer/panel.py") + assert "def _refresh_recents" in src + # __init__ delegates to it (so single source of truth). + assert "self._refresh_recents()" in src + + +def test_window_load_and_track_refreshes_recents(): + """_load_and_track must call _refresh_recents on every open + viewer after add_recent_file so the placeholder shows the + up-to-date list on next return.""" + src = _read("app/window.py") + # Locate the _load_and_track body. + body = src[src.find("def _load_and_track"): + src.find("def _open_in_new_tab")] + assert "_refresh_recents" in body + + +# ── #6 — Forms apply guard ────────────────────────────────────────────── + + +def test_apply_forms_guards_against_missing_acroform(): + """_apply_forms must short-circuit with editor.forms.no_fields + when the writer has no /AcroForm — pypdf's + update_page_form_field_values raises a cryptic error otherwise.""" + src = _read("app/editor/tab.py") + forms_body = src[src.find("def _apply_forms"): + src.find("def _apply_forms") + 2500] + assert '"/AcroForm" not in writer._root_object' in forms_body + assert 'editor.forms.no_fields' in forms_body + + +# ── #7 — bbox tiebreaker on viewer note delete ────────────────────────── + + +def test_viewer_note_delete_tiebreaks_on_bbox(): + """Two notes with identical content must be disambiguated by bbox + proximity so the right one is deleted.""" + src = _read("app/viewer/canvas.py") + assert "abs(ar.x0 - rect.x0) < 1" in src + assert "abs(ar.y0 - rect.y0) < 1" in src + + +# ── #8 — Editor delete confirmation ────────────────────────────────────── + + +def test_editor_delete_prompts_for_confirmation(): + """contextMenuEvent's delete path must surface a QMessageBox.question + using the existing viewer.confirm_delete_comment string.""" + src = _read("app/editor/canvas.py") + ctx_start = src.find("def contextMenuEvent") + ctx_body = src[ctx_start: ctx_start + 2000] + assert "QMessageBox.question" in ctx_body + assert "viewer.confirm_delete_comment" in ctx_body + assert "QMessageBox.StandardButton.No" in ctx_body + + +# ── #9 — .SRCINFO regex anchored to tarball context ───────────────────── + + +def test_srcinfo_regex_is_anchored(): + """The version-bump step must anchor v{ANY} replacements to a + file/path context so future history comments are not rewritten.""" + src = _read(".github/workflows/release.yml") + # No unanchored v{ANY} substitution for aur/pdfapps/.SRCINFO. + aur_block_start = src.find('"aur/pdfapps/.SRCINFO"') + aur_block_end = src.find('"aur/pdfapps-bin/.SRCINFO"', aur_block_start) + aur_block = src[aur_block_start:aur_block_end] + # The bare unanchored ``v{ANY}`` rule is gone. + assert "rf'v{ANY}'" not in aur_block + # New anchored rules: + assert ".tar.gz" in aur_block + + +def test_srcinfo_bin_regex_is_anchored(): + src = _read(".github/workflows/release.yml") + bin_start = src.find('"aur/pdfapps-bin/.SRCINFO"') + bin_end = src.find("winget/nelsonduarte", bin_start) + bin_block = src[bin_start:bin_end] + assert "rf'v{ANY}'" not in bin_block + assert "rf'pdfapps-{ANY}'" not in bin_block # unanchored is gone + assert "/v{ANY}/" in bin_block + + +# ── #10 — Tab close ordering audit (existing behaviour) ───────────────── + + +def test_window_closeevent_waits_for_workers_before_destroy(): + """Pre-existing: window.closeEvent must call wait_for_workers() + on each stacked page before super().closeEvent() so QThreads are + not destroyed mid-run. Audited in R10 #10 — no change required.""" + src = _read("app/window.py") + close_body = src[src.find("def closeEvent"): + src.find("def _toggle_sidebar")] + assert "wait_for_workers" in close_body + # super().closeEvent must come AFTER the wait loop. + wait_idx = close_body.find("wait_for_workers") + super_idx = close_body.find("super().closeEvent") + assert 0 < wait_idx < super_idx + + +# ── #11 — Uncached fallback on getmtime failure ───────────────────────── + + +def test_overlay_pixmap_uncached_on_mtime_error(): + """An OSError on os.path.getmtime() must not poison the cache — + fall back to QPixmap(path) directly.""" + src = _read("app/editor/canvas.py") + paint_body = src[src.find("def paintEvent"): + src.find("def mousePressEvent")] + # The except OSError path must NOT route through _load_overlay_pixmap + # (which would cache the null pixmap forever). + assert "except OSError:" in paint_body + # The fallback line is QPixmap(path) — not _load_overlay_pixmap(...). + fallback_block = paint_body[paint_body.find("except OSError"): + paint_body.find("except OSError") + 200] + assert "img_px = QPixmap(path)" in fallback_block + + +# ── #12 — Folder + URL drop handling ──────────────────────────────────── + + +def test_drop_event_handles_folders_and_urls(): + src = _read("app/window.py") + drop_body = src[src.find("def dropEvent"): + src.find("def dropEvent") + 2000] + # Folder drop iterates glob, URL drop surfaces a warning. + assert "os.path.isdir(path)" in drop_body + assert "viewer.drop_url_not_supported" in drop_body + assert "*.pdf" in drop_body + + +def test_drop_url_translation_parity(): + """8 locales must define viewer.drop_url_not_supported so users in + every language get the warning.""" + data = json.loads( + (ROOT / "app" / "translations.json").read_text(encoding="utf-8")) + + seen = 0 + + def walk(obj): + nonlocal seen + if isinstance(obj, dict): + if "viewer.drop_url_not_supported" in obj: + seen += 1 + assert isinstance( + obj["viewer.drop_url_not_supported"], str) + for v in obj.values(): + walk(v) + elif isinstance(obj, list): + for v in obj: + walk(v) + + walk(data) + assert seen == 8, ( + f"viewer.drop_url_not_supported missing in some locale " + f"(got {seen}/8)" + ) diff --git a/tests/test_editor_audit_r9.py b/tests/test_editor_audit_r9.py index 8f40eee..a7f3841 100644 --- a/tests/test_editor_audit_r9.py +++ b/tests/test_editor_audit_r9.py @@ -38,7 +38,14 @@ def _read(rel: str) -> str: def test_run_detects_encrypted_input(): src = _read("app/editor/tab.py") - assert "was_encrypted = bool(doc.needs_pass)" in src + # R10 (CRIT-2): the encryption peek now uses a short-lived + # ``peek = fitz.open(...)`` BEFORE releasing the canvas so a user + # cancel in the encryption prompt no longer strands the editor on + # the placeholder. Allow either the legacy ``doc.needs_pass`` + # phrasing or the new ``peek.needs_pass`` one — both detect input + # encryption, the rest of the prompt-choice flow is unchanged. + assert ("was_encrypted = bool(doc.needs_pass)" in src + or "was_encrypted = bool(peek.needs_pass)" in src) assert "_prompt_encryption_choice" in src @@ -139,10 +146,13 @@ def test_image_mode_only_picks_when_empty(): def test_overlay_pixmap_lru_cache_exists(): + # R10 #3 replaced the @lru_cache(maxsize=64) module-level decorator + # with an explicit FIFO dict so tab close paths can drop the + # cached pixmaps (otherwise the cache leaks across long sessions). src = _read("app/editor/canvas.py") - assert "from functools import lru_cache" in src assert "_load_overlay_pixmap" in src - assert "@lru_cache" in src + assert "_OVERLAY_PIXMAP_CACHE" in src + assert "clear_overlay_pixmap_cache" in src def test_paint_event_uses_cached_pixmap(): diff --git a/tests/test_viewer_safety.py b/tests/test_viewer_safety.py index 8c78fd3..c5fc231 100644 --- a/tests/test_viewer_safety.py +++ b/tests/test_viewer_safety.py @@ -36,7 +36,9 @@ def test_save_incr_is_inside_try_except(): # the saveIncr call and an except Exception below. idx = CANVAS.index("self._doc.saveIncr()") before = CANVAS[max(0, idx - 800): idx] - after = CANVAS[idx: idx + 400] + # The post-saveIncr handler grew in R10 to include backup-restore + # + reopen logic before reaching show_error, so widen the window. + after = CANVAS[idx: idx + 2000] assert "try:" in before, "saveIncr is no longer inside a try block" assert "except Exception" in after, "saveIncr error path is missing" assert "show_error" in after, "saveIncr errors no longer route to show_error"