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 support for embedding indexed images #443
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job really! Well done, the code is clean and you seemingly reduced the size of the PDFs generated when they embed indexed images.
My idea is to generate the PDF again using generate=True.
Seems like the best approach here.
Just make sure the new reference PDF files are smaller than the previous ones.
Codecov Report
@@ Coverage Diff @@
## master #443 +/- ##
==========================================
+ Coverage 91.88% 92.00% +0.12%
==========================================
Files 22 22
Lines 6408 6431 +23
Branches 1290 1298 +8
==========================================
+ Hits 5888 5917 +29
+ Misses 298 293 -5
+ Partials 222 221 -1
Continue to review full report at Codecov.
|
Thank you for you work on this! PS: don't forget to add a mention of this in the |
Sure! Thank you! Have a good week-end you too 😊 |
What was the error exactly?
Yeah, I've made some tests and Pillow definitively has trouble with those images... |
In the next days I will try open an issue in the Pillow's github page
Thank you!
|
In the last days I searched in the code of Pillow and I discovered I had the false assumption that "P" images don't support transparency and "PA" yes. Instead, both support transparency:
Seems that Pillow has a problem in generating the alpha layer when converting from "P" to "PA". This doesn't happen in the conversion "P" to "RGBA". I opened the issue and the relative PR that should fix it. Regarding fpdf2 and "P" images, currently the transparency is not ported in the PDF and for doing this I see 2 approaches:
Regarding "PA" images, the code in the fpdf2 seems to work correctly if the alpha layer is correctly set. So I suggest we keep supporting them. (I also changed the sample images in the test putting some pics of flowers that I took near my house 😊) |
Wow, you did an excellent job! I'm going to fix the minor file conflicts that prevent this PR from being merged, |
Thank you, that means a lot to me! it's really rewarding and encourages me to do more! 😊 |
It seems like some tests are failing:
Do you have the same errors on your computer? |
Yes I forgot to run all the tests again |
Merged! Thank you @RedShy 😊 |
This feature has been included in the new v2.5.5 release! |
I added the support for embedding indexed images in a PDF. The code was already there, I just had to do some minor fixes. I also added a test to cover the new feature.
There is a problem though: previous tests now breaks.
The tests processed indexed images by converting them to RGBA and then embedding them in the PDF, now instead they embeds them directly as indexed images.
Not sure how to proceed now.
Checklist:
The GitHub pipeline is OK (green),
meaning that both
pylint
(static code analyzer) andblack
(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/
folderA mention of the change is present in
CHANGELOG.md