954970 - Don't reset nick at reconnect if the last nick change was user initiated

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Instantbot]

*** Original post on bio 1538 by scrapmachines AT gmail.com at 2012-06-22 20:15:00 UTC ***

13:51	shu`china	and i guess it's rare enough that deopting is just as viable as compiling it
13:56	kaelan	ah, i see, so they do introduce complications for the optimizer
14:22	Optimizer	but my name was Optimized at that point :|
14:43	Yoric	Optimizer: Time to fight for your name, then :)
14:44	Optimizer	I meant that my name was Optimized, but kaelan types Optimizer, still I got a ping
14:44	kaelan	hah, that's weird
14:44	Optimizer	typed*
14:44	kaelan	maybe the client you're using does levenshtein distance on nicks

before 13:51, I changed my nick to Optimized by /nick Optimized and I got a message too that you are now know as Optimized. before typing at 14:22, I again changed my name to Optimizer by /nick Optimizer and I again got a message saying You are now known as Optimizer. So the name changed both times.

Here is a local log from using InstantBird 1.2a1pre. (note a time difference of 5:30)

7:03:03 PM - kaelan: dxr isn't able to show me the codepath that clears the global object since it has a common name
7:03:57 PM - jandem: kaelan: yes cleared global should be very uncommon
7:04:01 PM - kaelan: OK
7:05:12 PM - You are now known as Optimized.
7:06:02 PM - kaelan: is finally ever going to be implemented? it's very hard to emulate
7:11:12 PM - You are now known as Optimizer.
7:11:14 PM - The topic for #jsapi is: JS engine developer chat. Your questions welcome. || We are not always entirely serious here. (For reals!) || Patches land on mozilla-inbound (next train [choooo chooo!] departs for FF16: 2012-07-16) || Search Logs: http://logbot.glob.com.au/?c=mozilla%23jsapi.
7:11:36 PM - bz [[e-mail]] entered the room.
7:12:06 PM - jcranmer|away is now known as jcranmer.
7:17:53 PM - The topic for #jsapi is: JS engine developer chat. Your questions welcome. || We are not always entirely serious here. (For reals!) || Patches land on mozilla-inbound (next train [choooo chooo!] departs for FF16: 2012-07-16) || Search Logs: http://logbot.glob.com.au/?c=mozilla%23jsapi.
7:19:06 PM - shu`china: but in terms of compiling it
7:21:53 PM - tlt has left the room (Quit: Ping timeout).
7:21:53 PM - pierron has left the room (Quit: Ping timeout).
7:21:57 PM - pierron [[e-mail]] entered the room.
7:23:48 PM - shu`china: exceptions and finally introduce some problems in SSA-based compilers in artificially increasing live ranges of variables, like jandem said, and so some care and more work must go into the analysis to split live ranges, etc
7:24:13 PM - nmatsakis [[e-mail]] entered the room.
7:24:14 PM - shu`china: and i guess it's rare enough that deopting is just as viable as compiling it
7:28:45 PM - c0smikdebris has left the room (Quit: Computer has gone to sleep.).
7:28:46 PM - kaelan: ah, i see, so they do introduce complications for the optimizer
7:31:01 PM - gal [[e-mail]] entered the room.
7:32:45 PM - mcote|afk is now known as mcote.
7:33:25 PM - c0smikdebris [[e-mail]] entered the room.
7:34:12 PM - gal has left the room (Quit: gal).
7:37:41 PM - ehsan [[e-mail]] entered the room.
7:37:51 PM - bholley [[e-mail]] entered the room.
7:41:07 PM - gal [[e-mail]] entered the room.
7:47:12 PM - tlt [[e-mail]] entered the room.
7:49:43 PM - brambles has left the room (Quit: Ping timeout).
7:50:32 PM - brambles [[e-mail]] entered the room.
7:53:57 PM - past|away is now known as past.
7:54:35 PM - You are now known as asd.
7:54:39 PM - You are now known as Optimizer.
7:55:09 PM - Optimizer: but my name was Optimized at that point :|
7:55:12 PM - stransky [[e-mail]] entered the room.
7:57:37 PM - c0smikdebris has left the room (Quit: Ping timeout).
8:03:01 PM - The topic for #jsapi is: JS engine developer chat. Your questions welcome. || We are not always entirely serious here. (For reals!) || Patches land on mozilla-inbound (next train [choooo chooo!] departs for FF16: 2012-07-16) || Search Logs: http://logbot.glob.com.au/?c=mozilla%23jsapi.
8:06:23 PM - jrmuizel has left the room (Quit: Input/output error).
8:06:49 PM - jrmuizel [[e-mail]] entered the room.
8:12:09 PM - philor|away is now known as philor.
8:16:31 PM - Yoric: Optimizer: Time to fight for your name, then :)
8:16:54 PM - Optimizer: I meant that my name was Optimized, but kaelan types Optimizer, still I got a ping
8:17:03 PM - kaelan: hah, that's weird

Comment 1

[aleth [:aleth]]

*** Original post on bio 1538 at 2012-06-22 22:25:52 UTC ***

Optimizer had overlooked that the nick had been changed back to his default account nick on reconnect.

So, the fix for this would be to change this._originalNickname in the NICK command handler. Drawback: if the nick turns out to be invalid, we'll then nevertheless try to change to it again on the next reconnect, unless the user types /nick a second time when she realizes. I'm not sure we sanitize nicks even on account creation though, so not sure if this matters.

Comment 2

[aleth [:aleth]]

*** Original post on bio 1538 at 2012-06-22 22:43:42 UTC ***

Handling this error would be useful

Warning: Unhandled IRC message: :gravel.mozilla.org 432 aleth #momo :Erroneous Nickname: Illegal characters

Comment 3

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1538 as attmnt 1663 at 2012-06-23 10:22:00 UTC ***

The interesting part is the 432 handler, which I will file separately.

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8353420 [details] [diff] [review]
Patch

*** Original change on bio 1538 attmnt 1663 at 2012-06-23 11:39:15 UTC ***

>diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm
>@@ -240,17 +240,25 @@ var commands = [
>   {
>     name: "nick",
>     get helpString() _("command.nick", "nick"),
>-    run: function(aMsg, aConv) simpleCommand(aConv, "NICK", aMsg)
>+    run: function(aMsg, aConv) {
>+      let params = aMsg.trim().split(/\s+/);
First of all you should use the splitInput function here. :)

>+      if (!params[0] || params.length > 1)
>+        return false;
>+      // The user wants to change their nick, so overwrite the account
>+      // nickname for this session.
>+      getAccount(aConv)._originalNickname = params[0];
>+      return simpleCommand(aConv, "NICK", params[0]);
>+    }
>   },

I think I'd prefer if you trim the input (which we can assume the user wants), then check for a space and send the whole thing.

let newNick = aMsg.trim();
if (newNick.indexOf(/\s+/) != -1) return false;
...

Comment 5

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1538 as attmnt 1665 at 2012-06-23 11:48:00 UTC ***

Nit fixed.

Comment 6

[aleth [:aleth]]

Comment on attachment 8353420 [details] [diff] [review]
Patch

*** Original change on bio 1538 attmnt 1663 at 2012-06-23 11:48:20 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8353422 [details] [diff] [review]
Patch

*** Original change on bio 1538 attmnt 1665 at 2012-06-23 13:16:14 UTC ***

Looks good, thanks. :) Glad it was such a simple fix!

Comment 8

[Florian Quèze [:florian]]

*** Original post on bio 1538 at 2012-06-23 20:29:18 UTC ***

Shouldn't we still reset the nick to the account username when the account is disconnected through a click on disconnect in the account manager? (Is this even possible to do? :-S)

Comment 9

[aleth [:aleth]]

Attachment: Patch with reset on user disconnect

*** Original post on bio 1538 as attmnt 1677 at 2012-06-23 22:39:00 UTC ***

(In reply to comment #7)
> Shouldn't we still reset the nick to the account username when the account is
> disconnected through a click on disconnect in the account manager? (Is this
> even possible to do? :-S)

I believe this patch does this, though I couldn't test the non-reset still works on involuntary disconnects. (Also resets the nick on /offline + /back of course.)

Comment 10

[aleth [:aleth]]

Comment on attachment 8353422 [details] [diff] [review]
Patch

*** Original change on bio 1538 attmnt 1665 at 2012-06-23 22:39:14 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[aleth [:aleth]]

*** Original post on bio 1538 at 2012-06-23 22:45:29 UTC ***

(In reply to comment #8)
Re the last patch, I'm not sure if it would be better/easier to just store the account nickname in a variable on the account in the constructor (rather than introduce the accountNameParts getter).

Comment 12

[aleth [:aleth]]

Attachment: Patch with reset on user disconnect

*** Original post on bio 1538 as attmnt 1678 at 2012-06-23 22:58:00 UTC ***

Introduce this._accountNickname instead of a getter.

Comment 13

[aleth [:aleth]]

Comment on attachment 8353434 [details] [diff] [review]
Patch with reset on user disconnect

*** Original change on bio 1538 attmnt 1677 at 2012-06-23 22:58:05 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1538 as attmnt 1679 at 2012-06-23 23:05:00 UTC ***

(Typo)

Comment 15

[aleth [:aleth]]

Comment on attachment 8353435 [details] [diff] [review]
Patch with reset on user disconnect

*** Original change on bio 1538 attmnt 1678 at 2012-06-23 23:05:43 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 16

[Patrick Cloke [:clokep]]

Comment on attachment 8353436 [details] [diff] [review]
Patch

*** Original change on bio 1538 attmnt 1679 at 2012-06-25 12:25:34 UTC ***

This looks good, although the _originalNickname is kind of poorly named now. (As _accountNickname is really your "original" nickname.) Could we think if there's a better term for this, and if so update this to use it? If we can't come up with something better, than OK...we just need to document what it vs. _accountNickname is. For r+:
1. Add _accountNickname to the prototype.
2. Document both _accountNickname and _originalNickname.
3. Think about if there's a better term for _originalNickname

Also, any reasson you added the accountName variable? It doesn't seem necessary...if you're going by the "if you use a variable more than once save it"...I don't think that applies because we're just directly accessing a variable, we're not applying an operation to it multiple times.

Comment 17

[aleth [:aleth]]

*** Original post on bio 1538 at 2012-06-25 12:29:50 UTC ***

(In reply to comment #12)
> Comment on attachment 8353436 [details] [diff] [review] (bio-attmnt 1679) [details]
> Patch
> 
> This looks good, although the _originalNickname is kind of poorly named now.

This is true, maybe it should be _currentNickname now? But that could be confusing as the actual nickname might not be the current one...
_desiredNickname?
_wantedNickname?

> 1. Add _accountNickname to the prototype.
> 2. Document both _accountNickname and _originalNickname.

Ummm... it's already there. With comments :P

Comment 18

[aleth [:aleth]]

*** Original post on bio 1538 at 2012-06-25 12:31:51 UTC ***

(In reply to comment #12)
> Also, any reasson you added the accountName variable? It doesn't seem
> necessary...if you're going by the "if you use a variable more than once save
> it"...I don't think that applies because we're just directly accessing a
> variable, we're not applying an operation to it multiple times.

That's why initially I thought I'd just add a getter for it, rather than storing the value in a variable. The goal is to remove the otherwise duplicated code for splitting up the account name.

Comment 19

[Patrick Cloke [:clokep]]

*** Original post on bio 1538 at 2012-06-25 12:36:20 UTC ***

Comment on attachment 8353436 [details] [diff] [review] (bio-attmnt 1679)
Patch

(In reply to comment #14)
> (In reply to comment #12)
> > Also, any reasson you added the accountName variable? It doesn't seem
> > necessary...if you're going by the "if you use a variable more than once save
> > it"...I don't think that applies because we're just directly accessing a
> > variable, we're not applying an operation to it multiple times.
> 
> That's why initially I thought I'd just add a getter for it, rather than
> storing the value in a variable. The goal is to remove the otherwise duplicated
> code for splitting up the account name.

I'm not sure what you're referring to here? I think this will answer your question:

>   // Split the account name into usable parts.
>-  let splitter = aImAccount.name.lastIndexOf("@");
>-  this._nickname = aImAccount.name.slice(0, splitter);
>+  let accountName = this.imAccount.name; <-- Why are we storing this variable? Why can't we just refer to this.imAccount.name on the three lines below?
>+  let splitter = accountName.lastIndexOf("@");
>+  this._accountNickname = accountName.slice(0, splitter);
>+  this._server = accountName.slice(splitter + 1);

Comment 20

[Patrick Cloke [:clokep]]

*** Original post on bio 1538 at 2012-06-25 12:38:18 UTC ***

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 8353436 [details] [diff] [review] (bio-attmnt 1679) [details]
> > Patch
> > 
> > This looks good, although the _originalNickname is kind of poorly named now.
> 
> This is true, maybe it should be _currentNickname now? But that could be
> confusing as the actual nickname might not be the current one...
> _desiredNickname?
> _wantedNickname?
> 
> > 1. Add _accountNickname to the prototype.
> > 2. Document both _accountNickname and _originalNickname.
> 
> Ummm... it's already there. With comments :P

I had checked for _originalNickname in the current code, I didn't realize you documented it in this bug. Sorry about that. :)

> // The nickname that will be used when connecting.
Isn't this now used when *re*connecting only?

Comment 21

[aleth [:aleth]]

*** Original post on bio 1538 at 2012-06-25 12:50:35 UTC ***

(In reply to comment #16)
> > // The nickname that will be used when connecting.
> Isn't this now used when *re*connecting only?

No: http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#1078

Comment 22

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1538 as attmnt 1690 at 2012-06-25 13:09:00 UTC ***

Right, I understood what you meant now ;)

Comment 23

[aleth [:aleth]]

Comment on attachment 8353436 [details] [diff] [review]
Patch

*** Original change on bio 1538 attmnt 1679 at 2012-06-25 13:09:09 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 24

[Patrick Cloke [:clokep]]

Comment on attachment 8353447 [details] [diff] [review]
Patch

*** Original change on bio 1538 attmnt 1690 at 2012-06-25 13:13:11 UTC ***

OK, I like this one now. :) Thanks for fixing this! requestedNickname is a much better name IMO!

Comment 25

[Patrick Cloke [:clokep]]

*** Original post on bio 1538 at 2012-06-26 23:52:08 UTC ***

Fixed in http://hg.instantbird.org/instantbird/rev/7378ec77abd3

Thanks for fixing this aleth!