Illustration API enhancement

[veloman-yunkan]

Fixes #937

[codecov]

Codecov Report

❌ Patch coverage is 50.56180% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.92%. Comparing base (abf8d4a) to head (7e8bb1a).

Files with missing lines	Patch %	Lines
src/archive.cpp	41.93%	5 Missing and 13 partials ⚠️
src/tools.cpp	53.84%	1 Missing and 17 partials ⚠️
include/zim/illustration.h	55.55%	0 Missing and 4 partials ⚠️
src/writer/creator.cpp	60.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
- Coverage   58.15%   57.92%   -0.24%     
==========================================
  Files         100      101       +1     
  Lines        5325     5407      +82     
  Branches     2179     2219      +40     
==========================================
+ Hits         3097     3132      +35     
- Misses        778      801      +23     
- Partials     1450     1474      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rgaudin]

Thank you @veloman-yunkan ; implementation LGTM ; I have some general questions though:

• getIllustrationSizes has not changed. It's a separate discussion but what's the decision on this? @kelson42 ?
• I thought we'd get a public method to add/read with just the width, height and scale but we have to use a new type. That's OK.
• I find Archive::getIllustrations() confusing. We're only getting Illustration Info, not actual illustrations. Maybe that's in line with the rest of the libzim code base though?

[benoit74]

Besides rgaudin point, this looks good to me

[veloman-yunkan]

• getIllustrationSizes has not changed. It's a separate discussion but what's the decision on this? @kelson42 ?

Aren't we going to deprecate the old API?

• I thought we'd get a public method to add/read with just the width, height and scale but we have to use a new type. That's OK.

I will add wrappers. Let me see how to do it best.

• I find Archive::getIllustrations() confusing. We're only getting Illustration Info, not actual illustrations. Maybe that's in line with the rest of the libzim code base though?

That's a valid point, but I found Archive::getIllustrationInfos() a little too verbose in a typed language. In this context IllustrationInfo's act like handles to illustration data (that should be obtained via Archive::getIllustrationItem()). Suppose that getIllustrations() returns pointers to the illustration objects. Should it be named getIllustrationPtrs() then? That's the logic behind the name for that method that I ended up with.

[rgaudin]

• getIllustrationSizes has not changed. It's a separate discussion but what's the decision on this? @kelson42 ?

Aren't we going to deprecate the old API?

Indeed, it's even in the ticket. Is the deprecation doc and warning to be in a separate PR?

• I find Archive::getIllustrations() confusing. We're only getting Illustration Info, not actual illustrations. Maybe that's in line with the rest of the libzim code base though?

That's a valid point, but I found Archive::getIllustrationInfos() a little too verbose in a typed language. In this context IllustrationInfo's act like handles to illustration data (that should be obtained via Archive::getIllustrationItem()). Suppose that getIllustrations() returns pointers to the illustration objects. Should it be named getIllustrationPtrs() then? That's the logic behind the name for that method that I ended up with.

You are mistaking Type suffixes (which is a terrible practice) with semantic/meaningful naming. I'm just signaling that from my POV it's confusing to request illustrations and receive a struct of illustration metadata. Nothing serious though and I strongly believe that API coherence is more important than individual precision

[veloman-yunkan]

@rgaudin I've renamed getIllustrations() to getIllustrationInfos() and deprecated getIllustrationSizes().

• I thought we'd get a public method to add/read with just the width, height and scale but we have to use a new type. That's OK.

I will add wrappers. Let me see how to do it best.

I didn't add any wrappers. Instead I provided an overload of getIllustrationInfos() that allows to obtain a list of illustrations with the specified dimensions.

[veloman-yunkan]

Fixup commits will need to be eliminated before the PR is merged

[rgaudin]

API LGTM ; thank you and sorry for the delay.

[veloman-yunkan]

@kelson42 Please don't merge until I get rid of the fixup commits.

[veloman-yunkan]

The PR has been rebased.

CI failures on Linux are because since kiwix/kiwix-build#834 was merged dependencies are built on jammy, while libzim CI keeps running on focal. Should be fixed by #1002.

The failures on macOS have nothing to do with these changes. One of the failures is because of the following error:

meson.build:55:17: ERROR: Dependency "xapian-core" not found, tried pkgconfig, framework and cmake

The other three failures say:

meson.build:1:0: ERROR: Unknown compiler(s): [['/Applications/Xcode_16.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang']]
The following exception(s) were encountered:
Running /Applications/Xcode_16.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang --version gave "[Errno 2] No such file or directory: '/Applications/Xcode_16.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang'"