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

Translations from iso codes #347 #445

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

quantumchaos451
Copy link

Issue: #347

What works:

  • This works on Linux. I don't have access to a Windows or a Mac machine to test it on those.
  • All of the languages in the Bookshelf Manager appear to be correctly set.
  • If I run Bibletime with language say set to LANG=fr_FR then those that can be translated into French are in the Bookshelf Manager.

Potential issues with this PR:

@jaakristioja
Copy link
Member

This looks very promising. I want this feature in BibleTime and I very much appreciate what you've done. I really hope we can work together to get this merged. But I must warn you that I've become rather pedantic when it comes to code quality, so I ask for your forgiveness if I'm too rough on the code. Please don't take anything I say as a personal insult. 🙏🏻

I've only started to review the changes, but was somewhat taken back by the number of commits and the difficulty of reviewing them individually. This was also in part due to the multiple merge commits in the git history. In BibleTime we try to keep the git history as linear as possible, so that it were easier to understand when we need to look at it to understand the code or the rationale behind some changes. Hence I discovered that the diff between the current master branch and your branch looks much smaller and much more understandable than going through the commits one-by-one. I suggest to squash everything into one commit and rebase your changes on the current master. This can be done by using something like this:

  1. check out the respective git branch with your changes (probably using something like git checkout translations-from-iso-codes)
  2. Run git diff origin/master...HEAD > changes.diff to create a patch file with all actual changes
  3. Hard-reset the feature branch to origin/master by running git reset --hard origin/master ⚠️ POTENTIALLY DESTRUCTIVE Please understand what the command does before applying it, see git help reset.
  4. Re-apply the changes using patch -p1 < changes.diff
  5. Re-index the changes: git add cmake/BTApplication.cmake cmake/FindIsoCodes.cmake iso_codes_info.c.in iso_codes_info.h src/backend/language.cpp
  6. Review and commit the changes
  7. Force-push the new commit to quantumchaos451:translations-from-iso-codes

This might also make it easier to review using built-in GitHub core review functionality.

Copy link
Member

@jaakristioja jaakristioja left a comment

Choose a reason for hiding this comment

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

Did a preliminary review of the changes as a whole and not the individual commits. Since this seems to be a fairly small feature, it can probably be reduced one or a few commits.

src/backend/language.cpp Outdated Show resolved Hide resolved
iso_codes_info.h Outdated Show resolved Hide resolved
iso_codes_info.c.in Outdated Show resolved Hide resolved
cmake/BTApplication.cmake Outdated Show resolved Hide resolved
cmake/BTApplication.cmake Outdated Show resolved Hide resolved
src/backend/language.cpp Outdated Show resolved Hide resolved
src/backend/language.cpp Outdated Show resolved Hide resolved
src/backend/language.cpp Outdated Show resolved Hide resolved
addLanguage({"xnj"}, QT_TRANSLATE_NOOP("QObject", "Ngoni"));
addLanguage({"yrk-Cyrl"}, QT_TRANSLATE_NOOP("QObject", "Nenets (Cyrillic script)"));
addLanguage({"zh-Hans"}, QT_TRANSLATE_NOOP("QObject", "Chinese, Simplified"));
addLanguage({"zh-Hant"}, QT_TRANSLATE_NOOP("QObject", "Chinese, Traditional"));
Copy link
Member

Choose a reason for hiding this comment

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

A lot of addLanguage() calls for some language codes were removed by these changes, and a lot of new addLanguage() calls were added. What is the exact rationale for this? It might be best to describe this in commit message and code comments as well.

src/backend/language.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants