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

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
This is to handle the case where we get disconnected and automatically reconnect before the nick has timed out on the server.
(Assignee)

Updated

5 years ago
Summary: Automatically try to get your nick back if it's still in use on connecting. r=clokep → Automatically try to get your nick back if it's still in use on connecting
(Assignee)

Comment 1

5 years ago
Posted patch autorenick.diff (obsolete) — Splinter Review
Attachment #8509742 - Flags: review?(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)?
Attachment #8509742 - Flags: review?(clokep) → review-
(Assignee)

Comment 3

5 years ago
Posted patch autorenick2.diff (obsolete) — Splinter Review
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).
Attachment #8509742 - Attachment is obsolete: true
Attachment #8511225 - Flags: review?(clokep)
(Assignee)

Comment 4

5 years ago
Posted patch autorenick2.diff v3 (obsolete) — Splinter Review
Sorry, forgot to qref.
Attachment #8511225 - Attachment is obsolete: true
Attachment #8511225 - Flags: review?(clokep)
(Assignee)

Comment 5

5 years ago
Posted patch autorenick2.diff v4 (obsolete) — Splinter Review
Now with 10% more edge case protection free.
Attachment #8511227 - Attachment is obsolete: true
Attachment #8511393 - Flags: review?(clokep)
Can you be more specific about the edge case that this fixes? Thanks! I look forward to landing this. :-)
(Assignee)

Comment 7

5 years ago
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 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?
Attachment #8511393 - Flags: feedback+
(Assignee)

Comment 9

5 years ago
(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).
Attachment #8511393 - Attachment is obsolete: true
Attachment #8511393 - Flags: review?(clokep)
Attachment #8514248 - Flags: review?(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?
Attachment #8514248 - Flags: review?(clokep) → review+
(Assignee)

Comment 11

5 years ago
Fixed comment word-wrap.
Attachment #8514248 - Attachment is obsolete: true
Attachment #8514356 - Flags: review?(clokep)
Attachment #8514356 - Flags: review?(clokep) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

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