Fix build with poppler<0.64
ClosedPublic

Authored by arojas on Oct 24 2018, 4:49 PM.

Details

Summary

Last commit broke build with poppler<0.64, where getNextChar doesn't accept a const char first argument.

Test Plan

Builds with poppler as far back as 0.55

Diff Detail

Repository
R8 Calligra
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
arojas created this revision.Oct 24 2018, 4:49 PM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptOct 24 2018, 4:49 PM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald Transcript
arojas requested review of this revision.Oct 24 2018, 4:49 PM
pino requested changes to this revision.Oct 24 2018, 5:01 PM
pino added a subscriber: pino.

Please do a feature test at cmake time instead.

filters/karbon/pdf/SvgOutputDev.cpp
28

NACK, this is not poppler core API

This revision now requires changes to proceed.Oct 24 2018, 5:01 PM
arojas updated this revision to Diff 44199.Oct 25 2018, 6:50 AM

Check for poppler version at cmake time

aacid added inline comments.Oct 25 2018, 8:53 PM
filters/karbon/pdf/CMakeLists.txt
10

Do you think

if (NOT Poppler_VERSION VERSION_LESS "0.64.0")

makes it more readable?

I'm a bit undecided myself

rjvbb added a subscriber: rjvbb.Oct 25 2018, 9:19 PM

How about

if (Poppler_VERSION VERSION_LESS "0.64.0")
    add_definitions("-DHAVE_POPPLER_PRE_0_64")

That also removes the ambiguity while reading the code ("is this 0.64.0 and higher or only that specific version?") and *may* make it easier to clean obsolete code later on when support for Poppler < 0.64.0 is dropped.

I don't mind either way. In the future this could be changed to VERSION_GREATER_EQUAL when this can depend on cmake 3.7

aacid added a comment.Oct 26 2018, 7:09 PM

I'd say let's go with what René suggested, i guess it what makes more sense.

arojas updated this revision to Diff 44261.Oct 26 2018, 7:20 PM

Invert conditional

arojas marked an inline comment as done.Oct 26 2018, 7:20 PM
aacid added a comment.Oct 26 2018, 9:51 PM

Looks good, since you seem to have a pre 0.64 build at hand, could you check it still actually builds after i added all the overrides? Maybe we need some more ifdef

Indeed git master doesn't build with 0.55:

In file included from /home/antonio/Software/KDE/calligra/filters/karbon/pdf/PdfImport.cpp:23:
/home/antonio/Software/KDE/calligra/filters/karbon/pdf/SvgOutputDev.h:56:10: error: ‘void SvgOutputDev::drawString(GfxState*, const GooString*)’ marked ‘override’, but does not override

void drawString(GfxState * state, const GooString * s) override;
     ^~~~~~~~~~

But this one needs to be fixed in the 3.1 branch too, so I'd say let's push this to 3.1 and then fix the remaining issues in master in a separate commit

aacid accepted this revision.Oct 26 2018, 10:29 PM

When you say "But this one needs to be fixed in the 3.1 branch too" you mean D16406 (i.e. this very same patch) or the compilation error because of the overrides?

The fact that the 3.1 branch "compiles" with the new poppler is just that, "it compiles", but it doesn't really work since the signature of the function it is supposed to reimplement has changed.

So we need "all the fixes" in "all the branches".

But sure, i guess let's commit this and then figure out what to do, not sure with the rest, at least this is a step forward :)

This revision was not accepted when it landed; it landed in state Needs Review.Oct 26 2018, 10:36 PM
This revision was automatically updated to reflect the committed changes.

When you say "But this one needs to be fixed in the 3.1 branch too" you mean D16406 (i.e. this very same patch) or the compilation error because of the overrides?

I mean this one, the other error doesn't happen in 3.1 since you only pushed the changes to master

When you say "But this one needs to be fixed in the 3.1 branch too" you mean D16406 (i.e. this very same patch) or the compilation error because of the overrides?

I mean this one, the other error doesn't happen in 3.1 since you only pushed the changes to master

Sure, as i said the fact that the error doesn't happen "means nothing", the code isn't working with a newer poppler because the functions signature is wrong (aka old)