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 gettext support to the android version #10019

Closed
wants to merge 3 commits into from

Conversation

oleastre
Copy link
Contributor

@oleastre oleastre commented Jun 9, 2020

Add compact, short information about your PR for easier understanding:

To do

This PR is Ready for Review.

How to test

Build and launch an android client, set the language to something not english in the settings, restart and enjoy a translated mintest.

@MoNTE48
Copy link
Contributor

MoNTE48 commented Jun 9, 2020

Implementation of MultiCraft.
https://github.com/MultiCraft/MultiCraft/tree/master/lib/intl
Very minimal version of the library. It may be more rational to use it on Android, instead of a full (and pervasive) gettext.
It can also be used on other platforms for guaranteed support for locales.

@oleastre
Copy link
Contributor Author

oleastre commented Jun 9, 2020

Note that this PR does not automatically detect the android language.
I could add this in a further PR by adding a JNI binding to fetch the info from the java side of the code.

@MoNTE48
Copy link
Contributor

MoNTE48 commented Jun 9, 2020

I could add this in a further PR by adding a JNI binding to fetch the info from the java side of the code.

What for? JNI can detect language, just read the documentation.

@oleastre
Copy link
Contributor Author

oleastre commented Jun 9, 2020

As stated in the binary part of the PR, I used gettext, as it's already the translation engine for minetest. I also considered https://github.com/tinygettext/tinygettext and https://github.com/j-jorge/libintl-lite, but since the runtime part of gettext is small, self contained, and easy to compile using the NDK, I thought it would be more straight forward to use it to enable translations in the android client.

@oleastre
Copy link
Contributor Author

oleastre commented Jun 9, 2020

What for? JNI can detect language, just read the documentation.

That was the purpose of my comment, you have to add a JNI call to get the current locale.
Basically one need to all Locale.getDefault() in java and forward that to the gettext initialization code.
Or did I miss something in the doc ?

@sfan5 sfan5 added @ Translation Android Feature ✨ PRs that add or enhance a feature labels Jun 9, 2020
@oleastre
Copy link
Contributor Author

oleastre commented Jun 9, 2020

@oleastre yes, miss it:

Thanks, I'm not a full time android developer and most of the time, I only play with the java side and almost never touch the NDK.
So adding that call is quite easy. I'll add to this PR.

exec {
commandLine 'msgfmt', '-o', "${moPath}/minetest.mo", poFile
}
}
Copy link
Contributor

@MoNTE48 MoNTE48 Jun 9, 2020

Choose a reason for hiding this comment

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

There should be a comment that indicates that gradle will not be able to do this on Windows at all, as well as on macOS without brew install gettext!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Is there a place to add this note ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sfan5 your suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Windows can do that as long msgfmt is within the path. All it needs is a comment stating that this section requires gettext to be installed and available in PATH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if I can add a test and break early with a meaningful error message.

build/android/native/jni/Android.mk Outdated Show resolved Hide resolved
src/gui/modalMenu.cpp Show resolved Hide resolved
@oleastre
Copy link
Contributor Author

oleastre commented Jun 9, 2020

I added automatic locale detection if it's not set in the UI or minetest.conf

@oleastre
Copy link
Contributor Author

Rebased on latest master after 5.3.0 release

@SmallJoker
Copy link
Member

I don't have an Android setup, but the code looks good. 👍 if someone else can confirm this as working.

@@ -202,7 +207,12 @@ void init_gettext(const char *path, const std::string &configured_language,
#endif // ifndef _WIN32
}
else {
#ifdef __ANDROID__
char lang[3] = {0};
/* set current system default locale */
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this before #ifdef

@@ -59,6 +59,11 @@ include $(PREBUILT_STATIC_LIBRARY)
#LOCAL_SRC_FILES := deps/Android/OpenSSL/${NDK_TOOLCHAIN_VERSION}/$(APP_ABI)/libcrypto.a
#include $(PREBUILT_STATIC_LIBRARY)

include $(CLEAR_VARS)
LOCAL_MODULE := GetText
Copy link
Contributor

Choose a reason for hiding this comment

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

@MoNTE48
Copy link
Contributor

MoNTE48 commented Oct 20, 2020

I can confirm that this completely works and has been using MultiCraft for a while.
I support this PR!

@oleastre
Copy link
Contributor Author

I'll take time this week to address the few comments, I've left this pending for too much time

@MoNTE48
Copy link
Contributor

MoNTE48 commented Jan 26, 2021

@rubenwardy this really can be merged. Tested and used in my fork

@srifqi
Copy link
Member

srifqi commented Feb 5, 2021

I can confirm that this PR works, too.

@AntumDeluge
Copy link
Contributor

Is there a reason this hasn't been merged yet?

@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author label May 30, 2021
@Pevernow
Copy link
Contributor

Pevernow commented Jul 8, 2021

@oleastre Are you still work in it?
If not,I would like to make a new pull request for it to rebase it.

@oleastre
Copy link
Contributor Author

oleastre commented Jul 8, 2021

@Pevernow I did not updated it for a while, even locally.
If you want to rebase and create a new PR, I'm happy with it; otherwise, I'll have to re-install the full android stack to do it and test against the latest minetest versions

@SmallJoker
Copy link
Member

Rebased/continued in #11435

@SmallJoker SmallJoker closed this Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Feature ✨ PRs that add or enhance a feature Rebase needed The PR needs to be rebased by its author @ Translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants