Xapian headers are not exposed through libkiwix

[veloman-yunkan]

Fixes #469

[codecov]

Codecov Report

Merging #470 (2b3413d) into master (803cb1c) will decrease coverage by 0.03%.
The diff coverage is 71.42%.

❗ Current head 2b3413d differs from pull request most recent head aa2a031. Consider uploading reports for the commit aa2a031 to get more accurate results

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
- Coverage   63.07%   63.03%   -0.04%     
==========================================
  Files          50       50              
  Lines        3496     3501       +5     
  Branches     1776     1767       -9     
==========================================
+ Hits         2205     2207       +2     
- Misses       1289     1292       +3     
  Partials        2        2              

Impacted Files	Coverage Δ
include/library.h	80.00% <ø> (-20.00%)	⬇️
src/library.cpp	67.27% <71.42%> (-0.22%)	⬇️

---

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 803cb1c...aa2a031. Read the comment docs.

[kelson42]

@veloman-yunkan Considering that this PR seems really trivial, I consider to just merge it. Hopefully this is right.

[kelson42]

@veloman-yunkan Kiwix Desktop git master seems to fail to compile against this:

src/contentmanager.cpp: In member function ‘void ContentManager::updateRemoteLibrary(const QString&)’:
src/contentmanager.cpp:360:38: error: use of deleted function ‘void kiwix::Library::operator=(const kiwix::Library&)’
  360 |     m_remoteLibrary = kiwix::Library();
      |                                      ^
In file included from src/library.h:5,
                 from src/contentmanager.h:6,
                 from src/contentmanager.cpp:1:
/usr/local/include/kiwix/library.h:138:8: note: declared here
  138 |   void operator=(const Library& ) = delete;
      |        ^~~~~~~~
make: *** [Makefile:1486: contentmanager.o] Error 1

[veloman-yunkan]

@kelson42 This particular case can be easily fixed by adding a new method clear() to kiwix::Library, but the question is - will we have to update the Java wrapper interface? (If yes, then the fix becomes comparable in complexity to the next option).

Making kiwix::Library copiable is also possible but requires slightly more work (or adds some maintenance burden).

Finally, it can be fixed on the kiwix-desktop side by changing the type of m_remoteLibrary from kiwix::Library to std::unique_ptr<kiwix::Library>

[kelson42]

@veloman-yunkan Thx for the analysis, I will let you discuss these solutions with @mgautierfr and I will abort my project to merge this PR for the moment :)

[mgautierfr]

We probably need to declare a move assignment operator/constructor (using default).

[veloman-yunkan]

We probably need to declare a move assignment operator/constructor (using default).

I don't think that it is good/intuitive semantics for the kiwix::Library class. It is uglier than the third option:

Finally, it can be fixed on the kiwix-desktop side by changing the type of m_remoteLibrary from kiwix::Library to std::unique_ptr<kiwix::Library>

If we hit more cases when kiwix::Library needs to be copiable, we will decide how to fix that in the scope of kiwix::Library.

[mgautierfr]

I don't think that it is good/intuitive semantics for the kiwix::Library class. It is uglier than the third option:

Why ? This is totally valid to move a kiwix::Library instance.
This is what is needed when we do m_remoteLibrary = kiwix::Library();

There is a default (implicit) move constructor/assignment implemented when possible. But if we explicitly delete the copy constructor, then the default move constructor is not implicitly implemented. We need to explicitly "ask" it again with Library& Library(Library&& ) = default;

---

Implementing a move semantics doesn't invalidate the other options.
Having a clear() method or using a unique_ptr in the desktop are also valid options and we can (should?) implement all of them.
But forcing the use of smart pointer on the user side is a important decision in a api.

---

[...] intuitive semantics [...]

Most of the time the intuitive semantics is that we can copy/move a object.
This is how all base types are working. A "naive" user will expect the same behavior.

In complex type, copying may not be a easy step and this can be disputable.
On kiwix::Library, I agree that copying introduce a semantic not trivial to design (is the xapiandb some kind of cache ? Do we share books and reader with copies of libraries ? ...)
But moving is "easy". The only use case I see we don't want to move is when is it complicated (or impossible) to implement (internal reference to the object, ...)

[veloman-yunkan]

@mgautierfr I don't agree with your justification for making kiwix::Library moveable, however I don't think that the issue deserves further discussion. Let's see when and if this hack will be reverted.

[veloman-yunkan]

@mgautierfr I don't agree with your justification for making kiwix::Library moveable, however I don't think that the issue deserves further discussion. Let's see when and if this hack will be reverted.

I somehow missed that in general copyability and movability are not mutually exclusive. A non-copyable class with a move constructor and move assignment can be made copyable later without removing the move functionality (the latter then becomes just an optimization for r-value sources).

[kelson42]

As far as possible the libkiwix complexity should be ar the lowest level possible and its usage should be as flexible as possible. I understand the libkiwix API has to evolve to be better and offer more functionalities. Therefore this is OK to some extend to brake the compatibility if necessary, but then:

• It should be clearly for the better.
• It should be clearly explained and libkiwix users should be properly informed.
• Íf possible the code in the client (JNI/Kiwix-Desktop) should be changed as well (for the moment Kiwix Desktop code does not compile since 2 weeks against libkiwix git master HEAD)
• Kiwix Desktop should be tested if API changes... it is not good that me or any other discover that things have been broken later one.

[mgautierfr]

however I don't think that the issue deserves further discussion

It is a pity. The goal of all those discussion is to agree on a current solution "the best we can do for the moment".
"For the moment" doesn't means we don't care about the future. If we can do a change that will last long it is better.

Agree on which solution is the best now and avoid any API change later should globally be our goal (I personally don't want to go back on this and change all the use of kiwix::Library in few months.

If you have arguments to not implement a solution I prefer to discuss it now instead of having a "I told you so" later.

I somehow missed that in general copyability and movability are not mutually exclusive. A non-copyable class with a move constructor and move assignment can be made copyable later without removing the move functionality (the latter then becomes just an optimization for r-value sources).

Yes. I speaking only about the move semantic. All internal objects in kiwix::Library are easily movable I don't see why we should forbid a move of the library.

for the moment Kiwix Desktop code does not compile since 2 weeks against libkiwix git master HEAD)

The kiwix-build nigthly pass.
I've just realize the appveyor nightly is broken since a long time (I don't know why I have any mail on this) but it is not related.
It is the signing step failing because of some kindo of invalid ntp server.

Kiwix Desktop should be tested if API changes... it is not good that me or any other discover that things have been broken later one

I agree.

[veloman-yunkan]

If you have arguments to not implement a solution I prefer to discuss it now instead of having a "I told you so" later.

This is certainly not the case now. I never implied that discussions of important matters should end in such a way. My comment only reflected my opinion that this issue was starting to generate more discussion than it deserved.

[mgautierfr]

kiwix-desktop doesn't compile with this version.

We need to declare the move constructor/operator in the header but define it (using =default in the cpp).

Else, the default implementation tries to use the unique_ptr destructor which don't know the size of BookDB (at the time it is implemented).

---

On top of that, I recently realize we don't need to declare a intermediate object (BookDB).
We can do a forward declaration of Xapian::WritableDatabase and directly use it with a unique_ptr.
I tend to use a intermediate struct where it was unnecessary.

But I will not block for that, especially as BookDb also hides the writableDatabase constructor arguments.

[veloman-yunkan]

kiwix-desktop doesn't compile with this version.

We need to declare the move constructor/operator in the header but define it (using =default in the cpp).

Else, the default implementation tries to use the unique_ptr destructor which don't know the size of BookDB (at the time it is implemented).

I missed that. Fixed.

[mgautierfr]

We are good.
Please rebase-fixup before the merge.

[veloman-yunkan]

Done