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

Add support for exporting pdf to image #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

werenall
Copy link

@werenall werenall commented Apr 2, 2020

Implement a namespace for exporting a PDF or PDDocument to a BufferedImage

[Re #55]
This PR also slightly changes the prerequisites for split function
Previously it only allowed strings as inputs. IMHO it should also
accept files.

@dotemacs
Copy link
Owner

dotemacs commented Apr 2, 2020

Hi @werenall

thank you for the PR.

I'd like to think about this a little bit.

But from a quick look:

you seem to have extended the functionality of an existing function and added a new feature in the same commit message. I think that this should be split in separate commits.

This is just from a quick glance.

@werenall
Copy link
Author

werenall commented Apr 2, 2020

Yeah, I agree. Take all the time you need and just let me know in a bulk what I can improve.

@dotemacs
Copy link
Owner

dotemacs commented Sep 5, 2020

To summarise:

The changes in pdfboxing.split are they in any way related to this pull request?
I don't see how they are.

The same with pdfboxing.merge, is it related to this pull request, feature wise?

I'd say that only the addition of pdfboxing.image is relevant for this pull request.

Since you mentioned in #55 why you've created it, I have some context, but maybe you can add an example on how it can be used in the README?

For me to add this PR, it would be nice to see the following:

  • removal of the changes from this PR from pdfboxing.split & pdfboxing.mege
  • a clear example of the usage of the function pdfboxing.image/export-to-image
  • also, why does it only take the first page? I get that it satisfies your use case when creating thumbnails, but let's say that somebody wanted to use nth page from the PDF document, is this 0 on this line https://github.com/dotemacs/pdfboxing/pull/56/files#diff-f29a5a4bdf7329ef9a0076770cb59571R27 used to specify that first page? if it is, maybe it can be set as an option using :or in the destructuring argument?

Thank you for your work

@werenall werenall force-pushed the issue/55-export-to-image-support branch 2 times, most recently from c1d2a7c to d5d2713 Compare November 12, 2020 17:02
[Re dotemacs#55]
This commit also changes slightly the prerequisities for split function
Previously it only allowed strings as inputs. IMHO it should also
accept files.
@werenall werenall force-pushed the issue/55-export-to-image-support branch from d5d2713 to 25eff3f Compare November 12, 2020 17:07
@werenall
Copy link
Author

Ok @dotemacs, I think I resolved all your comments. Let me know if you still would like some fine-tuning.

@glfeng318
Copy link

how is this PR going? i'm looking into this feature too

@jgrodziski
Copy link

HI @dotemacs, I wonder if you plan to merge this PR?
thank you for your work on this lib.

@avocade
Copy link

avocade commented Mar 11, 2024

Gentle ping ☺️ Also re: this comment: #55

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.

None yet

5 participants