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 image files #202

Merged
merged 1 commit into from
May 17, 2020
Merged

Add support for image files #202

merged 1 commit into from
May 17, 2020

Conversation

mimi89999
Copy link
Contributor

@mimi89999 mimi89999 commented May 15, 2020

TODO:

  • Identify supported fileformat
  • Add setup changes
  • The pot file should be regenerated in a separate commit because there are unrelated changes
  • Don't keep 2 temp files

@jeromerobert I don't quite understand what you mean by optional (in [...] and in the setup.py).

@dreua

This comment has been minimized.

@dreua

This comment has been minimized.

@jeromerobert
Copy link
Member

Just some remark

  • I removed mimetypes usage because of Filetype: "application/wps-office.pdf" not supported! (Ubuntu) #101 but I don't mind readding it for images
  • Having img2pdf in the Windows installer will require some work and I don't plan to do it. img2pdf will be for Linux user's only until someone do it.
  • I think people will want a way to specify the DPI of the image (but let's keep that for an other issue/PR)

@mimi89999 mimi89999 mentioned this pull request May 16, 2020
@mimi89999
Copy link
Contributor Author

* By optional I mean that it must work without img2pdf which is what you almost did. We are just missing a `img2pdf=None` in the except to avoid `img2pdf not defined`.

Sorry, I missed that. Thanks for pointing that out.

* It should also be optional in `setup.py`. See https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies) to support `pip install pdfarranger[img2pdf]`.

Done

* `PDFDoc` is already creating a temporary file. I think we don't need to have 2 temporary files (especially when the first one is not deleted). May be you could add an other PDFDoc constructor that get a buffer as input ?

On my TODO

I also made the MIME handling way more robust, but the code isn't as nice now.

@mimi89999 mimi89999 marked this pull request as ready for review May 16, 2020 18:23
@mimi89999
Copy link
Contributor Author

It should be ready now. @jeromerobert could you please review?

@jeromerobert
Copy link
Member

@jeromerobert
Copy link
Member

You checked Add translations (can do fr and pl) but did not added those translation. If this is because you are concerned by the POT file synchronisation, don't bother. You can just regenerate the .pot file for you and commit only the line of the fr.po and pl.po files you modified.

@mimi89999
Copy link
Contributor Author

mimi89999 commented May 17, 2020

setup.py is missing the img2pdf dependency

Added

Please don't use MIME for pdf type (see #101). Your previous version was better (check mime for image, else just try to open the PDF).

I think that it's an issue with WPS Office and that MIME map shouldn't be changed.

You checked Add translations (can do fr and pl) but did not added those translation.

Removed that task.

@jeromerobert
Copy link
Member

Ok I agree that we should keep using mimetypes.

Now I get NameError: name 'img2pdf_supported_img' is not defined when trying your branch. You are missing a img2pdf_supported_img=[] right ?

@mimi89999
Copy link
Contributor Author

L762-768 should be wrapped in if img2pdf:. Missed that. Sorry

@mimi89999
Copy link
Contributor Author

I will push the change this evening. Is it OK besides that?

@jeromerobert
Copy link
Member

Is it OK besides that?

I think so.

@mimi89999
Copy link
Contributor Author

Done

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

3 participants