*** Original post on bio 653 by Mathnerd314 <mathnerd314...gph+mozilla AT gmail.com> at 2011-01-15 23:33:00 UTC *** Steps to reproduce: 1. Open another IRC client 2. Try to connect with Instantbird 3. Get a NickServ notice that your nick is unregistered Expected behavior: (first time) See message "nick in use" prompting for alternate nick(s) 3. Connect with an alternate nick not in use It would also be nice to define alternate nicks for other people as well.

*** Original post on bio 653 at 2011-01-16 04:38:55 UTC *** Taking this as the code will be a necessary part of JavaScript rewrite of IRC. This can be as simple as a comma separated list of alternate nicks to go through with a default of adding a _ or incrementing the last character, etc. It could also be done more complexly where a custom function could be provided where the failed nick is given and the function needs to return a different nick using whatever it wants.

*** Original post on bio 653 at 2012-02-27 15:38:02 UTC *** Moving IRC bugs to new IRC component.

*** Original post on bio 653 as attmnt 2377 at 2013-04-17 05:06:00 UTC *** Have tested the patch extensively on 4-5 parallel accounts with same nick preference. First, I look through the user's preference and see which nick he hasn't used yet. Then I try to use that nick, if it causes same problem, then I proceed to another one. In this way, I keep on traversing all of the user's nicks. And if all are occupied, then I generate one nick on the fly and assign it to user, after checking it's validity. User can set up the alternate nicks values in his account manager, in the advanced tab like this "nick,nick1,nick2,nick3".

Comment on attachment 8354144 [details] [diff] [review] Pretty much does the job *** Original change on bio 653 attmnt 2377 at 2013-04-17 10:27:01 UTC *** Initial nit: the entire function needs to be tabbed in two spaces. I haven't looked at the code yet though. I'd like aleth to look at this too as he wrote half of this code.

Comment on attachment 8354144 [details] [diff] [review] Pretty much does the job *** Original change on bio 653 attmnt 2377 at 2013-04-17 12:15:52 UTC *** >diff -r 4a7c4b36042f chat/locales/en-US/irc.properties >+options.alternateNicks=Alternate IRC Nicks Can't this just say "Alternative nicks"? The user should know it's an IRC account. >diff -r 4a7c4b36042f chat/protocols/irc/irc.js >+// Try a new nick if the previous tried nick is already in use. This needs to be expanded to describe the algorithm we're using (e.g. first try this list of alternative nicks, if none of those work try xyz). >+tryNewNick: function (aMessage) { >+ let nickParts = /^(.+?)(\d*)$/.exec(aMessage.params[1]); >+ let newNick = nickParts[1]; >+ let oldNick = aMessage.params[1]; >+ let allNicks = this._alternateNicks; I'd prefer to build the array here, this allows someone to change the preference while an account is connected. Additionally there's no reason to store the array on the account then. >+ let flag = false; This needs a more descriptive name, I think it'd also be clearer if you set this equal to allNicks.some(function(aNick) aNick == oldNick) and then skip the for loop altogether if this is false. >+ for each(let name in allNicks.trim().split(/,\s*/)) { We should probably split on /[,\s]+/, this will allow a space or comma separated list. Nit: Space before the ( >+ if (oldNick === name) { >+ flag = true; >+ } Nit: No { } here. >+ if (oldNick !== name && flag === true) { Why !== and ===? Also check if flag == true is unnecessary, it could just be && flag. Is what's found below this point just copied & pasted or are their algorithmic changes? >+ // No nick found from the user's preferences, so just give him one. [...] >+ this.LOG(aMessage.params[1] + " is already in use, trying " + newNick); >+ this.sendMessage("NICK", newNick); // Nick message. >+ return true; >+}, >@@ -1524,7 +1582,9 @@ >+ "alternateNicks": {label: _("options.alternateNicks"), >+ default: "altnick, altnick1, altnick2, altnick3"} This default value doesn't make sense, please remove it. This will almost definitely make our tests [1] fail, additionally we should add tests for alternative nicks. [1] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/test_tryNewNick.js

Comment on attachment 8354144 [details] [diff] [review] Pretty much does the job *** Original change on bio 653 attmnt 2377 at 2013-04-17 12:49:15 UTC *** >+ let flag = false; >+ >+ for each(let name in allNicks.trim().split(/,\s*/)) { >+ if (oldNick === name) { >+ flag = true; >+ } >+ >+ if (oldNick !== name && flag === true) { >+ newNick = name; >+ this.LOG(aMessage.params[1] + " is already in use, trying " + newNick); >+ this.sendMessage("NICK", newNick); // Nick message. >+ return true; >+ } >+ } I haven't looked at this fully, but I think this could be done more elegantly as split() returns an array. You can use indexOf() to check if oldNick is in the array, and save yourself the flag and the loop.

*** Original post on bio 653 at 2013-04-17 13:48:53 UTC *** I'm sorry for the fast patch. I actually wanted to submit before sleeping, thus nits are there. (In reply to comment #5) > Comment on attachment 8354144 [details] [diff] [review] (bio-attmnt 2377) [details] > Pretty much does the job > > >diff -r 4a7c4b36042f chat/locales/en-US/irc.properties > >+options.alternateNicks=Alternate IRC Nicks > Can't this just say "Alternative nicks"? The user should know it's an IRC > account. Will change it. > >diff -r 4a7c4b36042f chat/protocols/irc/irc.js > >+// Try a new nick if the previous tried nick is already in use. > This needs to be expanded to describe the algorithm we're using (e.g. first try > this list of alternative nicks, if none of those work try xyz). Will do it. > > >+tryNewNick: function (aMessage) { > >+ let nickParts = /^(.+?)(\d*)$/.exec(aMessage.params[1]); > >+ let newNick = nickParts[1]; > >+ let oldNick = aMessage.params[1]; > >+ let allNicks = this._alternateNicks; > I'd prefer to build the array here, this allows someone to change the > preference while an account is connected. Additionally there's no reason to > store the array on the account then. I'm not fully sure if we should build an array or not. oldNick and allNicks are used for our alternate things. And nickParts, newNick are used for our algorithm which generated a new nick for the user. > >+ let flag = false; > This needs a more descriptive name, I think it'd also be clearer if you set > this equal to allNicks.some(function(aNick) aNick == oldNick) and then skip the > for loop altogether if this is false. > >+ for each(let name in allNicks.trim().split(/,\s*/)) { > We should probably split on /[,\s]+/, this will allow a space or comma > separated list. > Nit: Space before the ( > > >+ if (oldNick === name) { > >+ flag = true; > >+ } > Nit: No { } here. > > >+ if (oldNick !== name && flag === true) { > Why !== and ===? Also check if flag == true is unnecessary, it could just be && > flag. Will do it. :-) > Is what's found below this point just copied & pasted or are their algorithmic > changes? Yes, I find the old algorithm quite good, thus decided to include it, in case all the alternate nicks are taken. > >+ // No nick found from the user's preferences, so just give him one. > [...] > >+ this.LOG(aMessage.params[1] + " is already in use, trying " + newNick); > >+ this.sendMessage("NICK", newNick); // Nick message. > >+ return true; > >+}, > > >@@ -1524,7 +1582,9 @@ > >+ "alternateNicks": {label: _("options.alternateNicks"), > >+ default: "altnick, altnick1, altnick2, altnick3"} > This default value doesn't make sense, please remove it. Sure. > This will almost definitely make our tests [1] fail, additionally we should add > tests for alternative nicks. Do I need to write another tests specifically for this purpose? > [1] > http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/test_tryNewNick.js

*** Original post on bio 653 at 2013-04-17 14:10:50 UTC *** (In reply to comment #7) > > >+tryNewNick: function (aMessage) { > > >+ let nickParts = /^(.+?)(\d*)$/.exec(aMessage.params[1]); > > >+ let newNick = nickParts[1]; > > >+ let oldNick = aMessage.params[1]; > > >+ let allNicks = this._alternateNicks; > > I'd prefer to build the array here, this allows someone to change the > > preference while an account is connected. Additionally there's no reason to > > store the array on the account then. > I'm not fully sure if we should build an array or not. oldNick and allNicks are > used for our alternate things. And nickParts, newNick are used for our > algorithm which generated a new nick for the user. I'm not sure what you're saying here...I'm saying don't make _alternativeNicks part of the class object, but create it each time you need it as a local variable. > > Is what's found below this point just copied & pasted or are their algorithmic > > changes? > Yes, I find the old algorithm quite good, thus decided to include it, in case > all the alternate nicks are taken. It would have been r- otherwise. ;) > > This will almost definitely make our tests [1] fail, additionally we should add > > tests for alternative nicks. > Do I need to write another tests specifically for this purpose? Yes.

*** Original post on bio 653 as attmnt 2432 at 2013-05-16 14:30:00 UTC *** Improved the nits. This *does* not include the tests yet. I'll upload the tests in the next patch.

Comment on attachment 8354144 [details] [diff] [review] Pretty much does the job *** Original change on bio 653 attmnt 2377 at 2013-05-16 14:30:23 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354199 [details] [diff] [review] Patch V2 *** Original change on bio 653 attmnt 2432 at 2013-05-16 15:29:33 UTC *** >diff -r 4a7c4b36042f chat/locales/en-US/irc.properties >+options.alternateNicks=Alternate Nicks Nit: Don't capitalize nicks. >diff -r 4a7c4b36042f chat/protocols/irc/irc.js >+ // Try a new nick if the previous tried nick is already in use. >+ // For this, we first try all the alternate nicks that are stored by >+ // the user, and if none of them works fine, then we generate our own, and >+ // use it as a new nick. >+ // New nick generating algorithm is as follows: >+ // First we check if there was a digit at the end of the nick >+ // if not, then we just append 1 at the end, of if there was a digit, >+ // then we just increment the number. After that we handle some special cases >+ // such all nines and ensuring that length of nick is not too long. >+ tryNewNick: function (aMessage) { >+ let nickParts = /^(.+?)(\d*)$/.exec(aMessage.params[1]); >+ let newNick = nickParts[1]; We should move the nickParts and newNick down to where they actually get used, before auto-generating a nick. >+ let oldNick = aMessage.params[1]; >+ let allNicks = this._accountNickname + "," + this.getString("alternateNicks"); >+ >+ let nickArray = allNicks.trim().split(/[,\s]+/); I would prefer: let nickArray = this.getString("alternateNicks").trim().split(/[,\s]+/).unshift(this._accountNickname); Or something like that...pretty much don't add something to the string and then parse it back out. >+ let oldIndex = nickArray.indexOf(oldNick); >+ >+ if (oldIndex!= -1 && oldIndex < nickArray.length - 1) { Nit: space before !=. Do we really need to check it is < nickArray.length? Put a comment above this saying that we're checking if it's in the array and not the last element. >+ newNick = nickArray[oldIndex+1]; Nit: Space before and after the +. >+ this.LOG(aMessage.params[1] + " is already in use, trying " + newNick); >+ this.sendMessage("NICK", newNick); // Nick message. >+ return true; >+ } >+ >+ // No nick found from the user's preferences, so just give him one. >+ this.DEBUG ("No match from the user's alternate nicks thus trying one "); "just give him one" makes it sound random, change this to "generate one". Is this debug statement really necessary? If so, it needs to be reworded. Nit: No space before the (. >+ // If there was not a digit at the end of the nick, just append 1. >+ let newDigits = "1"; >+ // If there was a digit at the end of the nick, increment it. I know these are my comments, but can we change these from "was" to "is"? Thanks! >@@ -1524,7 +1586,9 @@ > "quitmsg": {label: _("options.quitMessage"), > get default() Services.prefs.getCharPref("chat.irc.defaultQuitMessage")}, > "partmsg": {label: _("options.partMessage"), default: ""}, >- "showServerTab": {label: _("options.showServerTab"), default: false} >+ "showServerTab": {label: _("options.showServerTab"), default: false}, >+ "alternateNicks": {label: _("options.alternateNicks"), >+ default: ""} Nit: Line up default with label (does this not fit on the line above it?). (Also, I wonder about the order in here, but the end is fine for now.) So if my alternate nicks are set up as clokep, clokep_, clokep_foo. I have accounts logged in as all three. I then /nick clokep_ from an account, what is my nick? Is it clokep_1 or clokep_foo1? (I'd expect the former, but I think it's the latter.) [1] might help you fix this. [1] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#725

*** Original post on bio 653 at 2013-05-16 18:55:21 UTC *** I've completed the patch, but I need to go somewhere right now, thus I'll post it in the morning after testing it out. Thanks

*** Original post on bio 653 as attmnt 2438 at 2013-05-19 01:53:00 UTC *** This is not a final patch, just a sample one, thus I'm asking for feedback on this. I've handled the case of clokep, clokep_, clokep_foo that you mentioned. While I was thinking of how to do that, I was confused about what we want to do. Do we want to *original* nickname to be used for generating nicks or the nick that he was currently using, or the last successful nick? Please look at the patch, if you want me to change anything

Comment on attachment 8354199 [details] [diff] [review] Patch V2 *** Original change on bio 653 attmnt 2432 at 2013-05-19 01:53:20 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354205 [details] [diff] [review] Just a sample patch *** Original change on bio 653 attmnt 2438 at 2013-05-20 14:35:58 UTC *** We agreed [1] on a final algorithm for this: 1. Try all the alternate nicks in the order they're listed. 2. If one is found, stop. 3. If they're all used (this would include an empty list), then take the requested nick and apply the numbering algorithm to it. I also suggested writing the tests for this first, with the desired out come and ensuring this works afterward. [1] http://log.bezut.info/instantbird/130520/#m126

Comment on attachment 8354205 [details] [diff] [review] Just a sample patch *** Original change on bio 653 attmnt 2438 at 2013-09-26 21:18:11 UTC *** (In reply to comment #13) > > We agreed [1] on a final algorithm for this: > 1. Try all the alternate nicks in the order they're listed. > 2. If one is found, stop. > 3. If they're all used (this would include an empty list), then take the > requested nick and apply the numbering algorithm to it. The patch seems to behave like that already. > I also suggested writing the tests for this first, with the desired out come > and ensuring this works afterward. Definitely something that should be done. Feedback on the patch itself: > --- a/chat/protocols/irc/irc.js Thu May 16 22:44:07 2013 +0200 > +++ a/chat/protocols/irc/irc.js Sun May 19 07:16:19 2013 +0530 > + tryNewNick: function (aMessage) { > + let oldNick = aMessage.params[1]; > + let allNicks = this._accountNickname + "," + this.getString("alternateNicks"); > + > + let nickArray = allNicks.trim().split(/[,\s]+/); trim() won't help if someone has a leading or trailing comma in their alternate nicks list. I think we should filter empty values from the array (e.g. with filter(n => !!n)) > + let oldIndex = nickArray.indexOf(oldNick); > + > + > + // We're checking if it's in the array and not the last element Missing dot at the end of sentence. Does this comment really help much by the way? > + if (oldIndex != -1 && oldIndex < nickArray.length - 1) { > + newNick = nickArray[oldIndex + 1]; > + this.LOG(aMessage.params[1] + " is already in use, trying " + newNick ); No space before the closing bracket. > + let nickParts = /^(.+?)(\d*)$/.exec(this._accountNickname); > + let newNick = nickParts[1]; > + > + // No nick found from the user's preferences, so just generating one. > + // If there is not a digit at the end of the nick, just append 1. > + let newDigits = "1"; > + // If there is a digit at the end of the nick, increment it. > + if (nickParts[2]) { > + newDigits = (parseInt(nickParts[2], 10) + 1).toString(); > + // If there are 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) > + newDigits = "0" + newDigits; This can be improved by using String.repeat (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/repeat). > @@ -1551,7 +1616,8 @@ > "quitmsg": {get label() _("options.quitMessage"), > get default() Services.prefs.getCharPref("chat.irc.defaultQuitMessage")}, > "partmsg": {get label() _("options.partMessage"), default: ""}, > - "showServerTab": {get label() _("options.showServerTab"), default: false} > + "showServerTab": {get label() _("options.showServerTab"), default: false}, > + "alternateNicks": {label: _("options.alternateNicks"), default: ""} This should use a getter too lest we load the properties file right away.

*** Original post on bio 653 as attmnt 2941 at 2013-10-11 19:10:00 UTC *** This was almost there so I figured I'd finish it up since we haven't heard from Atul in a while. This meets the previous review comments and updates the tests.

Comment on attachment 8354205 [details] [diff] [review] Just a sample patch *** Original change on bio 653 attmnt 2438 at 2013-10-11 19:10:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354720 [details] [diff] [review] Patch *** Original change on bio 653 attmnt 2941 at 2013-10-12 17:45:06 UTC *** >+ /* >+ * Generate a new nick to change to if the user requested nick is already in >+ * use or is otherwise invalid. >+ * >+ * First try all the alternate nicks that were chosen by the user, and if none >+ * of them work, then generate a new nick by: >+ * 1. If there was not a digit at the end of the nick, append a 1. >+ * 2. If there was a digit, then increment the number. >+ * 3. Add leading 0s back on. >+ * 4. Ensure the nick is an appropriate length. >+ */ >+ tryNewNick: function (aMessage) { >+ let oldNick = aMessage.params[1]; Now this has moved to the account, I would prefer if it took the oldNick as the parameter. Apart from avoiding potential future mistakes, this will also make the tests simpler to read. >+ if (numLeadingZeros > 0) >+ newDigits = "0".repeat(numLeadingZeros) + newDigits; Finally a use case for String.repeat! :P >diff --git a/chat/protocols/irc/test/test_tryNewNick.js b/chat/protocols/irc/test/test_tryNewNick.js >+function test_altNicks() { >+ const testData = { >+ // Test account nick. >+ "clokep": [["clokep_", "clokep|"], "clokep_"], Can you put ["clokep_", "clokep|"] in a const to avoid duplication? >+ // Test last element in list. >+ "clokep|": [["clokep_", "clokep|"], "clokep|1"], I was surprised by this and would have expected "clokep1", but thinking about it I am not sure.

*** Original post on bio 653 at 2013-10-16 13:57:41 UTC *** (In reply to comment #16) > >+ // Test last element in list. > >+ "clokep|": [["clokep_", "clokep|"], "clokep|1"], > > I was surprised by this and would have expected "clokep1", but thinking about > it I am not sure. What surprises you about this? It seems like what I would expect. clokep isn't even a nick at all here!

*** Original post on bio 653 as attmnt 2960 at 2013-10-16 14:08:00 UTC *** Meets the review comments.

Comment on attachment 8354720 [details] [diff] [review] Patch *** Original change on bio 653 attmnt 2941 at 2013-10-16 14:08:29 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 653 at 2013-10-18 09:54:08 UTC *** (In reply to comment #17) > What surprises you about this? It seems like what I would expect. clokep isn't > even a nick at all here! You're right - I missed the different property name.

*** Original post on bio 653 at 2013-10-18 09:55:32 UTC *** Comment on attachment 8354741 [details] [diff] [review] (bio-attmnt 2960) Patch v2 >+ tryNewNick: function (aOldNick) { Nit: no space after function. (Sorry for not noticing this earlier!) r+ with this change.

*** Original post on bio 653 as attmnt 2962 at 2013-10-18 11:02:00 UTC *** Fixes nit.

Comment on attachment 8354741 [details] [diff] [review] Patch v2 *** Original change on bio 653 attmnt 2960 at 2013-10-18 11:02:49 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354743 [details] [diff] [review] Patch v3 *** Original change on bio 653 attmnt 2962 at 2013-10-18 11:03:35 UTC *** Thanks!

*** Original post on bio 653 at 2013-10-18 11:12:34 UTC *** http://hg.instantbird.org/instantbird/rev/7620e849458f