*** Original post on bio 1712 at 2012-09-27 20:17:00 UTC *** It's difficult to indicate how many characters are left reliably because of the possible number of characters depending on the length of the channel name, and because of multi-bytes UTF8 characters, but at the time of sending we can detect that a message is too long. In that case I think we should cut it and send it in several different messages. We may want to do this only if the whole message can be sent in at most 2 or 3 messages (does this need to be configurable in about:config?). Bonus point for cutting only in spaces and not in the middle of words.

*** Original post on bio 1712 as attmnt 1931 at 2012-09-30 04:48:00 UTC *** This is a brief proof of concept that this seems fairly doable. Note that this doesn't care how many messages the string would be split into, it just keeps splitting it until it's a "good" size.

*** Original post on bio 1712 at 2012-09-30 09:20:50 UTC *** Did you mean let index = message.lastIndexOf(" ", MAX_LEN); instead of let index = message.substr(0, MAX_LEN).lastIndexOf(" "); ?

*** Original post on bio 1712 at 2012-09-30 14:17:36 UTC *** (In reply to comment #2) > Did you mean > let index = message.lastIndexOf(" ", MAX_LEN); > instead of > let index = message.substr(0, MAX_LEN).lastIndexOf(" "); > ? Yes. :) I didn't know that existed.

*** Original post on bio 1712 as attmnt 2012 at 2012-10-25 01:00:00 UTC *** This should split on spaces, or if not possible in the middle of words. I've tested this a bit manually, but we need to write some tests for this function.

*** Original post on bio 1712 at 2012-10-25 10:48:32 UTC *** (In reply to comment #4) > Created attachment 8353772 [details] [diff] [review] (bio-attmnt 2012) [details] > Patch v1 Note that this has an issue in that it doesn't take into account your current nick, just your nick when logged in. But please review the rest of the patch. :)

Comment on attachment 8353772 [details] [diff] [review] Patch v1 *** Original change on bio 1712 attmnt 2012 at 2012-10-27 03:27:18 UTC *** >+ // A message is at least our prefix + a space + PRIVMSG + the target (nick >+ // or channel name) + a space and : + \r\n. >+ let maxLength = this._account.maxMessageLength - >+ this._account.countBytes(this._account.prefix) - 1 - 2 - 2 - >+ this._account.countBytes(this._account.buildMessage("PRIVMSG", this.name)); This is confusing. What about creating the whole message and calling countBytes only once? >+ if (length > maxLength) { Why not if (length <= maxLength) continue; ? This would reduce the indent level of the next code. >+ // Find the location of a space before the maximum length. >+ let index = message.lastIndexOf(" ", maxLength); >+ // Start the next message after the space. >+ let indexNext = index + 1; >+ // If no space was found. >+ if (index == -1) { >+ // Avoid the situation of adding empty strings into the array. >+ if (length < maxLength) >+ continue; I don't see how this can happen. >+ else { >+ // Take as much of the string as you can and start the next >+ // message on the next character. >+ index = maxLength; >+ indexNext = index; >+ } >+ } This all looks complicated. Isn't this doing the same thing as? let indexNext = message.lastIndexOf(" ", maxLength) || maxLength; Nit: some code in here has 4-space indent, please fix this. >@@ -670,6 +706,8 @@ ircAccount.prototype = { > _accountNickname: null, > // The nickname that will be used when connecting. > _requestedNickname: null, >+ // The full prefix (nick!user@host) as returned by the server. >+ prefix: null, You may want to explain in the comment why we care about this prefix. Thanks for working on this! :-)

*** Original post on bio 1712 as attmnt 2024 at 2012-10-31 00:56:00 UTC *** This does not try to take into account extra spaces around messages, etc. but I think it meets all the review comments. Oh, and this includes tests too. If someone can think of more tests, feel free to tell me about them. :)

Comment on attachment 8353772 [details] [diff] [review] Patch v1 *** Original change on bio 1712 attmnt 2012 at 2012-10-31 00:56:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353784 [details] [diff] [review] Patch v2 *** Original change on bio 1712 attmnt 2024 at 2012-11-01 23:42:22 UTC *** >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js >+ // Build the shortest possible message that could be sent. >+ let baseMessage = this._account._nickname + this._account.prefix + >+ " " + this._account.buildMessage("PRIVMSG", this.name) + >+ " :\r\n"; Shouldn't we try to adapt the this._account.maxMessageLength value after each nick change to at least take into account the parts that don't depend on the conversation name? I'm not sure if this is a good idea or not, as it won't be the exact value anyway. Just thinking out loud, feel free to discard :). >+ let maxLength = this._account.maxMessageLength - >+ this._account.countBytes(baseMessage); Nit: I think I would prefer the line break at this position: let maxLength = this._account.maxMessageLength - this._account.countBytes(baseMessage); (It really doesn't matter though.) >diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm >--- a/chat/protocols/irc/ircBase.jsm >+++ b/chat/protocols/irc/ircBase.jsm >@@ -345,6 +345,11 @@ var ircBase = { > // Check if our nick has changed. > if (aMessage.params[0] != this._nickname) > this.changeBuddyNick(this._nickname, aMessage.params[0]); >+ // Get our full prefix. >+ this.prefix = aMessage.params[1].slice( >+ aMessage.params[1].lastIndexOf(" ") + 1); I don't understand what aMessage.params[1].lastIndexOf(" ") is here, but I'm willing to assume it does what you want. >diff --git a/chat/protocols/irc/test/test_splitLongMessages.js b/chat/protocols/irc/test/test_splitLongMessages.js >+const inputMessages = [ >+ "This is a test.", // Exactly 50 characters. >+ "This is a message that is too long.", // Too long. >+ "Short msg.", // Too short. >+ "Thismessagecan'tbecut." >+]; >+ >+const expectedOuputMessage = [ >+ ["This is a test."], >+ ["This is a", "message that is", "too long."], >+ ["Short msg."], >+ ["Thismessagecan'", "tbecut."] >+] Nit: missing ; Possible way to simplify: const messages = { // Exactly 50 characters. "This is a test.": ["This is a test."], // Too long. "This is a message that is too long.": ["This is a", "message that is", "too long."], ... }; Then you can use for each for the loop, and can forget the 'i' variable. >+function run_test() { >+ for (let i = 0; i < inputMessages.length; ++i) { Nit: duplicated space. >+ irc.GenericIRCConversation.messages = []; >+ irc.GenericIRCConversation.sendMsg(inputMessages[i]); >+ >+ for (let j = 0; j < irc.GenericIRCConversation.messages.length; ++j) { You will never enter the loop and the test will never fail if irc.GenericIRCConversation.messages.length is 0. Possible ways to fix it: - use expectedOuputMessage[i].length instead of irc.GenericIRCConversation.messages.length - do_check_eq(expectedOuputMessage[i].length, irc.GenericIRCConversation.messages.length); I prefer the explicit length check, as otherwise you wouldn't fail the test if there's an unexpected empty message at the end of the array. >+ do_check_eq(irc.GenericIRCConversation.messages[j], >+ expectedOuputMessage[i][j]); >+ } >+ } >+} r- because I'll want to look quickly again at the next iteration, but this looks really good!

*** Original post on bio 1712 as attmnt 2027 at 2012-11-02 00:52:00 UTC *** (In reply to comment #8) > Comment on attachment 8353784 [details] [diff] [review] (bio-attmnt 2024) [details] > Patch v2 > > >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js > > >+ // Build the shortest possible message that could be sent. > >+ let baseMessage = this._account._nickname + this._account.prefix + > >+ " " + this._account.buildMessage("PRIVMSG", this.name) + > >+ " :\r\n"; > > Shouldn't we try to adapt the this._account.maxMessageLength value after each > nick change to at least take into account the parts that don't depend on the > conversation name? > I'm not sure if this is a good idea or not, as it won't be the exact value > anyway. Just thinking out loud, feel free to discard :). No, the maxMessageLength is for both incoming and outgoing messages. The length of OUR nick only effects messages that will be sent from the server to other users. We send: PRIVMSG #foo :Foo bar They receive: :nick!user@host PRIVMSG #foo :Foo bar Meets the other review comments.

Comment on attachment 8353784 [details] [diff] [review] Patch v2 *** Original change on bio 1712 attmnt 2024 at 2012-11-02 00:52:50 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353787 [details] [diff] [review] Patch v3 *** Original change on bio 1712 attmnt 2027 at 2012-11-02 01:29:15 UTC *** Thanks!

*** Original post on bio 1712 at 2012-11-02 01:35:42 UTC *** Committed as http://hg.instantbird.org/instantbird/rev/c2e1ebde9ee1