Repository navigation
Measure annotation rotation against the page, not a crop of it - #1436
Open
Krushil-Dhankecha wants to merge 1 commit into
Open
Krushil-Dhankecha wants to merge 1 commit into
Krushil-Dhankecha wants to merge 1 commit into
Conversation
Page.annots turns each annotation's corners through the page before flipping them into pdfplumber's top-left origin. The flip already reads the root page's height, but the rotation read self.width and self.height, which a CroppedPage redefines as the size of the crop. On a rotated page every annotation therefore moved by however much was cropped away, and because the crop filter then runs on the moved coordinates, crop() and within_bbox() kept the wrong annotations too. Which dimension went wrong followed the number of turns: a 90-degree page shifted when the crop changed the height, a 270-degree page when it changed the width, and a 180-degree page on either.
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
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.
Cropping a rotated page moves its annotations, and then the crop keeps the wrong ones.
Page.annotsturns each annotation's corners through the page, then flips them into pdfplumber's top-left origin. The flip readsself.root_page.height, but the turns readself.widthandself.height— and aCroppedPageredefines those as the size of the crop. So on a page with/Rotate, every annotation moves by however much was cropped away. Because_crop_fnthen filters on the moved coordinates,crop()andwithin_bbox()also keep the wrong annotations. This is the part of #1171 that the rotation case missed; the coordinates on an uncropped page have always been right.Which dimension goes wrong follows the number of turns: a 90-degree page shifts when the crop changes the height, a 270-degree page when it changes the width, and a 180-degree page on either.
To reproduce
Using
tests/pdfs/annotations-rotated-180.pdf:The characters do not move, so the annotation no longer sits over its text. Cropping further makes the filtering wrong as well — with
crop((0, 0, 420, 842))all five annotations are returned, shifted 175pt left, including one that really sits at x 594-595 and is nowhere near the crop:With the fix, four are returned, clipped to the crop, and the one at x 594-595 is dropped.
The change
rotate_pointreads the original page's dimensions, which is what the flip beside it already does. Nothing else changes: an uncropped page, an unrotated page, and the objects on any page are all byte-identical.Tests
tests/test_basics.py::Test::test_annots_rotated_and_croppedcovers the gap between the two tests that already exist —test_annots_croppedcrops an unrotated page,test_annots_rotatedreads a rotated page whole. For each of the four rotations it takes a strip off both the width and the height, so every rotation meets the dimension its turns are measured against, and asserts that the annotations inside the crop keep the coordinates they have on the whole page. It then checks the filtering on the 180-degree page: four of the five annotations reach left of x=420.The test fails on the parent commit and passes with the fix. The other three annotation tests pass either way.
Full suite: 168 passed. The 6 failures in
tests/test_repair.pyare Ghostscript not being installed here, and they fail the same way on the parent commit.black,isort,flake8andmypy --strictare all clean.