STR: In an IRC conversation, send: /me blah blah blah (go on, and on, and on, until you exceed the maximum possible length for an IRC message in that conversation) Expected result: - sending: /me blah blah blah blah blah (ie. split the long message, and send only the first part as a CTCP ACTION) Actual result: The whole thing is sent as one command, and on the other end, we get: ACTION blah blah blah <truncated message>

See bug 955690 too.

I sort of thought this was fixed, but it isn't. We still get "Message length too long (740 > 512"

This patch gives the expected result, But the problem is the maxLength of action messages is what I have set to the preceding messages too. If you want me to change that Ill add code for that too. thanks.

I'm a little confused at the behavior your patch creates (probably because we never defined the behavior we want). The confusion I have is whether we should be sending the subsequent messages as action messages or normal messages. Any thoughts on this?

As florian mentioned in the first comment, I am sending just the first part as an action message and subsequent messages as normal messages. I also think this must be the desired behaviour as messages like... *** John says that......long message.... *** John the room. Doesnt maks sense.

Comment on attachment 8829109 [details] [diff] [review] v1.patch Review of attachment 8829109 [details] [diff] [review]: ----------------------------------------------------------------- The idea is pretty reasonable, but I think it needs a bit of work. I think a better understanding of how OutgoingMessage interacts with the rest of the chat framework is needed (unfortunately, I don't fully remember this either!). Generally this gives add-ons (e.g. the OTR add-on) an opportunity to modify (e.g. encrypt) messages. We need to ensure all of the messages from this command still go through that. ::: chat/protocols/irc/ircCommands.jsm @@ +102,5 @@ > if (!aMsg || !aMsg.trim().length) > return false; > > let conv = getConv(aConv); > + let om = new OutgoingMessage(aMsg, aConv, true); This needs a comment above it about what's happening and why it's happening. Since you're not using the "conv" variable, please move all this code above it. @@ +108,4 @@ > > + // Inorder to send just the first part as action message. > + // actionMessage is removed from messages Array. > + // See Bug 1034061. I don't think this is confusing enough that we need to reference the bug, just be sure to explain the behavior in the comments. @@ +108,5 @@ > > + // Inorder to send just the first part as action message. > + // actionMessage is removed from messages Array. > + // See Bug 1034061. > + let actionMessage = messages.splice(0,1); This would be simpler as messages.shift() @@ +114,1 @@ > conv.notifyObservers(om, "sending-message"); Won't this now cause the observers to handle messages that aren't real? @@ -109,4 @@ > conv.notifyObservers(om, "sending-message"); > if (om.cancelled) > return true; > - Please don't arbitrarily remove whitespace. @@ +125,3 @@ > > + // Send the rest of the messages as normal messages. > + messages.forEach(msg => conv.sendMsg(msg)); This will probably skip the add-on framework for all these other messages.

notifying the addons before sending - fixed

Comment on attachment 8838491 [details] [diff] [review] v1.1.patch Clearing review on this. If someone would like to finish this up, please un-bitrot the patch and fix the incorrect formatting (which should now be easier with eslint).