Replace multiple comma-separated languages with 'mul' #1181

Optimus-NP · 2025-02-22T19:02:48Z

Fixes #724

Refactored language code handling to replace multiple comma-separated values with 'mul'.
Single-language entries remain unchanged.
Improved consistency in language representation.

Testing

/usr/local/bin/kiwix-serve --port=8080 --library /tmp/zimlibrary/linux_library.xml

Used this command to start the server locally. I ensured that the ZIM library have at least one book with multiple language codes.
Please check the image below to verify that it's working fine.

kelson42 · 2025-02-23T18:11:04Z

Thank you for your PR... We will have a look to it.

That said, it's a mystery why the Wikipedia in Bambara is tagged as containing English content!

Optimus-NP · 2025-02-24T07:13:33Z

Thank you for your PR... We will have a look to it.

Great, thanks so much.

That said, it's a mystery why the Wikipedia in Bambara is tagged as containing English content!

In my example test case, I linked the Wikipedia in Bambara to both eng and bam language codes. And yes, it is mystery the way ted zim files show up on the website: Kiwix Library NoJX endpoint

veloman-yunkan

For full consistency across the jsful and nojs versions you should also add a tooltip (via the title attribute of the <div> tag) that shows the languages names on hover.

veloman-yunkan · 2025-02-26T09:42:21Z

src/html_dumper.cpp

@@ -72,7 +72,7 @@ std::string HTMLDumper::dumpPlainHTML(kiwix::Filter filter) const
      contentId = urlEncode(nameMapper->getNameForId(bookId));
    } catch (...) {}
    const auto bookDescription = bookObj.getDescription();
-    const auto langCode = bookObj.getCommaSeparatedLanguages();
+    const auto langCode = (bookObj.getCommaSeparatedLanguages().find(',') != std::string::npos) ? "mul" : bookObj.getCommaSeparatedLanguages();


Long line. Please try to abide by the Thiruvananthapuram Convention on coding guidelines.

Thanks for the feedback, comment addressed.

Optimus-NP · 2025-02-27T16:26:52Z

For full consistency across the jsful and nojs versions you should also add a tooltip (via the title attribute of the <div> tag) that shows the languages names on hover.

Thanks for the suggestion, incorporated it in the code.

veloman-yunkan · 2025-03-03T09:23:57Z

src/html_dumper.cpp

+    const auto langList = bookObj.getCommaSeparatedLanguages();
+    const auto langCodes = kiwix::split(langList, ",", true, false);
+    std::string langCode = "";
+    std::string languageSelfName = "Undetermind";


In order to avoid issues with translation, better initialize this variable to "???"

The singular form used in the name of the variable can be misleading. I would rename it to bookLanguages.

Thank you, addressed the comment.

veloman-yunkan · 2025-03-03T10:01:55Z

src/html_dumper.cpp

    const auto bookIconUrl = rootLocation + "/catalog/v2/illustration/" + bookId +  "/?size=48";
    const auto tags = bookObj.getTags();
    const auto downloadAvailable = (bookObj.getUrl() != "");
    std::string faviconAttr = "style=background-image:url(" + bookIconUrl + ")";
+    std::string languageAttr = "title=" + languageSelfName + " aria-label=" + languageSelfName;


The values of HTML attributes should be quoted. This is best solved by updating static/templates/no_js_library_page.html - instead of passing languageAttr, the title and aria-label attributes should appear explicitly in the template and their value should be passed via a differently named parameter (as proposed in the other comment).

That's a great feedback, addressed.

veloman-yunkan · 2025-03-03T10:02:50Z

src/html_dumper.cpp

-    const auto langCode = bookObj.getCommaSeparatedLanguages();
+    const auto langList = bookObj.getCommaSeparatedLanguages();
+    const auto langCodes = kiwix::split(langList, ",", true, false);
+    std::string langCode = "";
+    std::string languageSelfName = "Undetermind";
+
+    if (langCodes.size() > 1) {
+      std::vector<std::string> mulLanguages;
+      langCode = "mul";
+      for (const auto& lang : langCodes) {
+        mulLanguages.push_back(getLanguageSelfName(lang));
+      }
+      languageSelfName = kiwix::join(mulLanguages, ",");
+    } else if (langCodes.size() == 1) {
+      langCode = langCodes[0];
+      languageSelfName = getLanguageSelfName(langCode);
+    }
+    languageSelfName = kiwix::toTitle(languageSelfName);


The function containing this change was already longer than typical human short-term memory can accommodate and this change makes the situation worse. Please encapsulate this enhancement in a couple of helper functions makeLanguagesShortString() and makeLanguagesFullString() so that the relevant code in dumpPlainHTML() reads (BTW, note the use of Book::getLanguages() instead of Book::getCommaSeparatedLanguages()):

const auto bookLangs = book.getLanguages(); booksData.push_back(kainjow::mustache::object{ ... {"langShortString", makeLanguagesShortString(bookLangs)}, {"langFullString", makeLanguagesFullString(bookLangs)}, ... });

Note, that the switch from langCode to langShortString suggests a further UX enhancement (not for this PR, though, since it has to be discussed first) - if the list of languages is limited to two (or, maybe, even three) languages, all of their codes may be shown instead of being replaced with mul.

Indeed, adding the helper function is much cleaner.

veloman-yunkan · 2025-03-03T10:03:44Z

src/html_dumper.cpp

+      langCode = langCodes[0];
+      languageSelfName = getLanguageSelfName(langCode);
+    }
+    languageSelfName = kiwix::toTitle(languageSelfName);


Shouldn't toTitle() be applied to each individual language?

In the attached demo, I have already showcased an example using multiple languages, which is displaying as expected. However, for consistency, I have called the Kiwix Title API for each individual language.

kelson42 · 2025-03-04T04:04:43Z

Sorry for the late feedback, but from the user perspective, it should work exactly like on the js homepage https://library.kiwix.org/#q=&category=ted

Optimus-NP · 2025-03-05T18:15:31Z

Sorry for the late feedback, but from the user perspective, it should work exactly like on the js homepage https://library.kiwix.org/#q=&category=ted

Thanks for the review.

veloman-yunkan

Please squash your changes into a single commit

Optimus-NP · 2025-03-06T15:58:29Z

Please squash your changes into a single commit

Addressed this comment.

Optimus-NP · 2025-03-06T15:59:45Z

@kelson42 Requesting approval on the PR.

veloman-yunkan

LGTM. Ok to merge.

Optimus-NP · 2025-03-06T18:01:28Z

@kelson42 Can you please help me understand this failure ?
And if it is safe to merge please help with the needful.

kelson42 · 2025-03-06T18:02:11Z

Many things in the CI does not pass (and this is not only packaging). I can not merge like this.

Optimus-NP · 2025-03-06T18:08:59Z

Many things in the CI does not pass (and this is not only packaging). I can not merge like this.

Thank you, I will look into the non packaging failure.

- Refactored language code handling to replace multiple comma-separated values with 'mul'. - Single-language entries remain unchanged. - created the helper function to get the lang tag - ensured the consistent behaviour for js and nojs version for the kiwix library view

Optimus-NP · 2025-03-08T19:42:17Z

Many things in the CI does not pass (and this is not only packaging). I can not merge like this.

I figured out nojs test were failing in the library server test. Now I have fixed them.

Optimus-NP · 2025-03-08T19:42:51Z

@kelson42 @veloman-yunkan Requesting approval on the PR.

kelson42 · 2025-03-09T11:26:31Z

@veloman-yunkan This is still failing, I guess we face something which might be unrelated to this PR?

Optimus-NP · 2025-03-09T18:36:55Z

@veloman-yunkan This is still failing, I guess we face something which might be unrelated to this PR?

Thank you for approving and running these workflows. I looked into the testcase failure on the link below is the diff observed

With diff:
@@ -100,5 +100,5 @@
             </a>
             <div class=\"book__meta\">
-              <div class=\"book__languageTag\" title=\"\" aria-label=\"\">fra</div>
+              <div class=\"book__languageTag\" title=\"Fran\xC3\xA7" "ais\" aria-label=\"Fran\xC3\xA7" "ais\">fra</div>
               <div class=\"book_tags\"><div class=\"book_tags--wrapper\">
                   <span class=\"tag__link\" aria-label='unittest' title='unittest'>unittest</span>
@@ -125,5 +125,5 @@
             </a>
             <div class=\"book__meta\">
-              <div class=\"book__languageTag\" title=\"\" aria-label=\"\">eng</div>
+              <div class=\"book__languageTag\" title=\"English\" aria-label=\"English\">eng</div>
               <div class=\"book_tags\"><div class=\"book_tags--wrapper\">
                   <span class=\"tag__link\" aria-label='public_tag_without_a_value' title='public_tag_without_a_value'>public_tag_without_a_value</span>
@@ -150,5 +150,5 @@
             </a>
             <div class=\"book__meta\">
-              <div class=\"book__languageTag\" title=\",\" aria-label=\",\">mul</div>
+              <div class=\"book__languageTag\" title=\"\xD0\xA0\xD1\x83\xD1\x81\xD1\x81\xD0\xBA\xD0\xB8\xD0\xB9,English\" aria-label=\"\xD0\xA0\xD1\x83\xD1\x81\xD1\x81\xD0\xBA\xD0\xB8\xD0\xB9,English\">mul</div>
               <div class=\"book_tags\"><div class=\"book_tags--wrapper\">
                   <span class=\"tag__link\" aria-label='public_tag_with_a_value:value_of_a_public_tag' title='public_tag_with_a_value:value_of_a_public_tag'>public_tag_with_a_value:value_of_a_public_tag</span>
@@ -169,3 +169,3 @@
     </body>
 </html>
-"

If I look at the difference, it seems that the full language name is missing, indicating that the getLanguageSelfName function returned an empty response. As you mentioned, this issue doesn’t appear to be caused by my PR.

@kelson42, could we proceed with merging this while we track the failure separately in a new GitHub issue?

kelson42 requested review from kelson42 and veloman-yunkan February 23, 2025 18:12

veloman-yunkan requested changes Feb 26, 2025

View reviewed changes

Optimus-NP requested a review from veloman-yunkan March 1, 2025 18:46

veloman-yunkan requested changes Mar 3, 2025

View reviewed changes

Optimus-NP requested a review from veloman-yunkan March 5, 2025 18:15

veloman-yunkan approved these changes Mar 6, 2025

View reviewed changes

veloman-yunkan requested changes Mar 6, 2025

View reviewed changes

Optimus-NP force-pushed the kiwix-tools_issues_724 branch from f12e219 to da16aea Compare March 6, 2025 15:57

Optimus-NP requested a review from veloman-yunkan March 6, 2025 15:58

veloman-yunkan approved these changes Mar 6, 2025

View reviewed changes

Optimus-NP force-pushed the kiwix-tools_issues_724 branch from da16aea to 7e80399 Compare March 8, 2025 18:17

Optimus-NP force-pushed the kiwix-tools_issues_724 branch from 7e80399 to fe1d3d9 Compare March 8, 2025 19:37

Optimus-NP requested a review from veloman-yunkan March 8, 2025 19:40

veloman-yunkan approved these changes Mar 9, 2025

View reviewed changes

Optimus-NP requested a review from veloman-yunkan March 9, 2025 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace multiple comma-separated languages with 'mul' #1181

Replace multiple comma-separated languages with 'mul' #1181

Optimus-NP commented Feb 22, 2025

kelson42 commented Feb 23, 2025

Optimus-NP commented Feb 24, 2025

veloman-yunkan left a comment

veloman-yunkan Feb 26, 2025

Optimus-NP Feb 27, 2025

Optimus-NP commented Feb 27, 2025

veloman-yunkan Mar 3, 2025

Optimus-NP Mar 5, 2025

veloman-yunkan Mar 3, 2025

Optimus-NP Mar 5, 2025

veloman-yunkan Mar 3, 2025

Optimus-NP Mar 5, 2025

veloman-yunkan Mar 3, 2025

Optimus-NP Mar 5, 2025

kelson42 commented Mar 4, 2025

Optimus-NP commented Mar 5, 2025

veloman-yunkan left a comment

Optimus-NP commented Mar 6, 2025

Optimus-NP commented Mar 6, 2025

veloman-yunkan left a comment

Optimus-NP commented Mar 6, 2025

kelson42 commented Mar 6, 2025

Optimus-NP commented Mar 6, 2025

Optimus-NP commented Mar 8, 2025

Optimus-NP commented Mar 8, 2025

kelson42 commented Mar 9, 2025

Optimus-NP commented Mar 9, 2025

Replace multiple comma-separated languages with 'mul' #1181

Are you sure you want to change the base?

Replace multiple comma-separated languages with 'mul' #1181

Conversation

Optimus-NP commented Feb 22, 2025

Testing

kelson42 commented Feb 23, 2025

Optimus-NP commented Feb 24, 2025

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Optimus-NP commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 commented Mar 4, 2025

Optimus-NP commented Mar 5, 2025

veloman-yunkan left a comment

Choose a reason for hiding this comment

Optimus-NP commented Mar 6, 2025

Optimus-NP commented Mar 6, 2025

veloman-yunkan left a comment

Choose a reason for hiding this comment

Optimus-NP commented Mar 6, 2025

kelson42 commented Mar 6, 2025

Optimus-NP commented Mar 6, 2025

Optimus-NP commented Mar 8, 2025

Optimus-NP commented Mar 8, 2025

kelson42 commented Mar 9, 2025

Optimus-NP commented Mar 9, 2025