Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scale page content #991

Merged
merged 16 commits into from
Oct 16, 2021
Merged

Conversation

JNK90
Copy link
Contributor

@JNK90 JNK90 commented Sep 14, 2021

I added two functions to the PDFPage to scale the content and the annots. This enables to resize the content and annots without violating private access modifiers. The code is inspired by the Resizer Example.

Copy link
Owner

@Hopding Hopding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds neat! Thanks for working on it @JNK90 🙂

I appreciate you adding good doc comments as well. That's something a lot of folks seem to forget about, haha.

In addition to my change request, can you:

  • Explain a bit more about how this works? Please include screenshots/examples.
  • Update one or two integration tests to showcase this behavior.

src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
@JNK90
Copy link
Contributor Author

JNK90 commented Sep 29, 2021

@Hopding I have added integration tests for node and web (I have problems to run the other environments on my system). I also replaced .forEach() and .asArray() calls.

To scale the content the method scaleContent(x, y) simply applys the scale-Operator to the contentstream.
To scale the annots the method scaleAnnots(x, y) loops over all annots ('RD', 'CL', 'Vertices', 'QuadPoints', 'L', 'Rect', 'InkList') and multiplies the PDFNumbers of an annot with x or y as the factor.

I have edited a PDF and added, text, comments, shapes, highlights to test the scaling.
Original:
With_annots
After scaling content by 50%:
With_annots_scaled_content
After scaling content and annots by 50%:
With_annots_scaled_content_and_annots

Copy link
Owner

@Hopding Hopding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great explanation, much appreciated @JNK90! Now that I have a good understanding of what this PR is for, I'm confident saying it'll be a heavily used feature!

I've requested a few changes below. Once these are addressed we should be good to go!

src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Show resolved Hide resolved
src/api/PDFPage.ts Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
apps/deno/tests/test19.ts Outdated Show resolved Hide resolved
Co-authored-by: Andrew Dillon <[email protected]>
@Hopding
Copy link
Owner

Hopding commented Oct 7, 2021

@JNK90 I see you marked all my comments as resolved, but I don't see any of the changes I requested in the diff? Did you forget to push your work? Or are you still working on this?

@JNK90
Copy link
Contributor Author

JNK90 commented Oct 8, 2021

@Hopding I pushed my changes to the wrong branch of my fork 🤓. All changes are now in the right branch.

@JNK90 JNK90 requested a review from Hopding October 11, 2021 05:15
@Hopding
Copy link
Owner

Hopding commented Oct 16, 2021

@JNK90 Ah, that would explain it!

Copy link
Owner

@Hopding Hopding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Excited to get this released!

apps/deno/tests/test19.ts Outdated Show resolved Hide resolved
src/api/PDFPage.ts Show resolved Hide resolved
src/api/PDFPage.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants