Closed Bug 954880 Opened 10 years ago Closed 10 years ago

Reset nick when reconnecting and ensure conversations are notified of nick changes

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 7 obsolete files)

*** Original post on bio 1446 at 2012-05-19 14:50:00 UTC ***

1) nick -> nick1 due to nick collision
2) Disconnect account
3) The first nick tried when next connecting should be nick, not nick1
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1446 as attmnt 1487 at 2012-05-19 15:32:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353240 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353240 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1487 at 2012-05-19 15:45:33 UTC ***

I would prefer we just parse this information once in the connect method, aleth is concerned something else might use it before connect gets called, but I do not think this is the case. (_nickname should only be used once we're connected.)
Attachment #8353240 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1446 as attmnt 1489 at 2012-05-19 20:13:00 UTC ***

Actually it's easier to store the original nickname for future use. This also ensures _server is always set (will be important when fixing bug 954869 (bio 1434), as account buddies get initialized before connect).
 
Also
- Ensure nick changes propagate to open chats
- Use changeBuddyNick when trying a new nick on nick collision so the changes propagate to open chats
- Restore missing blank lines to ircBase
Attachment #8353242 - Flags: review?(clokep)
Comment on attachment 8353240 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1487 at 2012-05-19 20:13:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353240 - Attachment is obsolete: true
Comment on attachment 8353242 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1489 at 2012-05-20 12:45:02 UTC ***

>diff --git a/components/irc.js b/components/irc.js
>   connect: function() {
>     this.reportConnecting();
> 
>     // Load preferences.
>     this._port = this.getInt("port");
>     this._ssl = this.getBool("ssl");
>+
>+    // Reset nickname (in case a previous nick collision has changed it).
>+    if (this._nickname != this._originalNickname) {
>+      this.changeBuddyNick(this._nickname, this._originalNickname);
>+      // Tell each chat the user's new nick (changeBuddyNick couldn't do this
>+      // as the user is no longer a participant).
>+      for (let conversation in this._conversations) {
>+        if (this.isMUCName(conversation))
>+          this._conversations[conversation].nick = this._nickname;
>+      }
Really? Can't we just change the changeBuddyNick method to:
1. Not check if the conversation has a participant at [1] (we do this anyway at [2], which would need to be updated to not throw an error if it's "our" nick).
2. Move the code to check if it's our nick [3] above the error statement in [2].

The rest of this patch looks good though. :)

[1] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#702
[2] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#188
[3] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#194
Attachment #8353242 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1446 as attmnt 1491 at 2012-05-20 17:46:00 UTC ***

Adresses review comments and fixes bug 954838 (bio 1403) as a bonus.
Attachment #8353244 - Flags: review?(clokep)
Comment on attachment 8353242 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1489 at 2012-05-20 17:46:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353242 - Attachment is obsolete: true
Comment on attachment 8353244 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1491 at 2012-05-21 03:02:27 UTC ***

>diff --git a/components/irc.js b/components/irc.js
>   updateNick: function(aOldNick, aNewNick) {
>+    if (this._account.normalize(aOldNick) == this._account.normalize(this.nick)) {
>+      // If this is the user's nick, change it.
>+      this.nick = aNewNick;
>+      // If the account was disconnected, the user will not be a participant.
>+      if (!this.hasParticipant(aOldNick))
>+        return;
I'm not a huge fan of having this check in two different places, but I don't see another way around it. (Perhaps it should be made into a variable rather than calling the method twice, but it's simple enough that I don't care either way.)

>@@ -681,32 +687,37 @@ ircAccount.prototype = {
>   },
>   getBuddy: function(aName) {
>     if (this.hasBuddy(aName))
>       return this._buddies[this.normalize(aName, this.userPrefixes)];
>     return null;
>   },
>   changeBuddyNick: function(aOldNick, aNewNick) {
>     let msg;
>-    if (this.normalize(aOldNick) == this.normalize(this._nickname)) {
>+    let isUserNick = this.normalize(aOldNick) == this.normalize(this._nickname);
>+    if (isUserNick) {
>       // Your nickname changed!
>       this._nickname = aNewNick;
>       msg = _("message.nick.you", aNewNick);
>     }
>     else
>       msg = _("message.nick", aOldNick, aNewNick);
> 
>     // Adjust the whois table where necessary.
>     this.removeBuddyInfo(aOldNick);
>     this.setWhoisFromNick(aNewNick);
> 
>     for each (let conversation in this._conversations) {
>-      if (conversation.isChat && conversation.hasParticipant(aOldNick)) {
>-        // Update the nick in every chat conversation the user is in.
>-        conversation.updateNick(aOldNick, aNewNick);
>+      if (isUserNick ||
>+          (conversation.isChat && conversation.hasParticipant(aOldNick))) {
>+        // Update the nick and inform the user in every conversation it
>+        // takes part in. The user may not be a participant if the account
>+        // was disconnected.
>+        if (conversation.isChat)
>+          conversation.updateNick(aOldNick, aNewNick);
>         conversation.writeMessage(aOldNick, msg, {system: true});
>       }
This new set of if statements seems more complicated than previous...is it actually the same logic? Could this be broken into two more distinct chains instead of checking the same statements multiple times? It might help to move the printing of the message into the updateNick method.

>+    // Reset nickname (e.g. a nick collision may have changed it).
>+    if (this._nickname != this._originalNickname)
Are these purposefully not normalized? I'm not sure if they should be or not, but did we think about that?
>+      this.changeBuddyNick(this._nickname, this._originalNickname);

>@@ -1008,17 +1024,17 @@ ircAccount.prototype = {
>     this._socket.disconnect();
>     delete this._socket;
> 
>     clearTimeout(this._isOnTimer);
>     delete this._isOnTimer;
> 
>     // Clean up each conversation: mark as left and remove participant.
>     for (let conversation in this._conversations) {
>-      if (this.isMUCName(conversation)) {
>+      if (conversation.isChat) {
I believe that "conversation" here is the normalized string name of the conversation, not the conversation object. We could change this to a for each loop though and use the isChat check (which probably makes more sense).
Attachment #8353244 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1446 as attmnt 1498 at 2012-05-21 13:40:00 UTC ***

(In reply to comment #6)
> This new set of if statements seems more complicated than previous...is it
> actually the same logic? Could this be broken into two more distinct chains
> instead of checking the same statements multiple times? It might help to move
> the printing of the message into the updateNick method.

I think the new patch is clearer, at the cost of some minor duplication.
 
> >+    // Reset nickname (e.g. a nick collision may have changed it).
> >+    if (this._nickname != this._originalNickname)
> Are these purposefully not normalized? I'm not sure if they should be or not,
> but did we think about that?

When the account is initialized, it has this._nickname == this._originalNickname, so this is appropriate.

> >     // Clean up each conversation: mark as left and remove participant.
> >     for (let conversation in this._conversations) {
> >-      if (this.isMUCName(conversation)) {
> >+      if (conversation.isChat) {
> I believe that "conversation" here is the normalized string name of the
> conversation, not the conversation object. We could change this to a for each
> loop though and use the isChat check (which probably makes more sense).

Oops, didn't spot the missing each, thanks!
Attachment #8353250 - Flags: review?(clokep)
Comment on attachment 8353244 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1491 at 2012-05-21 13:40:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353244 - Attachment is obsolete: true
*** Original post on bio 1446 at 2012-05-21 14:25:53 UTC ***

The alternative to the last patch would be to move everything into the updateNick methods (which then would always be called). But you would still end up with duplication, in this case of the "is the nick the user" check and the message writing.
Comment on attachment 8353250 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1498 at 2012-05-22 00:36:39 UTC ***

I don't fully love this patch (as the loop is duplicated), but I think it keeps the code the simplest.
Attachment #8353250 - Flags: review?(clokep) → review+
Attached patch Alternative approach (WIP) (obsolete) — Splinter Review
*** Original post on bio 1446 as attmnt 1507 at 2012-05-22 00:44:00 UTC ***

Just for completeness, this moves more of the logic to the updateNick methods. It seemed the less elegant route to me. Not tested.
Whiteboard: [checkin-needed]
Whiteboard: [checkin-needed]
Comment on attachment 8353250 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1498 at 2012-05-22 23:11:21 UTC ***

This causes many system messages to show (each time a new nick is tried) when initially logging in, as discussed on IRC.
Attachment #8353250 - Flags: review+ → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1446 as attmnt 1513 at 2012-05-22 23:53:00 UTC ***

Only calls changeBuddyNick from tryNewNick if we are already connected (i.e. if the nick clash was provoked by the user using the "/nick" command). If a nick clash happens during connection, or our nick has changed back to the original nick since we were disconnected, call changeBuddyNick from the 001 handler.
Attachment #8353265 - Flags: review?(clokep)
Comment on attachment 8353250 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1498 at 2012-05-22 23:53:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353250 - Attachment is obsolete: true
Comment on attachment 8353259 [details] [diff] [review]
Alternative approach (WIP)

*** Original change on bio 1446 attmnt 1507 at 2012-05-22 23:53:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353259 - Attachment is obsolete: true
Blocks: 954838
Summary: Reset nick modified due to nick collision when disconnecting the account → Reset nick when reconnecting and ensure conversations are notified of nick changes
Comment on attachment 8353265 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1513 at 2012-05-23 02:12:51 UTC ***

Using this patch, I took the nicks clokep and clokep1 with two accounts. Then tried to log another account in as clokep. It got renamed to clokep2, but didn't notify me. I joined a chat and quit and repeated the process and I got the following:
10:09:33 PM - You are now known as clokep.
10:09:35 PM - You are now known as clokep2.

This is because the connect method uses the changeBuddyNick, then we set it again on log in (in the 001 handler). I guess we can't just directly set this._nickname in the connect method because we have to update the chats...

By the way, one way to help fix some of this stuff might be to have the chat use this._account._nickname as it's own nick name automatically. Although I think jsProtoHelper does some magic when setting the nick for chats and that's what I didn't do this.
Attachment #8353265 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1446 as attmnt 1516 at 2012-05-23 10:32:00 UTC ***

(In reply to comment #13)
> Using this patch, I took the nicks clokep and clokep1 with two accounts. Then
> tried to log another account in as clokep. It got renamed to clokep2, but
> didn't notify me. 

If no conversation is open before you connect, that is expected. Even autojoined conversations get joined "too late". One could work around this by showing a system message after each successful JOIN if the nick isn't the original one. Not sure this is wanted though (there is no way to distinguish between autojoins and later joins I think)?

> This is because the connect method uses the changeBuddyNick, then we set it
> again on log in (in the 001 handler). I guess we can't just directly set
> this._nickname in the connect method because we have to update the chats...

Thanks for spotting this edge case! It should be fixed now, at the cost of another variable. Seemed the cleanest way to do it.
Attachment #8353268 - Flags: review?(clokep)
Comment on attachment 8353265 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1513 at 2012-05-23 10:32:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353265 - Attachment is obsolete: true
*** Original post on bio 1446 at 2012-05-23 10:37:16 UTC ***

(In reply to comment #14)
> Created attachment 8353268 [details] [diff] [review] (bio-attmnt 1516) [details]
> (In reply to comment #13)
> > Using this patch, I took the nicks clokep and clokep1 with two accounts. Then
> > tried to log another account in as clokep. It got renamed to clokep2, but
> > didn't notify me. 
> 
> If no conversation is open before you connect, that is expected. Even
> autojoined conversations get joined "too late". One could work around this by
> showing a system message after each successful JOIN if the nick isn't the
> original one. Not sure this is wanted though (there is no way to distinguish
> between autojoins and later joins I think)?
I would not worry about this as part of this patch. I was just trying to give a full description of what happens.
*** Original post on bio 1446 at 2012-05-23 10:47:21 UTC ***

I think the extra variable is unnecessary, you can probably get around it by sending this._originalNickname in the _connectionRegistration method [1]. I haven't tested this, of course. :) But that's what it looks like!

[1] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#1056
Comment on attachment 8353268 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1516 at 2012-05-23 10:48:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353268 - Flags: review?(clokep)
Attached patch PatchSplinter Review
*** Original post on bio 1446 as attmnt 1518 at 2012-05-23 11:15:00 UTC ***

This is a much better patch I think. Also it's now possible to completely remove nick change updating from tryNewNick, which apart from being more elegant prevents a bug when an intermediate nick (which we try to change to) is a participant in some chat.
Attachment #8353270 - Flags: review?(clokep)
Comment on attachment 8353268 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1516 at 2012-05-23 11:15:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353268 - Attachment is obsolete: true
Comment on attachment 8353270 [details] [diff] [review]
Patch

*** Original change on bio 1446 attmnt 1518 at 2012-05-24 00:00:12 UTC ***

This looks good. It doesn't notify of a changed nick on log on, which I'm OK with and every other situation I could think of looks good (and I like this patch a lot better than the others!)

Thanks!
Attachment #8353270 - Flags: review?(clokep) → review+
*** Original post on bio 1446 at 2012-05-24 17:46:30 UTC ***

Florian comment on IRC that attachment 8353270 [details] [diff] [review] (bio-attmnt 1518) has bitrotted. :(
*** Original post on bio 1446 at 2012-05-24 21:13:42 UTC ***

The bitrot was actually trivial.
Checked-in as https://hg.instantbird.org/instantbird/rev/51c6216fe4e1
Thanks for fixing this! :-)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Depends on: 954901
You need to log in before you can comment on or make changes to this bug.