Participants list does not delete old nick when a participant changes it in XMPP MUC

Categories

(Chat Core :: XMPP, defect)

This is the core/back-end of Instantbird and Thunderbird chat (all code in the chat/ directory). This includes the JavaScript protocol implementations, our custom protocols that are not part of libpurple, and the purplexpcom library which allows the use of libpurple from scriptable XPCOM (XUL/JS).

(In reply to Patrick Cloke [:clokep] from comment #2)
> I think "nick" is an IRC term, what does XMPP call this?
Also nick or nickname.
> The IRC code calls "chat-buddy-update", do you know the ramifications of
> removing the participant and then adding it back?
Yes, I know and I was searching for something like "chat-buddy-update".

(In reply to aleth [:aleth] from comment #4)
> Can there be more than one code in the stanza? What happens then?
Yes, as it must include at least code "110", so that the user knows this presence refers to itself as an occupant.
(In reply to Patrick Cloke [:clokep] from comment #5)
> I suspect that this code is SUPER similar to the IRC code [1]...could we
> abstract this to jsProtoHelper?
>
> [1] http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#335
Yes, that would be better.

(In reply to aleth [:aleth] from comment #11)
> normalizeNick only exists in irc. This makes me suspect you never tested
> this patch...
Sorry, I did not test that patch as I was in a hurry when I prepared it.

(In reply to Patrick Cloke [:clokep] from comment #14)
> Maybe instead of this we should have a third parameter "isOwnNick" which IRC
> can pass in as |this.normalizeNick(aOldNick) ==
> this.normalizeNick(this.nick)| and XMPP can pass in as True/False depending
> on the situation?
Yes, it can.
> removeParticipant and removeAllParticipants were completely identical across
> different protocols, correct?
For current protocols that we support, yes.
> Where does IRC do this message? I guess I'm a little surprised this isn't in
> the factored out code.
In (https://dxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#1292)
I factored out the message as it is usually used with updateNick to be within it.

(In reply to Patrick Cloke [:clokep] from comment #16)
> Can this string also be combined?
After checking protocols, no. As the way that IRC uses can not be easily to combine, and also the reason for removing participant (kick, ban or part) needs to be determined by protocol that calls |removeParticipant| method.

(In reply to aleth [:aleth] from comment #27)
> This looks like duplicated code, or am I missing something?
According to spec XEP-0045, the presence of code 303 has type attribute and its value is unavailable, but code 210 does not. (check examples 50 and 51)

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #28)
> (In reply to aleth [:aleth] from comment #27)
> > This looks like duplicated code, or am I missing something?
>
> According to spec XEP-0045, the presence of code 303 has type attribute and
> its value is unavailable, but code 210 does not. (check examples 50 and 51)
Ah, thanks.
What's inside the if clause is still duplicated code though. How about a local helper function (inside onPresenceStanza), something like
let changeNick = item => { ... }; ?
(Avoiding duplicated code means avoiding future potential breakages because one copy is changed while the other is overlooked)