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

RESOLVED FIXED in Instantbird 43

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: abdelrhman, Assigned: abdelrhman)

Tracking

trunk
Instantbird 43

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

17.99 KB, patch
aleth
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
When a participant in a MUC changes his/her nick, the participants list does not delete the old nick. Check XEP-0045 (7.6) [1].

[1] http://xmpp.org/extensions/xep-0045.html#changenick
(Assignee)

Comment 1

3 years ago
Created attachment 8626886 [details] [diff] [review]
rev 1 - update participants list
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Attachment #8626886 - Flags: review?(aleth)
Comment on attachment 8626886 [details] [diff] [review]
rev 1 - update participants list

Review of attachment 8626886 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +103,5 @@
> +#   This is displayed as a system message when a participant changes his/her
> +#   nickname in a room.
> +#   %1$S is the old nick.
> +#   %2$S is the new nick.
> +conversation.muc.nick=%1$S is now known as %2$S.

I think "nick" is an IRC term, what does XMPP call this?

::: chat/protocols/xmpp/xmpp.jsm
@@ +321,5 @@
> +    participant.name = aNewNick;
> +    this.removeParticipant(aOldNick);
> +    this._participants.set(aNewNick, participant);
> +    this.notifyObservers(new nsSimpleEnumerator([participant]),
> +                         "chat-buddy-add");

The IRC code calls "chat-buddy-update", do you know the ramifications of removing the participant and then adding it back?
Attachment #8626886 - Flags: review?(aleth) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8626933 [details] [diff] [review]
rev 2 - update participants list

(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".
Attachment #8626886 - Attachment is obsolete: true
Attachment #8626933 - Flags: review?(clokep)
(Assignee)

Updated

3 years ago
Attachment #8626933 - Flags: review?(aleth)

Comment 4

3 years ago
Comment on attachment 8626933 [details] [diff] [review]
rev 2 - update participants list

Review of attachment 8626933 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +244,5 @@
>          this.joining = false;
>          return true;
>        });
>      }
> +    else if (codes.indexOf("210") != -1) {

Can there be more than one code in the stanza? What happens then?
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
Flags: needinfo?(a.ahmed1026)
(Assignee)

Comment 6

3 years ago
(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.
Flags: needinfo?(a.ahmed1026)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8626933 [details] [diff] [review]
rev 2 - update participants list

># HG changeset patch
># User Abdelrhman Ahmed <a.ahmed1026@gmail.com>
># Parent  59ae2306da5684907c1e72e1e1c5a14d2088702a
>Bug 1176958 - Update participants list when a nick is changed in XMPP MUC. r=clokep
>
>diff --git a/chat/locales/en-US/xmpp.properties b/chat/locales/en-US/xmpp.properties
>--- a/chat/locales/en-US/xmpp.properties
>+++ b/chat/locales/en-US/xmpp.properties
>@@ -94,16 +94,29 @@ chatRoomField.password=_Password
> #   received.
> #   %1$S is the inviter.
> #   %2$S is the room.
> #   %3$S is the reason which is a message provided by the person sending the
> #   invitation.
> conversation.muc.invitationWithReason=%1$S has invited you to join %2$S: %3$S
> conversation.muc.invitationWithoutReason=%1$S has invited you to join %2$S
> 
>+# LOCALIZATION NOTE (conversation.muc.*):
>+#   This is displayed as a system message when a participant changes his/her
>+#   nickname in a room.
>+#   %1$S is the old nick.
>+#   %2$S is the new nick.
>+conversation.muc.nick=%1$S is now known as %2$S.
>+
>+# LOCALIZATION NOTE (conversation.muc.*):
>+#   This is displayed as a system message when you changes your nickname in
>+#   a room or service changes it for you due to its policies.
>+#   %S is your new nick.
>+conversation.muc.nick.you=You are now known as %S.
>+
> # LOCALIZATION NOTE (options.*):
> #   These are the protocol specific options shown in the account manager and
> #   account wizard windows.
> options.resource=Resource
> options.priority=Priority
> options.connectionSecurity=Connection security
> options.connectionSecurity.requireEncryption=Require encryption
> options.connectionSecurity.opportunisticTLS=Use encryption if available
>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>--- a/chat/protocols/xmpp/xmpp.jsm
>+++ b/chat/protocols/xmpp/xmpp.jsm
>@@ -196,18 +196,20 @@ const XMPPMUCConversationPrototype = {
>       if (codes.indexOf("303") != -1) {
>         // XEP-0045 (7.6): Changing Nickname.
>         // Service Updates Nick for user.
>         if (!item  || !item.attributes["nick"]) {
>           this.WARN("Received a MUC presence code 303 stanza without an item " +
>                     "element or a nick attribute.");
>           return;
>         }
>-        let participant = this._participants.get(nick);
>-        participant.name = item.attributes["nick"];
>+        let newNick = item.attributes["nick"];
>+        this.updateNick(nick, newNick);
>+        let message =  _("conversation.muc.nick", nick, newNick);
>+        this.writeMessage(this.name, message, {system: true});
>         return;
>       }
>       if (item && item.attributes["role"] == "none") {
>         // XEP-0045: the user is no longer an occupant.
>         this.removeParticipant(nick);
>         if (codes.indexOf("110") != -1) {
>           // XEP-0045 7.14: Self-presence.
>           // This presence refers to this account.
>@@ -238,16 +240,32 @@ const XMPPMUCConversationPrototype = {
> 
>         // XEP-0045: Service Informs New Room Owner of Success
>         // for instant and reserved rooms.
>         this.left = false;
>         this.joining = false;
>         return true;
>       });
>     }
>+    else if (codes.indexOf("210") != -1) {
>+      // XEP-0045 (7.6): Changing Nickname.
>+      // Service modifies this user's nickname in accordance with local service
>+      // policies.
>+      if (!item  || !item.attributes["nick"]) {
>+        this.WARN("Received a MUC presence code 210 stanza without an item " +
>+                  "element or a nick attribute.");
>+        return;
>+      }
>+      let newNick = item.attributes["nick"];
>+      this.updateNick(this.nick, newNick);
>+      this.nick = newNick;
>+      let message =  _("conversation.muc.nick.you", newNick);
>+      this.writeMessage(this.name, message, {system: true});
>+      return;
>+    }
>     else if (codes.indexOf("110") != -1) {
>       // XEP-0045: Room exists and joined successfully.
>       this.left = false;
>       this.joining = false;
>       // TODO (Bug 1172350): Implement Service Discovery Extensions (XEP-0128) to obtain
>       // configuration of this room.
>     }
> 
>@@ -292,16 +310,31 @@ const XMPPMUCConversationPrototype = {
>       this._messageIds.add(id);
>     }
>     this.writeMessage(from, aMsg, flags);
>   },
> 
>   getNormalizedChatBuddyName: function(aNick)
>     this._account.normalizeFullJid(this.name + "/" + aNick),
> 
>+  // Updates the nick of a participant in MUC conversation to a new one.
>+  updateNick: function(aOldNick, aNewNick) {
>+    if (!this._participants.has(aOldNick)) {
>+      this.ERROR("Trying to rename nick that doesn't exist! " + aOldNick +
>+                 " to " + aNewNick);
>+      return;
>+    }
>+
>+    let participant = this._participants.get(aOldNick);
>+    participant.name = aNewNick;
>+    this._participants.delete(aOldNick);
>+    this._participants.set(aNewNick, participant);
>+    this.notifyObservers(participant, "chat-buddy-update", aOldNick);
>+  },
>+
>   // Removes a participant from MUC conversation.
>   removeParticipant: function(aNick) {
>     if (!this._participants.has(aNick))
>       return;
> 
>     this._participants.delete(aNick);
>     let nickString = Cc["@mozilla.org/supports-string;1"]
>                        .createInstance(Ci.nsISupportsString);
Attachment #8626933 - Flags: review?(clokep)
Attachment #8626933 - Flags: review?(aleth)
(Assignee)

Comment 8

3 years ago
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #6)
> Yes, that would be better.

We can also do that for methods: removeParticipant and removeAllParticipants.

Comment 9

3 years ago
Don't forget to also look at yahoo when moving those methods to jsProtoHelper.
(Assignee)

Comment 10

3 years ago
Created attachment 8629114 [details] [diff] [review]
rev 3 - update participants list
Attachment #8626933 - Attachment is obsolete: true
Attachment #8629114 - Flags: review?(aleth)

Comment 11

3 years ago
Comment on attachment 8629114 [details] [diff] [review]
rev 3 - update participants list

Review of attachment 8629114 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/jsProtoHelper.jsm
@@ +624,5 @@
> +
> +  // Updates the nick of a participant in conversation to a new one.
> +  updateNick: function(aOldNick, aNewNick) {
> +    let isParticipant = this._participants.has(aOldNick);
> +    if (this.normalizeNick(aOldNick) == this.normalizeNick(this.nick)) {

normalizeNick only exists in irc. This makes me suspect you never tested this patch...
Attachment #8629114 - Flags: review?(aleth) → review-
(Assignee)

Comment 12

3 years ago
Created attachment 8631933 [details] [diff] [review]
rev 4 - update participants list

(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.
Attachment #8629114 - Attachment is obsolete: true
Attachment #8631933 - Flags: review?(aleth)

Comment 13

3 years ago
Comment on attachment 8631933 [details] [diff] [review]
rev 4 - update participants list

Review of attachment 8631933 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/jsProtoHelper.jsm
@@ +614,5 @@
>      this._joining = aJoining;
>      this.notifyObservers(null, "update-conv-chatjoining");
>    },
>  
> +  normalizeNick: function(aNick) aNick,

Not sure about the wisdom of introducing yet another normalize function for all JS prpls...
Attachment #8631933 - Flags: review?(aleth) → review?(clokep)
Comment on attachment 8631933 [details] [diff] [review]
rev 4 - update participants list

Review of attachment 8631933 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/jsProtoHelper.jsm
@@ +614,5 @@
>      this._joining = aJoining;
>      this.notifyObservers(null, "update-conv-chatjoining");
>    },
>  
> +  normalizeNick: function(aNick) aNick,

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?

@@ +651,5 @@
> +    this.notifyObservers(participant, "chat-buddy-update", aOldNick);
> +  },
> +
> +  // Removes a participant from conversation.
> +  removeParticipant: function(aNick) {

removeParticipant and removeAllParticipants were completely identical across different protocols, correct?

::: chat/protocols/xmpp/xmpp.jsm
@@ +203,5 @@
>          }
> +        let newNick = item.attributes["nick"];
> +        this.updateNick(nick, newNick);
> +        let message =  _("conversation.muc.nick", nick, newNick);
> +        this.writeMessage(this.name, message, {system: true});

Where does IRC do this message? I guess I'm a little surprised this isn't in the factored out code.

Minor nit: two spaces after the = sign on the line above this.
Attachment #8631933 - Flags: review?(clokep) → feedback+
(Assignee)

Comment 15

3 years ago
Created attachment 8636027 [details] [diff] [review]
rev 5 - update participants list

(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.
Attachment #8631933 - Attachment is obsolete: true
Attachment #8636027 - Flags: review?(clokep)
Comment on attachment 8636027 [details] [diff] [review]
rev 5 - update participants list

Review of attachment 8636027 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good! If the answer to the question is no, then just a couple of nits in comments.

::: chat/locales/en-US/conversations.properties
@@ +62,5 @@
> +#   %1$S is the old nick.
> +#   %2$S is the new nick.
> +nickSet=%1$S is now known as %2$S.
> +# LOCALIZATION NOTE (nickSet.you):
> +#   This is displayed as a system message when you changes your nickname in

Nit: "when you change your nickname"

I think this comment might be clearer as "This is displayed as a system message when your nickname is changed."

::: chat/modules/jsProtoHelper.jsm
@@ +642,5 @@
> +    }
> +    else
> +      message = _("nickSet", aOldNick, aNewNick);
> +
> +    // Get the original ircParticipant and then remove it.

Nit: Fix this comment to not refer to IRC.

::: chat/protocols/yahoo/yahoo.js
@@ +288,5 @@
>      let conf = this._conferences.get(aRoom);
>      conf.removeParticipant(aUsername);
> +    conf.writeMessage(this._roomName,
> +                      _("system.message.conferenceLogoff", aName),
> +                      {system: true});

Can this string also be combined?
Attachment #8636027 - Flags: review?(clokep) → review-
(Assignee)

Comment 17

3 years ago
Created attachment 8638752 [details] [diff] [review]
rev 6 - update participants list

(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.
Attachment #8636027 - Attachment is obsolete: true
Attachment #8638752 - Flags: review?(clokep)
Comment on attachment 8638752 [details] [diff] [review]
rev 6 - update participants list

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #17)
> Created attachment 8638752 [details] [diff] [review]
> rev 6 - update participants list
> 
> (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.

Great! Looks good. :)
Attachment #8638752 - Flags: review?(clokep) → review+
Keywords: checkin-needed

Comment 19

3 years ago
url:        https://hg.mozilla.org/comm-central/rev/aeca087abe829d1c64f3665de34deed05b3c72be
changeset:  aeca087abe829d1c64f3665de34deed05b3c72be
user:       Abdelrhman Ahmed <a.ahmed1026@gmail.com>
date:       Fri Jul 24 13:32:00 2015 +0200
description:
Bug 1176958 - Update participants list when a nick is changed in XMPP MUC. r=clokep a=aleth CLOSED TREE

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6

Comment 20

3 years ago
url:        https://hg.mozilla.org/comm-central/rev/ae81bda0a5582a7d5d279f79c99f497324580147
changeset:  ae81bda0a5582a7d5d279f79c99f497324580147
user:       aleth <aleth@instantbird.org>
date:       Mon Jul 27 15:36:06 2015 +0200
description:
Back out aeca087abe82 (Bug 1176958 - Update participants list when a nick is changed) for producing error messages. a=aleth CLOSED TREE

Comment 21

3 years ago
Backed out as it produces "Failed to get nickSet" errors.

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updating milestone from 1.6 to 42.
Target Milestone: 1.6 → Instantbird 42

Updated

3 years ago
Blocks: 954959
Summary: participants list does not delete old nick when a participant changes it in XMPP MUC → Participants list does not delete old nick when a participant changes it in XMPP MUC

Comment 23

3 years ago
Comment on attachment 8638752 [details] [diff] [review]
rev 6 - update participants list

Review of attachment 8638752 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/irc/irc.js
@@ +1195,5 @@
>      let msg;
>      if (this.normalizeNick(aOldNick) == this.normalizeNick(this._nickname)) {
>        // Your nickname changed!
>        this._nickname = aNewNick;
> +      msg = _conv("nickSet.you", aNewNick);

Inline this in the !isChat case - the common case for IRC is that all conversations are MUCs, so we don't need to pull this outside the loop.

@@ +1206,4 @@
>        });
>      }
>      else {
> +      msg = _conv("nickSet", aOldNick, aNewNick);

Not used.

::: chat/protocols/irc/ircUtils.jsm
@@ +13,5 @@
>    l10nHelper("chrome://chat/locale/irc.properties")
>  );
>  
> +XPCOMUtils.defineLazyGetter(this, "_conv", function()
> +  l10nHelper("chrome://chat/locale/conversation.properties")

Typo causing errors.
Attachment #8638752 - Flags: review-

Comment 24

3 years ago
Can we fix the outstanding issues and land this?
Flags: needinfo?(a.ahmed1026)
(Assignee)

Comment 25

3 years ago
(In reply to aleth [:aleth] from comment #24)
> Can we fix the outstanding issues and land this?

Yes, sure.
Flags: needinfo?(a.ahmed1026)
(Assignee)

Comment 26

3 years ago
Created attachment 8651903 [details] [diff] [review]
rev 7 - update participants list
Attachment #8638752 - Attachment is obsolete: true
Attachment #8651903 - Flags: review?(aleth)

Comment 27

3 years ago
Comment on attachment 8651903 [details] [diff] [review]
rev 7 - update participants list

Review of attachment 8651903 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +346,5 @@
>          this.joining = false;
>          return true;
>        });
>      }
> +    else if (codes.indexOf("210") != -1) {

This looks like duplicated code, or am I missing something?

How about if (codes.indexof("210")... || codes.indexof("303") { ...
You can explain the difference in the comment.
(Assignee)

Comment 28

3 years ago
(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)

Comment 29

3 years ago
(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)
(Assignee)

Comment 30

3 years ago
Created attachment 8651967 [details] [diff] [review]
rev 8 - update participants list
Attachment #8651903 - Attachment is obsolete: true
Attachment #8651903 - Flags: review?(aleth)
Attachment #8651967 - Flags: review?(aleth)

Comment 31

3 years ago
Comment on attachment 8651967 [details] [diff] [review]
rev 8 - update participants list

Review of attachment 8651967 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +260,5 @@
>        }
>        if (codes.indexOf("303") != -1) {
>          // XEP-0045 (7.6): Changing Nickname.
>          // Service Updates Nick for user.
>          if (!item  || !item.attributes["nick"]) {

Sorry if I wasn't clear, but you can move this if clause to changeNick too! (It's OK if the warning message says "303 or 210" as the stanza itself is also logged.)
(Assignee)

Comment 32

3 years ago
Created attachment 8651985 [details] [diff] [review]
rev 9 - update participants list
Attachment #8651967 - Attachment is obsolete: true
Attachment #8651967 - Flags: review?(aleth)
Attachment #8651985 - Flags: review?(aleth)

Comment 33

3 years ago
Comment on attachment 8651985 [details] [diff] [review]
rev 9 - update participants list

Review of attachment 8651985 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8651985 - Flags: review?(aleth) → review+

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Target Milestone: Instantbird 42 → ---

Comment 34

3 years ago
This patch no longer applies, please rebase.
Keywords: checkin-needed
(Assignee)

Comment 35

3 years ago
Created attachment 8654497 [details] [diff] [review]
rev 9 - update participants list
Attachment #8651985 - Attachment is obsolete: true
Attachment #8654497 - Flags: review+

Comment 36

3 years ago
Comment on attachment 8654497 [details] [diff] [review]
rev 9 - update participants list

Review of attachment 8654497 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/irc/ircUtils.jsm
@@ +13,5 @@
>    l10nHelper("chrome://chat/locale/irc.properties")
>  );
>  
> +XPCOMUtils.defineLazyGetter(this, "_conv", function()
> +  l10nHelper("chrome://chat/locale/conversations.properties")

Unfortunately, expression closures are going to be deprecated (bug 1083458, bug 1151473), use an arrow function instead.
Attachment #8654497 - Flags: review+ → review-
(Assignee)

Comment 37

3 years ago
Created attachment 8654556 [details] [diff] [review]
rev 10 - update participants list
Attachment #8654497 - Attachment is obsolete: true
Attachment #8654556 - Flags: review?(aleth)

Updated

3 years ago
Attachment #8654556 - Flags: review?(aleth) → review+

Updated

3 years ago
Keywords: checkin-needed

Comment 38

3 years ago
https://hg.mozilla.org/comm-central/rev/48f86f412c85272ca4e57813f812f905570e6e3e
Bug 1176958 - Update participants list when a nick is changed in XMPP MUC. r=aleth,clokep

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 43
You need to log in before you can comment on or make changes to this bug.