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

Bitonal images are now encoded using CCITTFaxDecode - fix #691 #695

Merged
merged 15 commits into from
Feb 17, 2023
Merged

Bitonal images are now encoded using CCITTFaxDecode - fix #691 #695

merged 15 commits into from
Feb 17, 2023

Conversation

eroux
Copy link

@eroux eroux commented Feb 16, 2023

Fixes #691

I couldn't run the tests through pytest for some reason:

pytest
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov=fpdf --cov-report=xml
  inifile: /home/eroux/BUDA/softs/fpdf2/setup.cfg
  rootdir: /home/eroux/BUDA/softs/fpdf2

Checklist:

  • 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

  • [Not Applicable] 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.

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.

Thank you for the PR @eroux!

For reference, basic support for monochromatic images was initially introduced on september last year: #527

Some minor issues were spotted by Pylint in the GitHub Actions pipeline, could you please fix them?

fpdf/image_parsing.py:136:0: C0303: Trailing whitespace (trailing-whitespace)
fpdf/image_parsing.py:73:12: W0105: String statement has no effect (pointless-string-statement)
fpdf/image_parsing.py:200:12: C0209: Formatting a regular string which could be a f-string (consider-using-f-string)

fpdf/image_parsing.py Outdated Show resolved Hide resolved
test/image/image_types/test_insert_images.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
eroux and others added 7 commits February 16, 2023 18:09
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
@eroux
Copy link
Author

eroux commented Feb 16, 2023

I've addressed your comments (all of them I think?)

@Lucas-C
Copy link
Member

Lucas-C commented Feb 16, 2023

I've addressed your comments (all of them I think?)

I think there are a few ones left 😊
But thank you for your diligence and the improvements you already made!

I couldn't run the tests through pytest for some reason:

Have you checked our documentation regarding development on the project?
https://pyfpdf.github.io/fpdf2/Development.html

I think you just need to pip install -r test/requirements.txt 😊

Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
@Lucas-C
Copy link
Member

Lucas-C commented Feb 17, 2023

Could you please format the code with black?
This will allow the GitHub Actions pipeline to execute the unit tests suite.

Run black --check .
would reformat /home/runner/work/fpdf2/fpdf2/fpdf/image_parsing.py

Oh no! 💥 💔 💥
1 file would be reformatted, 144 files would be left unchanged.

@eroux
Copy link
Author

eroux commented Feb 17, 2023

I did reformat it with black, thanks! There's an issue when I try to install the test dependencies so I still can't run the unit tests, but I'll try on another system

@eroux
Copy link
Author

eroux commented Feb 17, 2023

@Lucas-C I finally managed to launch the tests, the only that fails on my machine is the performance comparison, I don't know what the value of 0.3s is computed, but on my computer fpdf is too fast to pass the test(!), is that a problem?

@Lucas-C
Copy link
Member

Lucas-C commented Feb 17, 2023

@Lucas-C I finally managed to launch the tests, the only that fails on my machine is the performance comparison, I don't know what the value of 0.3s is computed, but on my computer fpdf is too fast to pass the test(!), is that a problem?

It's OK, don't worry, as long as the unit tests pass in the GitHub Actions pipelines that's fine 😊
Thank you for taking the time to run the tests on your machine.

I think this PR is close to be ready to be merged!
Thank your for your work on this.
I have one last question: why are the PDF reference files in test/template/ impacted by your change?

@eroux
Copy link
Author

eroux commented Feb 17, 2023

Great, thanks! The impact is that all bitonal images are now encoded in CCITT by default and these contain some bitonal images

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Base: 93.93% // Head: 93.77% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (aff4b21) compared to base (61348c5).
Patch coverage: 76.47% of modified lines in pull request are covered.

❗ Current head aff4b21 differs from pull request most recent head 513276f. Consider uploading reports for the commit 513276f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #695      +/-   ##
==========================================
- Coverage   93.93%   93.77%   -0.17%     
==========================================
  Files          26       26              
  Lines        6780     6829      +49     
  Branches     1206     1216      +10     
==========================================
+ Hits         6369     6404      +35     
- Misses        242      251       +9     
- Partials      169      174       +5     
Impacted Files Coverage Δ
fpdf/image_parsing.py 83.33% <76.47%> (-5.17%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

Lucas-C commented Feb 17, 2023

@allcontributors please add @eroux for code

@allcontributors
Copy link

@Lucas-C

I've put up a pull request to add @eroux! 🎉

@Lucas-C
Copy link
Member

Lucas-C commented Feb 17, 2023

Merged!
Thank you for your contribution 😊

@Lucas-C Lucas-C changed the title fix for #691 Bitonal images are now encoded using CCITTFaxDecode - fix #691 Oct 4, 2024
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: allow CCITT image encoding
2 participants