[PATCH] Lobby - Player name appears twice in list

[elexis]

It frequently happens that a nickname is displayed multiple times in the multiplayer lobby. Sometimes one as busy and the other one as online. If one reconnects to the lobby, the dupe disappears. Sometimes when the buddy goes offline one busy dupe remains, so that the player is displayed as connected to the lobby, although he isn't. It is probably only a bug in the GUI. I'll try to figure it out.

[historic_bruno]

Hi, please leave tickets for Backlog unless there is a patch (or they are a confirmed release blocker).

[elexis]

Also if the player has left the lobby, but was selected previously, then the ranking information diplayed is not cleared. Might be considered a feature, but might also leave the impression that the player is still online.

See #3244

[elexis]

Replying to [ #3206:comment:6 sanderd17]:

I guess the simulation update arrives at the popup window, rather than the underlaying simulation. So perhaps the easiest way to update the gui is to forward the function when received?

Maybe the lobby userlist updates don't reach the lobby window, but the session GUI instead. In this case forwarding would fix the issue. Another option would be to clear the userlist and fill it again after finishing a game and returning to lobby.

[elexis]

I used this patch to dump the playerlist received by the c++ xmpp client. That one seems to work fine. Each time I got an update, there were no dupes in the dump, although there were dupes in the GUI (even an offline player displayed 2 times as busy).

[elexis]

The file above contains another patch which I tested today. It shows an error message each time the lobby playerlist is refreshed, an update for an individual player is received, or an error occurs. I added two anonymized logfiles that shows the output produced by the patch. For each logfile I only connected to the lobby, played a game, returned to the lobby and quit the application.

Observation:

1) No updates while the game is running. While the gamesetup is displayed and while the game is displayed, there are no errors thrown, i.e. the playerlist is not updated.

2) Hundreds of updates after the game. When I returned from the game to the lobby, the playerlist is refreshed, but after that hundreds of muc updates appear. In the logfiles provided it were 496 muc updates in 2 seconds and 1720 muc updates in 8 seconds which were processed just after returning to the lobby.

3) Inconsistent muc messages When processing those hundreds of muc messages after returning to the lobby, some code lines are reached that shouldn't be reached if all messages were processed in the right order. For example updates are processed where a player goes offline that wasn't online before.

---

To reproduce, apply the patch, connect to the a18 lobby, start a game (you dont need to play it, just leave it open for some time) and then return to the lobby.

---

Conclusion (2) and (3) indicate that the muc updates are saved while the game is running, but processed after the game has ended and after the playerlist was refreshed.

---

Fix Don't process old messages after returning to the lobby, i.e. clear the message queue when updating the player list.

[elexis]

This bug will then also be the reason why the app freezes for some seconds when clicking the return to lobby button after playing a long game.

How the code works: All the lobby presence notifications and text messages are saved in the c++ part of the code in m_GuiMessageQueue in XmppClient.cpp and polled in OnTick() in lobby.js. The lobby code parses the following message types, which are all contained in that queue:

case "mucmessage": // For room messages
case "message": // For private messages
case "muc":
	case "join":
	case "leave":
	case "nick":
	case "presence":
	case "subject":
case "system":
	case "standard":
	case "error":
	case "internal":
		case "gamelist updated":
		case "boardlist updated":
		case "ratinglist updated":
		case "profile updated":

The best way to fix this is to clear those outdated muc updates, i.e. loop over all messages in the c++ part of the code and delete them if they have the type "muc". This way the old text messages will still be received. Additionally the timestamp of the "muc" messages should be compared in the lobby.js code, so that old updates will definitely not processed.

An alternative idea would be to call the lobby.js OnTick() function from session.js and the scores. But this would probably not fix inconsistent muc updates, if we don't clear old muc updates when fetching a new playerlist.

[elexis]

Clears all muc messages (presence updates) in the c++ part when fetching fresh player lists. Should fix the issue. TODO: Find a way to reproduce the originally described bug and prove that the bug fixes it. I will test it in a18 too.

[elexis]

Moved the call to clearing the muc messages to c++ to further increase performance. Less code. Unfortunately the muc messages don't get a timestamp by the glooxwrapper, so checking the timestamp before processing the remaining messages is not the most viable option.

[elexis]

I applied t3171_clear_muc_messages_v2.patch​ to alpha 18 and also the patch in test2_patch_and_logs.7z​ in order to test it. Here are the resulting anonymized logfiles of four test sessions.

[elexis]

As mentioned one comment above, I tested alpha 18 with the patch attachment:t3171_clear_muc_messages_v2.patch:ticket:3171​ and that debug patch in attachment:test2_patch_and_logs.7z:ticket:3171​.

We can see that the patch works as expected, i.e.

1. No more hundreds and thousands of outdated muc updated after returning to the lobby.

2. No more inconsisten muc updates received (Things that shouldn't happen, like players leaving who are not online).

3. I couldn't find any dupes in all the test sessions.

So far I'd say the patch (attachment:t3171_clear_muc_messages_v2.patch:ticket:3171​) is ready to go...

There is one curiousity that can be noticed when looking at the logfiles. When initially logging into the lobby, the app fetches a clean playerlist 2 times instantaneously. The first playerlist only contains a small subset of all the players online. The second playerlist is complete. Between the first and second playerlist there are a bunch of muc updates (like 5-20). Those two fetches and muc updates happen in the same second. There are calls to updatePlayerList() on init, join, system messages and rating list update, so it isn't an issue.

[elexis]

I still use this patch in my a18 and couldn't find any problems with it for now.

Since all muc updates are deleted now when fetching clean playerlists, all those updates (Player has joined / has left) will not be displayed if you join the lobby initially or return from a game, but only when you stay in the lobby for some time and updates come in.

This means if you return from the lobby, you receive all text messages but no updates.

This has a small side effect: If a user you just played with (1) left the game before you did, posted a text message to the lobby and went offline before you returned to the lobby, then you see his text in the lobby but not that notification that he went offline. His/her nickname is not in the online player list, so you still can identify that he's offline.

The only way to fix that side effect AND the originally described issue would be to process all muc updates while the game is running AND to never fetch clean playerlists (since fetching clean updates invalidates old updates).

So I still think this is the most feasible solution.

[elexis]

If one returns to the lobby, the presence states (away / online / busy) are also initialized correctly.

[elexis]

Same patch but compatible to a18. Having a playerlist that works also has the advantage that it is alphabetically sorted, so that you can find a specific player easier.

[elexis]

Removes the g_joined variable which is not needed anymore with this patch. The removal of this variable also fixes #3287 then.

[elexis]

Cleaned code by using erase-remove idiom.

[elexis]

Cleaned even more using lambda.

[elexis]

Moved the g_joined variable and functionality from JS to C++, so that its value is saved properly instead of being reset when returning from the gamesetup. Cleans code that way too.

[elexis]

The patch above is buggy because it removes the g_joined function completely. Cleaning that way doesn't work obviously. Joshua noted on irc on June 25th that this part is needed, we should just save the variable in the C++ part.

---

r16796 introduced another critical flaw. It changed push_back to emplace_back and this change in XmppClient::PushGuiMessage somehow prevents the "joined" muc message for the current player from being sent (The message sets "g_joined = true;" in lobby.js in line 706 in the function onTick(). Since g_joined will never be true, no joins will be displayed at all!

Solution: If you use push_back instead of emplace_back, it works again and the message is being sent again.

[Itms]

Replying to elexis:

Solution: If you use push_back instead of emplace_back, it works again and the message is being sent again.

No, this is not a "solution" as long as you don't know what is the problem. What should be understood is why the message is not sent, which must happen somewhere else in the code. Taking a look at the content of m_GuiMessageQueue before and after the emplacing would be a good start.

Side note: we all now you have a nice pile of tickets awaiting for review. :p So it might be nice to remove the review keyword whenever you find one of your patches is not working. As you're working on it, you can leave that to A19 milestone, that's not a problem.

[elexis]

Moves g_joined to the c++, so that presence updates will be sent only after you joined. Yesterday multiple people experienced the bug described in the comment above, but today I couldn't reproduce it with svn. I will have to apply this thing and the other lobby commits to a18 to test it thoroughly.

[Josh]

Fixed in r16961:

Fix 3171 by moving some logic to C++. These changes do not optimize the presence buffer and only fix the state bug.

[elexis]

The duplicate-users bug and performance-fix mentioned in comments 1-9 will be done in #3386.