Closed Bug 1176958 Opened 9 years ago Closed 9 years ago

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

Categories

(Chat Core :: XMPP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 43

People

(Reporter: abdelrahman, Assigned: abdelrahman)

References

Details

Attachments

(1 file, 10 obsolete files)

17.99 KB, patch
aleth
: review+
Details | Diff | Splinter Review
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
Attached patch rev 1 - update participants list (obsolete) — Splinter Review
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-
Attached patch rev 2 - update participants list (obsolete) — Splinter Review
(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)
Attachment #8626933 - Flags: review?(aleth)
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)
(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)
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)
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #6)
> Yes, that would be better.

We can also do that for methods: removeParticipant and removeAllParticipants.
Don't forget to also look at yahoo when moving those methods to jsProtoHelper.
Attached patch rev 3 - update participants list (obsolete) — Splinter Review
Attachment #8626933 - Attachment is obsolete: true
Attachment #8629114 - Flags: review?(aleth)
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-
Attached patch rev 4 - update participants list (obsolete) — Splinter Review
(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 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+
Attached patch rev 5 - update participants list (obsolete) — Splinter Review
(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-
Attached patch rev 6 - update participants list (obsolete) — Splinter Review
(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+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
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
Backed out as it produces "Failed to get nickSet" errors.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updating milestone from 1.6 to 42.
Target Milestone: 1.6 → Instantbird 42
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 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-
Can we fix the outstanding issues and land this?
Flags: needinfo?(a.ahmed1026)
(In reply to aleth [:aleth] from comment #24)
> Can we fix the outstanding issues and land this?

Yes, sure.
Flags: needinfo?(a.ahmed1026)
Attached patch rev 7 - update participants list (obsolete) — Splinter Review
Attachment #8638752 - Attachment is obsolete: true
Attachment #8651903 - Flags: review?(aleth)
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.
(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)
Attached patch rev 8 - update participants list (obsolete) — Splinter Review
Attachment #8651903 - Attachment is obsolete: true
Attachment #8651903 - Flags: review?(aleth)
Attachment #8651967 - Flags: review?(aleth)
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.)
Attached patch rev 9 - update participants list (obsolete) — Splinter Review
Attachment #8651967 - Attachment is obsolete: true
Attachment #8651967 - Flags: review?(aleth)
Attachment #8651985 - Flags: review?(aleth)
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+
Keywords: checkin-needed
Target Milestone: Instantbird 42 → ---
This patch no longer applies, please rebase.
Keywords: checkin-needed
Attached patch rev 9 - update participants list (obsolete) — Splinter Review
Attachment #8651985 - Attachment is obsolete: true
Attachment #8654497 - Flags: review+
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-
Attachment #8654497 - Attachment is obsolete: true
Attachment #8654556 - Flags: review?(aleth)
Attachment #8654556 - Flags: review?(aleth) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/48f86f412c85272ca4e57813f812f905570e6e3e
Bug 1176958 - Update participants list when a nick is changed in XMPP MUC. r=aleth,clokep
Status: REOPENED → RESOLVED
Closed: 9 years ago9 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.