1034061 - Long /me messages aren't split

Status: NEW

(Chat Core :: IRC, defect)

Description

[Florian Quèze [:florian]]

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>

Comment 1

[Patrick Cloke [:clokep]]

See bug 955690 too.

Comment 2

[aleth [:aleth]]

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

Comment 3

[Pavan Karthik [:matrixisreal]]

Attachment: v1.patch

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.

Comment 4

[Patrick Cloke [:clokep]]

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?

Comment 5

[Pavan Karthik [:matrixisreal]]

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 6

[Patrick Cloke [:clokep]]

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.

Comment 7

[Pavan Karthik [:matrixisreal]]

Attachment: v1.1.patch

notifying the addons before sending - fixed

Comment 8

[Patrick Cloke [:clokep]]

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).