Reported by GunChleoc on Transifex.
Details
Details
- Reviewers
elexis
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
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 9924 Build 16730: Vulcan Build Jenkins Build 16729: Vulcan Build (Windows) Jenkins Build 16728: arc lint + arc unit
Event Timeline
Comment Actions
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
Comment Actions
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)
Comment Actions
Removed the note prefix, and added the ternary operator syntax to avoid declaring a variable.
Comment Actions
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
Comment Actions
Is that lengthy comment really required? Seems obvious from the code (or it is me who has such a language as mother tongue)
Comment Actions
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”?
Comment Actions
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.
Comment Actions
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).
Comment Actions
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)
Comment Actions
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)Comment Actions
Fixed the plural call by repeating the English string. Notice that the first parameter of the function will never be used in English.
Comment Actions
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
Comment Actions
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
Comment Actions
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
Comment Actions
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
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/509/display/redirect
Comment Actions
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
Comment Actions
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.
Comment Actions
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 -.-
Comment Actions
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.
Comment Actions
(For reference, there is also this bug #5675, where the simulation sends a wrong victory chat notification when ceasefire ended after the only player of the game had won already, showing " and undefined have won". But I guess that victory message should just not be sent, then it probably won't affect this patch.)
Comment Actions
Not really satisfied with this solution, since we now try to fix translations ourselfs for some specifc cases (n=1, n=2, n>2). That does no allow the full generality:
(03:29:46 PM) elexis: v1 something v2 somethingelse ..., v(n-1) somethingelsetoo v(n)
Also as the Gnu getText documentation says The consequence of this is that application writers should not try to solve the problem in their code.
Given this I don't think that the currently proposed solution is correct, neither I think my original implementation is correct (far from). Probably the best solution would be to file a report at GNU getText too see if that can handle enumerations properly. Couldn't find any documentation on that, but it sounds like something getText should fix. E.g. Latin can add que behind each word to get an enumeration, which wouldn't be allowed in this implementation. Obviously Latin can get around by using et, but you get what I mean.
| binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js | ||
|---|---|---|
| 123–125 | } and { on a new line | |