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 ICC profiles to PDFs #709

Merged
merged 11 commits into from
Feb 27, 2023
Merged

add ICC profiles to PDFs #709

merged 11 commits into from
Feb 27, 2023

Conversation

eroux
Copy link

@eroux eroux commented Feb 27, 2023

Fixes #697

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@eroux eroux requested a review from Lucas-C as a code owner February 27, 2023 11:12
@eroux
Copy link
Author

eroux commented Feb 27, 2023

@Lucas-C I don't have a Winows machine so I couldn't update test/image/image_types/image_types_insert_png_alpha_dctdecode_windows.pdf, is there any way you could do it? Thanks!

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

Hey @eroux
Thank you for taking the time to work on this!
That's a very valuable addition for fpdf2 🥳😀

CHANGELOG.md Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/output.py Show resolved Hide resolved
fpdf/output.py Outdated Show resolved Hide resolved
fpdf/output.py Outdated Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/output.py Show resolved Hide resolved
test/image/test_load_image.py Outdated Show resolved Hide resolved
test/image/image_types/test_insert_images.py Show resolved Hide resolved
fpdf/image_parsing.py Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
fpdf/output.py Outdated Show resolved Hide resolved
@Lucas-C
Copy link
Member

Lucas-C commented Feb 27, 2023

@Lucas-C I don't have a Winows machine so I couldn't update test/image/image_types/image_types_insert_png_alpha_dctdecode_windows.pdf, is there any way you could do it? Thanks!

Sure, but I just ran unit test test_insert_png_alpha_dctdecode() on my Windows environment, and it passed!

By the way, you may want to get rid of PYTHONMALLOCSTATS as part of thi PR to avoid the GitHub Actions logs being bloated, cf. #657 (comment)

@Lucas-C
Copy link
Member

Lucas-C commented Feb 27, 2023

Sure, but I just ran unit test test_insert_png_alpha_dctdecode() on my Windows environment, and it passed!

My bad, I did not run the test correctly... 🤦‍♂️

I generated a reference PDF file, but I got this error when performing git push on your branch:

ERROR: Permission to buda-base/fpdf2.git denied to Lucas-C.

You can either allow me push to your fork: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Or use the PDF file included with this comment:
image_types_insert_png_alpha_dctdecode_windows.pdf

@Lucas-C
Copy link
Member

Lucas-C commented Feb 27, 2023

Apart from the nice mention you already added in CHANGELOG.md, I think this feature would be worth a section in docs/images.md! 😊

In order to mention this automatic inclusion of ICC profiles from images, and also how to remove /alter ICC profiles:
simply by modifying .info["icc_profile"] with Pillow before passing PIL.Image instances to FPDF.image()!

@eroux
Copy link
Author

eroux commented Feb 27, 2023

thanks for the PDF, I've pushed it. I'm working on the two changes but I just wanted to say that updating docs/images.md is a good idea.

I'm thinking also that perhaps releasing a new version with those changes could be a good option? The changes since the last version are pretty significant for use cases involving images

@Lucas-C
Copy link
Member

Lucas-C commented Feb 27, 2023

I'm thinking also that perhaps releasing a new version with those changes could be a good option? The changes since the last version are pretty significant for use cases involving images

I agree.
Given the maturity of the 4 current open PRs, I'm currently planning to perform a release once they are all merged.
If ever any of them is delayed, I may publish a new version of fpdf2 beforehand, we'll see.

In the meantime, you can still use the latest fpdf2 version from the master branch of this repository:

pip install git+https://github.com/PyFPDF/fpdf2.git@master

@eroux
Copy link
Author

eroux commented Feb 27, 2023

I think it should be ok now

@Lucas-C Lucas-C merged commit e9dfb18 into py-pdf:master Feb 27, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Feb 27, 2023

Merged!
Thank you @eroux for this contribution 👍

@eroux
Copy link
Author

eroux commented Feb 27, 2023

amazing, thanks!

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.

Feature request: include ICC profile in documents
2 participants