Page MenuHomeWildfire Games

Move GUI translate functions far far away from the simulation
ClosedPublic

Authored by elexis on Nov 28 2017, 12:10 AM.

Details

Summary

In D1062 it was noted that the l10n.js GUI functions are located in globalscripts, thus available to the simulation and AI code too.
But adding any translate call to there would be beyond bad practice and should rather yield a reference error at JS compile time to indicate that.

Test Plan

Notice that all calls to these functions are only in gui/ subdirectories.
Notice that mods/mod/gui/pregame/ doesn't contain any translation and that every other GUI page includes gui/common/.
Notice that following D619, every GUI page includes the entire gui/common/ directory, thus also the modmod/gui/common/l10n.js file.
Thus no code required to load the new gui/common/l10n.js file.
(Notice that the globalscripts files are reloaded upon each page switch / stack, so emptying of the cache globals happens either way.)
Optionally notice Phabricator being weird with svn cp.

Current GUI pages:

modmod:
./page_modmod.xml
./page_pregame.xml

public:
./page_aiconfig.xml
./page_atlas.xml
./page_civinfo.xml
./page_credits.xml
./page_gamesetup_mp.xml
./page_gamesetup.xml
./page_loadgame.xml
./page_loading.xml
./page_lobby.xml
./page_locale_advanced.xml
./page_locale.xml
./page_manual.xml
./page_msgbox.xml
./page_options.xml
./page_pregame.xml
./page_prelobby.xml
./page_replaymenu.xml
./page_savegame.xml
./page_session.xml
./page_splashscreen.xml
./page_structree.xml
./page_summary.xml

(all pages proofread)

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

elexis created this revision.Nov 28 2017, 12:10 AM
Vulcan added a subscriber: Vulcan.Nov 28 2017, 1:24 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/mod/gui/common/l10n.js
|  47| »   »   g_TranslationsWithContext[context]·=·{}
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
bb requested changes to this revision.Nov 28 2017, 4:53 PM
bb added a subscriber: bb.

I do not notice that pregame is the only page that doesn't include gui/common. (modmod)

This revision now requires changes to proceed.Nov 28 2017, 4:53 PM

Other than that correct and complete?

bb added inline comments.Nov 28 2017, 5:46 PM
binaries/data/mods/mod/gui/common/l10n.js
115 ↗(On Diff #4417)

Well one could start yelling about trailing comma's,

140 ↗(On Diff #4417)

that a cap can only on a period

149 ↗(On Diff #4417)

or these braces.

184 ↗(On Diff #4417)

But as all that looks out of scope, y

Did you see Itms latest Review Guideline proposal? ;-)

bb added a comment.Nov 28 2017, 6:24 PM

Do you see a error out as a small issue? There could f.e. be some implications when simply adding the directory to that page (duplicate definitions, useless functions etc.). That they aren't present here is not a proof they can't occur, even with such a simple fix.

In D1075#42832, @bb wrote:

Do you see a error out as a small issue?

No, because I don't see any issue

That directory contains exactly one file that is included. All the arguments from D619 apply here equally.
If mods want to change the mod loader they can then add a new file to the directory without having to maintain a copy.
I can't prove to you that no developer will add uncommon code to modmod/common/. If they would that would be their mistake however.

elexis retitled this revision from Move GUI translate calls far far away from the simulation to Move GUI translate functions far far away from the simulation.Nov 28 2017, 6:44 PM
bb added a comment.Nov 28 2017, 6:52 PM
In D1075#42834, @elexis wrote:
In D1075#42832, @bb wrote:

Do you see a error out as a small issue?

No, because I don't see any issue

Have you opened the mod selector screen with the patch applied? The mod selector code has translate calls, but does not include gui/common, thus there are errors.

In D1075#42838, @bb wrote:
In D1075#42834, @elexis wrote:
In D1075#42832, @bb wrote:

Do you see a error out as a small issue?

No, because I don't see any issue

Have you opened the mod selector screen with the patch applied? The mod selector code has translate calls, but does not include gui/common, thus there are errors.

Where is the issue with including mod/gui/common/ from mod/gui/modmod/?

bb accepted this revision.Nov 28 2017, 7:11 PM

There is none in that, but the issue is that isn't done in the patch, so we get those errors.

As we seem to agree on adding that => accept

This revision is now accepted and ready to land.Nov 28 2017, 7:11 PM

Thanks for the review!

This revision was automatically updated to reflect the committed changes.