Page MenuHomeWildfire Games

Sanitize playername in sanitizePlayerData in gamesetup
ClosedPublic

Authored by Imarok on May 4 2017, 3:25 PM.

Details

Summary

This should fix #4501

Test Plan

Can be tested with most scenario demo maps (e.g. "We Are Legion")

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
https://svn.wildfiregames.com/svn/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1452
Build 2298: Vulcan BuildJenkins
Build 2297: arc lint + arc unit

Event Timeline

Imarok created this revision.May 4 2017, 3:25 PM
Imarok updated the Trac tickets for this revision.May 4 2017, 3:25 PM
Imarok updated this revision to Diff 1648.May 4 2017, 4:03 PM

Use a loop

Imarok updated this revision to Diff 1649.May 4 2017, 4:14 PM

Unification

Imarok updated this revision to Diff 1650.May 4 2017, 4:20 PM

Add comment wrt Atlas

Imarok updated this revision to Diff 1651.May 4 2017, 4:36 PM

Agnosticism

Vulcan added a subscriber: Vulcan.May 5 2017, 2:10 AM

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/1005/ for more details.

Vulcan added a comment.May 5 2017, 2:56 AM

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/1006/ for more details.

Vulcan added a comment.May 5 2017, 3:43 AM

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/1007/ for more details.

Vulcan added a comment.May 5 2017, 5:17 AM

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/1008/ for more details.

Vulcan added a comment.May 5 2017, 6:11 AM

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/1009/ for more details.

elexis added a subscriber: elexis.May 6 2017, 5:38 PM

Patch works as expected, but there is a design issue.

By copying the names, the lobby will show "Player 2" and so forth. It would be better to show "Player 2 (unassigned)".
It should also be seen in the session equally "Player 2 (unassigned)" (instead of "Player 2 (unassigned)" sometime.
Looks like it is possible to determine the unassigned state from the absence of an assigned online or offline player in g_PlayerAssignments (the NetServer equally keeps those for offliners) + the absence of an assigned AI in g_GameAttributes.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
869

This comment is slightly wrong, because we could change the underlying defaults file, it is just a decision that we want to have different defaults in atlas and gamesetup.

The player_defaults.json file should also contain the Team property, so that all properites are well defined in that file.
I've teted it, but atlas seems to ignore that part and should copy that over to the Team dropdown.
Someone should fix that or write a ticket (could be simple if it explains which files to modify exactly and where).

elexis accepted this revision.May 7 2017, 12:58 AM

I want this code to be committed. The session.js and gamedescription.js changes to display unassigned player slots can go in independently.
It seems changing the civ in the player_defaults.json doesn't even affect atlas, so it's probably better to just not add a comment until we have this resolved.
Player.cpp just selects the jth civ for the jth player in L720.

This revision is now accepted and ready to land.May 7 2017, 12:58 AM
elexis added a comment.May 7 2017, 1:03 AM

Also notice "Player 2" should be translated in SP onl (just like botnames), but that is already not translated in Alpha 21.

elexis added inline comments.May 7 2017, 1:30 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1349

Guess that is a bit nicer with index instead of 0

Also for ... in, not for ... of

Imarok updated this revision to Diff 1739.May 7 2017, 10:10 PM

index and for in

Imarok updated this revision to Diff 1740.May 7 2017, 10:55 PM

Fix the thing

Imarok updated this revision to Diff 1742.May 7 2017, 11:11 PM

s/n explicitly/ value

Tested again, looks much better with that duplication removal. Thanks!

This revision was automatically updated to reflect the committed changes.
Vulcan added a comment.May 8 2017, 1:55 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1071/
See console output for more information: http://jw:8080/job/phabricator/1071/console

Vulcan added a comment.May 8 2017, 1:55 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1072/
See console output for more information: http://jw:8080/job/phabricator/1072/console

Vulcan added a comment.May 8 2017, 1:55 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1073/
See console output for more information: http://jw:8080/job/phabricator/1073/console