Page MenuHomeWildfire Games
Differential D835

Allow specifying custom properties in the XmppClient GUIMessage struct (instead of nuking it and using JS Value)
ClosedPublic
Actions

Authored by elexis on Aug 26 2017, 4:19 AM.

Details

Reviewers
None
Commits
rP20064: XmppClient cleanup.
Trac Tickets
#4482
Summary

Purpose:
The lobby.js parsing XmppClient.cpp GUI messages is ugly, because it forces every message to use the property names text and data,
while every message should use its own names, for example oldnick and newnick.
Furthermore messages might want to send an arbitrary number of messages.

The patch is working besides two issues:

Challenge 1: It segfaults when starting to host a game in the lobby (somewhere in SpiderMonkey after the Engine.LobbySetPlayerPresence("playing"); call in gamesetup_mp.js.
I assume this is because it uses the g_GUI->GetActiveGUI()->GetScriptInterface() after having switched to another GUI page.
I tried to create a new ScriptInterface in the ctor (this is what the NetServer that runs in a separate thread does), but that segfaults immediately on that statement.
Also tried to use the ScriptInterface that is passed in JSI_Lobby::StartXmppClient, but that also doesn't work (crashes much sooner).

Since the previous code didn't save JS::Values, it only came into contact with any ScriptInterface when the GUI ordered something.
But now we need a ScriptInterface whenever receiving a message from gloox.

Challenge 2: Using macros to remove the duplication in CREATE_GUI_MESSAGE_0, CREATE_GUI_MESSAGE_1, CREATE_GUI_MESSAGE_2 and potentially support arbitrary number of arguments.

Test Plan
  1. Halp. 2. Make sure the lobby still works as advertized.

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.Aug 26 2017, 4:19 AM
Vulcan added a subscriber: Vulcan.Aug 26 2017, 5:07 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!
Checking XML files...

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

Vulcan added a comment.Aug 26 2017, 5:10 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/lobby/prelobby.js
| 163| »   »   switch(message.level)·{
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 223| »   switch(page)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 155| »   while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  41|  41|  * The playerlist will be assembled using these values.
|  42|  42|  */
|  43|  43| const g_PlayerStatuses = {
|  44|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  44|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  42|  42|  */
|  43|  43| const g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  45|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  43|  43| const g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  46|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  47|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|  50|  50| 

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

binaries/data/mods/public/gui/lobby/lobby.js
|1164| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/lobby/lobby.js
|1307| »   »   switch·(command)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/lobby.js
| 483| »   if·(mapSizeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 487| »   if·(playersNumberFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 491| »   if·(mapTypeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 622| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/439/ for more details.

Harbormaster completed remote builds in B2901: Diff 3316.Aug 26 2017, 5:10 AM
elexis updated this revision to Diff 3329.Aug 27 2017, 12:49 PM

CloneValueFromOtherContext in GuiPollMessage as conjectured by leper does fix the segfault.
It still seems like bad practice to use the script interface of the topmost GUI page.

elexis updated this revision to Diff 3330.Aug 27 2017, 1:02 PM

Use ScriptInterface of the GUI Manager instead of the one of the topmost GUI page.
Should be less likely to f up when opening the lobby page on top of the gamesetup page.
That ScriptInterface most likely won't be available to the dedicated server, but we can probably pass a custom interface for the XmppClient to be used.

Harbormaster completed remote builds in B2919: Diff 3330.Aug 27 2017, 1:02 PM
elexis updated this revision to Diff 3331.Aug 27 2017, 3:30 PM

Remove the duplicated macros with a method enforcing std::strings and optional arguments to mimic overloading.

elexis updated this revision to Diff 3332.Aug 27 2017, 4:21 PM

Fix prelobby.js, remove 2 comments, remove braces, inline one var, give up on the argument type freedom macro could provide.

Harbormaster completed remote builds in B2921: Diff 3332.Aug 27 2017, 4:21 PM
ffffffff added a subscriber: ffffffff.Aug 27 2017, 5:27 PM
Vulcan added a comment.Aug 27 2017, 10:40 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/lobby/prelobby.js
| 165| »   »   switch(message.level)·{
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 225| »   switch(page)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 157| »   while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  41|  41|  * The playerlist will be assembled using these values.
|  42|  42|  */
|  43|  43| var g_PlayerStatuses = {
|  44|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  44|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  42|  42|  */
|  43|  43| var g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  45|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  43|  43| var g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  46|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  47|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|  50|  50| 

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

binaries/data/mods/public/gui/lobby/lobby.js
|1164| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/lobby/lobby.js
|1305| »   »   switch·(command)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/lobby.js
| 483| »   if·(mapSizeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 487| »   if·(playersNumberFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 491| »   if·(mapTypeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 622| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/445/ for more details.

Vulcan added a comment.Aug 28 2017, 12:23 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/lobby/prelobby.js
| 165| »   »   switch(message.level)·{
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 225| »   switch(page)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 157| »   while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  41|  41|  * The playerlist will be assembled using these values.
|  42|  42|  */
|  43|  43| var g_PlayerStatuses = {
|  44|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  44|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  42|  42|  */
|  43|  43| var g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  45|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  43|  43| var g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  46|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  47|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|  50|  50| 

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

binaries/data/mods/public/gui/lobby/lobby.js
|1164| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/lobby/lobby.js
|1305| »   »   switch·(command)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/lobby.js
| 483| »   if·(mapSizeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 487| »   if·(playersNumberFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 491| »   if·(mapTypeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 622| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/446/ for more details.

Vulcan added a comment.Aug 28 2017, 2:50 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!
Checking XML files...

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

Harbormaster completed remote builds in B2918: Diff 3329.Aug 28 2017, 2:51 AM
Vulcan added a comment.Aug 28 2017, 3:37 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!
Checking XML files...

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

Harbormaster completed remote builds in B2919: Diff 3330.Aug 28 2017, 3:37 AM
ffffffff added a comment.Aug 28 2017, 7:40 AM

this seems very good.

elexis added a comment.Aug 28 2017, 4:16 PM
In D835#32999, @ffffffff wrote:

this seems very good.

Should check for correctness and completeness too.

Errors you could have realistically found in this iteration of the patch:

  • forgot to update the kicked case in lobby.js
  • The error cases in the C++ part missed the property name text
  • doesn't actually work with the GUI Manager script interface (random spidermonkey segfault stacktrace) but only with the topmost GUI page

Also the defines don't add anything useful, they can be inlined.

I've tried again creating a new ScriptInterface and it amazingly works. Until opening the gamesetup and returning that is.
Somehow it can't read from the GUI message queue anymore and segfaults in SpiderMonkey.

xmpp_custom_scriptinterface.patch29 KBDownload

Using a new ScriptInterface would be much more reliable and would work for the dedicated server for example, if the GUI isn't initialized.

But since I gave up on the macro (thus being forced to use (A) std::strings and (B) empty strings if a property isn't defined and (C) not provide more than 2 custom properties),
there is no reason anymore to use a custom ScriptInterface to begin with. The two new strings can just be part of the message queue.

So if we want to add further properties in the future (more than 2 or something that isn't string), then we will have to get back to this.

elexis retitled this revision from Nuke XmppClient GUIMessage struct and use JS Value instead to Allow specifying custom properties in the XmppClient GUIMessage struct (instead of nuking it and using JS Value).Aug 28 2017, 4:21 PM
elexis added inline comments.Aug 28 2017, 5:06 PM
source/lobby/XmppClient.cpp
207 ↗(On Diff #3332)

Either the default values have to be specified in the header file or this file has to be here at the top before any use of it. Prefering the prior in the new patch.

source/lobby/XmppClient.h
148 ↗(On Diff #3332)

Also no reason to copy these strings -> const std::string&

elexis updated this revision to Diff 3351.Aug 28 2017, 5:07 PM

Give up on everything the ticket wants to achieve except better property names.

elexis mentioned this in D819: Keep lobby history.Aug 28 2017, 5:17 PM
Vulcan added a comment.Aug 28 2017, 5:55 PM

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!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1927/ for more details.

Vulcan added a comment.Aug 28 2017, 5:57 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/lobby/prelobby.js
| 165| »   »   switch·(message.level)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 232| »   switch(page)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 157| »   while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  41|  41|  * The playerlist will be assembled using these values.
|  42|  42|  */
|  43|  43| var g_PlayerStatuses = {
|  44|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  44|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  42|  42|  */
|  43|  43| var g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  45|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  43|  43| var g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  46|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  47|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobb

http://jenkins-master:8080/job/phabricator_lint/447/ for more details.

Harbormaster completed remote builds in B2925: Diff 3351.Aug 28 2017, 5:57 PM
elexis added a comment.Aug 28 2017, 6:27 PM

(17:38:24) Vladislav: elexis: why you changed wstring to string? In d835
(17:41:52) Vladislav: elexis: I don't know how it's used. I'm wondering, if there is a wstring depended place

Thanks, that would have been a regression.
By posting an uncommon character like ȡ (LATIN SMALL LETTER D WITH CURL (U+0221)) in the lobby with this patch applied,
we see it only correctly displayed if the wstring_from_utf8 is applied to the string before passing it to JS.
Since every single of our data sources is a std::string, storing them as such is perfectly fine as long as we do the conversion in the scriptInterface.SetProperty call.

elexis added a comment.Aug 28 2017, 7:21 PM

Also found an unused variable, so might just as well add a "foo changed the subject to" chat message while at it:

Closed by commit rP20064: XmppClient cleanup. (authored by elexis). · Explain WhyAug 28 2017, 7:47 PM
This revision was automatically updated to reflect the committed changes.
elexis mentioned this in rP20069: Fix typo in rP20064 / D835 reported by gameboy..Aug 29 2017, 2:22 PM
elexis added inline comments.Nov 16 2019, 12:44 AM
ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js
155

Nick seems always non-empty, and the specs say it MUST be non-empty:

The MUC service MUST reflect the message to all other occupants with a 'from' address equal to the room JID or to the occupant JID that corresponds to the sender of the subject change:

https://xmpp.org/extensions/xep-0045.html#subject-mod

elexis mentioned this in rP23172: Rewrite lobby page to use class semantics, add more gamedetails labels, improve….Nov 21 2019, 2:45 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
lobby/
44 lines
11 lines
source/
lobby/
22 lines
136 lines

Diff 3352

View Options

ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/lobby/prelobby.js

Loading...
View Options

ps/trunk/source/lobby/XmppClient.h

Loading...
View Options

ps/trunk/source/lobby/XmppClient.cpp

Loading...
Wildfire Games · Phabricator