Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop libzim wrapper #430

Closed
mgautierfr opened this issue Dec 2, 2020 · 18 comments · Fixed by #536, #576 or #789
Closed

Drop libzim wrapper #430

mgautierfr opened this issue Dec 2, 2020 · 18 comments · Fixed by #536, #576 or #789
Assignees
Milestone

Comments

@mgautierfr
Copy link
Member

With the last version of libzim, a lot of things in kiwix-lib become a simple wrapper around the libzim objects.

kiwix::Reader is a wrapper of libzim::Archive.
kiwix::Entry is a wrapper of libzim::Entry and libzim::Item
kiwix::Searcher is a wrapper of libzim::Search

They provide almost nothing more than an intermediate step and for the few real differences, they should probably be moved to libzim itself or are relics due to api compatibility.

It would be better to drop all those wrapping and let user code directly use libzim when needed.
We would have a clear distinction between libzim and kiwix-lib :

  • libzim is about reading a zim archive, getting its content or searching in it.
  • kiwix-lib is about handling zim archive themselves (download them, organise them in a library, ...) and other things (server, bookmark, ...)

Most of the cpp application could be simply being adapted (even without waiting for the wrappers being dropped).
The main question are about the wrapper in another language of kiwix-lib that need to be adapted.

  • macos/ios : The wrapping is managed by @automactic, I suppose it would not be to complicated to change.
  • android : The wrapping is made in kiwix-lib itself. We should probably move it to a separated repository. Then we may have to wrapper for kiwix-lib and libzim or keep one wrapper wrapping the both library. @macgills probably has his word to say here.

What do you think about this ?

(I was pretty sure to already have opened a issue about this but I cannot find it. If you do, feel free to link both issues).

@kelson42
Copy link
Collaborator

kelson42 commented Dec 2, 2020

Sounds a good idea but of course it has to work fine in reality. If good for @mgautierfr and @veloman-yunkan, good for me.

@macgills
Copy link
Contributor

macgills commented Dec 3, 2020

On the Android side a separate repository to wrap probably makes sense. It was done as it is as a build convenience I think? Having both wrapped is convenient for how the app is currently structured but if it makes more sense to separate them it could be good for project health.
Sidenote: Bookmarks are managed through kiwix-lib? That is not the case in android

@stale
Copy link

stale bot commented Feb 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Feb 2, 2021
@kelson42 kelson42 pinned this issue Mar 6, 2021
@stale stale bot removed the stale label Mar 6, 2021
soumyankar added a commit to soumyankar/kiwix-lib that referenced this issue Mar 17, 2021
soumyankar added a commit to soumyankar/kiwix-lib that referenced this issue Mar 23, 2021
soumyankar added a commit to soumyankar/kiwix-lib that referenced this issue Mar 24, 2021
soumyankar added a commit to soumyankar/kiwix-lib that referenced this issue Mar 24, 2021
@kelson42 kelson42 removed the question label Apr 3, 2021
@kelson42 kelson42 added this to the 10.0.0 milestone May 13, 2021
@kelson42
Copy link
Collaborator

@mgautierfr @veloman-yunkan @maneeshpm I believe this is important we do that in libkiwix10. But the strategy to do it and who will do what is unclear to me. Any hint?

@mgautierfr
Copy link
Member Author

The plan I see is the following:

  1. Stop using the wrappers (Reader, Entry, Searcher, ....) in libkiwix itself. This step does not change the libkiwix API. It make libkiwix use libzim structures internally and provide the wrappers as a API only.
  2. Extend the libkiwix API to returns libzim structure (the Library/Book will be able to return(/be constructed from) zim::Archive). This is a API extension, methods using a wrapper class will still be here.
  3. Adapt all projects using libkiwix (kiwix-tools, kiwix-desktop, ...) to remove the use of wrapper structure.
  4. Remove all the wrappers structures as they are not used anymore.

The JNI wrapper case can be handle different ways :

  • Keep it in libkiwix (and make the change in the first step) and then moving the JNI wrapper out of libkiwix.
  • Move it out of libkiwix and handle the change in step three.
  • Create a "new" JNI wrapper, directly wrapping libzim and the "stable" API of libkiwix.

My preference is for the third or second option :)

@kelson42
Copy link
Collaborator

* Move it out of libkiwix and handle the change in step three.

Already done https://github.com/kiwix/java-libkiwix. Just expecting someone moving the code and adapting CI+Kiwix-build to deal with it.

@maneeshpm
Copy link
Contributor

@mgautierfr Are you already working on this one? If not, I can start on this(ofc with a lot of help initially 😅). I feel like we should fix this ticket on priority as it will further help in solving #166 and #509.

@mgautierfr
Copy link
Member Author

You can start the first step yes (excluding jni part).

@maneeshpm
Copy link
Contributor

For now, I have identified the following subtasks in the first step.

  • Dropping usage of wrappers from InternalServer completely
    We can directly mirror the usage of Reader with Archive for most parts. But for others like handle_search, it gets much more interdependent on the subtask given below involving SearchRenderer and other libkiwix structures.

  • SearchRenderer should be able to work with libzim structures
    Currently, it's dependent on kiwix::Searcher to generate HTML. It should be modified(or by adding a new method) to use libzim structures. This will be necessary to fix handle_search.

  • Mirror methods that use Reader/Searcher with one that use libzim structures.
    For example, in the internal server we depend on mp_library to retrieve readers using getReaderForId, we will add mirrors of the form getArchiveById etc.

Once we can make sure that everything works perfectly, we can drop the mirrored functions later and only keep the ones using libzim structures.
@mgautierfr Do you want me to add anything or have any suggestions on this?

@kelson42
Copy link
Collaborator

kelson42 commented Jan 9, 2022

@mgautierfr Neither apple nor Android are managed here (anymore). I don't really see the value of keeping this ticket open here.

@veloman-yunkan
Copy link
Collaborator

@mgautierfr Neither apple nor Android are managed here (anymore). I don't really see the value of keeping this ticket open here.

If we are going to implement what is suggested in this ticket then we will have to change code in libkiwix. Thus we can close this ticket only if we open proposed subtasks as separate tickets.

@kelson42 kelson42 added this to the 10.1.0 milestone Jan 11, 2022
@kelson42 kelson42 changed the title Drop libzim wrapper in kiwix-lib Drop libzim wrapper Feb 10, 2022
@kelson42 kelson42 modified the milestones: 10.1.0, 10.2.0, 10.3.0 Mar 23, 2022
@kelson42
Copy link
Collaborator

Depends on kiwix/kiwix-desktop#805

@kelson42 kelson42 modified the milestones: 10.2.0, 10.3.0 Apr 7, 2022
@kelson42 kelson42 modified the milestones: 10.2.0, 10.3.0 Apr 23, 2022
@kelson42
Copy link
Collaborator

kelson42 commented May 9, 2022

Ticket for Kiwix iOS and macOS kiwix/kiwix-apple#445

@mgautierfr
Copy link
Member Author

Following the issue openzim/python-libzim#139, we have to move convertTags (and associated) function in the public API.

@kelson42
Copy link
Collaborator

@mgautierfr @veloman-yunkan At this stage, and AFAIK, nothing stops us anymore to finally implement this ticket. Right?

@mgautierfr
Copy link
Member Author

We need (at least) to do a release of kiwix-desktop which don't use the wrapper first.

@kelson42
Copy link
Collaborator

@mgautierfr OK, but this is a matters of days, you could make one anytime, milestone is empty https://github.com/kiwix/kiwix-desktop/milestone/7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment