-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Implemented Category Translation #1175
Conversation
6d91f37
to
089b555
Compare
@ShaopengLin I have relaunched the CI, but in case id dies again: CI should be green? |
@kelson42 Yes it should. The failed CI doesn't seem to be up to date with the new i18n.h changes? I will check if its the problem with including paths of my own code. |
089b555
to
c51f4b7
Compare
@ShaopengLin Strongly suspect that |
@ShaopengLin I'm a bit puzzled, for the online library, the list of categories is not retrieved from https://kiwix-tools.readthedocs.io/en/latest/kiwix-serve.html#catalog-v2-categories ? Or do we retrieve from library.kiwix.org but we translated locally? How does that work exactly for both local and remote catalogue? At this stage it seems we have "forgotten" to implement kiwix/libkiwix#844 and we kind of workaround the problem of lack of proper API?! |
@kelson42 I believe its from library.kiwix.org. The recieved categories doesn't seem to be translated as the API doesn't take translated categories as query parameters. We are translating the recieved categpries manually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShaopengLin Root cause of the broken CI is broken PR in kiwix-build. @mgautierfr Please fix ASAP
For the rest the situation is clear now:
- For local library, we anyway only display the list of categories based on available ZIM in the local library. These ones are now translated via libkiwix (instead of directly in kiwix-desktop)
- For remote library, we download (said @ShaopengLin) the list of categories and we translate locally via libkiwix. @ShaopengLin Hopefully this piece of code can handle the fact that it can mismatch (not exactly the same list), in that case the concerned category should be displayed in English.
So this PR is really only about deporting the category translation from kiwix-desktop to libkiwix. Once kiwix/libkiwix#844 implemented, we should stop to do that for remote library. I have open an issue requesting this #1181
Otherwise LGTM, ready for code review
This is fixed |
c51f4b7
to
9c0ad5a
Compare
@ShaopengLin Can you please rebase and fix conflict? |
9c0ad5a
to
674c15d
Compare
@veloman-yunkan Are we finally ready to merge? |
a900048
to
6e95e7e
Compare
@veloman-yunkan @kelson42 I added changes that address #1185. From my testing, when the user upgrades, they will see the categories from old session files as simply the untranslated state. Once they de-select those and restart the application, those choices will be gone. |
6e95e7e
to
996031d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Note however that I didn't test the changes.
setSelection parses two SelectionList differently
Public interfaces uses this typedef.
Storing filter pairs is redundant.
Translations come from libkiwix
996031d
to
5f1feeb
Compare
@ShaopengLin Would that be easy to just remove the filter in such situation, so user starts again on a fresh foot? |
@kelson42 we can do that, though it will involve back porting code that ran only once. @veloman-yunkan what do you think? |
@ShaopengLin Let me merge this and I will test it first. This PR is too old already... |
I don't think I correctly understand what you mean. After reading the category filter list from saved settings, we can simply drop entries containing the |
I was asking if we should do this porting as it will only be ran once. I can do a quick PR to do this then. |
Fix #972
Changes: