Page MenuHomeWildfire Games

Get a buddy functionality for playerlist in lobby
ClosedPublic

Authored by elexis on Mar 11 2017, 3:05 PM.

Details

Test Plan

test try all dropdowns a while. should be to much option. but we need to test it first to strip down correctly. maybe only one dropbox for buddies, maybe pings or checkbox for only buddies and ping integrated.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

refactored and changed

ffffffff updated this revision to Diff 1115.Apr 5 2017, 7:51 PM

buddy sign change

buddy dot round + cleaned lobby

elexis he said he doesnt want that overlapping sign in the column header for buddy column. do u agree scy or is it better to have sign showing some meaning for column. im open.

ffffffff updated this revision to Diff 1134.Apr 7 2017, 6:57 PM

splitted the map and filter command in update player und game list on the arrays. minor code optis.

I'd agree with elexis. The extra header doesn't add much in terms of helping the user understand the meaning of the column while making it less attractive.

ffffffff updated this revision to Diff 1135.Apr 7 2017, 7:09 PM

minor changes in buddy game sorting for priorizing buddies then game state and then game name in buddy sorting

ffffffff updated this revision to Diff 1136.Apr 7 2017, 7:10 PM

removing square block from column header in buddy column.

ive question do we want to prefer ordering by buddy now in playerlist and gameslist for default when entering lobby or do we stay with name sorting.

binaries/data/config/default.cfg
375

this one "|" is added due to error in empty value in config.

binaries/data/mods/public/gui/lobby/lobby.js
84

for now i split buddy names by "|" sign if thinks is no allowed in names in lobby.

410

as suggest by vladis seperat sort and filter command.

417

i wanted to prefer here when selecting buddy sorting buddy then presence [online,away,playing..] and then name in this prio if its ok.

728

prefer game state over name in buddysorting. this b/a change in buddy attribute object is willingly due to buddy numberings from 0 (nobuddy game) to 1 (buddy in observer game), 2 (buddy in game playing), 3 (user playing in game for easier finding when dropped from game). so highest is priorized

I'd lean toward name sorting, but either is fine.

elexis commandeered this revision.Apr 17 2017, 7:25 PM
elexis edited reviewers, added: ffffffff; removed: elexis.

Gamelist example:

[{name:"melpin's game", ip:"1.2.3.4", port:"20595", state:"running", nbp:"4", maxnbp:"4", players:"{\"-1\":\\[{\"Name\":\"melpin (1059)\",\"Team\":-1},{\"Name\":\"THEKIM (1235)\",\"Team\":-1},{\"Name\":\"tristyfisty (1199)\",\"Team\":-1},{\"Name\":\"gabrielpotumati (1420)\",\"Team\":-1}]}", mapName:"maps/random/deep_forest", niceMapName:"Deep Forest", mapSize:"320", mapType:"random", victoryCondition:"Conquest", startTime:"1492394490"}, {name:"Reaperisonline's game", ip:"1.2.3.4", port:"20595", state:"waiting", nbp:"0", maxnbp:"5", players:"{\"0\":\\[{\"Name\":\"lou_diamonds (1213)\",\"Team\":0,\"Offline\":true,\"State\":\"won\"},{\"Name\":\"mvdiogo (1152)\",\"Team\":0,\"Offline\":true,\"State\":\"won\"},{\"Name\":\"Zwiebelringe (983)\",\"Team\":0,\"Offline\":true,\"State\":\"won\"}],\"1\":\\[{\"Name\":\"pablo006 (1465)\",\"Team\":1,\"Offline\":true,\"State\":\"defeated\"}],\"2\":\\[{\"Name\":\"WarriorSpartan (1623)\",\"Team\":2,\"Offline\":true,\"State\":\"defeated\"}],\"observer\":\\[{\"Name\":\"Reaperisonline (1511)\",\"Team\":\"observer\"}]}", mapName:"maps/random/corinthian_isthmus", niceMapName:"Corinthian Isthmus", mapSize:"256", mapType:"random", victoryCondition:"Conquest", startTime:"1492387346"}, {name:"Glitchatron's game", ip:"1.2.3.4", port:"20595", state:"running", nbp:"2", maxnbp:"4", players:"{\"0\":\\[{\"Name\":\"Glitchatron\",\"Team\":0},{\"Name\":\"Jharr12\",\"Team\":0}],\"1\":\\[{\"Name\":\"R\xE6dwald of East Anglia\",\"Team\":1,\"AI\":\"petra\",\"AIDiff\":2},{\"Name\":\"Ambiorix\",\"Team\":1,\"AI\":\"petra\",\"AIDiff\":2}]}", mapName:"maps/skirmishes/Corsica and Sardinia (4)", niceMapName:"Corsica and Sardinia (4)", mapSize:"Default", mapType:"skirmish", victoryCondition:"Conquest All", startTime:"1492394870"}, {name:"3v2", ip:"1.2.3.4", port:"20595", state:"init", nbp:"5", maxnbp:"5", players:"{\"-1\":\\[{\"Name\":\"iApprove (1326)\"},{\"Name\":\"phoenixdesk (1299)\"},{\"Name\":\"LongWolf (1228)\"},{\"Name\":\"r4pt0r\"},{\"Name\":\"DEWA (1266)\"}],\"observer\":\\[{\"Name\":\"PrincipalityOfZeon (1659)\",\"Team\":\"observer\"},{\"Name\":\"peledebutoconunpibe\",\"Team\":\"observer\"},{\"Name\":\"chrstgtr (1452)\",\"Team\":\"observer\"}]}", mapName:"maps/random/unknown_land", niceMapName:"Unknown Land", mapSize:"256", mapType:"random", victoryCondition:"Conquest", startTime:""}]

Playelrist example:

[{name:"DEWA", presence:"playing", rating:"1266", role:"participant"}, {name:"Elvith92", presence:"available", rating:"1249", role:"participant"}, {name:"Glitchatron", presence:"playing", rating:"", role:"participant"}, {name:"Jharr12", presence:"playing", rating:"", role:"participant"}, {name:"LongWolf", presence:"playing", rating:"1228", role:"participant"}, {name:"PTK.JAEM", presence:"playing", rating:"1370", role:"participant"}, {name:"PrincipalityOfZeon", presence:"playing", rating:"1659", role:"participant"}, {name:"Ratings", presence:"available", rating:"", role:"moderator"}, {name:"Reaperisonline", presence:"playing", rating:"1511", role:"participant"}, {name:"THEKIM", presence:"playing", rating:"1235", role:"participant"}, {name:"Tango_", presence:"available", rating:"1304", role:"participant"}, {name:"WFGbot", presence:"available", rating:"", role:"moderator"}, {name:"chrstgtr", presence:"playing", rating:"1452", role:"participant"}, {name:"elexis3", presence:"available", rating:"1156", role:"participant"}, {name:"gabrielpotumati", presence:"playing", rating:"1420", role:"participant"}, {name:"iApprove", presence:"playing", rating:"1326", role:"participant"}, {name:"lapatiencos", presence:"playing", rating:"", role:"participant"}, {name:"melpin", presence:"playing", rating:"1059", role:"participant"}, {name:"peledebutoconunpibe", presence:"playing", rating:"", role:"participant"}, {name:"phoenixdesk", presence:"playing", rating:"1299", role:"participant"}, {name:"r4pt0r", presence:"playing", rating:"", role:"participant"}, {name:"scythetwirler", presence:"playing", rating:"", role:"moderator"}, {name:"temple", presence:"playing", rating:"1647", role:"participant"}, {name:"tristyfisty", presence:"playing", rating:"1199", role:"participant"}, {name:"user1", presence:"available", rating:"", role:"moderator"}, {name:"vasilis", presence:"playing", rating:"1159", role:"participant"}, {name:"xPlague", presence:"available", rating:"1320", role:"participant"}, {name:"zivk", presence:"playing", rating:"1330", role:"participant"}]
binaries/data/config/default.cfg
375

ah, #3990

binaries/data/mods/public/gui/common/functions_utility.js
219

This is only used in lobby/ and I don't see a future usage for this outside of lobby, so -> lobby.

You have also copied that from another place in the lobby without replacing that copy with a call to this function

binaries/data/mods/public/gui/lobby/lobby.js
74

a sign is +/- or something seen in traffc, the word symbol seems to fit better.

I know all the rest is const and I made it const, but sanderd17 has convinced me some time ago that var is better for mods, so they can change it from an external file

77

Comments having the exact same character composition a the variable name should be removed.
Comments should give information (information implies uniqueness).

So if we want to keep this JSdoc format, it should explain in which situation this icon is used.

84

Vladislav recommended to use comma, as it resembles CSV format and since that is also forbidden by our lobby policy (see sanitizePlayerName).

A more stable solution would be using a character that is prohibited by the XMPP specs, but it seems to allow every special character on my keyboard, even slash, backslash, so comma is ok.

407

It's a design decision whether the player will be shown as a buddy of oneself. Might make the code shorter if not caring about this, but we can keep that design decision.

I'd rather check for g_Username == player.name || g_UserBuddies.indexOf(player.name) != -1 though instead of the concat

410

Indeed a bit odd with the whitespace, but having a straight concatenation means that it is a tiny bit more obvious that these things are executed consecutively on the same array

417

The design looks weird, but it's basically correct, since players have all unique names and ratings.
For consistency we should add the name tiebreaker to the status sorting too.
The toString can be avoided since the toLowerCase is already a string and we'll end up with a string therefore.
The +1 is not needed either as 0 < 1 < ...

454

Using the alpha transparency to hide the sign we later add is pointless if we can just not add anything instead.
\n somewhere since we ought to have 80 characters per line at most (http://trac.wildfiregames.com/wiki/Coding_Conventions) and really not more than 120 if possible

474

tabs instead of spaces

476

Those hardcoded numbers are bad. People should be able to only change the XML without having to change the JS numbers. Write code to compute the numbers dynamically.

Why do we have this code at all, the button should always be visible.

477

missing spaces at =

487

.hidden = can be pulled out of the if statement to reduce code

495

let? also unused, so delete

498

Should have an early return, so that the user can't be a buddy of himself.

selected == -1 if no player has been selected before can't occur with your patch on init becaues you hide the button, but instead the button should always be visible and just not do anything if the player is not selected.

However selected == -1 can occur if a player is selected but goes offline!

505

g_UserBuddies.join(",") duplication can be reduced

701

hasBuddies (in accordance with isBuddy)

704

If a player doesn't have a name, that's a serious bug outside of this code.
If we want this code to be immune towards players who register broken games, then this should be in the code that parses

706

just save the name without rating once, then we can get rid of that rating removal function altogether

727

String() is more explicit than adding an empty string to a number in order to convert it

728

Okay with the playing buddy vs. spectating buddy priorization, should have a comment in the code to clarify to future devs who don't read through this webpage

binaries/data/mods/public/gui/lobby/lobby.xml
99

I know we rejected doubleclick before, in particular since the feature might go entirely unnoticed if we didn't have a button instead, but IMO it is great to have both the button and the doubleclick feature, so we can quickly set many buddies without moving the mouse over half the screen.

We can remove it later if we want to use doubleclicking for something else.

elexis updated this revision to Diff 1309.Apr 17 2017, 7:25 PM

See remarks above

Vulcan added a subscriber: Vulcan.Apr 17 2017, 8:26 PM

Build is green

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

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

elexis updated this revision to Diff 1321.Apr 18 2017, 2:37 AM

Rename g_UserBuddies to g_Buddies, move the comma to g_BuddyListDelimiter and add an early return if somone used a chat client to /nick a name containing that delimiter.

elexis updated this revision to Diff 1322.Apr 18 2017, 2:44 AM

Reupload

Build is green

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

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

Build is green

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

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

elexis updated this revision to Diff 1351.Apr 19 2017, 8:40 AM

Add buddy symbol to the game info frame, so that it's also shown in the game selection details of the lobby, replay and load/savegame window, as proposed by ffffffff and causative.
Use smaller buddy symbol as proposed by ffffffff.
Filter empty buddy names as long as we have that workaround in default.cfg.

elexis updated this revision to Diff 1352.Apr 19 2017, 8:42 AM

Make that gamelist a chain of array functions too.

Build is green

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

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

Build is green

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

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

causative accepted this revision.EditedApr 19 2017, 11:01 AM

Looks good. Tested with elexis' data for playerlist and gamelist, buddying different users and sorting, as discussed on IRC.

My only concern is that it might be possible for a player to put newlines in their username using an XMPP client, which might allow them to add arbitrary lines to your user.cfg file if you buddy them. However, it's not proven.

Imarok added a subscriber: Imarok.Apr 19 2017, 11:12 AM
Imarok added inline comments.
binaries/data/mods/public/gui/common/gamedescription.js
15 ↗(On Diff #1352)

Why not just use a normal js array and JSON.stringify()/JSON.parse() to save/load it?
Then you'll have no problems with names containing g_BuddyListDelimiter.

elexis added inline comments.
binaries/data/mods/public/gui/common/gamedescription.js
15 ↗(On Diff #1352)

Wanted to keep that user.cfg editable by humans

oh, that happens when we edit the proposal simultaneously

Imarok added inline comments.Apr 19 2017, 11:53 AM
binaries/data/mods/public/gui/common/gamedescription.js
15 ↗(On Diff #1352)

Ähhmm, JSON is human readable ^^

elexis added inline comments.Apr 19 2017, 11:55 AM
binaries/data/mods/public/gui/common/gamedescription.js
15 ↗(On Diff #1352)

Still easy to introduce syntax errors for non-programmers since quotes must match everywhere, no trailing comma, array braces, potentially escaping. Better have it as simple as possible -> CSV

elexis updated this revision to Diff 1358.Apr 19 2017, 12:45 PM

Move removeRatingFromNick function to functions_utilities.js near the other playername functions, so it could be used without including the gamedescriptions file, as proposed by ffffffff.

ffffffff accepted this revision.Apr 19 2017, 1:07 PM
ffffffff added inline comments.
binaries/data/mods/public/gui/common/gamedescription.js
25 ↗(On Diff #1352)

this can be stored in some common file e.g. binaries/data/mods/public/gui/common/functions_utility.js as we can us it then also in other files like gamesdescription.js or so

binaries/data/mods/public/gui/lobby/lobby.js
410

as suggest by vladis seperat sort and map command. (previous mentioned filter was wrong).

467

im for showing username as buddied alltime

695

nice work u applied my status order for gamelists thx!

This revision was automatically updated to reflect the committed changes.

Thanks for the great patch on which we can base several new features (like notifications if a buddy joins a game or late observer joins only for buddies) ffffffff, and for the review and testing causative!

binaries/data/mods/public/gui/lobby/lobby.xml
26

all changed numbers ok

40

Don't need to translate an empty string I propose

213

same

In D209#14294, @elexis wrote:

Thanks for the great patch on which we can base several new features (like notifications if a buddy joins a game or late observer joins only for buddies) ffffffff, and for the review and testing causative!

tu too. we will make 0ad great again. happy coding for next features.

Build is green

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

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

Imarok added inline comments.Apr 19 2017, 5:34 PM
binaries/data/mods/public/gui/common/gamedescription.js
15 ↗(On Diff #1352)

Why should users edit the buddy list manually?