[benoit74]

Fix #232
Fix #272

To be merged after #234 (which is simpler / narrower scope)

Changes:

• new illustration module with the IllustrationInfo class, used by reader and writer modules
• new signatures of reader.Archive.get_illustration_item: get "default" and get specific sizes/scales
• new method reader.Archive.get_illustrations_infos
• new signature of writer.Creator.add_illustration : pass detailed illustration infos instead of one fixed size

[codecov]

Codecov Report

❌ Patch coverage is 97.01493% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.33%. Comparing base (e6af0b4) to head (de9032f).

Files with missing lines	Patch %	Lines
libzim/libzim.pyx	97.01%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   94.09%   94.33%   +0.23%     
==========================================
  Files           1        1              
  Lines         542      600      +58     
==========================================
+ Hits          510      566      +56     
- Misses         32       34       +2     

☔ View full report in Codecov by Harness.
📢 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.

[benoit74]

Ready for review, wheels build still failing on ubuntu arm64 for "known reasons" but rest is in place so ready to be analyzed

[rgaudin]

Thank you @benoit74,

When we designed this on libzim, it was very clear that:

• attributes are an optional, for-the-future feature that we have no real use case for. It's still not in the spec.
• independant width and height are important now
• scale is important now

We thus requested to be able to directly add illustration directly using width, height, scale. That's not the case with this.
If there are libzim primitives to do it, bind them, if it's the same, add the necessary python glue.

On the reader side, it's less important but I feel like a lot is left on the hands of the libzim-user.
How do I know if 48x48 has various scales? I have to loop through all the getIllustrationInfos() results…

[benoit74]

Thank you!

Regarding addIllustrations, there is "only" four variants available in the libzim: https://github.com/openzim/libzim/blob/8ef511ba0de6a8180d79fbc3c7edcfec4202ae5e/include/zim/writer/creator.h#L165-L197 ; I did not exposed the "contentProvider" variants because the one previously present was not exposed either. I could add the two missing, but it will change nothing to your point.

I generally second your remarks and frustrations about how "complex" stuff still is with current PR, but I considered we've agreed python-libzim should be a very thin wrapper on top of the libzim, and everything more "complex" should go into the python-scraperlib. I feel like your python-glue is something which should be implemented in the python-scraperlib.

For instance even with only this PR, it is already possible to do things like below which are not "that" awful. IllustrationInfo is just a structure to hold all details about the illustration:

creator.add_illustration(IllustrationInfo(48, 48), png_data)
creator.add_illustration(IllustrationInfo(48, 48, 2), png_data)

Details point are all valid.

[rgaudin]

I feel like your python-glue is something which should be implemented in the python-scraperlib.

I don't think so but this is debatable of course.

scraperlib is not intended for many users. It is, as its name implies, scraper-oriented and quite large and have many dependencies. We can do as we please there because we are the main users and requiring explicit extra types for this is OK there. We're already enforcing a lot of stuff, because we're the ones using it.

pylibzim on the other end is to be used by hopefully many users. It has zero runtime dependency, is lightweight and lean.
I am a strong advocate of the pure-binding strategy but in this case, the libzim API is technically-oriented and not user oriented. I'd prefer the same I asked for months ago which is exposing what we are using: width, height, scale. If it were not for those extraAttributes that were not part of the original request, we would not have this intermediate type.

It just seems silly to push this uncomfortable change to our users but let's keep this PR like this and open a separate ticket to discuss whether we want to make an exception or not.

[benoit74]

Simple changes pushed, but I'm not comfortable with merging this if you are not comfortable with this PR "design" ; changing this afterwards might means breaking the API so needs a major, uncomfortable changes for "users", etc ...

I see no urgency in pushing these changes to the python-libzim, since we have no use-case so far, and would prefer to have a decision comfortable for everyone. And I somehow share your concerns, especially on the reader side. I'm less embarrassed by the fact that we need the new IllustrationInfo() type than you are.

Since python-libzim is anyway not used by our readers (so far, unfortunately?), maybe a simple middle ground would be:

• add additional glue for add_illustration(width, height, scale, data), add_illustration(size, scale, data) (squared), get_illustration_item(width, height, scale) and get_illustration_item(size, scale) (squared), since they really don't provide anything besides simple glue we should be able to support "forever"
• let the more complex issue about how readers find their illustration untouched, it is still unclear if we need a common solution to that or should let each readers find their way through this based on their needs (e.g. when a reader would prefer a 96x96@1 and it does not exists but 48x48@2 and 126x126@1 do exist, do we return both, only biggest res, ...)

WDYT?

[rgaudin]

I'm fine with this simple glue strategy. It's dumb enough to not be a problem, as you wrote.

Regarding the more complex reader usage, there's a ticket for that on libzim, it will definitely be implemented there but first step was to allow addition of multiple scales and width.

[benoit74]

Hum, I'm afraid that in fact this simple glue strategy is not that simple, because it will break the API or introduce dirty glue code for add_illustration.

Currently second argument to add_illustration is the content. If content can be either second or third or fourth depending on other arguments, this is either brittle (we count arguments to know where content is) or breaking (we force content to be a named argument).

Do you have suggestion about how to handle this? Should we introduce different method names? I'm puzzled

Same kind of issue with the scale in get_illustration_item(width, height, scale) and get_illustration_item(size, scale), should we have scale first or should we named arguments or something else?

[rgaudin]

Well libzim 9.4.0 did break the API (version doesn't tell because they don't respect semver) so I see no reason we wouldn't allow ourselves to.

Also, it's a binding not a pure copy which it can't be, especially since python doesn't support overloading natively. To me it would only make sense to have the content the first param and the rest afterwards. It could even be a single func with width_or_squaresize, height, scale.

What do you think?

[benoit74]

I don't know ; I will make a first release with 9.4.0 and Python 3.13 but without this PR, and then we will have time to take informed decision

[benoit74]

Pushed two follow-up commits to move this forward after being stuck on the API design question:

1. 67f6423 — expose width/height/scale directly

add_illustration/get_illustration_item required building an IllustrationInfo for anything beyond a plain square size, and the single-argument shape made it ambiguous where to add more optional params without colliding with content's position. Moved content to the first positional argument of add_illustration, and both methods now accept width, height (defaults to width, i.e. square) and scale (defaults to 1.0) directly. IllustrationInfo is still accepted for the advanced case (e.g. extra_attributes), but is no longer required for the common path.

2. 171017b — make IllustrationInfo immutable

The previous IllustrationInfo was a mutable Cython cdef class with hand-written property setters wrapping the libzim C++ struct directly, which is not really "pythonic" and it had already produced a real bug (mutating the dict returned by the extra_attributes getter silently didn't persist, since the getter built a fresh dict on every access). Reimplemented it as an immutable namedtuple-based value object: width/height/scale/extra_attributes are read-only fields, extra_attributes is defensively copied into an immutable mapping on construction, and changing a value means building a new instance (e.g. via ._replace()). as_metadata_item_name/from_metadata_item_name still bridge to libzim's own C++ implementation for correctness.

[benoit74]

@rgaudin feedback is welcomed on these changes

[rgaudin]

Looks OK ; can you resolve or revive remaining discussions?

[benoit74]

Thank you. They are indeed all resolved ; shall I squash and rebase on main?

[rgaudin]

See inline comments ; otherwise looks good to me. didn't test it.

[benoit74]

Pushed `48ddb3a` addressing the inline comments:

• Naming / "info deserves its own arg": split `add_illustration`/`get_illustration_item` into a width/height/scale shortcut (no `IllustrationInfo` involved, so no more `squaresize_or_width_or_info`) and a separate `add_illustration_info`/`get_illustration_item_info` that mirrors libzim's `IllustrationInfo`-based overload directly. The shortcut just builds an `IllustrationInfo` and calls the `_info` method, like you suggested.
• "infos" has no plural: renamed `get_illustration_infos` to `get_illustrations_info`, and updated the `get_illustration_sizes()` deprecation message to point at the new name.
• List comprehension: done in `get_illustrations_info`.
• `all()`/validation simplification: reworked the width/height/min_scale check into `all(v is not None ...)` / `any(v is not None ...)`, keeping `is not None` (not plain truthiness) so a `0` value still works as you noted.
• Docstring scope: trimmed the "immutable value object, use `._replace()`" paragraph from `IllustrationInfo`'s docstring — that's PR-discussion content, not docstring content.

[rgaudin]

👍