Support for multiple illustrations in a Book

[veloman-yunkan]

Fixes #625.

Note that this fix only adds support for multiple illustrations inside the kiwix::Book class, without enhancing the library format. As a result, ZIM files with multiple illustrations will not appear as such in the OPDS output if the kiwix server is started with a trusted library.

[codecov]

Codecov Report

Merging #630 (4a01081) into master (ef1ad4b) will increase coverage by 0.48%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master     #630      +/-   ##
==========================================
+ Coverage   66.14%   66.62%   +0.48%     
==========================================
  Files          53       53              
  Lines        3819     3823       +4     
  Branches     1914     1931      +17     
==========================================
+ Hits         2526     2547      +21     
+ Misses       1291     1274      -17     
  Partials        2        2              

Impacted Files	Coverage Δ
src/book.cpp	93.10% <85.36%> (-1.79%)	⬇️
include/book.h	96.15% <100.00%> (+2.82%)	⬆️
src/opds_dumper.cpp	100.00% <100.00%> (ø)
src/tools/archiveTools.cpp	88.40% <0.00%> (-1.45%)	⬇️
src/tools/base64.cpp	52.54% <0.00%> (+33.89%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef1ad4b...4a01081. Read the comment docs.

[mgautierfr]

This goes in the right direction but it is not enough :

• "Default" book should have no illustration.
• A book/zim may have no default illustration or even no illustration at all. The spec says we must create a zim file with a illustration but nothing really prevent someone to create a zim not following the spec (And it is what we done in old zim files)

I would move to :

• Use Illustrations vector all the time. (const Illustration).
• Create illustration only when we are sure we have one.
• I'm not sure we need a getDefaultIllustration at all.

And we must make evolved the library xml format to support multiple illustrations (which is not a easy part as we also need to keep (forward at least) compatibility)

[veloman-yunkan]

• "Default" book should have no illustration.

@mgautierfr This contradicts Book's (old) C++ API - there is a getFavicon() method and no hasFavicon() method. Should we drop all 3 Book::getFavicon*() functions? Or throw an exception if they are called on a Book that doesn't have any illustrations?

[mgautierfr]

Yes, the getFavicon() is returning an empty string if there is no Favicon. This is a wrong API and we should drop it.
This feature will probably come in a major release, it is ok to break the (C++) api. What we don't wont is to break the web/opds api as user may use a old client with a up to date kiwix-serve used at library.kiwix.org

However, in the same time, we should keep the android API (at least until we have moved the wrapper in https://github.com/kiwix/java-libkiwix and have a proper wrapper there).

On my side it is ok to drop the Book::getFavicon*() methods from public c++ api. And either port the wrapper to the new methods or keep getFavicon() as internal helper.

[veloman-yunkan]

I would move to :

• Use Illustrations vector all the time. (const Illustration).

• Create illustration only when we are sure we have one.

• I'm not sure we need a getDefaultIllustration at all.

Done. getDefaultIllustration() was preserved in order to avoid dealing with the removal of get/setFavicon*() API at this point. The only change with respect to that deprecated API is that if no 48x48 illustration is available in the Book then the getFavicon*() functions will throw.

[veloman-yunkan]

The only remaining arguable point seems to be whether getDefaultIllustration() should throw. Also, the history of this PR has to be cleaned up before it is merged.

[mgautierfr]

I've got one last issue. I think it would be better to handle it now but I'm ok if you think we should not.

Else we are good.

[mgautierfr]

I've got a segmentation fault when I run kiwix-desktop compiled with this PR.
@veloman-yunkan, can you do the test and confirm this ?

[veloman-yunkan]

I've got a segmentation fault when I run kiwix-desktop compiled with this PR. @veloman-yunkan, can you do the test and confirm this ?

@mgautierfr No segfault with my native_dyn build (I don't have a native_static build for kiwix-desktop). Does kiwix-desktop crash on start-up?

[mgautierfr]

Does kiwix-desktop crash on start-up?

Some time (I'm not sure now).
But it crashes when it tries to parse opds stream (when you click on "All files" in the library view)

[veloman-yunkan]

But it crashes when it tries to parse opds stream (when you click on "All files" in the library view)

@mgautierfr My build doesn't crash

[mgautierfr]

I've rebuild all the projects and it doesn't crash now. Probably some kind of ABI change conflict with a .o not rebuild.

[veloman-yunkan]

The PR is now cleaned-up and rebased.

[mgautierfr]

There is still few things I've found.
The m_illustrations.clear() is pretty easy to add so it would be nice to have it right now.

The race condition may be addressed now or with the other potential race condition in Book::getDefaultIllustration