Open Bug 1034061 Opened 6 years ago Updated 9 months ago
Long /me messages aren't split
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).
You need to log in before you can comment on or make changes to this bug.