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

Added PDF read for macOS #1969

Closed
wants to merge 1 commit into from
Closed

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Jun 20, 2016

OS X has a command line tool called 'sips' installed by default. It is able to convert the first page of a PDF to an image. So this PR adds PDF read functionality for OS X by converting a PDF into a temporary file and then loading the file as a PngImageFile. Not quite sure about this approach, so I thought I'd put it up here for feedback before adding anything like documentation or tests.

This could be considered a partial implementation of #471

raise SyntaxError("PDF file could not be converted")

im = Image.open(filepath)
im.load()
Copy link
Member

Choose a reason for hiding this comment

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

Why we need this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok. Because we are deleting source file.

@homm
Copy link
Member

homm commented Aug 7, 2016

In my opinion, this is not PDF image files reader, Instead this is PDF converter. Of course, it can be very useful, but I don't think that Pillow is the best place for this code. In my opinion it is better to create separate package.

@radarhere radarhere changed the title Added PDF read for OS X Added PDF read for macOS Sep 24, 2016
@aclark4life
Copy link
Member

@homm What do you think about #471 in general?

@aclark4life
Copy link
Member

I'd probably prefer a more generic approach to reading PDFs (i.e. cross plat, not macOS only)

@homm
Copy link
Member

homm commented Oct 3, 2016

@aclark4life I'm for adding PDF reading, but not with such approach.

@radarhere radarhere added the macOS label Oct 7, 2016
@radarhere
Copy link
Member Author

Sure. Feel free to close. The thinking was just that perhaps adding functionality for one platform was better than no functionality.

@aclark4life
Copy link
Member

Thanks

@aclark4life aclark4life closed this Oct 7, 2016
@radarhere radarhere deleted the pdf branch October 7, 2016 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants