Closed
Bug 954780
Opened 10 years ago
Closed 10 years ago
Replace alphabetic increment of last letter on nick collision
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: clokep)
Details
(Whiteboard: [1.2-wanted])
Attachments
(1 file, 8 obsolete files)
3.83 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1346 at 2012-03-20 00:32:00 UTC *** The current behaviour of incrementing the last letter of a user's nick when a nick already exists on logon is not good imho, for two reasons: - Breaks pings for the user - Makes it hard/confusing to tell whether someone is who you think they might be, especially for short nicks I'd propose replacing it by nick_, nick__,... or nick1, nick2,... Leaving this unconfirmed, as I'm not sure everyone will agree ;)
Assignee | ||
Comment 1•10 years ago
|
||
*** Original post on bio 1346 at 2012-03-20 00:36:02 UTC *** (In reply to comment #0) > The current behaviour of incrementing the last letter of a user's nick when a > nick already exists on logon is not good imho, for two reasons: > > - Breaks pings for the user > - Makes it hard/confusing to tell whether someone is who you think they might > be, especially for short nicks > > I'd propose replacing it by nick_, nick__,... or nick1, nick2,... Personally I like incrementing the last number if it's a number, otherwise adding a 1 and then incrementing that (so it would be nick --> nick1 --> nick2 --> ... --> nick9 --> nick0 --> nick01 --> etc.) I'll accept a patch for this.
Assignee | ||
Comment 2•10 years ago
|
||
*** Original post on bio 1346 at 2012-03-20 13:59:46 UTC *** Setting to new. :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
Reporter | ||
Updated•10 years ago
|
Whiteboard: [1.2-wanted]
Reporter | ||
Comment 3•10 years ago
|
||
*** Original post on bio 1346 at 2012-03-30 12:45:30 UTC *** Also need to decide whether one should be pinged by all the possible nick variants or not.
Reporter | ||
Comment 4•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1288 at 2012-03-31 00:01:00 UTC *** Any thoughts on the approach sketched here? (I didn't include the 8 -> 9 -> 0 -> 01 -> 02 numbering scheme here because it seems more confusing than normal increments, and because I'm not sure what is supposed to happen after 09 ;) )
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8353041 [details] [diff] [review] Rough patch *** Original change on bio 1346 attmnt 1288 at 2012-03-31 12:51:39 UTC *** (In reply to comment #4) > Created attachment 8353041 [details] [diff] [review] (bio-attmnt 1288) [details] > Rough patch > > Any thoughts on the approach sketched here? > > (I didn't include the 8 -> 9 -> 0 -> 01 -> 02 numbering scheme here because it > seems more confusing than normal increments, and because I'm not sure what is > supposed to happen after 09 ;) ) I do not like the way this patch is, I'd like only changes to tryNewNick. Something like: let lastCharInd = "012345678".indexOf(aMessage.params[1].slice(-1)); if (lastCharInd != -1) aAccount._nickname = aMessage.params[1].slice(0, -1) + (lastCharInd + 1); else aAccount._nickname += "0"; // Or use a 1 here or whatever. So you check the last character, if it's 0 - 8, you increment to 1 - 9, if it's anything else you append a 0 or a 1.
Attachment #8353041 -
Flags: review-
Reporter | ||
Comment 6•10 years ago
|
||
*** Original post on bio 1346 at 2012-03-31 14:19:41 UTC *** My hope was that you'd end up with something more like nick -> nick1 -> nick over time, if the issue was that the server hadn't realized you had been disconnected or timed out. Hard to test though.
Comment 7•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-02 14:40:08 UTC *** I thought you were going to write something like: (pseudo code) let nickParts = /(first part)([0-9]*)$/.match(currentNick); let newNick = first part + (parseInt(second part) + 1);
Assignee | ||
Comment 8•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-02 14:51:47 UTC *** (In reply to comment #6) > My hope was that you'd end up with something more like nick -> nick1 -> nick > over time, if the issue was that the server hadn't realized you had been > disconnected or timed out. Hard to test though. I think that would be part of bug 954088 (bio 653). Unless you're suggesting if there's a nick collision check if your "real" nick is available first? I think the real test here should be that we should always use the "real" nick when reconnecting, not the current nick (i.e. it's a different bug).
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1304 at 2012-04-06 00:51:00 UTC *** This is the patch implementing flo's suggestion.
Attachment #8353057 -
Flags: review?(bugzilla)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8353041 [details] [diff] [review] Rough patch *** Original change on bio 1346 attmnt 1288 at 2012-04-06 00:51:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353041 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Assignee: aleth → clokep
Assignee | ||
Comment 11•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1306 at 2012-04-06 01:15:00 UTC *** Forgot to update a comment in the previous patch. :)
Attachment #8353059 -
Flags: review?(bugzilla)
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8353057 [details] [diff] [review] Patch *** Original change on bio 1346 attmnt 1304 at 2012-04-06 01:15:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353057 -
Attachment is obsolete: true
Attachment #8353057 -
Flags: review?(bugzilla)
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8353059 [details] [diff] [review] Patch *** Original change on bio 1346 attmnt 1306 at 2012-04-06 09:57:49 UTC *** >+ let nickParts = /^([^\d]+)(\d*)$/.exec(aMessage.params[1]); This regexp will fail on nicks which contain numbers, e.g. "clo1kep" or "clo22kep1".
Attachment #8353059 -
Flags: review?(bugzilla) → review-
Assignee | ||
Comment 14•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1308 at 2012-04-06 10:25:00 UTC *** (In reply to comment #11) > Comment on attachment 8353059 [details] [diff] [review] (bio-attmnt 1306) [details] > Patch > > >+ let nickParts = /^([^\d]+)(\d*)$/.exec(aMessage.params[1]); > > This regexp will fail on nicks which contain numbers, e.g. "clo1kep" or > "clo22kep1". Good catch! :)
Attachment #8353061 -
Flags: review?(bugzilla)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8353059 [details] [diff] [review] Patch *** Original change on bio 1346 attmnt 1306 at 2012-04-06 10:25:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353059 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1312 at 2012-04-06 10:51:00 UTC *** With an actual tested regexp! :)
Attachment #8353065 -
Flags: review?(bugzilla)
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8353061 [details] [diff] [review] Patch v2 *** Original change on bio 1346 attmnt 1308 at 2012-04-06 10:51:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353061 -
Attachment is obsolete: true
Attachment #8353061 -
Flags: review?(bugzilla)
Assignee | ||
Comment 18•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1313 at 2012-04-06 10:53:00 UTC *** aleth thought of another edge case...currently clokep01 --> clokep2 which is undesired. This ignores leading 0s (perhaps not the best situation...but it's better).
Attachment #8353066 -
Flags: review?(bugzilla)
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8353065 [details] [diff] [review] Patch v2.1 *** Original change on bio 1346 attmnt 1312 at 2012-04-06 10:53:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353065 -
Attachment is obsolete: true
Attachment #8353065 -
Flags: review?(bugzilla)
Assignee | ||
Comment 20•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-06 10:54:39 UTC *** (In reply to comment #14) > Created attachment 8353066 [details] [diff] [review] (bio-attmnt 1313) [details] > Patch v3 > > This ignores leading 0s (perhaps not the best situation...but it's better). To be clear, this doesn't work well for 09 (which should go to 10 I think?).
Reporter | ||
Comment 21•10 years ago
|
||
Comment on attachment 8353066 [details] [diff] [review] Patch v3 *** Original change on bio 1346 attmnt 1313 at 2012-04-06 11:00:17 UTC *** Looks good!
Attachment #8353066 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 22•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-06 11:03:49 UTC *** (In reply to comment #16) > Comment on attachment 8353066 [details] [diff] [review] (bio-attmnt 1313) [details] > Patch v3 > > Looks good! So I just realized that this doesn't work. It doesn't match clokep. I need to think about this more, but we have to match: clokep --> clokep1 clokep1 --> clokep2 clokep10 --> clokep11 clokep0 --> clokep01 (ideally clokep1) clokep01 --> clokep02 clokep09 --> clokep010 (ideally clokep10)
Assignee | ||
Comment 23•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-06 11:52:54 UTC *** Originally I had this set up this way to not increase the length of the nick (i.e. if you try to set a nick to bob, but nicks can only be two characters long the server returns "bo is already in use!", we then try "bp"). We should add the edge case where oldnick == maxlength, then remove the last character and replace it by 1. (Note that this only needs to be checked if the last character is not a digit). Also we need to check all the cases in comment 17 with numbers in them too. (I think I'll write up a unit test for all this by the way.)
Reporter | ||
Comment 24•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-06 12:21:55 UTC *** (In reply to comment #17) > So I just realized that this doesn't work. It doesn't match clokep. I need to > think about this more, but we have to match: > > clokep --> clokep1 > clokep1 --> clokep2 > clokep10 --> clokep11 > clokep0 --> clokep01 (ideally clokep1) > clokep01 --> clokep02 > clokep09 --> clokep010 (ideally clokep10) /^(.+?0*)(\d*)$/ works OK, I think?
Assignee | ||
Comment 25•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1315 at 2012-04-06 21:25:00 UTC *** New version which properly handles zeros (and magically keeps them!). Oh...and I wrote tests too.
Attachment #8353068 -
Flags: review?(bugzilla)
Assignee | ||
Comment 26•10 years ago
|
||
Comment on attachment 8353066 [details] [diff] [review] Patch v3 *** Original change on bio 1346 attmnt 1313 at 2012-04-06 21:25:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353066 -
Attachment is obsolete: true
Reporter | ||
Comment 27•10 years ago
|
||
Comment on attachment 8353068 [details] [diff] [review] Patch v4 *** Original change on bio 1346 attmnt 1315 at 2012-04-06 21:52:32 UTC *** This should fix it :) That got a bit more elaborate than at first expected... Requesting additional review from flo, especially for the test part which I know nothing about.
Attachment #8353068 -
Flags: review?(bugzilla) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8353068 -
Flags: review?(florian)
Comment 28•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-06 22:27:59 UTC *** Comment on attachment 8353068 [details] [diff] [review] (bio-attmnt 1315) Patch v4 >diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm >+ if (nickParts[2]) { >+ newDigits = (parseInt(nickParts[2], 10) + 1) + ""; I think .toString() would be more explicit than: + "" >+ // If there were leading 0s, add them back on, after we've incremented (e.g. >+ // 009 --> 010). >+ for (let len = nickParts[2].length - newDigits.length; len > 0; len--) Nit: --len instead of len-- >diff --git a/chat/protocols/irc/test/test_tryNewNick.js b/chat/protocols/irc/test/test_tryNewNick.js >new file mode 100644 >--- /dev/null >+++ b/chat/protocols/irc/test/test_tryNewNick.js >@@ -0,0 +1,56 @@ >+/* Any copyright is dedicated to the Public Domain. >+ * http://creativecommons.org/publicdomain/zero/1.0/ */ >+ >+Components.utils.import("resource:///modules/imServices.jsm"); Nit: If it's only to use the scriptloader, maybe the Services.jsm from the platform would be enough? >+function test_tryNewNick() { >+ for (let currentNick in testData) { >+ let account = { >+ _nickname: null, >+ maxNicknameLength: 9, >+ // Dummy function. >+ sendMessage: function(aCommand, aNewNick) {} >+ }; >+ let message = {params: [null, currentNick]}; >+ >+ dump("---------------->>>>>>>> " + currentNick + "\n"); Isn't this unnecessary noise? I think that when a test fails both the expected and actual result will be printed in the log, so this doesn't seem required. Congrats for adding a test with your fix! :)
Assignee | ||
Comment 29•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1316 at 2012-04-06 22:40:00 UTC *** Fixes the nits and a really silly edge case where a99999999 would cause us to want to use 100000000, which is not a valid nick, so we choose a00000000.
Attachment #8353069 -
Flags: review?(florian)
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 8353068 [details] [diff] [review] Patch v4 *** Original change on bio 1346 attmnt 1315 at 2012-04-06 22:40:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353068 -
Attachment is obsolete: true
Attachment #8353068 -
Flags: review?(florian)
Assignee | ||
Comment 31•10 years ago
|
||
*** Original post on bio 1346 as attmnt 1317 at 2012-04-06 23:02:00 UTC *** Use the gre version of Services.jsm.
Comment 32•10 years ago
|
||
Comment on attachment 8353070 [details] [diff] [review] Patch v4.1.1 *** Original change on bio 1346 attmnt 1317 at 2012-04-06 23:10:21 UTC *** Looks great, thanks!
Attachment #8353070 -
Flags: review+
Comment 33•10 years ago
|
||
Comment on attachment 8353069 [details] [diff] [review] Patch v4.1 *** Original change on bio 1346 attmnt 1316 at 2012-04-06 23:12:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353069 -
Attachment is obsolete: true
Attachment #8353069 -
Flags: review?(florian)
Assignee | ||
Comment 34•10 years ago
|
||
*** Original post on bio 1346 at 2012-04-06 23:32:55 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/7d7de5899d69
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•