search

PhilterPaper / Perl-PDF-Builder

Extended version of the popular PDF::API2 Perl-based PDF library for creating, reading, and modifying PDF documents
https://www.catskilltech.com/FreeSW/product/PDF%2DBuilder/title/PDF%3A%3ABuilder/freeSW_full
Other
6 stars 7 forks source link

Improve loading optional dependencies #139

Closed ppisar closed 3 years ago

ppisar commented 3 years ago

This patch set improves loading the optional dependencies for PNG and TIFF images.

PhilterPaper commented 3 years ago

Is the intent here to replace the hard-coded eval check with a call to the LA_* (Library Available) calls? Your new code behaves a little differently than the old code, returning -1 if "don't use the library" regardless of whether it's installed, instead of 0 if it's not installed. I will have to look at the usage and see whether this makes any difference.

ppisar commented 3 years ago

On Thu, Nov 05, 2020 at 07:09:04AM -0800, Phil Perry wrote:

Is the intent here to replace the hard-coded eval check with a call to the LA_* (Library Available) calls?

Yes.

Your new code behaves a little differently than the old code, returning -1 if "don't use the library" regardless of whether it's installed, instead of 0 if it's not installed.

Which piece of code does return -1?

image_png() always returns an object reference. LA_IPL() always returns 0 or 1. Never -1.

PhilterPaper commented 3 years ago

Which piece of code does return -1?

The segments you changed in Builder.pm set the library usage flag to -1 if the library is available but you choose not to use it, 0 if it's unavailable, or 1 if it's available and you choose to use it. Your change sets it to -1 even if the library is UNavailable.

ppisar commented 3 years ago

I see. It only affects printing the "Your system does not have..." warning. But a comment above the warning reads:

# TBD give warning message once, unless silenced (-silent) or
# deliberately not using Graphics::TIFF (rc == -1)

So it actualy gets the code in line to the comment. Why should a user be informed about missing the library if he decided that does not want the library? I think it's an improvement.

PhilterPaper commented 3 years ago

You're misreading the comment and the original code. You see the message (once) if 1) $rc==0 (no library available) and 2) not silenced with -silent. If $rc==-1 (library available but not wanted), you will not see the message. And of course, if $rc==1 (library available and being used), you will not see the message.

If you feel that the existing comments and documentation are unclear, please list the problem(s) that I could try to do something about.

PhilterPaper commented 3 years ago

OK, a bit of a compromise here. I now use $self->LA_* to get the initial $rc 0 or 1 setting (saves about 8 lines of code), but leave the rest of the code unchanged, so I'm not merging this pull request. Thank you anyway for the suggestion to use the "LA_" functions.