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

Adding a warnings member to the Fpdf struct #44

Closed
jessp01 opened this issue Aug 25, 2023 · 6 comments
Closed

Adding a warnings member to the Fpdf struct #44

jessp01 opened this issue Aug 25, 2023 · 6 comments

Comments

@jessp01
Copy link

jessp01 commented Aug 25, 2023

Hi all,

First of all, excellent project.

The reason for raising this is that some issues encountered during processing are not fatal and generating the doc in spite of them may be worthwhile. For example, consider https://github.com/go-pdf/fpdf/blob/main/png.go#L57 (I bring it as an example because I've actually encountered it):

	if bpc > 8 {
		f.err = fmt.Errorf("16-bit depth not supported in PNG file")
	}

With my input, if I simply change the code to print the error rather than assign it to f.err, I get the image in the generated PDF doc. You can easily reproduce this with the below command but I'm also attaching the resulting PDF to this issue:

go install github.com/mandolyte/mdtopdf/cmd/md2pdf@latest
md2pdf -i https://github.com/fogleman/gg/raw/master/README.md -o gg.pdf --page-size A3

gg.pdf

But, because in mdtopdf we call OutputFileAndClose(), which returns immediately if if f.err != nil, no doc is generated.

My suggestion is to add a warnings string array to the Fpdf struct, populate it when encountering non-fatal errors (instead of assigning them to the err member) and print these in OutputFileAndClose(), rather than give up and return without generating the file.

If you find this acceptable, I'm happy to submit a pull request.

Cheers,

@sbinet
Copy link
Contributor

sbinet commented Sep 13, 2023

(apologies for the belated answer: a fine mix of holidays and a backlog at work)

is this the sole instance of such an error ?

because in this instance, the error should actually be raised iff the pdfVersion we are generating is < 1.5. 16 bit images are allowed starting from PDF 1.5.
alternatively, as is done in other parts of the code, we could automatically bump the pdfVersion to 1.5 when encountering images with 16-bit depth...

@jessp01
Copy link
Author

jessp01 commented Sep 13, 2023

Hi @sbinet ,

It's the only case I've encountered but of course, you're in a better position to say whether there are other, similar cases:)

I can see that pdfVersion is set to "1.3" by default. Is there a reason for that?

alternatively, as is done in other parts of the code, we could automatically bump the pdfVersion to 1.5 when encountering images with 16-bit depth...

That would address the case in question, as I said, I leave to you to say whether this should occur in other places as well.

Cheers,

PS
Do you reckon adding a method to allow setting pdfVersion to a different value (from outside of the fpdf code) is a good idea?

@sbinet
Copy link
Contributor

sbinet commented Sep 13, 2023

AFAICT (not being the original author of the package) the pdfVersion is set to the lowest common denominator.
it's automatically bumped as new features from the PDF specs are required.

I can't think of a use case where one would want an explicit version. (but I am open to counter examples.)
PR #50 (now merged into main) has the automatic pdf-version bump.
I tried it locally with your reproducer. it seems to be working fine.

@jessp01
Copy link
Author

jessp01 commented Sep 13, 2023

Could you please create a tag that includes this change (the last tag was created in April and there were some changes this fix relies on since)? I'll be happy to test once you do.

@sbinet
Copy link
Contributor

sbinet commented Sep 13, 2023

@jessp01
Copy link
Author

jessp01 commented Sep 13, 2023

Works as expected:)

Cheers,

@jessp01 jessp01 closed this as completed Sep 13, 2023
jessp01 added a commit to jessp01/mdtopdf that referenced this issue Sep 13, 2023
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

No branches or pull requests

2 participants