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

[New Feature] PdfPageBuilder: Allow to copy pages from another document #250

Merged
merged 2 commits into from
Feb 6, 2021

Conversation

InusualZ
Copy link
Collaborator

@InusualZ InusualZ commented Dec 21, 2020

This is a very naive implementation, because if you copy a lot of pages from a given document you would have duplicated resources. But, I would like to get some thoughts on this implementation.

I was surprise by the amount of code that this feature needed to be implemented. I was expecting a lot more. Even so, there is some duplicated code in two parts:

  • Copying Token This piece of code keep reappearing, I really don't like it.
  • Resource Management I think this piece of code and the one that write the resources to the stream can be transformed into a nice interface behind a read only interface and mutable interface, kind of like IReadOnlyList<> and IList<>, but IReadOnlyResourceStore and IResourceStore. I would like yours thought about that

Anyway, This code have some TODO I would like to address them, but I don't know If you even want the feature in it's current path of implementation, so maybe the effort to implement them are not worth.

If you have any suggestions, thought, anything feel free to comment.

You can create new content stream (NewContentStreamAfter, NewContentStreamBefore) and select (SelectContentStream) which one we are editing at the moment
This is a naive implementation, because if you copy multiple pages from the same document, the recipient document would be bloated with duplicated resources
@codecov-io
Copy link

Codecov Report

Merging #250 (7126564) into master (237fd96) will decrease coverage by 0.01%.
The diff coverage is 64.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #250      +/-   ##
==========================================
- Coverage   64.53%   64.51%   -0.02%     
==========================================
  Files         600      600              
  Lines       39725    39921     +196     
==========================================
+ Hits        25637    25756     +119     
- Misses      14088    14165      +77     
Impacted Files Coverage Δ
src/UglyToad.PdfPig/Content/Page.cs 87.50% <ø> (+3.12%) ⬆️
src/UglyToad.PdfPig/Writer/PdfPageBuilder.cs 69.44% <51.14%> (-13.62%) ⬇️
src/UglyToad.PdfPig/Writer/PdfDocumentBuilder.cs 81.86% <95.94%> (+2.93%) ⬆️
src/UglyToad.PdfPig/Parser/PageFactory.cs 81.08% <0.00%> (-1.36%) ⬇️
src/UglyToad.PdfPig/Content/PageContent.cs 97.14% <0.00%> (+2.85%) ⬆️
...Operations/TextShowing/ShowTextsWithPositioning.cs 85.29% <0.00%> (+8.82%) ⬆️
.../MarkedContent/BeginMarkedContentWithProperties.cs 96.77% <0.00%> (+9.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 237fd96...7126564. Read the comment docs.

Copy link
Member

@EliotJones EliotJones left a comment

Choose a reason for hiding this comment

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

This looks like a useful feature to have, I'm just uncertain what the behaviour looks like for the copying a page's resource dictionary where the dictionary is using inherited properties, e.g. fonts, rotation or mediabox are set by the page trees pages node rather than at the page level. I think we can find out later and just start with this approach for now 👍

For the questions you raise in the opening PR comment I think we're fine taking the current approach for now, I'd avoid trying to derive the abstractions prematurely so I'm fine with doing things in a maybe less efficient manner to begin with.

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.

3 participants