Skip to content

fix: Round 8 polish — encrypt wipe, update worker leak, CLI, DPR change#77

Closed
nelsonduarte wants to merge 7 commits into
fix/audit-highsfrom
fix/round8-polish
Closed

fix: Round 8 polish — encrypt wipe, update worker leak, CLI, DPR change#77
nelsonduarte wants to merge 7 commits into
fix/audit-highsfrom
fix/round8-polish

Conversation

@nelsonduarte
Copy link
Copy Markdown
Owner

@nelsonduarte nelsonduarte commented Jun 5, 2026

Summary

Closes 7 selective findings from audit Round 8: 2 HIGH, 3 MEDIUM, 2 BONUS polish items.

Note: PR base is fix/audit-highs (PR-B / #73). After #72 → #73 → main, GitHub will auto-rebase this PR. Sits on top of the 5 critical + 7 HIGH PRs.

Fixes

  • 🟠 R8-H1 wipe encrypt passwords (app/tools/encrypt.py) — edit_owner/edit_owner_confirm/edit_user/edit_pwd QLineEdits persisted password text for the entire session. New _clear_password_fields invoked from try/finally in _run() wipes them on both success and exception paths.
  • 🟠 R8-H2 update worker leak (app/window.py) — _update_worker QObject was never deleteLater()'d, leaking permanently. New _release_update_worker helper called from both _on_update_found and closeEvent. (Caveat: no-update path only releases at closeEvent — see commit body for future symmetric cleanup option.)
  • 🟡 R8-M1 CLI argparse (pdfapps.py) — only sys.argv[1] was opened; -h/--help silently ignored; no --version. Now uses argparse with prog=\"pdfapps\", add_help=True, --version returning `PDFApps {APP_VERSION}`, accepts multiple PDF paths via nargs=\"*\", opens each as a tab via existing _load_and_track. Validation requires .pdf extension + os.path.isfile.
  • 🟠 R8/D1 DPR change handler (app/viewer/canvas.py, app/editor/canvas.py) — neither canvas had a handler for QWindow.screenChanged; moving the window between 100%↔200% monitors left pages blurry until next zoom. New showEvent override connects to window().windowHandle().screenChanged and invalidates cached pixmaps via _gen bump + entry.pixmap = None + _schedule_visible().
  • 🟢 Dead _update_ready signal (app/window.py:271) — declared and connected to _notify_update but never emitted (the actual notification path is via _update_worker.done). Removed.

Bonus fixes

  • 🟢 Watermark encrypted PDF (app/tools/watermark.py) — PdfReader(wm_path) raised PdfReadError cryptically on encrypted watermark PDFs. New _prompt_watermark_password handles it via prompt_pdf_password helper, distinct from self._pdf_password (which tracks the source PDF).
  • 🟢 _populate_toc graceful failure (app/viewer/panel.py) — malformed TOC could leave the panel in inconsistent state. Now wrapped in try/except + logging.warning; TOC panel hides on failure.

Tests

10 new tests in tests/test_round8_fixes.py:

  • test_encrypt_clears_password_fields
  • test_update_worker_release_helper_exists
  • test_update_ready_signal_removed
  • test_pdfapps_uses_argparse
  • test_pdfapps_version_flag_runs (subprocess)
  • test_pdfapps_help_flag_runs (subprocess)
  • test_viewer_canvas_handles_screen_changed
  • test_editor_canvas_handles_screen_changed
  • test_watermark_prompts_for_encrypted_wm
  • test_populate_toc_logs_on_failure

Note: Qt-bound paths use source-level pattern matching, not functional Qt event-loop testing. The CLI subprocess tests are genuine integration. Behavioural validation of Qt paths requires manual QA.

Final

  • 170 passed, 1 failed (Flatpak tag drift, pre-existing on main), 1 skipped
  • CLI verified:
    • python pdfapps.py --version → `PDFApps 1.13.14`
    • python pdfapps.py --help → usage + flags
  • No i18n changes (internal-only).
  • No new pyflakes warnings.
  • Compatible with PR-A and PR-B (sits on PR-B).

Optional follow-ups (not blocking)

  1. Surface invalid CLI paths via stderr instead of silent skip.
  2. Flip watermark prompt order — source PDF first, then watermark.
  3. Release _update_worker from _update_thread.finished for symmetric cleanup on no-update path.
  4. Add header comment in test file documenting source-grep limitation.

@nelsonduarte
fix(encrypt): wipe password fields after run
14c6526 
QLineEdit text for owner/user/decrypt passwords persisted in memory
for the entire session. Now cleared via try/finally in _run() to align
with the broader wipe_pdf_password() pattern.

Addresses R8-H1.
@nelsonduarte
fix(window): deleteLater _update_worker after check completes
99b4573 
The _update_worker QObject lived for the lifetime of the application
because only _update_thread.deleteLater was wired. Now released in
_on_update_found() via _release_update_worker() and called defensively
from closeEvent.

Also removes the dead _update_ready signal: it was declared at the
class level and connected to _notify_update, but never emitted — the
actual update notification path is via _update_worker.done.

Addresses R8-H2 and R8 dead-code finding.
@nelsonduarte
feat(cli): argparse with multiple PDFs, --help, --version
52f3ce7 
Previously only sys.argv[1] was opened, so multi-file "Open With" or
drag-and-drop of N>1 PDFs onto the executable silently dropped every
file after the first. -h / --help was treated as an invalid path and
the app launched into the welcome screen.

Now uses argparse: accepts multiple PDF paths (each opens in its own
tab via _load_and_track), and exposes standard --help / --version
flags that exit before QApplication is constructed.

Addresses R8-M1.
@nelsonduarte
fix(viewer): re-render canvas on screen DPR change
d9f9072 
Both viewer/canvas.py and editor/canvas.py read devicePixelRatioF()
only inside _schedule_visible(). Without a screenChanged handler,
moving the window between 100% and 200% monitors (or changing the
DPR of an active monitor via Windows Display Settings) left every
visible page blurry until the next zoom interaction.

New showEvent override connects to window.windowHandle().screenChanged
and invalidates cached pixmaps on DPR change, then re-queues the
visible range. The connect is guarded against duplicate handlers on
re-show / tab switch.

Addresses R8/D1.
@nelsonduarte
fix(watermark): prompt for password on encrypted watermark PDF
fdeb6af 
PdfReader(wm_path) raised PdfReadError cryptically when the watermark
PDF was itself encrypted (rare but observed with corporate stamp
PDFs). Now a dedicated _prompt_watermark_password() helper prompts
for the watermark's password before pre-flight and the worker
decrypts the second reader with it, kept separate from
self._pdf_password so source and watermark credentials don't mix.

Addresses R8 bonus #6.
@nelsonduarte
fix(viewer): wrap _populate_toc in try/except
49c64f5 
A malformed outline (cyclic refs, bad page indexes, unexpected entry
shape) could raise mid-build and leave the TOC panel in an
inconsistent state — half-populated and surfaced as a stack trace.
Now the failure is logged via logging.warning and the TOC tree is
reset to an empty / hidden state so the rest of the viewer stays
usable.

Addresses R8 bonus #7.
@nelsonduarte
test: source-level regressions for Round 8 selective fixes
1173982 
10 new tests guard the PR-C wiring against silent regressions:

- encrypt._clear_password_fields() + try/finally hookup
- _release_update_worker helper + _update_ready signal removal
- argparse CLI (multi-PDF, --help, --version) — also exercises
  the subprocess version/help flags end-to-end
- screenChanged DPR handler on viewer + editor canvases
- watermark encrypted PDF prompt path
- _populate_toc try/except + logging
@nelsonduarte nelsonduarte added bug Something isn't working enhancement New feature or request labels Jun 5, 2026
@nelsonduarte nelsonduarte mentioned this pull request Jun 5, 2026
Merged
@nelsonduarte
Copy link
Copy Markdown
Owner Author

nelsonduarte commented Jun 5, 2026

Superseded by #79 (same 7 commits, cherry-picked onto current main after #72/#78 squashes).

@nelsonduarte nelsonduarte deleted the fix/round8-polish branch June 5, 2026 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nelsonduarte