1087566 - Automatically try to get your nick back if it's still in use on connecting

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[aleth [:aleth]]

This is to handle the case where we get disconnected and automatically reconnect before the nick has timed out on the server.

Comment 1

[aleth [:aleth]]

Attachment: autorenick.diff

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 8509742 [details] [diff] [review]
autorenick.diff

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

Overall I like it! What happens if someone manually changes their nick, however? We should cancel the timer then.

::: chat/locales/en-US/irc.properties
@@ +99,5 @@
>  message.nick=%1$S is now known as %2$S.
>  #    %S is your new nick.
>  message.nick.you=You are now known as %S.
>  #    Could not change the nickname. %S is the user's nick.
> +message.nick.fail=Could not use the desired nickname. Your nick remains %S.

This property name needs to be updated since the messages was modified.

::: chat/protocols/irc/irc.js
@@ +844,5 @@
>    // adding digits).
>    _sentNickname: null,
> +  // If we don't get the desired nick on connect, we try again a bit later,
> +  // to see if it wasn't just our nick not having timed out yet.
> +  _nickinuseTimeout: null,

Nit: Camelcase?

::: chat/protocols/irc/ircBase.jsm
@@ +1232,5 @@
>        // <nick> :Nickname is already in use
> +      // Try to get the desired nick back in 2.5 minutes,
> +      // the first time this happens after connection, in case it was
> +      // just the user's nick not having timed out yet on the server.
> +      if (!this._nickinuseTimeout) {

Should this check if this.connecting (or !this.connected)?

Comment 3

[aleth [:aleth]]

Attachment: autorenick2.diff

Keeps the timer, but only sets it during connection. Opportunistically reacts to the QUIT message of the wanted nick if we receive one (this is not guaranteed as we may not be joined to any channel in which the desired nick is present).

Comment 4

[aleth [:aleth]]

Attachment: autorenick2.diff v3

Sorry, forgot to qref.

Comment 5

[aleth [:aleth]]

Attachment: autorenick2.diff v4

Now with 10% more edge case protection free.

Comment 6

[Patrick Cloke [:clokep]]

Can you be more specific about the edge case that this fixes? Thanks! I look forward to landing this. :-)

Comment 7

[aleth [:aleth]]

It just checks the QUIT message wasn't for the nick we already have - I'm not sure if this can happen but I thought it better to be safe.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8511393 [details] [diff] [review]
autorenick2.diff v4

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

How does this interact with the alternative nicks capability?

::: chat/protocols/irc/ircBase.jsm
@@ +322,5 @@
>          buddy.setStatus(Ci.imIStatusInfo.STATUS_OFFLINE, "");
> +
> +      // If we wanted this nickname, grab it.
> +      if (nickname == this._requestedNickname &&
> +          nickname != this._nickname) {

Something I find a little concerning is that after a LONGGGG time, this code will still run (like hours later) if the nickname times out. Perhaps adding a check for this._nickInUseTimeout would avoid that (assuming we want to avoid that).

@@ +1226,5 @@
>        // <nick> :Nickname is already in use
> +      // Try to get the desired nick back in 2.5 minutes if this happens
> +      // when connecting, in case it was just due to the user's nick not
> +      // having timed out yet on the server.
> +      if (this.connecting && aMessage.params[1] == this._requestedNickname) {

What is the aMessage.params[1] == this._requestedNickname check for? Is this to avoid setting our nick to foo1 if we both foo, foo1 are in use and we end up with foo2?

Comment 9

[aleth [:aleth]]

Attachment: autorenick2.diff v4, unbitrotted

(In reply to Patrick Cloke [:clokep] from comment #8)
> Something I find a little concerning is that after a LONGGGG time, this code
> will still run (like hours later) if the nickname times out. Perhaps adding
> a check for this._nickInUseTimeout would avoid that (assuming we want to
> avoid that).

_requestedNickname always contains the last nick the user wanted to have, so I didn't think we need to worry about the timeout here. (That would also cover instances where someone logs in with the same nick with two clients in a handover scenario between devices, where the QUIT isn't due to a timeout.) I don't mind making the change though if you prefer.
 
> @@ +1226,5 @@
> > +      if (this.connecting && aMessage.params[1] == this._requestedNickname) {
> 
> What is the aMessage.params[1] == this._requestedNickname check for? Is this
> to avoid setting our nick to foo1 if we both foo, foo1 are in use and we end
> up with foo2?

The nick is always set to _requestedNickname, this is merely so we only set the timeout once (i.e. not when we receive NICKINUSE for foo1).

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8514248 [details] [diff] [review]
autorenick2.diff v4, unbitrotted

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

Please fix the commit message while you're at it.

::: chat/protocols/irc/ircBase.jsm
@@ +334,5 @@
> +      // If we wanted this nickname, grab it.
> +      if (nick == this._requestedNickname && nick != this._nickname) {
> +        this.changeNick(this._requestedNickname);
> +        clearTimeout(this._nickInUseTimeout);
> +        delete this._nickInUseTimeout;

I think this is OK that _nickInUseTimeout might have ran already, but please just think about this briefly.

@@ +1235,5 @@
>        // <nick> :Nickname is already in use
> +      // Try to get the desired nick back in 2.5 minutes if this happens
> +      // when connecting, in case it was just due to the user's nick not
> +      // having timed out yet on the server.
> +      if (this.connecting && aMessage.params[1] == this._requestedNickname) {

Something looks funky here with the line length. Is this line too long or are the comments above this too short?

Comment 11

[aleth [:aleth]]

Attachment: autorenick2.diff v5

Fixed comment word-wrap.

Comment 12

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/643e216d8eeb