Page MenuHomeWildfire Games

Clientside Lobby Kick support
ClosedPublic

Authored by elexis on Feb 1 2017, 5:06 AM.

Details

Summary

Players getting kicked or banned in the lobby is supported natively on the server as that runs ejabberd.
But the 0 A.D. application has no support for handling kick/ban events at all:

  • The kicked client will leave the muc room, but it won't disconnect.
  • It will appear entirely functional, as the player and gamelist is still remaining and the host button is still enabled.
  • The kicked client will not receive any (chat nor messagebox) notification that it was kicked.
  • The other clients won't notice that the client was kicked (it will show the client left the room).
  • The kick reason is not being shown either.
  • Moderators using the 0 A.D. application to kick clients can't send a disconnect reason.

The patch fixes all of that. Also contains some cleanup:

  • Removes duplicate "color" property from addChatMessage as that is only used for system messages (right? no external files?)
  • Adds debug output when leaving the lobby room for unkown reasons.
  • Removes gloox 1.0.0 check, since minimum required version is 1.0.9 now (http://trac.wildfiregames.com/wiki/BuildInstructions).
  • GetPlayerList always returns an array, even when disconnected.

Looked into replacing all ircSplit occurances with regex, but would just add much more parsing code and three or more regex.

Test Plan

Needs a moderator account and usual account to get kicked. Make sure the features work as advertised and that the code looks sane.
Make sure the popup box has enough gui space for long disconnect reasons.

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.Feb 1 2017, 5:06 AM
elexis updated this revision to Diff 425.Feb 1 2017, 5:22 AM

Don't remove the "leave" gui message and use "special" chat messages when displaying the kick to others.

binaries/data/mods/public/gui/lobby/lobby.js
119 ↗(On Diff #424)

As far as I could reproduce, this event is only called when calling DisconnectXmppClient. The patch adds the first occurance of that.

@fatherbushido you had many lobby disconnects recently, was the "Disconnected." chat message printed then?

204 ↗(On Diff #424)

ternaries are right-associative

223 ↗(On Diff #424)

Displaying the reason publicly since that's in line with xmpp. We could have a business policy to hide that, idc

820 ↗(On Diff #424)

multiple regex here would use much more code than this one ugly char

source/lobby/XmppClient.cpp
815 ↗(On Diff #424)

There are some other flags we could test for. For example controlled room shutdown. The case probably doesn't occur, as on maintenance the entire server will be down.

source/lobby/scripting/JSInterface_Lobby.cpp
182 ↗(On Diff #424)

This change is needed, as the JS code expects an array on disconnect

elexis updated the Trac tickets for this revision.Feb 1 2017, 5:46 AM
elexis added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
199 ↗(On Diff #425)

This function could be inlined if we had #4482

Vulcan added a subscriber: Vulcan.Feb 1 2017, 5:51 AM

Build is green

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

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

Vulcan added a comment.Feb 1 2017, 6:37 AM

Build is green

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

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

elexis marked an inline comment as done.Feb 4 2017, 5:53 PM
elexis added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
119 ↗(On Diff #424)

Confirmed. fatherbushido receives only the CASE(ConnIoError, g_L10n.Translate("An I/O error occured")); string, but never saw the one removed. The disconnect never becomes visible until he starts trying to send a chat message, which fails and returns that I/O error.

Sandarac added inline comments.
source/lobby/scripting/JSInterface_Lobby.cpp
72 ↗(On Diff #425)

Is it possible that using a cast here might be (ever-so-slightly) slower?

elexis marked an inline comment as done.Feb 9 2017, 9:38 PM

Also mentioned by scythetwirler, we should show who kicked (actor), even though gloox hasn't implemented it yet and the ejabberd version we use it neither:
http://xmpp.org/extensions/xep-0045.html#kick
https://support.process-one.net/browse/EJAB-1206

The most recent guy on the lobby that got banned about 5 times on the same day requested that we ban him not only from the lobby but also from the game he joined:
" (...) I am in actually in a game right now. (...) Next time you gonna ban someone, at least do it right. (...)",
proving our moderator @Hannibal_Barca correct who often used the 'christmas-tree' bug to crash lobby participants who deserved to be kicked.

Instead of using such a workaround, this can be implemented properly (in a separate patch) by pulling only those GUI messages where the local client got kicked from the lobby and if such a message exists, show the message box with the kick reason and return to the main menu.

Imarok added inline comments.Feb 24 2017, 4:46 PM
binaries/data/mods/public/gui/lobby/lobby.js
119 ↗(On Diff #424)

That's wrong. Join the lobby with two instances of the game and the same account: One will get disconnected and receive the system message: "Disconnected.Stream error"

168 ↗(On Diff #425)

throws a warning for undefined reason (msg.data)

source/lobby/scripting/JSInterface_Lobby.cpp
185 ↗(On Diff #425)

Don't remove that.

elexis updated this revision to Diff 599.Feb 24 2017, 7:41 PM

Thanks for the proper review and finding my bugs!

  • Revert a wrong removal of an undefined check in C++
  • Remove unneeded cast
  • Don't remove the disconnect chat message as it can actually be triggered. The g_Kicked variable can be removed once we can create GUI messages with arbitrary data.
  • Fix msg.data undefined error if no reason is given. (Strange, almost certain I tested that)
  • Display a system message for the kicked ones while showing a special (white "== foo") message for the other lobby participants
  • Disable leaderboard and profile lookup button as well. (Notice join button becoming hidden after clearing the gamelist)
  • Notice we had established that no user called "system" can register anymore (nick already taken) and that it wouldn't matter much if the user with that nick would be displayed in the system color

Looking into adding the session kick support now

leper added inline comments.Feb 24 2017, 7:47 PM
source/lobby/glooxwrapper/glooxwrapper.cpp
150 ↗(On Diff #599)

Removing this without erroring out if the version used is not new enough is just asking for trouble.

Build is green

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

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

elexis updated this revision to Diff 608.Feb 25 2017, 5:54 AM

Don't remove the workaround for an unsupported gloox version until I got around to testing whether it throws an error when trying to compile against it.

elexis added inline comments.Feb 25 2017, 5:55 AM
source/lobby/glooxwrapper/glooxwrapper.cpp
150 ↗(On Diff #599)

As the build instructions say 1.0.9 is the minimum verison required, I had hoped for a compile error with earlier versions.
I'll remove the hunk for now and can test once I'll compile gloox again for the update to D87 (patch was merged upstream).

Thanks!

Build is green

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

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

The mention of 1.0.9 is only there because earlier versions had some connection problems. IIRC everything since 1.0.0 is source code compatible, so it will build, and then it would leak.

Imarok added inline comments.Feb 25 2017, 5:22 PM
binaries/data/mods/public/gui/lobby/lobby.js
228 ↗(On Diff #608)

missing ;

Imarok requested changes to this revision.Feb 27 2017, 9:05 PM

missing semicolon and the duplicated nick == g_Username check

This revision now requires changes to proceed.Feb 27 2017, 9:05 PM
elexis updated this revision to Diff 636.Feb 28 2017, 10:58 AM
elexis edited edge metadata.

Fix an actual bug, the reason was printed twice in the message box!
Add two missing semicolons.
Don't merge the two "nick == g_Username" checks as that implies not only duplicating the + " " + reason part but also makes the addChatMessage calls uglier.

Build is green

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

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

Imarok accepted this revision.Feb 28 2017, 11:53 AM

can committed with the two missing spaces

source/lobby/XmppClient.cpp
804 ↗(On Diff #636)

Space missing

809 ↗(On Diff #636)

Space missing

This revision is now accepted and ready to land.Feb 28 2017, 11:53 AM
elexis updated this revision to Diff 638.Feb 28 2017, 11:58 AM

Added \n to the message box without adding it to the chat message, which incidentally gets rid of the sprintf which is only used in one of two cases.

Thanks for picking this up everyone else didn't care about!

Build is green

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

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

This revision was automatically updated to reflect the committed changes.
scythetwirler edited edge metadata.
This comment was removed by scythetwirler.