Page MenuHomeWildfire Games

Improve the plural translation of the "<players> have won" string
Needs ReviewPublic

Authored by Gallaecio on Aug 18 2018, 7:55 PM.

Details

Reviewers
elexis
Summary

Reported by GunChleoc on Transifex.

Test Plan

I’ve not actually tested this change, and although it looks fine my eyes are not a JavaScript interpreter, so before merging this change I should perform some testing, e.g. getting both messages to appear as expected.

Event Timeline

Gallaecio created this revision.Aug 18 2018, 7:55 PM
Vulcan added a subscriber: Vulcan.Aug 18 2018, 7:58 PM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/EndGameManager.js
| 174| »   »   »   let·message·=·markForTranslation(
|    | [MAJOR] ESLintBear:
|    | Parsing error: Unexpected token let

binaries/data/mods/public/simulation/components/EndGameManager.js
| 174| »   »   »   let·message·=·markForTranslation(
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

binaries/data/mods/public/simulation/components/EndGameManager.js
| 177| »   »   »   let·message·=·markForPluralTranslation(
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

Link to build: https://jenkins.wildfiregames.com/job/differential/720/display/redirect

elexis requested changes to this revision.Aug 26 2018, 1:19 PM
elexis added a subscriber: elexis.

That what the bot says: let limits the scope of the variable to the scope of the statement it's defined in.

(Marking as red so people know that it wasn't a jenkins malfunction.)

You can use

`"message":
         allies.length == 1 ?
             markForTranslation("%(lastPlayer)s has won (last player alive).") : 
              markForPluralTranslation(%(players)s and %(lastPlayer)s have won (last players alive).", allies.length),

or something like that.

(Also note that the NOTE: note is redundant with the fact that this is a code comment, which always represents a note)

This revision now requires changes to proceed.Aug 26 2018, 1:19 PM
Gallaecio updated this revision to Diff 7578.Mar 17 2019, 5:45 PM

Removed the note prefix, and added the ternary operator syntax to avoid declaring a variable.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
| 107| 107| 	cmpGUIInterface.PushNotification({
| 108| 108| 		"type": "won",
| 109| 109| 		"players": [winningPlayers[0]],
| 110|    |-		"allies" : winningPlayers,
|    | 110|+		"allies": winningPlayers,
| 111| 111| 		"message": victoryString(winningPlayers.length)
| 112| 112| 	});
| 113| 113| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
| 115| 115| 		cmpGUIInterface.PushNotification({
| 116| 116| 			"type": "defeat",
| 117| 117| 			"players": [defeatedPlayers[0]],
| 118|    |-			"allies" : defeatedPlayers,
|    | 118|+			"allies": defeatedPlayers,
| 119| 119| 			"message": defeatString(defeatedPlayers.length)
| 120| 120| 		});
| 121| 121| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
| 167| 167| 		cmpGuiInterface.PushNotification({
| 168| 168| 			"type": "won",
| 169| 169| 			"players": [allies[0]],
| 170|    |-			"allies" : allies,
|    | 170|+			"allies": allies,
| 171| 171| 			// Keep a separate string for the case of a single player. This is required for languages that have the same
| 172| 172| 			// plural forms for 1 and other numbers (e.g. Scottish Gaelic, where 1 and 11 are considered the same plural
| 173| 173| 			// form), where we want the string for 1 to have "%(lastPlayer)s" while the string for other number has
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1113/display/redirect

bb added a subscriber: bb.Mar 17 2019, 6:09 PM

Is that lengthy comment really required? Seems obvious from the code (or it is me who has such a language as mother tongue)

I think a comment is necessary to prevent someone from accidentally reverting to something in the lines of the original code to "simplify" it.

What about simplifying the comment to “Separate strings needed for i18n, see D1613”?

bb added a comment.Mar 17 2019, 6:53 PM

Code should stand on itself, not referring to a phabricator revision. But isn't markForPluralTranslation designed to cover this case? So how does this solve the issue?
Furthermore markForPlural expects 3 arguments, now it only has 2.

Damn, you have a point in that the markForPluralTranslation function in the new code needs to have the first parameter also passed as second parameter. So the patch requires changes for sure.

But for most developers it will not be obvious why we are using markForTranslation followed by a separate markForPluralTranslation which has a plural string (from an English perspective) both as singular and as plural input. I think we need a comment to explain that this is this way because in some language the "singular" form is used for values other than 1 (e.g. 11 in Scottish Gaelic), and for any allies.length other than 1, including 11, the plural string should be used (and adapted to language-specific plural forms).

bb added a comment.Mar 17 2019, 8:51 PM

I get that some languages use singular for every other prime number, so to speak. But isn't exactly that the reason why we have markForPluralTranslation, so a translation can choose for which values it takes whatever form. If that isn't the case in the current code, we might have found a bug in our code. Stating the same problem more concretely: how is this particular call different from all the other markForPluralTranslation calls? or are all of them just wrong? (there are some in buildrestriction, wonderVictory, CaptureTheRelic, triggerHelper and Treasure Island)

Gallaecio added a comment.EditedMar 17 2019, 9:12 PM

markForPluralTranslation is for when strings have the same content, and only change for grammatical number (e.g. singular words become plural).

The problem here is that the string for singular is different from the plural string beyond what is grammatical. The singular string starts with "%(lastPlayer)s", while the plural starts with "%(players)s and %(lastPlayer)s". In English that is not a problem. In Scottish Gaelic, it is, because we are forcing them to have a broken string for allies.length == 11, since instead of listing the 11 winners, that string would only mention one of them. They really need to use something like "%(players)s and %(lastPlayer)s has won (last player alive)." (plural prefix, singular wording).

The proposed solution, although unintuitive (hence the comment), is nonetheless the simplest approach. Another approach, maybe cleaner but requiring more complex changes in code, would be to extract the prefix into a variable that is filled based on allies.length, e.g. in pseudo code:

if (allies.length == 1) {
    playerList = lastPlayer;
} else {
    playerList = markForTranslation("%(players)s and %(lastPlayer)s")
}
message = markForPluralTranslation("%(playerList)s has won (last player alive).", "%(playerList)s have won (last players alive).", allies.length)
Gallaecio updated this revision to Diff 7584.Mar 17 2019, 9:18 PM

Fixed the plural call by repeating the English string. Notice that the first parameter of the function will never be used in English.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
| 107| 107| 	cmpGUIInterface.PushNotification({
| 108| 108| 		"type": "won",
| 109| 109| 		"players": [winningPlayers[0]],
| 110|    |-		"allies" : winningPlayers,
|    | 110|+		"allies": winningPlayers,
| 111| 111| 		"message": victoryString(winningPlayers.length)
| 112| 112| 	});
| 113| 113| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
| 115| 115| 		cmpGUIInterface.PushNotification({
| 116| 116| 			"type": "defeat",
| 117| 117| 			"players": [defeatedPlayers[0]],
| 118|    |-			"allies" : defeatedPlayers,
|    | 118|+			"allies": defeatedPlayers,
| 119| 119| 			"message": defeatString(defeatedPlayers.length)
| 120| 120| 		});
| 121| 121| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/EndGameManager.js
| 167| 167| 		cmpGuiInterface.PushNotification({
| 168| 168| 			"type": "won",
| 169| 169| 			"players": [allies[0]],
| 170|    |-			"allies" : allies,
|    | 170|+			"allies": allies,
| 171| 171| 			// Keep a separate string for the case of a single player. This is required for languages that have the same
| 172| 172| 			// plural forms for 1 and other numbers (e.g. Scottish Gaelic, where 1 and 11 are considered the same plural
| 173| 173| 			// form), where we want the string for 1 to have "%(lastPlayer)s" while the string for other number has
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1116/display/redirect

bb added a comment.EditedApr 21 2019, 10:32 PM

k, now I understand the problem, gaelic hardcodes that 1 and 11 must always be the singular form, however that isn't the case in this string.

For me (as programmer) this isn't a bug in our code, but it is a bug in the translation: to me it sounds much more intuitive to split 1 and 11 in Gaelic in two plural forms, giving both just the singular form everywhere, except this string where they actually differ (it sounds very plausible more such strings will be added in future).
EDIT: already found a candidate: Player.js L350

In D1613#75586, @bb wrote:

For me (as programmer) this isn't a bug in our code, but it is a bug in the translation

Actually, I believe it is the other way around. GNU Gettext plural strings are meant to handle cases where language grammar (and only grammar!) forces strings to be different depending on a number. When anything other than grammar changes, plural translation units are not meant to be used. We are currently abusing that feature of GNU Gettext for strings that change not only in grammar, as their content varies beyond grammar between the singular and plural forms.

Also, the translators do not choose how they handle plural strings on each translation unit or even on each translation file (the latter may be technically possible, but completely discouraged). GNU Gettext has clearly defined the different plural rules that can be used in GNU Gettext files, and indicated which languages use each: https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html

From http://irclogs.wildfiregames.com/2019-10/2019-10-20-QuakeNet-%230ad-dev.log:

(03:28:37 PM) elexis: gallaecio: perhaps we can just remove the "and"?
(03:29:03 PM) elexis: the provided arguments only seem to work for english and similar languages
(03:29:17 PM) elexis: v1, v2, ..., v(n-1) and v(n)
(03:29:46 PM) elexis: but some language might want to do
(03:29:46 PM) elexis: v1 something v2 somethingelse ..., v(n-1) somethingelsetoo v(n)
(03:30:06 PM) elexis: so why not just pass a list of players in any case
(03:33:12 PM) gallaecio: That would work form me. I’ll update the patch next week.
(03:33:31 PM) elexis: bb: that would work for you too? ^
(03:42:26 PM) bb: That could work
Gallaecio updated this revision to Diff 10239.Nov 2 2019, 8:01 AM

Use a more flexible approach which also make the code more readable

Vulcan added a comment.Nov 2 2019, 8:02 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/509/display/redirect

Vulcan added a comment.Nov 2 2019, 8:05 AM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
| 123| »   »   if·(playerCount·==·1)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
| 125| »   »   }·else·if·(playerCount·==·2)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Closing curly brace appears on the same line as the subsequent block.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
| 125| »   »   }·else·if·(playerCount·==·2)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
| 125| »   »   }·else·if·(playerCount·==·2)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
| 133| »   »   }·else·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Closing curly brace appears on the same line as the subsequent block.
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/EndGameManager.js
| 107| 107| 	cmpGUIInterface.PushNotification({
| 108| 108| 		"type": "won",
| 109| 109| 		"players": [winningPlayers[0]],
| 110|    |-		"allies" : winningPlayers,
|    | 110|+		"allies": winningPlayers,
| 111| 111| 		"message": victoryString(winningPlayers.length)
| 112| 112| 	});
| 113| 113| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/EndGameManager.js
| 115| 115| 		cmpGUIInterface.PushNotification({
| 116| 116| 			"type": "defeat",
| 117| 117| 			"players": [defeatedPlayers[0]],
| 118|    |-			"allies" : defeatedPlayers,
|    | 118|+			"allies": defeatedPlayers,
| 119| 119| 			"message": defeatString(defeatedPlayers.length)
| 120| 120| 		});
| 121| 121| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'allies'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/EndGameManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/EndGameManager.js
| 167| 167| 		cmpGuiInterface.PushNotification({
| 168| 168| 			"type": "won",
| 169| 169| 			"players": [allies[0]],
| 170|    |-			"allies" : allies,
|    | 170|+			"allies": allies,
| 171| 171| 			"message": markForPluralTranslation(
| 172| 172| 				"%(players)s has won (last player alive).",
| 173| 173| 				"%(players)s have won (last players alive).",
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1024/display/redirect

elexis added a comment.Nov 3 2019, 8:26 PM

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let

let allows you to declare variables that are limited to a scope of a block statement

So the current code can't work.
The keyword was introduced specifically to avoid declaring a variable in one scope (for example if statement cases) and use it outside of that scope (after the if statement).
One can define it before the if statement and then assign the value.

Also { and } on separate lines (From https://trac.wildfiregames.com/wiki/Coding_Conventions#Introduction

Primarily, try to match the style of the functions that you're editing (assuming it's at least self-consistent and not too bizarre), in order to avoid making it less self-consistent.

Use a more flexible approach

That's because there are now three strings marked with translate calls, that enable both more english specializations and more translation variants?
But on the other side, the translatePlural calls were changed to translate calls, doesn't this limit the ability of translators to make case distinctions depending on the plural grammar of their language?
There may be one or more languages where the third case translate("%(firstPlayers), and %(lastPlayer)"), might have different translations for two different playercounts I would expect.
It could be tested / determined by looking into the existing translations I guess.

which also make the code more readable

Well it were 5 lines before and 25 lines now with three case distinctions in english. The above quoted irc discussion proposal was just using players.join(", "), that would make it 1 line instead of 5 lines (but grammatical correctness should be more decisive than code complexity).
If its 20-25 lines of code to concatenate a playerlist, perhaps it would be reasonable to move it to a separate function (separation of concerns and easier to reuse).

Reviewing because I have to touch this code slightly: Instead of return string, it will call this.postMessage(string) soon (past 23128).

Also in my diff I had changed the if (!plural) { translate; return} translatePlural pattern to if (plural) translatePlural else translate, but I'll leave that change out for now to make this diff easier to merge.
Was wondering whether the n=1 case is actually necessary if there is a given !plural, but I forgot that this string code was a labyrinth.

elexis added a comment.Nov 5 2019, 8:38 PM

There is also this in components/Player.js in Player.prototype.SubtractResourcesOrNotify:

		if (i < 1)
			warn("Amounts needed but no amounts given?");
		else if (i == 1)
			msg = markForTranslation("Insufficient resources - %(resourceAmount1)s %(resourceType1)s");
		else if (i == 2)
			msg = markForTranslation("Insufficient resources - %(resourceAmount1)s %(resourceType1)s, %(resourceAmount2)s %(resourceType2)s");
		else if (i == 3)
			msg = markForTranslation("Insufficient resources - %(resourceAmount1)s %(resourceType1)s, %(resourceAmount2)s %(resourceType2)s, %(resourceAmount3)s %(resourceType3)s");
		else if (i == 4)
			msg = markForTranslation("Insufficient resources - %(resourceAmount1)s %(resourceType1)s, %(resourceAmount2)s %(resourceType2)s, %(resourceAmount3)s %(resourceType3)s, %(resourceAmount4)s %(resourceType4)s");
		else
			warn("Localisation: Strings are not localised for more than 4 resources");

This could use the same concatenation mechanism, so that the duplication is avoided and that this list supports mods with more than 4 resource types.
That is unless the grammar of some translation requires differentiation of the strings depending on whether one concatenates resources or playernames -.-

You are right, it looks like i18n-aware list-building is more complex than I though. On the bright side, it looks like ICU 50 provides a function for this, so we should be able to solve the issue without exposing any list-building string to our translators. I’ll try to work on this next weekend, and improve Player.prototype.SubtractResourcesOrNotify as well as part of these changes.