Status

defect
--
minor
5 years ago
5 months ago

People

(Reporter: florian, Assigned: matrixisreal)

Tracking

(Depends on 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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>
I sort of thought this was fixed, but it isn't. We still get "Message length too long (740 > 512"
Assignee: nobody → pavankarthikboddeda
Depends on: 1225468
Posted patch v1.patch (obsolete) — Splinter Review
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.
Attachment #8829109 - Flags: review?(florian)
Attachment #8829109 - Flags: review?(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?
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.
Attachment #8829109 - Flags: review?(florian)
Attachment #8829109 - Flags: review?(clokep)
Attachment #8829109 - Flags: review-
Posted patch v1.1.patchSplinter Review
notifying the addons before sending - fixed
Attachment #8829109 - Attachment is obsolete: true
Attachment #8838491 - Flags: review?(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).
Attachment #8838491 - Flags: review?(clokep)
You need to log in before you can comment on or make changes to this bug.