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

Extend libkiwix structures to be contructed/updated from libzim structures #576

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

maneeshpm
Copy link
Contributor

Fixes #430
This PR depends upon #536

This is the second stage to remove usage of wrapper structures from libkwix. In this step, we will extend the libkiwix structures to use libzim structures directly. This will still include the wrapper structures in the API but their usage will be derived from that of libzim structures.

Changes included in this PR:

  • Modify kiwix::Book to be updated from a zim::Archive
  • Modify kiwix::Manager to import a zim file from a zim::Archive
    Add overloads for libzim structure in kiwix::Book

@maneeshpm
Copy link
Contributor Author

After the changes of the last PR and this PR, the use of kiwix::Entry is also dropped. It is only being used inside kiwix::Reader and kiwix::Searcher (excluding JNI). Hence no change is needed there.

@mgautierfr I guess we have covered all the wrapper structures now excluding JNI. Is there anything I am missing?

@maneeshpm
Copy link
Contributor Author

This PR depends upon openzim/libzim#582

Base automatically changed from internally_drop_reader_searcher to master July 6, 2021 14:18
@kelson42
Copy link
Collaborator

kelson42 commented Jul 6, 2021

I guess this PR would benefit of a rebase

@maneeshpm maneeshpm force-pushed the extend_libkiwix_structures_to_use_libzim branch from 280262a to 7f46fdd Compare July 6, 2021 16:42
@maneeshpm maneeshpm changed the title [WIP] Extend libkiwix structures to be contructed/updated from libzim structures Extend libkiwix structures to be contructed/updated from libzim structures Jul 6, 2021
@maneeshpm
Copy link
Contributor Author

@kelson42 done!

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things to change.

Please rebase (again) on master to make the CI download the correct dependencies archive.
Remove the draft status on the PR (if accurate) (remove WIP from the title is not enough)

include/book.h Outdated Show resolved Hide resolved
src/tools/archiveTools.cpp Outdated Show resolved Hide resolved
src/tools/archiveTools.cpp Outdated Show resolved Hide resolved
@maneeshpm maneeshpm force-pushed the extend_libkiwix_structures_to_use_libzim branch from 7f46fdd to 8eeef00 Compare July 7, 2021 18:18
@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #576 (19afe94) into master (b4f7dfa) will decrease coverage by 0.38%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #576      +/-   ##
==========================================
- Coverage   65.17%   64.79%   -0.39%     
==========================================
  Files          53       53              
  Lines        3765     3730      -35     
  Branches     1886     1856      -30     
==========================================
- Hits         2454     2417      -37     
- Misses       1309     1311       +2     
  Partials        2        2              
Impacted Files Coverage Δ
include/book.h 93.33% <ø> (ø)
include/reader.h 88.88% <ø> (ø)
src/reader.cpp 28.49% <20.00%> (-15.46%) ⬇️
src/book.cpp 94.89% <94.11%> (-1.46%) ⬇️
src/tools/archiveTools.cpp 89.85% <96.29%> (+2.09%) ⬆️
src/manager.cpp 77.98% <100.00%> (ø)

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 b4f7dfa...19afe94. Read the comment docs.

@maneeshpm maneeshpm marked this pull request as ready for review July 7, 2021 18:29
@maneeshpm maneeshpm requested a review from mgautierfr July 7, 2021 18:29
@kelson42
Copy link
Collaborator

kelson42 commented Jul 9, 2021

@maneeshpm Does it fix #166. If not, updating #166 with what is left to do would be helpful.

@maneeshpm maneeshpm mentioned this pull request Jul 9, 2021
@kelson42 kelson42 force-pushed the extend_libkiwix_structures_to_use_libzim branch from 8eeef00 to c1ede74 Compare July 13, 2021 12:31
@stale
Copy link

stale bot commented Jul 20, 2021

This pull request 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 Jul 20, 2021
@kelson42
Copy link
Collaborator

@mgautierfr Can we merge this?

@stale stale bot removed the stale label Jul 29, 2021
@maneeshpm maneeshpm force-pushed the extend_libkiwix_structures_to_use_libzim branch from c1ede74 to 1219b50 Compare July 29, 2021 06:08
@maneeshpm
Copy link
Contributor Author

@kelson42 rebase fixup done.

Some methods in kiwix::Book uses wrapper structure reader. This usage should
be extended from the native libzim structure zim::Archive
kiwix::Manager uses Reader to import a zim file, it should be using
zim::Archive directly.
@mgautierfr mgautierfr force-pushed the extend_libkiwix_structures_to_use_libzim branch from 1219b50 to 19afe94 Compare August 3, 2021 09:43
@mgautierfr mgautierfr merged commit a032d65 into master Aug 3, 2021
@mgautierfr mgautierfr deleted the extend_libkiwix_structures_to_use_libzim branch August 3, 2021 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop libzim wrapper
3 participants