Page MenuHomeWildfire Games

Fix lobby playerlist rebuild following presence update message removal in rP22855/D2265
ClosedPublic

Authored by elexis on Sep 11 2019, 1:15 PM.

Details

Summary

In rP22855/D2265, the presence message was removed for the purpose of
(1) removing the LobbyClearPresenceUpdates function and for
(2) removing the boolean return values in lobby.js that hardcoded whether the playerlist should be updated.

There was a logical mistake however.

The playerlist needs to be updated whenever a property changes that influences the lobby playerlist state.

This is the case whenever m_PlayerMap is updated, not only for this one message type.
In particular it should always be updated where the previous lobby.js code returned true and not when it returned false.

Test Plan

On the concept:
Count the number of cases in JS where the author had to make a decision to update the playerlist,
and count the number of cases in C++ where the author has to consider the member boolean.
Compare the two numbers and notice that the new code is easier to maintain than the previous one.

On the correctness:
Compare the booleans from rP20040 removed in rP22855 in lobby.js to the XmppClient member boolean.

On the function name:
Notice that the function name doesn't include a Get, like the other functions above, but a Poll.
Consider that this is consitent with the name LobbyGuiPollNewMessage which pops one element of the queue and returns that (the analogy is to pop the boolean).

I don't mind about renaming Poll to GetAndClear.
Notice that there are other function name inconsistencies that could be cleaned in a separate commit if there is demand:

  • Some functions use Gui, other functions use GUI
  • Some functions have Gui, other functions don't have that prefix.

Notice that the bug is quite bad (sorry about that!) and blocks future work on the lobby.

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.Sep 11 2019, 1:15 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/131/display/redirect

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

Linter detected issues:
Executing section Source...

source/lobby/scripting/JSInterface_Lobby.h
|  26| namespace·JSI_Lobby
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_Lobby{' is invalid C code. Use --std or --language to configure the language.

source/lobby/IXmppClient.h
|  24| namespace·StunClient·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/lobby/XmppClient.h
|  24| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  47|  47|  * The playerlist will be assembled using these values.
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  50|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'away'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away": { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'playing'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing": { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'offline'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline": { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'unknown'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|    |-	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    |  54|+	"unknown": { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|  57|  57| var g_RoleNames = {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 210| 210| 					me ?
| 211| 211| 						translate("You have been muted.") :
| 212| 212| 						translate("%(nick)s has been muted.") :
| 213|    |-				msg.newrole == "moderator" ?
|    | 213|+					msg.newrole == "moderator" ?
| 214| 214| 					me ?
| 215| 215| 						translate("You are now a moderator.") :
| 216| 216| 						translate("%(nick)s is now a moderator.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 211| 211| 						translate("You have been muted.") :
| 212| 212| 						translate("%(nick)s has been muted.") :
| 213| 213| 				msg.newrole == "moderator" ?
| 214|    |-					me ?
|    | 214|+						me ?
| 215| 215| 						translate("You are now a moderator.") :
| 216| 216| 						translate("%(nick)s is now a moderator.") :
| 217| 217| 				msg.oldrole == "visitor" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 212| 212| 						translate("%(nick)s has been muted.") :
| 213| 213| 				msg.newrole == "moderator" ?
| 214| 214| 					me ?
| 215|    |-						translate("You are now a moderator.") :
|    | 215|+							translate("You are now a moderator.") :
| 216| 216| 						translate("%(nick)s is now a moderator.") :
| 217| 217| 				msg.oldrole == "visitor" ?
| 218| 218| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 213| 213| 				msg.newrole == "moderator" ?
| 214| 214| 					me ?
| 215| 215| 						translate("You are now a moderator.") :
| 216|    |-						translate("%(nick)s is now a moderator.") :
|    | 216|+							translate("%(nick)s is now a moderator.") :
| 217| 217| 				msg.oldrole == "visitor" ?
| 218| 218| 					me ?
| 219| 219| 						translate("You have been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 214| 214| 					me ?
| 215| 215| 						translate("You are now a moderator.") :
| 216| 216| 						translate("%(nick)s is now a moderator.") :
| 217|    |-				msg.oldrole == "visitor" ?
|    | 217|+						msg.oldrole == "visitor" ?
| 218| 218| 					me ?
| 219| 219| 						translate("You have been unmuted.") :
| 220| 220| 						translate("%(nick)s has been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 215| 215| 						translate("You are now a moderator.") :
| 216| 216| 						translate("%(nick)s is now a moderator.") :
| 217| 217| 				msg.oldrole == "visitor" ?
| 218|    |-					me ?
|    | 218|+							me ?
| 219| 219| 						translate("You have been unmuted.") :
| 220| 220| 						translate("%(nick)s has been unmuted.") :
| 221| 221| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 216| 216| 						translate("%(nick)s is now a moderator.") :
| 217| 217| 				msg.oldrole == "visitor" ?
| 218| 218| 					me ?
| 219|    |-						translate("You have been unmuted.") :
|    | 219|+								translate("You have been unmuted.") :
| 220| 220| 						translate("%(nick)s has been unmuted.") :
| 221| 221| 					me ?
| 222| 222| 						translate("You are not a moderator anymore.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 217| 217| 				msg.oldrole == "visitor" ?
| 218| 218| 					me ?
| 219| 219| 						translate("You have been unmuted.") :
| 220|    |-						translate("%(nick)s has been unmuted.") :
|    | 220|+								translate("%(nick)s has been unmuted.") :
| 221| 221| 					me ?
| 222| 222| 						translate("You are not a moderator anymore.") :
| 223| 223| 						translate("%(nick)s is not a moderator anymore.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 218| 218| 					me ?
| 219| 219| 						translate("You have been unmuted.") :
| 220| 220| 						translate("%(nick)s has been unmuted.") :
| 221|    |-					me ?
|    | 221|+							me ?
| 222| 222| 						translate("You are not a moderator anymore.") :
| 223| 223| 						translate("%(nick)s is not a moderator anymore.");
| 224| 224| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 219| 219| 						translate("You have been unmuted.") :
| 220| 220| 						translate("%(nick)s has been unmuted.") :
| 221| 221| 					me ?
| 222|    |-						translate("You are not a moderator anymore.") :
|    | 222|+								translate("You are not a moderator anymore.") :
| 223| 223| 						translate("%(nick)s is not a moderator anymore.");
| 224| 224| 
| 225| 225| 			addChatMessage({
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 220| 220| 						translate("%(nick)s has been unmuted.") :
| 221| 221| 					me ?
| 222| 222| 						translate("You are not a moderator anymore.") :
| 223|    |-						translate("%(nick)s is not a moderator anymore.");
|    | 223|+								translate("%(nick)s is not a moderator anymore.");
| 224| 224| 
| 225| 225| 			addChatMessage({
| 226| 226| 				"text": "/special " + sprintf(txt, { "nick": msg.nick }),

binaries/data/mods/public/gui/lobby/lobby.js
|1012| »   »   switch·(sortBy)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/640/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.Sep 11 2019, 1:51 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 11 2019, 1:51 PM