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.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7011
Build 11464: Vulcan BuildJenkins

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