Client-side translations: gettext and plural support #13687

Ekdohibs · 2023-07-24T16:36:45Z

This is a rebase of #12446.

This PR is complete, see instructions on the previous PR for how to test.

How to test

For now, take a mod using client-side translations, and replace the .tr file with a .po one, with the following entries for each original translated string:

msgctx "<textdomain>"
msgid "<original string>"
msgstr "<translated string>"

rubenwardy · 2024-03-03T00:44:22Z

I will have time to review this soon, so please rebase. I've given it a quick look through, and have two comments:

The parser is in the Translation class, would be nice to have a separate class for this with unit tests. Although, I suppose same for .tr
I wonder if the script is outdated now that we have a util script for extracting .tr

Ekdohibs · 2024-03-07T07:59:00Z

I just rebased it, it should be good now. I'll try to write some unit tests and move the parser to its own class, it might be a bit harder to do for .mo files as they are a binary format.
Concerning the script, I can't seem to find the util script in the minetest/ repository, has it been merged there? The objective of including it was so that it is distributed with mintest itself; it is also updated to support plural translations and .po output. If the script is hosted somewhere else, I can make the PR there, however.

y5nw

Nitpicking.

doc/lua_api.md

src/translation.h

y5nw · 2024-03-07T17:12:27Z

src/translation.cpp

+			line.resize(line.length() - 1);
+
+		if (line.empty() || line[0] == '#')
+			continue;


Since xgettext can generate fuzzy entries it would make sense to check for (and skip) those.

From the documentation of gettext, it seems to say that fuzzy entries are considered as normal entries for most purposes, so I wouldn't drop them. In particular, the .po to .mo compilation would use them, so we should use them as well if we don't want to introduce a discrepancy.

From the documentation of gettext, it seems to say that fuzzy entries are considered as normal entries for most purposes, so I wouldn't drop them.

While that is the case, the problem with fuzzy matching is that it is enabled by default in msgmerge (unless the -N flag is passed) and can often match entries with different (sometimes opposite, e.g. "enabled" and "disabled") meanings while assigning the same translation if one is missing, and there are also reasons to generate fuzzy entries, e.g. if the source string does not change much (adding a period to the end of the source string) or to give translators a base string to modify/work with.

In particular, the .po to .mo compilation would use them, so we should use them as well if we don't want to introduce a discrepancy.

msgfmt does not seem to include fuzzy entries by default (there is the -f flag for including them which GNU does not recommend using).

Thanks for the documentation! I'll implement matching the fuzzy entries in that case.

doc/lua_api.md

rubenwardy · 2024-03-08T20:47:44Z

Concerning the script, I can't seem to find the util script in the minetest/ repository, has it been merged there? The objective of including it was so that it is distributed with mintest itself; it is also updated to support plural translations and .po output. If the script is hosted somewhere else, I can make the PR there, however.

I was referring to https://github.com/minetest/minetest/blob/master/util/mod_translation_updater.py

Makes sense for someone to update that in another PR though

rubenwardy · 2024-03-08T21:01:46Z

games/devtest/mods/testtranslations/init.lua

+	description = "Test translations",
+	privs = {},
+	func = function(name, param)
+		minetest.chat_send_player(name, "Please ensure your locale is set to \"fr\"")


Suggested change

minetest.chat_send_player(name, "Please ensure your locale is set to \"fr\"")

if minetest.get_player_information(name).lang_code ~= "fr" then

return false, "Please restart Minetest with language = fr"

end

Is there a reason why you resolved the comment without applying the suggestion? Please reply if you don't like a suggestion

I applied the suggestion, but I must have forgotten to pull before rebasing, deleting the commit. I'll commit it again, thanks for catching it!
Although, now that I'm thinking about it, we really want to test this both with fr and default locale (to see both untranslated and translated strings), I'll need to check what lang_code is in that case.

src/translation.cpp

Ekdohibs · 2024-03-17T10:28:10Z

Hmmmm, I'm not why the CI fails, do you have an idea?

rubenwardy · 2024-03-08T21:02:48Z

games/devtest/mods/testtranslations/translation_mo.fr.po

+msgid "Testing .mo files: untranslated"
+msgstr "Testing .mo files: translated"
+
+msgctxt "testtranslations"


Why the duplicate msgctxt?

Looks like this is required on every translation to set the text domain, are you sure gettext requires this?

If this msgctxt is required for every message, this should be checked by the engine and warned/errored about

I assume this file is used to generate locale/translation_mo.fr.mo? The translation file reader uses the base filename (here: translation_mo) as the text domain if one is not specified.

The GNU Gettext doc on PO files does not seem clear about about the scope (i.e. the entries that the msgctxt applies to) of msgctxt. However, considering that

The documentation states that "an empty context string and an absent msgctxt line do not mean the same thing";

Entries with msgctxt are considered as "entries with a context specifier"; and

The entire documentation does not mention "scopes" of other specifiers (that said, having msgid and msgtxt to multiple entries would make little sense anyway)

It would be safe to assume that entries without an explicit context specifier would be considered as entries without context information, which, combined with the fact that Minetest does not differentiate between "text domain" and "context" (and, in this PR, uses the former if the latter is absent), means that any entry without an explicit msgctxt specifier would be put under the default (here: translation_mo) domain, which would explain the duplicate msgctxt as later entries would otherwise be put into the translation_mo domain.

It is probably also worth noting that the generated .mo file includes context information on every entry as well.

That said, while entries without msgctxt are allowed (see #13687 (comment)), there does not seem to be any testcase/testfile related to this, so it would make sense to expand the test file to include entries without an explicit context specifier.

rubenwardy · 2024-03-08T21:03:39Z

src/server.cpp

@@ -2551,8 +2551,8 @@ bool Server::addMediaFile(const std::string &filename,
 		".png", ".jpg", ".bmp", ".tga",
 		".ogg",
 		".x", ".b3d", ".obj",
-		// Custom translation file format
-		".tr",
+		// Translation file format


Suggested change

// Translation file format

// Translation file formats

rubenwardy · 2024-03-08T21:05:57Z

games/devtest/mods/testtranslations/locale/translation_tr.fr.tr

@@ -0,0 +1,2 @@
+# textdomain: testtranslations
+Testing .tr files: untranslated=Testing .tr files: translated


Missing EOF new line

Suggested change

Testing .tr files: untranslated=Testing .tr files: translated

Testing .tr files: untranslated=Testing .tr files: translated

rubenwardy · 2024-03-24T17:50:52Z

doc/lua_api.md

+  ```lua
+  local S, NS = minetest.get_translator()
+  NS("@1 file", "@1 files", n, tostring(n))
+  ```


Code blocks need blank lines around them

Suggested change

```lua

local S, NS = minetest.get_translator()

NS("@1 file", "@1 files", n, tostring(n))

```

```lua

local S, NS = minetest.get_translator()

NS("@1 file", "@1 files", n, tostring(n))

rubenwardy · 2024-03-24T17:51:32Z

doc/lua_api.md

+Old translation file format
+---------------------------


Suggested change

Old translation file format

---------------------------

Old translation file format (.tr)

---------------------------------

rubenwardy · 2024-03-24T17:52:11Z

doc/lua_api.md

-Translation file format
-----------------------
+Old translation file format
+---------------------------

 A translation file has the suffix `.[lang].tr`, where `[lang]` is the language


Suggested change

A translation file has the suffix `.[lang].tr`, where `[lang]` is the language

Note: You should use the Gettext translation file format instead.

A translation file has the suffix `.[lang].tr`, where `[lang]` is the language

rubenwardy · 2024-03-24T17:53:27Z

games/devtest/mods/testtranslations/init.lua

+	description = "Test translations",
+	privs = {},
+	func = function(name, param)
+		minetest.chat_send_player(name, "Please ensure your locale is set to \"fr\"")


Is there a reason why you resolved the comment without applying the suggestion? Please reply if you don't like a suggestion

rubenwardy · 2024-03-24T18:25:07Z

Here's a patch to fix content translation with .po files

commit cfb914572ebd41066587f59af1022ecee87529f3
Author: rubenwardy <rw@rubenwardy.com>
Date:   Sun Mar 24 18:24:30 2024 +0000

    Fix content translation and Gettext format

diff --git a/src/gui/guiEngine.cpp b/src/gui/guiEngine.cpp
index 4e22c3ae8..529079ea2 100644
--- a/src/gui/guiEngine.cpp
+++ b/src/gui/guiEngine.cpp
@@ -214,15 +214,28 @@ GUIEngine::GUIEngine(JoystickController *joystick,
 
 
 /******************************************************************************/
-std::string findLocaleFileInMods(const std::string &path, const std::string &filename)
+std::string findLocaleFileWithExtension(const std::string &path)
+{
+	if (fs::PathExists(path + ".mo"))
+		return path + ".mo";
+	if (fs::PathExists(path + ".po"))
+		return path + ".po";
+	if (fs::PathExists(path + ".tr"))
+		return path + ".tr";
+	return "";
+}
+
+
+/******************************************************************************/
+std::string findLocaleFileInMods(const std::string &path, const std::string &filename_no_ext)
 {
 	std::vector<ModSpec> mods = flattenMods(getModsInPath(path, "root", true));
 
 	for (const auto &mod : mods) {
-		std::string ret = mod.path + DIR_DELIM "locale" DIR_DELIM + filename;
-		if (fs::PathExists(ret)) {
+		std::string ret = findLocaleFileWithExtension(
+				mod.path + DIR_DELIM "locale" DIR_DELIM + filename_no_ext);
+		if (!ret.empty())
 			return ret;
-		}
 	}
 
 	return "";
@@ -235,19 +248,26 @@ Translations *GUIEngine::getContentTranslations(const std::string &path,
 	if (domain.empty() || lang_code.empty())
 		return nullptr;
 
-	std::string filename = domain + "." + lang_code + ".tr";
-	std::string key = path + DIR_DELIM "locale" DIR_DELIM + filename;
+	std::string filename_no_ext = domain + "." + lang_code;
+	std::string key = path + DIR_DELIM "locale" DIR_DELIM + filename_no_ext;
 
 	if (key == m_last_translations_key)
 		return &m_last_translations;
 
 	std::string trans_path = key;
-	ContentType type = getContentType(path);
-	if (type == ContentType::GAME)
-		trans_path = findLocaleFileInMods(path + DIR_DELIM "mods" DIR_DELIM, filename);
-	else if (type == ContentType::MODPACK)
-		trans_path = findLocaleFileInMods(path, filename);
-	// We don't need to search for locale files in a mod, as there's only one `locale` folder.
+
+	switch (getContentType(path)) {
+	case ContentType::GAME:
+		trans_path = findLocaleFileInMods(path + DIR_DELIM "mods" DIR_DELIM,
+				filename_no_ext);
+		break;
+	case ContentType::MODPACK:
+		trans_path = findLocaleFileInMods(path, filename_no_ext);
+		break;
+	default:
+		trans_path = findLocaleFileWithExtension(trans_path);
+		break;
+	}
 
 	if (trans_path.empty())
 		return nullptr;

rubenwardy · 2024-03-24T18:30:37Z

Here's a zip file for testing gettext content translation

gettext_content_translation.zip

Not sure why this happens just for games, will check master as well. See list title vs title on the right

rubenwardy · 2024-03-24T18:50:37Z

The parser is in the Translation class, would be nice to have a separate class for this with unit tests. Although, I suppose same for .tr

The reason I say this is because it's a good idea to have unit tests when writing a parser for a file format as it gives some confidence that the parser actually works. The tests should cover a lot of edge cases (missing ", missing msgctxt, comments, etc)

Ekdohibs added @ Translation Testing needed Feature ✨ PRs that add or enhance a feature labels Jul 24, 2023

Ekdohibs force-pushed the translations-gettext3 branch 2 times, most recently from db06c26 to 0819a09 Compare July 24, 2023 17:08

rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Sep 17, 2023

Zughy added Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR and removed Concept approved Approved by a core dev: PRs welcomed! labels Jan 21, 2024

Zughy assigned rubenwardy Jan 21, 2024

rubenwardy mentioned this pull request Mar 3, 2024

Support gettext-style language priority list for Minetest's built-in translation system #14428

Draft

12 tasks

rubenwardy added the Rebase needed The PR needs to be rebased by its author. label Mar 3, 2024

Ekdohibs force-pushed the translations-gettext3 branch from 0819a09 to 574b948 Compare March 7, 2024 07:53

Ekdohibs removed the Rebase needed The PR needs to be rebased by its author. label Mar 7, 2024

y5nw reviewed Mar 7, 2024

View reviewed changes

rubenwardy self-requested a review March 8, 2024 20:30

rubenwardy reviewed Mar 8, 2024

View reviewed changes

y5nw reviewed Mar 8, 2024

View reviewed changes

src/translation.cpp Outdated Show resolved Hide resolved

rubenwardy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 9, 2024

Ekdohibs removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 17, 2024

Ekdohibs and others added 2 commits March 19, 2024 09:58

Client-side translations: gettext support

8632f63

review

7d842b2

Ekdohibs force-pushed the translations-gettext3 branch from 191b397 to 7d842b2 Compare March 19, 2024 08:58

rubenwardy requested changes Mar 24, 2024

View reviewed changes

rubenwardy added Action / change needed Code still needs changes (PR) / more information requested (Issues) Rebase needed The PR needs to be rebased by its author. labels Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client-side translations: gettext and plural support #13687

Client-side translations: gettext and plural support #13687

Ekdohibs commented Jul 24, 2023 •

edited by rubenwardy

rubenwardy commented Mar 3, 2024 •

edited

Ekdohibs commented Mar 7, 2024

y5nw left a comment

y5nw Mar 7, 2024

Ekdohibs Mar 17, 2024

y5nw Mar 17, 2024 •

edited

Ekdohibs Mar 21, 2024

rubenwardy commented Mar 8, 2024

rubenwardy Mar 8, 2024 •

edited

rubenwardy Mar 24, 2024 •

edited

Ekdohibs Mar 25, 2024

Ekdohibs commented Mar 17, 2024

rubenwardy Mar 8, 2024 •

edited

rubenwardy Mar 25, 2024

y5nw Apr 17, 2024 •

edited

rubenwardy Mar 8, 2024

rubenwardy Mar 8, 2024

rubenwardy Mar 24, 2024

rubenwardy Mar 24, 2024

rubenwardy Mar 24, 2024

rubenwardy Mar 24, 2024 •

edited

rubenwardy commented Mar 24, 2024 •

edited

rubenwardy commented Mar 24, 2024 •

edited

rubenwardy commented Mar 24, 2024 •

edited

-		minetest.chat_send_player(name, "Please ensure your locale is set to \"fr\"")
+		if minetest.get_player_information(name).lang_code ~= "fr" then
+				return false, "Please restart Minetest with language = fr"
+		end

		@@ -0,0 +1,2 @@
		# textdomain: testtranslations
		Testing .tr files: untranslated=Testing .tr files: translated

Client-side translations: gettext and plural support #13687

Are you sure you want to change the base?

Client-side translations: gettext and plural support #13687

Conversation

Ekdohibs commented Jul 24, 2023 • edited by rubenwardy

How to test

rubenwardy commented Mar 3, 2024 • edited

Ekdohibs commented Mar 7, 2024

y5nw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y5nw Mar 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenwardy commented Mar 8, 2024

rubenwardy Mar 8, 2024 • edited

Choose a reason for hiding this comment

rubenwardy Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ekdohibs commented Mar 17, 2024

rubenwardy Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y5nw Apr 17, 2024 • edited

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

rubenwardy Mar 24, 2024 • edited

Choose a reason for hiding this comment

rubenwardy commented Mar 24, 2024 • edited

rubenwardy commented Mar 24, 2024 • edited

rubenwardy commented Mar 24, 2024 • edited

Ekdohibs commented Jul 24, 2023 •

edited by rubenwardy

rubenwardy commented Mar 3, 2024 •

edited

y5nw Mar 17, 2024 •

edited

rubenwardy Mar 8, 2024 •

edited

rubenwardy Mar 24, 2024 •

edited

rubenwardy Mar 8, 2024 •

edited

y5nw Apr 17, 2024 •

edited

rubenwardy Mar 24, 2024 •

edited

rubenwardy commented Mar 24, 2024 •

edited

rubenwardy commented Mar 24, 2024 •

edited

rubenwardy commented Mar 24, 2024 •

edited