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

Support for writing and reading manifest data from simple PDFs (without incremental updates or signatures) #249

Merged
merged 25 commits into from
Sep 11, 2023

Conversation

dyro
Copy link
Contributor

@dyro dyro commented May 17, 2023

Changes in this pull request

This PR introduces the pdf_utils::Pdf struct and the pdf_utils::C2paPdf trait, public (to the crate), lopdf abstractions with functionality to read and write manifest data from PDFs. This code works against wasm and non-wasm targets.

I intend to add read support next, then round out the work with incremental update support. I opened this PR against origin/pdf instead of main as I want to get this in front of folks before getting to far and this work doesn't support reading manifests or incremental PDF updates.

Checklist

  • This PR represents a single feature, fix, or change.
  • All applicable changes have been documented.
  • Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.

@scouten-adobe scouten-adobe changed the title supports writing manifest data to simple (pdf without incremental docs) pdfs Support writing manifest data to simple (PDF without incremental docs) PDFs May 17, 2023
sdk/src/utils/pdf_utils.rs Outdated Show resolved Hide resolved
sdk/src/utils/pdf_utils.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Patch coverage: 94.73% and project coverage change: +0.20% 🎉

Comparison is base (760009c) 78.73% compared to head (1e97fbb) 78.94%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
+ Coverage   78.73%   78.94%   +0.20%     
==========================================
  Files          76       77       +1     
  Lines       22465    22764     +299     
==========================================
+ Hits        17687    17970     +283     
- Misses       4778     4794      +16     
Files Changed Coverage Δ
sdk/src/utils/pdf_utils.rs 94.68% <94.68%> (ø)
sdk/src/claim.rs 83.15% <100.00%> (-0.03%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sdk/Cargo.toml Outdated Show resolved Hide resolved
sdk/src/utils/pdf_utils.rs Show resolved Hide resolved
@dyro dyro force-pushed the dyross/pdf branch 3 times, most recently from adc4694 to a1e902d Compare August 24, 2023 01:40
Copy link
Collaborator

@scouten-adobe scouten-adobe left a comment

Choose a reason for hiding this comment

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

One tiny documentation nit and a couple of non-blocker questions to consider.

I'd like @mauricefisher64 to review this since he knows PDF better than I do, but otherwise, this looks really solid. Easy code to follow!

sdk/src/utils/pdf_utils.rs Outdated Show resolved Hide resolved
@dyro dyro changed the title Support writing manifest data to simple (PDF without incremental docs) PDFs Support for writing and reading manifest data from simple PDFs (without incremental updates or signatures) Aug 25, 2023
Copy link
Collaborator

@mauricefisher64 mauricefisher64 left a comment

Choose a reason for hiding this comment

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

👍

@dyro
Copy link
Contributor Author

dyro commented Sep 7, 2023

hey @scouten-adobe & @mauricefisher64 : I believe I've addressed all of your feedback. Please take a look again when you have a moment!

Copy link
Collaborator

@scouten-adobe scouten-adobe left a comment

Choose a reason for hiding this comment

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

Please either explain the Vec of slices or remove the abstraction. Otherwise 👍

sdk/src/utils/pdf_utils.rs Show resolved Hide resolved
Copy link
Collaborator

@mauricefisher64 mauricefisher64 left a comment

Choose a reason for hiding this comment

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

👍

@mauricefisher64
Copy link
Collaborator

Not holding up this PR but does the lopdf handle PDF permissions and passwords?

@dyro
Copy link
Contributor Author

dyro commented Sep 11, 2023

@mauricefisher64: Yes, lopdf has support for passwords. After creating a Document instance, you call the its decrypt method with a password. Wasn't sure how to integrate that into the c2pa-rs public API

@dyro dyro merged commit 82c2aa2 into main Sep 11, 2023
23 checks passed
@dyro dyro deleted the dyross/pdf branch September 11, 2023 22:22
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.

4 participants