Closed
Bug 954970
Opened 10 years ago
Closed 10 years ago
Don't reset nick at reconnect if the last nick change was user initiated
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: bugzilla, Assigned: aleth)
References
Details
Attachments
(1 file, 5 obsolete files)
4.30 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
*** 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 [bzbarsky@moz-B58892FB.bstnma.fios.verizon.net] 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 [pierron@moz-7EB03C5F.fbx.proxad.net] 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 [nmatsakis@1BC4D9FE.37EF1718.2321E71E.IP] 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 [gal@moz-7327DB99.hsd1.ca.comcast.net] entered the room. 7:32:45 PM - mcote|afk is now known as mcote. 7:33:25 PM - c0smikdebris [c0smikdebr@85A27849.ECDD7E84.BE90E62C.IP] entered the room. 7:34:12 PM - gal has left the room (Quit: gal). 7:37:41 PM - ehsan [ehsan@F2D29657.F60B0462.67AC9B1.IP] entered the room. 7:37:51 PM - bholley [anonymous@moz-104CC309.mv.mozilla.com] entered the room. 7:41:07 PM - gal [gal@moz-7327DB99.hsd1.ca.comcast.net] entered the room. 7:47:12 PM - tlt [ripley@F8F6D15E.8B3F00A.8920A73.IP] entered the room. 7:49:43 PM - brambles has left the room (Quit: Ping timeout). 7:50:32 PM - brambles [brambles@4CBAB088.F3076E90.1822ACA6.IP] 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 [stransky@moz-107AD163.redhat.com] 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 [jrmuizel@F2D29657.F60B0462.67AC9B1.IP] 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
Assignee | ||
Updated•10 years ago
|
Severity: normal → minor
OS: Windows 7 → All
Hardware: x86 → All
Summary: If you change your nick, and someone types your previous nick, then you might get a ping. → Don't reset nick at reconnect if the last nick change was user initiated
Assignee | ||
Comment 1•10 years ago
|
||
*** 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•10 years ago
|
||
*** 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
Assignee | ||
Comment 3•10 years ago
|
||
*** 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.
Attachment #8353420 -
Flags: review?(clokep)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
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; ...
Attachment #8353420 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 5•10 years ago
|
||
*** Original post on bio 1538 as attmnt 1665 at 2012-06-23 11:48:00 UTC *** Nit fixed.
Attachment #8353422 -
Flags: review?(clokep)
Assignee | ||
Comment 6•10 years ago
|
||
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 ***
Attachment #8353420 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
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!
Attachment #8353422 -
Flags: review?(clokep) → review+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Comment 8•10 years ago
|
||
*** 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)
Assignee | ||
Comment 9•10 years ago
|
||
*** 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.)
Attachment #8353434 -
Flags: review?(florian)
Assignee | ||
Comment 10•10 years ago
|
||
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 ***
Attachment #8353422 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
*** 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).
Assignee | ||
Comment 12•10 years ago
|
||
*** Original post on bio 1538 as attmnt 1678 at 2012-06-23 22:58:00 UTC *** Introduce this._accountNickname instead of a getter.
Attachment #8353435 -
Flags: review?(clokep)
Assignee | ||
Comment 13•10 years ago
|
||
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 ***
Attachment #8353434 -
Attachment is obsolete: true
Attachment #8353434 -
Flags: review?(florian)
Assignee | ||
Comment 14•10 years ago
|
||
*** Original post on bio 1538 as attmnt 1679 at 2012-06-23 23:05:00 UTC *** (Typo)
Attachment #8353436 -
Flags: review?(clokep)
Assignee | ||
Comment 15•10 years ago
|
||
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 ***
Attachment #8353435 -
Attachment is obsolete: true
Attachment #8353435 -
Flags: review?(clokep)
Comment 16•10 years ago
|
||
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.
Attachment #8353436 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 17•10 years ago
|
||
*** 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
Assignee | ||
Comment 18•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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?
Assignee | ||
Comment 21•10 years ago
|
||
*** 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
Assignee | ||
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 22•10 years ago
|
||
*** Original post on bio 1538 as attmnt 1690 at 2012-06-25 13:09:00 UTC *** Right, I understood what you meant now ;)
Attachment #8353447 -
Flags: review?(clokep)
Assignee | ||
Comment 23•10 years ago
|
||
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 ***
Attachment #8353436 -
Attachment is obsolete: true
Comment 24•10 years ago
|
||
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!
Attachment #8353447 -
Flags: review?(clokep) → review+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Comment 25•10 years ago
|
||
*** 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!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•