Page MenuHomeWildfire Games

Support glooxwrapper::MUCRoom for muc events
ClosedPublic

Authored by elexis on Sep 13 2019, 4:42 PM.

Details

Summary

The XmppClient functions handleMUCMessage, handleMUCParticipantPresence, handleMUCSubject, handleMUCError receive a glooxwrapper::MUCRoom as the first argument.
That argument is currently unused.
If someone was to consider using it, he would get nothing but a null-deref segfault.
I suppose the intent of a library is not to segfault if one of its arguments is used.
Therefore either the argument must be removed because it is considered useless (from the glooxwrapper library POV),
or the argument must be provided, because it is considered useful (from the glooxwrapper library POV).

At some later time (needs more refactoring), one may become able to support using the XmppClient with multiple rooms.
That could have a realistic use case for current 0ad, since there is the players chat room and the moderators chat room.
But the point of the diff is to implement on the XmppClient what gloox provides us and pass it on to JS, so that JS can decide what to do with that information (Instead of segfaulting on top of it)

Test Plan

Deduce the objective of the patch from the mentioned circumstances and verify that it implements that in the stated scope.
Triplecheck that there is no memory leak introduced, and verify with valgrind while finding other leaks.
Notice that there shouldn't be a leak, because no new pointer is introduced, and because setting m_Owned to true results in double-free.
Not ice that the performance impact should be verly low, since the wrapper class only stores two pointers and a boolean.
Notice that there are many unsupported features that would be better off being supported (visitor role, ban support are historic examples), so this is one of them and thus a good idea to implement.
Noice that it would be very inviting to add the "room": room.name() line to every CreateGUIMessage call.
Notice that displaying the roomname is pointless as long as there is only support for one room however.
Notice that adding the commented out property also is useless, hence UNUSED() being used properly (instead of semi-hiding the NULL).
Notice that we want a mutable reference to the room, so that one could call room.setNick("foobar"); or other mutable methods of the room.

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 13 2019, 4:42 PM

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

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

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

Linter detected issues:
Executing section Source...

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

source/lobby/glooxwrapper/glooxwrapper.h
|  88| namespace·glooxwrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

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

elexis edited the test plan for this revision. (Show Details)Sep 13 2019, 4:52 PM
elexis updated this revision to Diff 9739.Sep 13 2019, 5:01 PM

Remove const to allow setNick and alike.

elexis updated this revision to Diff 9740.Sep 13 2019, 5:02 PM

Don't run arc diff --update while svn is locked.

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

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

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

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

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

Linter detected issues:
Executing section Source...

source/lobby/glooxwrapper/glooxwrapper.h
|  88| namespace·glooxwrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceglooxwrapper{' 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...
Executing section cli...

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

elexis removed a reviewer: Itms.Sep 13 2019, 5:09 PM

Apparently it's not a good idea to start svn commit, start typing a commit message, then deciding to run another arc diff --update, that had uploaded more than 1000 files for some reason.

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

Linter detected issues:
Executing section Source...

source/lobby/glooxwrapper/glooxwrapper.h
|  88| namespace·glooxwrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceglooxwrapper{' 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...
Executing section cli...

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

This revision was not accepted when it landed; it landed in state Needs Review.Sep 13 2019, 5:24 PM
This revision was automatically updated to reflect the committed changes.
In D2287#95144, @Vulcan wrote:

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

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

@Itms should not this have gloox flag?