Closed Bug 955183 Opened 10 years ago Closed 10 years ago

No feedback when the /nick command fails

Categories

(Chat Core :: IRC, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: aleth)

Details

Attachments

(1 file, 5 obsolete files)

*** 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.
Attached patch Patch (obsolete) — Splinter Review
*** 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: nobody → aleth
Status: NEW → ASSIGNED
Severity: normal → minor
OS: Other → All
Hardware: x86 → All
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-
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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
*** 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. :)
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1751 as attmnt 2056 at 2012-11-05 20:41:00 UTC ***

Remove obsolete line.
Attachment #8353816 - Flags: review?(clokep)
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)
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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)
*** 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 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 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+
Whiteboard: [checkin-needed]
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
*** 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
*** 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.