Fix several small correctness bugs - #5076
Open
A831ARD0 wants to merge 1 commit into
Open
Conversation
- Archive.add(): fix broken assert (missing comma turned the error message into part of the boolean expression, disabling the empty-name check for binary content). - Annot.file_info: avoid UnboundLocalError when a crafted/malformed attachment's /FS dict has neither /UF nor /F. - Page.set_language(): call pdfpage.obj() instead of passing the bound method itself, which broke every call that actually sets a language. - Widget.on_state(): skip None entries from button_states() instead of raising TypeError. - Pixmap.tobytes(): add missing f-string prefix so the alpha error message actually includes the format name. - JM_matrix_from_py(): fix copy-paste bug returning FzRect() instead of FzMatrix() on the float-conversion failure path. - utils.get_text(): remove a bare assert that made the documented "misspelled option falls back to text" behavior unreachable. - utils.get_label_pno(): return "" (as documented) instead of raising IndexError when no label rule applies to the given page number. - utils.get_sorted_text(): guard against ZeroDivisionError for zero-width glyph bboxes. - _apply_pages.py (method='fork'): send the actual page numbers to workers instead of plain list indices, which silently processed the wrong pages for any non-contiguous/non-zero-based `pages` list. - table.chars_in_rect(): compare against c["top"]/c["bottom"] (top-down, matching the rect from page.get_drawings()) instead of c["y0"]/c["y1"] (PDF-native bottom-up space), fixing table detection for frames drawn as filled rectangles.
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
Comment on lines
-140
to
+145
| page_num = queue_down.get() | ||
| item = queue_down.get() | ||
| if verbose: | ||
| pymupdf.log(f'{os.getpid()=}: {page_num=}.') | ||
| if page_num is None: | ||
| pymupdf.log(f'{os.getpid()=}: {item=}.') | ||
| if item is None: | ||
| break | ||
| index, page_num = item |
Collaborator
There was a problem hiding this comment.
I think the current code is ok here. This code is in the worker function def childfn(): so reads page numbers from queue_down and writes (page_num, text) to queue up.
Comment on lines
-175
to
+178
| pymupdf.log(f'{os.getpid()=}: sending {page_num=} {ret=}') | ||
| queue_up.put( (page_num, ret) ) | ||
| pymupdf.log(f'{os.getpid()=}: sending {index=} {ret=}') | ||
|
|
||
| queue_up.put( (index, ret) ) |
Collaborator
There was a problem hiding this comment.
As above, this is worker code.
Comment on lines
-500
to
501
| assert option in formats | ||
| if option not in formats: | ||
| option = "text" |
Collaborator
There was a problem hiding this comment.
I think the intention here is to fail if an unrecognised option is specified, so we should remove the if option not in formats: option = "text" code, rather than the assert.
Collaborator
|
Thanks for submitting this PR, looks like some good fixes in the areas i know about. I've enabled the pre-push tests. You should be able to build/test fairly easily if you run |
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.
Summary
A batch of small, independent correctness fixes found while reviewing
src/__init__.py,src/utils.py,src/_apply_pages.py, and the table-extraction module. Each item below is a separate, self-contained fix; happy to split into individual PRs if preferred.Archive.add(): a missing comma in anassertturned the error message into part of the boolean expression, silently disabling the empty-name check for binary content (Archive().add(b"...", path="")no longer raised).Annot.file_info:UnboundLocalErrorwhen a malformed/crafted attachment's/FSdict has neither/UFnor/F.Page.set_language(): passed the bound methodpdfpage.objinstead of calling it (pdfpage.obj()), breaking every call that actually sets a language.Widget.on_state():TypeErrorwhenbutton_states()leaves an entry asNone.Pixmap.tobytes(): missing f-string prefix meant the "cannot have alpha" error literally printed'{output}'instead of the format name.JM_matrix_from_py(): copy-paste bug returningmupdf.FzRect()instead ofmupdf.FzMatrix()on the float-conversion failure path (latent — every other sibling converter returns its own correct type).utils.get_text(): a bareassert option in formatsran before the followingif, making the documented "misspelled option falls back to'text'" behavior unreachable (and behavior differed under-O).utils.get_label_pno(): raisedIndexErrorinstead of returning""(as documented) when no label rule applies to the given page number.utils.get_sorted_text():ZeroDivisionErrorfor a zero-width glyph bbox._apply_pages.py(method='fork'): workers received plain list indices (range(len(pages))) instead of the actual page numbers, soapply_pages(path, fn, pages=[3, 7, 9], method='fork')silently processed pages 0, 1, 2 instead — results were silently attributed to the wrong pages. Fixed by passing(index, page_number)pairs through the queue.table.chars_in_rect(): compared a top-down rect (frompage.get_drawings()) againstc["y0"]/c["y1"], which are in PDF-native (bottom-up) space, instead of the top-downc["top"]/c["bottom"]. This made box-drawn table borders (filled/connected rectangles instead of ruling lines) fail to register as table candidates.Testing
I don't have a build of the compiled
_fitz/mupdf C extension available, so I could not run the full test suite. Each fix was verified by:python3 -m py_compileon every modified file to confirm no syntax errors were introduced.Given that, please review carefully and let me know if you'd like any of these split out, reverted, or backed by a regression test before merging.