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

Add mappings for languages not given by libicu #701

Merged
merged 3 commits into from
Mar 5, 2022
Merged

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Feb 5, 2022

Fixes #611

There are 7,893 languages under ISO 639-3. I could not find a mapping between lang code and native names (there's this langCode vs english names but we display native names).
Instead, this just adds the languages missed by libicu in library.kiwix.org. If this way is approved, I would like to ask:

  • Should the map be rather moved to C++?

@juuz0 juuz0 requested a review from kelson42 February 5, 2022 17:24
@codecov
Copy link

codecov bot commented Feb 5, 2022

Codecov Report

Merging #701 (4bd02f0) into master (cfab560) will increase coverage by 0.10%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #701      +/-   ##
==========================================
+ Coverage   57.93%   58.03%   +0.10%     
==========================================
  Files          54       54              
  Lines        3573     3584      +11     
  Branches     2010     2019       +9     
==========================================
+ Hits         2070     2080      +10     
- Misses       1502     1503       +1     
  Partials        1        1              
Impacted Files Coverage Δ
src/server/internalServer.cpp 81.06% <ø> (ø)
src/opds_dumper.cpp 98.31% <94.11%> (-0.76%) ⬇️

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 cfab560...4bd02f0. Read the comment docs.

Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@juuz0 From a a user perspective it works as expected. I also appreciate you made an effort to identify the other languages impacted by a similar problem. I'll let @veloman-yunkan review from a code perspective, but I believe this would be more efficient to merge iso6391To3.js and iso639_3.js, this is a bit a waste of resources to have to make two HTTP requests IMO.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

I think you have missed the fact that loadAndDisplayOptions() is used for populating not only the list of languages but also the list of categories. Your (already merged) PR #697 suffers from the same problem (and introduces a potential bug). Please try to take the mentioned fact into account adjust this PR accordingly (as well as identify and fix the potential bug introduced by #697).

@juuz0
Copy link
Collaborator Author

juuz0 commented Feb 6, 2022

I believe this would be more efficient to merge iso6391To3.js and iso639_3.js, this is a bit a waste of resources to have to make two HTTP requests IMO.

Done.

I think you have missed the fact that loadAndDisplayOptions() is used for populating not only the list of languages but also the list of categories...

Correct, done.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Should the map be rather moved to C++?

Since the static resources of kiwix-serve are compiled into the executable rather than hosted separately keeping the list of additional language mappings in js file doesn't seem to have any advantage, since we can't deploy it without building (and releasing) a new version of libkiwix and kiwix-tools. Fixing getLanguageSelfName() in src/opds_dumper.cpp will be a more coherent implementation.

If we are going to merge this PR with the solution implemented in the frontend, then the order of commits should be reversed:

  1. first fix the bug introduced earlier
  2. then make this new enhancement correctly.

IMHO, we better quickly do 1. as a separate PR and quietly decide whether this fix will be in C++ or JS.

@kelson42
Copy link
Collaborator

kelson42 commented Feb 7, 2022

Fixing getLanguageSelfName() in src/opds_dumper.cpp will be a more coherent implementation.

I agree with this comment.

src/opds_dumper.cpp Outdated Show resolved Hide resolved
src/opds_dumper.cpp Outdated Show resolved Hide resolved
src/opds_dumper.cpp Outdated Show resolved Hide resolved
src/opds_dumper.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

The latest changes are fine, but there is still one unresolved comment.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

There is no need to use fixup commit's for this small PR. Please submit the next request for review as a single-commit history.

src/server/internalServer.cpp Outdated Show resolved Hide resolved
@juuz0
Copy link
Collaborator Author

juuz0 commented Feb 25, 2022

Still not ready for a new review (for those unresolved comments), will be done by today night I hope.
Added dagbani language in latest commit

src/opds_dumper.cpp Outdated Show resolved Hide resolved
src/opds_dumper.cpp Outdated Show resolved Hide resolved
Adds a std::map<std::string, std::string> with display names for language codes not given by libicu
Fault language codes are taken from library.kiwix.org
Adds dagbani (dag) language in iso639_3 language map
@kelson42 kelson42 merged commit 833bbc8 into master Mar 5, 2022
@kelson42 kelson42 deleted the ladakhISO branch March 5, 2022 16:07
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.

Ladakhi does not appears properly in the list of (kiwix-serve) language
3 participants