Introduce Manager::addBooksFromDirectory() #1220

juuz0 · 2025-09-06T10:41:10Z

Fixes kiwix/kiwix-tools#739
Pre-requirement for: kiwix/kiwix-tools#743

Added a function to load books from a directory. Requires rootPath to iterate over.

veloman-yunkan

The output of the verbose mode may be somewhat confusing in the context of the full log - the purpose of iterating over files and directories is not obvious. Maybe the output should start (and end) with the description of the operation being performed.

include/manager.h

veloman-yunkan · 2025-09-08T13:15:35Z

src/manager.cpp

+        else if (verboseFlag)
+          std::cout << "Already iterated over " << pathString << ". Skipping..." << std::endl;
+      } else {
+        if (!this->addBookFromPath(pathString, pathString, "", false)) {


Is it wise trying to treat all files (regardless of their extension) as ZIM?

Shouldn't this operation also be reported in the verbose output?

Probably not, and with the recent changes in 1baa142. This would report a std:cerrr for each non ZIM file. I will push a commit for it but wanted to ask if you've tackled this before? Should checking the extensions for .zim, .zimaa, .zimab (and their capitals) be sufficient?

Done in fixup.

Should checking the extensions for .zim, .zimaa, .zimab (and their capitals) be sufficient?

I think that you should go only after .zim and .zimaa files.

@kelson42 Please confirm

100% agree with @veloman-yunkan, this is already what I said in my first review, see kiwix/kiwix-tools#743 (comment)

src/manager.cpp

juuz0 · 2025-09-08T14:49:09Z

The output of the verbose mode may be somewhat confusing in the context of the full log - the purpose of iterating over files and directories is not obvious. Maybe the output should start (and end) with the description of the operation being performed.

I tried improving them some. The caller would know the purpose though, wont they?

veloman-yunkan · 2025-09-08T15:12:57Z

src/manager.cpp

  dirQueue.push(fs::absolute(path).u8string());
+  int totalBooksAdded = 0;
+  if (verboseFlag)
+    std::cout << "Starting BFS traversal at root directory: " << dirQueue.front() << std::endl;


I would prefer if the message reads "Adding books from the directory tree: /path/to/dir"

veloman-yunkan · 2025-09-08T15:18:04Z

src/manager.cpp

+        else if (verboseFlag)
+          std::cout << "Already iterated over " << pathString << ". Skipping..." << std::endl;
+      } else {
+        if (!this->addBookFromPath(pathString, pathString, "", false)) {


Should checking the extensions for .zim, .zimaa, .zimab (and their capitals) be sufficient?

I think that you should go only after .zim and .zimaa files.

@kelson42 Please confirm

kelson42

I wanted to compile and test the code but for me it does not compile:

skin/i18n/tr.json ['skin', 'i18n', 'tr_json']
skin/i18n/zh-hans.json ['skin', 'i18n', 'zh_hans_json']
skin/i18n/zh-hant.json ['skin', 'i18n', 'zh_hant_json']
[14/81] Compiling C++ object src/libkiwix.so.14.0.0.p/manager.cpp.o
FAILED: src/libkiwix.so.14.0.0.p/manager.cpp.o 
c++ -Isrc/libkiwix.so.14.0.0.p -Isrc -I../src -Iinclude -I../include -I/usr/include/kainjow -Istatic -I/usr/local/include -I/usr/include/x86_64-linux-gnu -I/usr/include/p11-kit-1 -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=c++17 -O0 -g -fPIC -pthread -MD -MQ src/libkiwix.so.14.0.0.p/manager.cpp.o -MF src/libkiwix.so.14.0.0.p/manager.cpp.o.d -o src/libkiwix.so.14.0.0.p/manager.cpp.o -c ../src/manager.cpp
../src/manager.cpp: In member function ‘void kiwix::Manager::addBooksFromDirectory(const std::string&, bool)’:
../src/manager.cpp:263:8: error: ‘queue’ is not a member of ‘std’
  263 |   std::queue<std::string> dirQueue;
      |        ^~~~~
../src/manager.cpp:28:1: note: ‘std::queue’ is defined in header ‘<queue>’; this is probably fixable by adding ‘#include <queue>’
   27 | #include <iostream>
  +++ |+#include <queue>
   28 | #include <set>
../src/manager.cpp:263:25: error: expected primary-expression before ‘>’ token
  263 |   std::queue<std::string> dirQueue;
      |                         ^
../src/manager.cpp:263:27: error: ‘dirQueue’ was not declared in this scope
  263 |   std::queue<std::string> dirQueue;
      |                           ^~~~~~~~
[23/81] Compiling C++ object src/libkiwix.so.14.0.0.p/library.cpp.o
ninja: build stopped: subcommand failed.

codecov · 2025-09-26T02:54:05Z

Codecov Report

❌ Patch coverage is 0% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.55%. Comparing base (33f22eb) to head (2dc1989).

Files with missing lines	Patch %	Lines
src/manager.cpp	0.00%	36 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1220      +/-   ##
==========================================
- Coverage   42.87%   42.55%   -0.33%     
==========================================
  Files          60       60              
  Lines        4744     4780      +36     
  Branches     2498     2527      +29     
==========================================
  Hits         2034     2034              
- Misses       1088     1124      +36     
  Partials     1622     1622

☔ View full report in Codecov by Sentry.
📢 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

kelson42 · 2025-09-26T03:31:36Z

@juuz0 Thx, I guess the compilation error is foxed? I will gove a new run on saturday.

juuz0 · 2025-09-26T03:38:28Z

@kelson42 Yes, it should be. I didn't face that and probably veloman either, but from your error logs it showed missing the queue header. I added that explicitly now.

kelson42 · 2025-09-26T03:43:07Z

@kelson42 Yes, it should be. I didn't face that and probably veloman either, but from your error logs it showed missing the queue header. I added that explicitly now.

Yes, I use g++14 on Ubuntu 24.04.

kelson42 · 2025-09-28T14:58:15Z

@juuz0 Thank you for the multiple fixes.

It compiles fine now for me.

At this stage:

I will report user feedback at kiwix-tools level
Please give feedback about each comment so we can close the discussions
Regarding filtering by extension, please add a toLower(), so a file called .ZIM or .ZIMAA (few people have this strange habit to make file extensions uppercase) get a chance to be detected

kelson42 · 2025-10-31T19:41:51Z

@juuz0 Any news? I just rebase briefly.

Added a function to load books from a directory. Requires rootPath to iterate over.

skipInvalid=false makes little sense, if an invalid book is found, we can simply choose to ignore it rather stopping the whole operation midway.

juuz0 mentioned this pull request Sep 6, 2025

Allow directories in kiwix-serve paths kiwix/kiwix-tools#743

Open

juuz0 requested review from kelson42 and veloman-yunkan September 6, 2025 10:46

veloman-yunkan reviewed Sep 8, 2025

View reviewed changes

juuz0 requested a review from veloman-yunkan September 8, 2025 14:49

veloman-yunkan reviewed Sep 8, 2025

View reviewed changes

kelson42 force-pushed the dirScan branch from 1baa142 to 0b61464 Compare September 14, 2025 19:21

kelson42 reviewed Sep 14, 2025

View reviewed changes

juuz0 force-pushed the dirScan branch from 0b61464 to 45d0ec7 Compare September 26, 2025 02:50

juuz0 requested review from kelson42 and veloman-yunkan September 26, 2025 02:50

kelson42 force-pushed the dirScan branch from 45d0ec7 to 194ef68 Compare October 31, 2025 19:41

This comment was marked as off-topic.

Sign in to view

juuz0 added 4 commits November 21, 2025 06:51

Introduce Manager::addBooksFromDirectory()

46e7454

Added a function to load books from a directory. Requires rootPath to iterate over.

fixup! Introduce Manager::addBooksFromDirectory()

a1f271b

Remove skipInvalid flag

ed92c48

skipInvalid=false makes little sense, if an invalid book is found, we can simply choose to ignore it rather stopping the whole operation midway.

fixup! fixup! Introduce Manager::addBooksFromDirectory()

2dc1989

kelson42 force-pushed the dirScan branch from 194ef68 to 2dc1989 Compare November 21, 2025 05:51

Uh oh!

Introduce Manager::addBooksFromDirectory() #1220

Are you sure you want to change the base?

Introduce Manager::addBooksFromDirectory() #1220

Uh oh!

Conversation

juuz0 commented Sep 6, 2025

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

veloman-yunkan Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

juuz0 Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

kelson42 Sep 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juuz0 commented Sep 8, 2025

Uh oh!

veloman-yunkan Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

kelson42 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kelson42 commented Sep 26, 2025

Uh oh!

juuz0 commented Sep 26, 2025

Uh oh!

kelson42 commented Sep 26, 2025

Uh oh!

kelson42 commented Sep 28, 2025

Uh oh!

kelson42 commented Oct 31, 2025

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Sep 26, 2025 •

edited

Loading