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.
Details
Details
- Reviewers
scythetwirler ffffffff causative - Commits
- rP19433: Mark players as buddies in the lobby and add a symbol to identify them in the…
- Trac Tickets
- #4159
Diff Detail
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
Comment Actions
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.
Comment Actions
splitted the map and filter command in update player und game list on the arrays. minor code optis.
Comment Actions
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.
Comment Actions
minor changes in buddy game sorting for priorizing buddies then game state and then game name in buddy sorting
Comment Actions
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 ↗ | (On Diff #1136) | this one "|" is added due to error in empty value in config. |
| binaries/data/mods/public/gui/lobby/lobby.js | ||
| 84 ↗ | (On Diff #1136) | for now i split buddy names by "|" sign if thinks is no allowed in names in lobby. |
| 410 ↗ | (On Diff #1136) | as suggest by vladis seperat sort and filter command. |
| 728 ↗ | (On Diff #1136) | 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 |
| 417 ↗ | (On Diff #1134) | i wanted to prefer here when selecting buddy sorting buddy then presence [online,away,playing..] and then name in this prio if its ok. |
Comment Actions
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 ↗ | (On Diff #1136) | ah, #3990 |
| binaries/data/mods/public/gui/common/functions_utility.js | ||
| 219 ↗ | (On Diff #1136) | 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 ↗ | (On Diff #1136) | 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 ↗ | (On Diff #1136) | Comments having the exact same character composition a the variable name should be removed. So if we want to keep this JSdoc format, it should explain in which situation this icon is used. |
| 84 ↗ | (On Diff #1136) | 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 ↗ | (On Diff #1136) | 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 ↗ | (On Diff #1136) | 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 |
| 454 ↗ | (On Diff #1136) | Using the alpha transparency to hide the sign we later add is pointless if we can just not add anything instead. |
| 474 ↗ | (On Diff #1136) | tabs instead of spaces |
| 476 ↗ | (On Diff #1136) | 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 ↗ | (On Diff #1136) | missing spaces at = |
| 487 ↗ | (On Diff #1136) | .hidden = can be pulled out of the if statement to reduce code |
| 495 ↗ | (On Diff #1136) | let? also unused, so delete |
| 498 ↗ | (On Diff #1136) | 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 ↗ | (On Diff #1136) | g_UserBuddies.join(",") duplication can be reduced |
| 701 ↗ | (On Diff #1136) | hasBuddies (in accordance with isBuddy) |
| 704 ↗ | (On Diff #1136) | If a player doesn't have a name, that's a serious bug outside of this code. |
| 706 ↗ | (On Diff #1136) | just save the name without rating once, then we can get rid of that rating removal function altogether |
| 727 ↗ | (On Diff #1136) | String() is more explicit than adding an empty string to a number in order to convert it |
| 728 ↗ | (On Diff #1136) | 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 |
| 417 ↗ | (On Diff #1134) | The design looks weird, but it's basically correct, since players have all unique names and ratings. |
| binaries/data/mods/public/gui/lobby/lobby.xml | ||
| 99 ↗ | (On Diff #1136) | 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. |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
| 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? |
| binaries/data/mods/public/gui/common/gamedescription.js | ||
|---|---|---|
| 15 ↗ | (On Diff #1352) | Wanted to keep that user.cfg editable by humans |
| binaries/data/mods/public/gui/common/gamedescription.js | ||
|---|---|---|
| 15 ↗ | (On Diff #1352) | Ähhmm, JSON is human readable ^^ |
| 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 |
Comment Actions
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.
| 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 | ||
| 467 ↗ | (On Diff #1352) | im for showing username as buddied alltime |
| 695 ↗ | (On Diff #1352) | nice work u applied my status order for gamelists thx! |
| 410 ↗ | (On Diff #1136) | as suggest by vladis seperat sort and map command. (previous mentioned filter was wrong). |
Comment Actions
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 ↗ | (On Diff #1358) | all changed numbers ok |
| 40 ↗ | (On Diff #1358) | Don't need to translate an empty string I propose |
| 213 ↗ | (On Diff #1358) | same |
Comment Actions
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.
| binaries/data/mods/public/gui/common/gamedescription.js | ||
|---|---|---|
| 15 ↗ | (On Diff #1352) | Why should users edit the buddy list manually? |