Page MenuHomeWildfire Games

Nuke session/utility_functions.js
ClosedPublic

Authored by elexis on Jan 29 2017, 7:48 PM.

Details

Summary

That file is quite short (192 lines), while the other files using these functions in session/ have 500 to 1500 lines of code.
The functions are also not logically related, so the code would benefit from moving them to a place where they are used and expected.
This is the only file to which these remarks apply.

While at it, the affected code was cleaned a bit:

  • remove duplication and translation comments in the trading gain tooltip
  • let instead of var
  • remove else following returns and weird inverted and unneeded checks in getRankIconSprite
  • remove unneeded variable in resourcesToAlphaMask
  • remove checks in hasClass that are always satisfied
  • rename getPlayerData to updatePlayerData, since the function is only useful to change g_Players. Eventually that variable should be nuked entirely ("single source of truth" pattern).
  • Remove if (g_PlayerAssignments) condition which is always true.
  • sendLobbyPlayerlistUpdate moved to the other function sending data to the lobby
Test Plan
  • Check that the removed functions are identical to the new ones, besides the changes

In particular:

  • Ensure that the trading tooltip string isn't broken
  • Ensure that the offline tooltip doesn't disappear when changing the diplomacy or resigning

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.Jan 29 2017, 7:48 PM

Oh I wanted to use it for D81 :)

Oh I wanted to use it for D81 :)

Since your function hasSameRestrictionCategory is used only in selection_panels.js, you can move it there.

I don't mind if we add a new file with helper functions, but they should be at least remotely logically related, not a random mix.
And don't worry about committing D81 beforehand, rebasing will be easy here, just moving of a function.

fatherbushido added inline comments.Jan 29 2017, 8:04 PM
binaries/data/mods/public/gui/session/unit_actions.js
1406 ↗(On Diff #385)

As that one is used in some other places, is it the best place to put that one? (I guess you have a reason to put it here instead of selection_panels.js or input.js ?

In D108#3655, @elexis wrote:

Oh I wanted to use it for D81 :)

Since your function hasSameRestrictionCategory is used only in selection_panels.js, you can move it there.

I don't mind if we add a new file with helper functions, but they should be at least remotely logically related, not a random mix.
And don't worry about committing D81 beforehand, rebasing will be easy here, just moving of a function.

Thanks it looks indeed the better place :)
I found that coincidence funny :)

elexis marked an inline comment as done.Jan 29 2017, 8:08 PM
elexis added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
1406 ↗(On Diff #385)

My rationale is that input.js should primarily contain function that deal with handling mouse and keyboard events and
selection_panels.js doesn't contain any helper functions besides 2 colorization helpers, while this file has 5 already that all receive entity states as arguments.

Vulcan added a subscriber: Vulcan.Jan 29 2017, 9:54 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/268/ for more details.

fatherbushido added inline comments.Jan 31 2017, 11:51 AM
binaries/data/mods/public/gui/session/unit_actions.js
1406 ↗(On Diff #385)

(I wonder if we can't call straightly MatchClassList
Also TriggerHelper.EntityHasClass is a bit duplicated (not exactly)
Contexts are different though.
And that's not really in the scope of the patch.)

elexis marked an inline comment as done.Jan 31 2017, 12:03 PM
elexis added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
1406 ↗(On Diff #385)

hasClass is used 13 times and it makes the code a bit shorter to read, seems acceptable

Sorry :/ I add that function to utility_functions.js ;-) Can you rebase?

elexis updated this revision to Diff 471.Feb 7 2017, 12:10 PM

Rebase following rP19195 and add two \n to that function
Move resourcesToAlphaMask and getPlayerHighlightColor from selection_panels.js to selection_panels_helpers.js
Remove one translation comment without translation
Move tradingGainString to a global function so as to avoid defining it each time when calling the function

I'm aware that the markets array shouldn't be constructed each time the function is called, but that should be addressed in a separate patch,
as that also affects the simulation and other files that contain some duplication due to the variable names.

Tested the offline string and trading tooltip again, but nothing else.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/325/ for more details.

shookees commandeered this revision.Feb 23 2017, 7:50 PM
shookees added a reviewer: elexis.
shookees added a subscriber: shookees.
shookees added inline comments.
binaries/data/mods/public/gui/session/selection_details.js
483 ↗(On Diff #471)

Why not using switch instead?

528 ↗(On Diff #471)

Originally

translateWithContext("Separation mark in an enumeration", ", ")

was used, instead of

translate(", ")
binaries/data/mods/public/gui/session/selection_panels_helpers.js
20 ↗(On Diff #471)

Ack

34 ↗(On Diff #471)

Ack

42 ↗(On Diff #471)

Ack

binaries/data/mods/public/gui/session/session.js
329 ↗(On Diff #471)

Ack, only variable name changed

425 ↗(On Diff #471)

Ack

1281 ↗(On Diff #471)

Ack

elexis commandeered this revision.Feb 23 2017, 7:59 PM
elexis edited reviewers, added: shookees; removed: elexis.
elexis added inline comments.
binaries/data/mods/public/gui/session/selection_details.js
483 ↗(On Diff #471)

Since the third case checks for a different thing, otherwise I'd agree

528 ↗(On Diff #471)

Yes, I was wondering how that translation context helps. Translators on transifex do complain about commas not having context though, I guess the context is useful for pointing out that it's not used in a sentence, so I'll add it back. Good catch!

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/393/ for more details.

elexis updated this revision to Diff 588.Feb 23 2017, 9:11 PM
elexis marked 9 inline comments as done.

Don't remove that translation context.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/394/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/396/ for more details.

shookees accepted this revision.Feb 24 2017, 12:16 AM
This revision is now accepted and ready to land.Feb 24 2017, 12:16 AM
elexis added inline comments.Feb 24 2017, 12:28 AM
binaries/data/mods/public/gui/session/session.js
329 ↗(On Diff #471)

(Also changed whitespace in that push command and more importantly, removed the pointless return and input arg)

This revision was automatically updated to reflect the committed changes.