fix(import): tolerate legacy text encodings, guard zero-page saves, harden editor load - #137
Merged
Merged
Conversation
…arden editor load MAJOR #1: TXT/MD/HTML import read user files with strict open(encoding="utf-8") in do_work, crashing with UnicodeDecodeError on Windows-1252/Latin-1 files (common Notepad/Excel .txt). The font pre-scan already used errors="replace" and passed, so the user was told the file was valid before the real conversion blew up. Aligned all three do_work read sites with the pre-scan (errors="replace"); no new dependency added (charset-normalizer is not a project dep). MINOR #2: when every input was empty/rejected/unreadable the fitz doc had zero pages and doc.save() raised the cryptic "cannot save with zero pages" (in _convert_images this also masked the "N images skipped" feedback). Added a _NoContent sentinel returned by every converter when doc.page_count == 0, routed through a shared handler that shows the new, translated tool.import.no_content message (8-language parity). MINOR #3: PdfEditCanvas.load closed the previous doc then assigned self._doc = fitz.open(path); a corrupt PDF that raised left self._doc pointing at the already-closed Document (use-after-close). Now clears self._doc before fitz.open and only publishes on success. Tests: tests/test_import_encoding_and_editor_doc.py (10 discriminative cases covering each bug, incl. an inverse test proving the strict read would have raised, and i18n parity for the new key). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Rename the module-level QApplication holder _app -> _unused_app
(matches the repo's existing test_encrypted_pdf_tools pattern) so the
liveness reference no longer trips CodeQL's unused-global note.
- Drop the unused bs4 binding; keep pytest.importorskip("bs4") purely
for its skip-if-absent side effect.
- Import QMessageBox from app.tools.import_pdf at module top and remove
the redundant 'import app.tools.import_pdf as mod', unifying on the
from-import style; the monkeypatch still targets the same class object.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploying pdfapps with
|
| Latest commit: |
b8458f0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a3f6a222.pdfapps.pages.dev |
| Branch Preview URL: | https://fix-import-encoding-and-edit.pdfapps.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adversarial bug-hunt fixes. Scope limited to
app/tools/import_pdf.py,app/editor/canvas.py,app/translations.jsonand a new test module. No viewer/requirements changes.MAJOR #1 — legacy text encodings crash TXT/MD/HTML import
Trigger: import a
.txt(or.md/.html) that Notepad/Excel saved as Windows-1252 / Latin-1, e.g. bytesb'Ol\xe1 mundo'.Symptom: the font pre-scan (
~L142) already usederrors="replace"and passed, telling the user the file was valid — then the realdo_workread (open(src, "r", encoding="utf-8"), noerrors=) raisedUnicodeDecodeErrorand the conversion died.Root cause: strict decoding in a context that must tolerate legacy encodings, inconsistent with the pre-scan.
Fix: aligned all three
do_workread sites (TXT, MD, HTML) with the pre-scan usingerrors="replace".charset-normalizeris not a project dependency, so — per the brief — no new dep was added;errors="replace"is sufficient and consistent.MINOR #2 — zero-page save raises a cryptic ValueError (and masks skip feedback)
Trigger: all inputs empty/rejected (only non-image files dropped into the image list, gigapixel-rejected images, empty docx,
page_count == 0).Symptom:
BasePage._atomic_pdf_write→doc.save()raisedValueError: cannot save with zero pages; in_convert_imagesthis also masked the "N images skipped" status.Root cause: no guard before writing a page-less document.
Fix: every converter now returns a
_NoContentsentinel whendoc.page_count == 0, routed through a shared_on_resulthandler that surfaces the new translatedtool.import.no_contentmessage (and preserves the images skip count). New i18n key added with 8-language parity.MINOR #3 — corrupt PDF leaves a dangling closed Document in the editor
Trigger:
PdfEditCanvas.load()on a corrupt PDF (fitz.openraisesFileDataError).Symptom:
if self._doc: self._doc.close()thenself._doc = fitz.open(path)— the assignment never happens, soself._dockeeps referencing the just-closed Document (use-after-close).Root cause: the previous reference wasn't cleared before the fallible
fitz.open.Fix: clear
self._doc = Nonebeforefitz.open, open into a local, and only publishself._doc = docon success.Tests
tests/test_import_encoding_and_editor_doc.py— 10 discriminative cases:.txt, latin-1.md, latin-1.htmleach produce a valid PDF; an inverse test proves the old strict read would have raisedUnicodeDecodeError._NoContent, writes nothing, andon_doneshowstool.import.no_contentwithout calling_done; docx zero-page guard; i18n parity for the new key.load()leavesself._doc is None(not a closed doc) and a subsequent validload()works.Validation
ruff check --select F,E9on the touched files: clean.translations.json: valid JSON, all 8 languages at equal key counts, new key present in all.🤖 Generated with Claude Code