The function in l10n.js creates one or two variable for each translate* call when it can just check for the cache variable directly or return a copy of the string in the cache object directly.
Some of these variables are only used for read access once in the line following that, so pointless and not making it easier to read either.
The comment to use these functions as a performance improvement to Engine.Translate* is duplicated once per function, when having it once on the top of the file is sufficient (even if someone solely looks at the JSdoc).
While already changing almost all lines, use the capital letter for globals in accordance with the coding conventions.
Some calls are simplified a lot.
While at it, use let for the three remaining vars.
Details
- Reviewers
• leper - Commits
- rP19487: JS l10n cleanup.
That every of the functions is still working as expected can be tested by opening the tech tree, scrolling through civs and hovering some icons,
especially one that has a projectile limit tooltip like the fortress.
Open the gamesetup to the other functions still being called correctly.
Add a warn("functionName"); to the top of each function (and to both cases of translateObjectKeys) to see that indeed each of these functions is called.
That the translate* functions still return a copy of the string can be verified the following way:
var x = translate("Linear Splash Damage");
x = "nope";
warn(translate("Linear Splash Damage"));Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/903/ for more details.
| binaries/data/mods/mod/globalscripts/l10n.js | ||
|---|---|---|
| 13 ↗ | (On Diff #1508) | This is a behaviour change, since now " " returns false, while it previously returned true. (Might not be an issue, but at least translating "" isn't really possible and might lead to strange results (caused by the po format).) I guess this could use && variable.trim() or && !!variable.trim() if we really want that original behaviour. |
| 17 ↗ | (On Diff #1508) | English |
| 28 ↗ | (On Diff #1508) | English |
| 43 ↗ | (On Diff #1508) | English |
| 58 ↗ | (On Diff #1508) | Well, what have we got here :P Also s/ / /. |
Remove isNonEmptyString altogether and capitalize language names, even if it's an adjective, because grammar. Fix whitespace.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/916/ for more details.
| binaries/data/mods/mod/globalscripts/l10n.js | ||
|---|---|---|
| 149 ↗ | (On Diff #1523) | This now allows calling translate(""), which might result in something unexpected. |
| binaries/data/mods/mod/globalscripts/l10n.js | ||
|---|---|---|
| 13 ↗ | (On Diff #1508) | Renaming isNonEmptyStringto isTranslatableString for clarification, as it fits more to the context and is less prone to naming conflicts. Returning an actual boolean as that will be expected from this global. |
| 17 ↗ | (On Diff #1508) | Apparently so
|
| 149 ↗ | (On Diff #1523) | Agree. Calling translate("")in this version is an issue because it would now save the property to that object whereas before it wouldn't be set. The result of translate("") isn't be cached because of the foo[bar] existence checks , so tinygettext shows the "Could not translate:" logwarning each call (independent of this proposal). However since translating a (nonemptyish) string to an empty string isn't possible either, it consistent and not worthwhile to go for the uglier check for a function call that shouldn't exist to begin with. |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/930/ for more details.