Page MenuHomeWildfire Games

Little cleanup/fix in ChatMessageFormatPlayer.js
Needs ReviewPublic

Authored by Polakrity on Dec 3 2019, 8:01 PM.

Details

Reviewers
Gallaecio
Summary

Cleanup
Remove an useless reassign of isMe with the same value. (notified in: D2409)
Change a translateWithContext() by translate() cause the extractor doesn't extract variables.
So the translators in Transifex will never have this to translate. There is no need to use this function.

Temporary fix
Split the format string in a variable and translate it after.
This is to prevent the extractor from extracting a string that doesn't need. ("no-context")
Notified here: rP23062

The best thing is to make the extractor "smarter".

Test Plan
  • Check if the messages are correctly displayed.
  • Execute the Translation extract script and notice if "no-context" isn't extracted anymore.

Diff Detail

Event Timeline

Polakrity created this revision.Dec 3 2019, 8:01 PM
Vulcan added a comment.Dec 3 2019, 8:02 PM

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

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

Vulcan added a comment.Dec 3 2019, 8:05 PM

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

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

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
|  57| »   »   let·formatType·=·this.strings[isMe·?·"me"·:·"regular"][msg.context·?·"context"·:·"no-context"]);
|    | [MAJOR] ESLintBear:
|    | Parsing error: Unexpected token )

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
|  57| »   »   let·formatType·=·this.strings[isMe·?·"me"·:·"regular"][msg.context·?·"context"·:·"no-context"]);
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
|  57| »   »   let·formatType·=·this.strings[isMe·?·"me"·:·"regular"][msg.context·?·"context"·:·"no-context"]);
|    | [MAJOR] JSHintBear:
|    | Expected an identifier and instead saw ')'.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
|  57| »   »   let·formatType·=·this.strings[isMe·?·"me"·:·"regular"][msg.context·?·"context"·:·"no-context"]);
|    | [NORMAL] JSHintBear:
|    | Expected an assignment or function call and instead saw an expression.
Executing section cli...

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

Polakrity updated this revision to Diff 10470.Dec 4 2019, 1:09 PM

fix linter warning

Vulcan added a comment.Dec 4 2019, 1:11 PM

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

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

Vulcan added a comment.Dec 4 2019, 1:14 PM

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

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

Stan added subscribers: Gallaecio, Stan.

Sounds like something @Gallaecio could review.

elexis added a subscriber: elexis.Dec 5 2019, 12:13 PM

Remove an useless reassign of isMe with the same value.

From D2409

the same value than above

Incorrect.

Split the format string in a variable and translate it after.

This is to prevent the extractor from extracting a string that doesn't need. ("no-context")

That should be a bug in extractors.py, since it the script should only extract string literals, not literals that are part of an expression of the translate argument, i.e. not extract anything as it would with translate(msg.contextType).

Temporary fix
The best thing is to make the extractor "smarter".

If the problem is the extractors.py script, then the patch doesn't fix the problem.
extractJavascriptFromFile is only 100 lines long, and if the performed test plan already includes Execute the Translation extract script and notice if "no-context" isn't extracted anymore. then one can quickly test too whether a patch is correct (I'm currently failing to install pology)

Theres also the question whether the translate should happen in every function call or in the prototype property assignment.
I started with the former, assuming that the selected language may be changed during a game in the future, but later switched to the latter for simplicity after considering that the GUI already doesn't translate the XML specified strings again unless reloading the page after changing a new language, hence it being unlikely to be able to change the language during a running game in the future without reloading the page.
Or perhaps it's easy enough to add support for language changes in the C++ GUI to cover that case somehow.
Probably not a strong enough case to replace all markForTranslation with translate calls.
Then the cleanest diff would indeed be fixing the extractors script, as I see it quite likely that the next author to this line would inline the variable again.
It sounds like that concatenate_next mechanism may need an early return if it encounters anything other than a string literal, but it's easier to tell by running the script.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
59

Why is this change performed? If the string was extracted with markForTranslationWithContext why would one translate it with translate instead of translateWithContext.