Closed
Bug 955183
Opened 10 years ago
Closed 10 years ago
No feedback when the /nick command fails
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.4
People
(Reporter: florian, Assigned: aleth)
Details
Attachments
(1 file, 5 obsolete files)
2.50 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1751 at 2012-11-02 17:23:00 UTC *** I would have expected it in the conversation where I typed the command.
Assignee | ||
Comment 1•10 years ago
|
||
*** Original post on bio 1751 as attmnt 2040 at 2012-11-04 15:11:00 UTC *** This only happens in the edge case where the user is e.g. "florian1" and tries to change to "florian", but ends up back at "florian1" due to a nick collision. This produces no error message, so we handle it explicitly here.
Attachment #8353800 -
Flags: review?(clokep)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Severity: normal → minor
OS: Other → All
Hardware: x86 → All
Comment 2•10 years ago
|
||
Comment on attachment 8353800 [details] [diff] [review] Patch *** Original change on bio 1751 attmnt 2040 at 2012-11-05 02:37:50 UTC *** >diff --git a/chrome/en-US/locale/en-US/chat/irc.properties b/chrome/en-US/locale/en-US/chat/irc.properties >+# Could not change the nickname. >+message.nick.fail=The nickname you tried to change to is still in use. I agree that this is what happened, but I think we should mention (explicitly) somewhere that <foo> was kept as a nick? >diff --git a/components/irc.js b/components/irc.js >+ let nickChanged = false; >+ if (this.normalize(aNewNick) == this.normalize(this._nickname)) { >+ // The user attempted to change his nickname, but it failed. >+ msg = _("message.nick.fail"); >+ } >+ else { >+ // Your nickname changed! >+ nickChanged = true; >+ this._nickname = aNewNick; >+ msg = _("message.nick.you", aNewNick); >+ } It looks to me like you really want (+ appropriate simplifications): let nickChanged = this.normalize(aNewNick) != this.normalize(this._nickname); >diff --git a/modules/ircBase.jsm b/modules/ircBase.jsm >+ if (newNick == aAccount._nickname) { Why did you choose not to normalize this? Why is this special cased? I don't think this comment actually explains what happened, what is actually happening is that we are trying to change our nick to a nick we already have, no?
Attachment #8353800 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 3•10 years ago
|
||
*** Original post on bio 1751 as attmnt 2048 at 2012-11-05 11:07:00 UTC *** (In reply to comment #2) > It looks to me like you really want (+ appropriate simplifications): > let nickChanged = this.normalize(aNewNick) != this.normalize(this._nickname); Yes, that's neater. > >diff --git a/modules/ircBase.jsm b/modules/ircBase.jsm > >+ if (newNick == aAccount._nickname) { > Why did you choose not to normalize this? Why is this special cased? I don't > think this comment actually explains what happened, what is actually happening > is that we are trying to change our nick to a nick we already have, no? You're right, it's safer to normalize here too. I somehow assumed both nicks were coming from the server in this case, but it's not necessarily true.
Attachment #8353808 -
Flags: review?(clokep)
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8353800 [details] [diff] [review] Patch *** Original change on bio 1751 attmnt 2040 at 2012-11-05 11:07:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353800 -
Attachment is obsolete: true
Comment 5•10 years ago
|
||
*** Original post on bio 1751 at 2012-11-05 20:05:43 UTC *** We should make sure that this patch properly handles us only changing the case of our nickname. This is still a nick change, but they normalize to the same value. We might need to check an unnormalized nick somewhere. If doing this, please add a comment about why it isn't being normalized. :)
Assignee | ||
Comment 6•10 years ago
|
||
*** Original post on bio 1751 as attmnt 2055 at 2012-11-05 20:28:00 UTC *** Thanks for catching this edge case! Note we still want the normalize in the if clause in tryNick because if the user tried to change to "aLeTh" and this failed because aleth was blocked, we don't want to change to "aLeTh1" if we are already "aleth1". If the user really wants that, he can do it directly, but most likely it was due to a typo.
Attachment #8353815 -
Flags: review?(clokep)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8353808 [details] [diff] [review] Patch *** Original change on bio 1751 attmnt 2048 at 2012-11-05 20:28:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353808 -
Attachment is obsolete: true
Attachment #8353808 -
Flags: review?(clokep)
Assignee | ||
Comment 8•10 years ago
|
||
*** Original post on bio 1751 as attmnt 2056 at 2012-11-05 20:41:00 UTC *** Remove obsolete line.
Attachment #8353816 -
Flags: review?(clokep)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8353815 [details] [diff] [review] Patch *** Original change on bio 1751 attmnt 2055 at 2012-11-05 20:41:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353815 -
Attachment is obsolete: true
Attachment #8353815 -
Flags: review?(clokep)
Assignee | ||
Comment 10•10 years ago
|
||
*** Original post on bio 1751 as attmnt 2057 at 2012-11-05 20:53:00 UTC *** Changed my mind - this is simpler. I think we can take this.
Attachment #8353817 -
Flags: review?(clokep)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8353816 [details] [diff] [review] Patch *** Original change on bio 1751 attmnt 2056 at 2012-11-05 20:53:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353816 -
Attachment is obsolete: true
Attachment #8353816 -
Flags: review?(clokep)
Assignee | ||
Comment 12•10 years ago
|
||
*** Original post on bio 1751 as attmnt 2058 at 2012-11-05 21:32:00 UTC *** Take your pick ;)
Attachment #8353818 -
Flags: review?(clokep)
Comment 13•10 years ago
|
||
Comment on attachment 8353817 [details] [diff] [review] Patch *** Original change on bio 1751 attmnt 2057 at 2012-11-07 16:05:20 UTC *** I like the other string better.
Attachment #8353817 -
Flags: review?(clokep) → review-
Comment 14•10 years ago
|
||
Comment on attachment 8353818 [details] [diff] [review] Patch with alternative string *** Original change on bio 1751 attmnt 2058 at 2012-11-16 00:20:07 UTC *** I think this looks good. Let's give it a try. :)
Attachment #8353818 -
Flags: review?(clokep) → review+
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8353817 [details] [diff] [review] Patch *** Original change on bio 1751 attmnt 2057 at 2012-11-16 09:52:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353817 -
Attachment is obsolete: true
Comment 16•10 years ago
|
||
*** Original post on bio 1751 at 2012-11-18 18:57:59 UTC *** Committed as http://hg.instantbird.org/instantbird/rev/735829d0bdc9 with a slightly different string discussed over IRC.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
Reporter | ||
Comment 17•10 years ago
|
||
*** Original post on bio 1751 at 2012-11-19 00:53:15 UTC *** This busted the test_tryNewNick xpcshell test. Here's the log of the failure: TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/chat/protocols/irc/test/test_tryNewNick.js | test failed (with xpcshell return code: 0), see following log: >>>>>>> TEST-INFO | (xpcshell/head.js) | test 1 pending TEST-INFO | (xpcshell/head.js) | test 2 pending TEST-INFO | (xpcshell/head.js) | test 2 finished TEST-INFO | (xpcshell/head.js) | running event loop TEST-INFO | (xpcshell/head.js) | test 2 pending TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/chat/protocols/irc/test/test_tryNewNick.js | Starting test_tryNewNick TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/head.js | TypeError: aAccount.normalize is not a function - See following stack: JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 451 JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _run_next_test :: line 891 JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: .run :: line 418 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-INFO | (xpcshell/head.js) | exiting test TEST-INFO | (xpcshell/head.js) | test 2 finished <<<<<<< Pushed http://hg.instantbird.org/instantbird/rev/1a273f525619 to fix this.
You need to log in
before you can comment on or make changes to this bug.
Description
•