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

BUG: 1376 Acrobat Scale #1499

Closed
wants to merge 13 commits into from
Closed

Conversation

joshhendo
Copy link
Contributor

Fixes #1376

I'm opening this to have a discussion about the idea of having a context field. I'm not suggesting this PR as is, rather a starting point to start a discussion. This is based on the suggestion from @programmarchy in this comment: #1267 (comment)

I think this issue does need to be addressed. Currently using scaling where it causes more than 19 decimal places, the resulting PDF is not usable in Acrobat. Whilst the PDF spec doesn't specify the precision, I think most people expect a PDF to work in Acrobat.

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Base: 92.03% // Head: 91.93% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (457200b) compared to base (e5e26ad).
Patch coverage: 65.21% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1499      +/-   ##
==========================================
- Coverage   92.03%   91.93%   -0.11%     
==========================================
  Files          32       33       +1     
  Lines        5976     5999      +23     
  Branches     1163     1164       +1     
==========================================
+ Hits         5500     5515      +15     
- Misses        312      319       +7     
- Partials      164      165       +1     
Impacted Files Coverage Δ
PyPDF2/context.py 63.63% <63.63%> (ø)
PyPDF2/generic/_base.py 99.64% <100.00%> (+<0.01%) ⬆️

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.

@chrysn
Copy link

chrysn commented Dec 27, 2022

Take it for what a drive-by commenter's input is worth, but I don't think that changing the Decimal precision is the way to go -- users might be calculating with that, to whichever precision they configured, and context handling in a library is a tricky thing.

Wouldn't it suffice to, in the __repr__ of FloatObject, create another branch like this?

     def __repr__(self) -> str:
         if self == self.to_integral():
             # If this is an integer, format it with no decimal place.
             return str(self.quantize(decimal.Decimal(1)))
+        elif self > 1:
+            quantized = self.quantize(decimal.Decimal("0.0000000000000000001"))
+            return f"{quantized:f}".rstrip("0")
         else:
             # Otherwise, format it with a decimal place, taking care to
             # remove any extraneous trailing zeros.
             return f"{self:f}".rstrip("0")

(or >= 1 -- the == 1 case is handled above anyway)

If my observations about what Acrobat tolerates (around #1376 (comment)) are true, then this will preserve the maximum accuracy Acrobat will tolerate.

@MartinThoma
Copy link
Member

@joshhendo @programmarchy It took me a while to decide on how to continue with this topic.

I finally decided to get rid of Decimal completely. This should fix #1376 .

A couple of other PDF libraries / applications also just represent the numbers with PDF documents as IEEE 754 floats/doubles, hence I hope that this is fine.

Thank you for your work on this PR ❤️

@MartinThoma MartinThoma closed this Feb 5, 2023
@joshhendo
Copy link
Contributor Author

I've updated to the latest version and it works great with the fix from #1376. 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.

Acrobat cannot display transformed PDFs with a decimal precision > 19
3 participants